Re: Re: [PATCH v1] x86/nSVM: Check the L1 IOPM_BASE and MSRPM_BASE

[Abdelkareem Abdelsaamad <abdelkareem.abdelsaamad@xxxxxxxxxx>]

On 28.07.2026 16:00, Jan Beulich wrote: 
>On 28.07.2026 13:41, Abdelkareem Abdelsaamad wrote:
>> --- a/xen/arch/x86/hvm/svm/nestedsvm.c
>> +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
>> @@ -18,6 +18,8 @@
>>  
>>  #define NSVM_ERROR_VVMCB        1
>>  #define NSVM_ERROR_VMENTRY      2
>> +#define IOPM_MAX_PAGES_DIFF     2
>> +#define MSRPM_MAX_PAGES_DIFF    1

>I see little value in these constants, the more that from their names alone
>one cannot infer what they're meant to represent. Furthermore the difference
>between the two checks you add makes clear that in the latter one you open-
>code MSRPM_MAX_PAGES_DIFF being 1.
I will change to IOPM_PAGES and MSRPM_PAGES in v2. I think it is preferrable to
use named constants rather than magic numbers?

>> @@ -295,6 +297,23 @@ static int nsvm_vmrun_permissionmap(struct vcpu *v, 
>> bool viopm)
>>      unsigned long *ns_viomap;
>>      bool ioport_80 = true, ioport_ed = true;
>> 
>> +    if ( (domain_get_maximum_gpfn(v->domain) < IOPM_MAX_PAGES_DIFF) ||
>>
>Leaving aside that it's questionable whether a domain with just two pages
>can actually run sensibly, I think there's an off-by-one here, but I
>further think the check isn't needed. After all ...
>
>> +         (gfn_x(_gfn(ns_vmcb->_iopm_base_pa >> PAGE_SHIFT)) >
>
>... the shift here guarantees that adding IOPM_MAX_PAGES_DIFF to the
>result would not overflow (unless that constant was really huge).
For the calculation, my thinking is that domain_get_maximum_gpfn returns the
maximum frame number index and the IOPM spans 3 pages, which is why I do not
think there is an off-by-one in the calculation. I completely agree with the
larger point: because of the PAGE_SHIFT and the 2-page domain, I also do not
believe, is a realistic guest. I will change the check in v2.
>
>Also please use gaddr_to_gfn() (or PFN_DOWN()) instead of open-coding.
I Will change in v2.

>> +        gdprintk(XENLOG_ERR, "nsvm_vmrun_permissionmap invalid 
>> _iopm_base_pa"
>> +                 " address (%#"PRIx64")\n", ns_vmcb->_iopm_base_pa);

>When you want the function name in a log message, please use __func__.
Yes, I will change in v2.
>Jan