Re: Re: [PATCH v3] x86/nSVM: Check the L1 IOPM_BASE assigned physical address

[Abdelkareem Abdelsaamad <abdelkareem.abdelsaamad@xxxxxxxxxx>]

On ,13.08.2026  10:40 Jan Beulich wrote:
>On 07.08.2026 17:58, Abdelkareem Abdelsaamad wrote:
>> --- a/xen/arch/x86/hvm/svm/nestedsvm.c
>> +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
>> @@ -18,6 +18,7 @@
>>  
>>  #define NSVM_ERROR_VVMCB        1
>>  #define NSVM_ERROR_VMENTRY      2
>> +#define IOPM_PAGES_COUNT        3
>
>This new item is separate from the NSVM_ERROR_* values and hence wants 
>separating
>by a blank line. Especially with the three numbers being in sequence, not doing
>so could end up being confusing.
>
>Considering the constant is used exactly once - do we actually need a constant?
>Can't we ...
>
>> @@ -294,6 +295,15 @@ static int nsvm_vmrun_permissionmap(struct vcpu *v, 
>> bool viopm)
>>      enum hvm_translation_result ret;
>>      unsigned long *ns_viomap;
>>      bool ioport_80 = true, ioport_ed = true;
>> +    gfn_t ns_iopm_end =
>> +        gfn_add(gaddr_to_gfn(ns_vmcb->_iopm_base_pa), IOPM_PAGES_COUNT - 1);
>
>... use a suitable expression here, e.g. PFN_DOWN((0xffff + 3) / 8)?
OK. I will change it like that in V4.
>> @@ -302,13 +312,12 @@ static int nsvm_vmrun_permissionmap(struct vcpu *v, 
>> bool viopm)
>>      if ( ret != HVMTRANS_okay )
>>      {
>>          gdprintk(XENLOG_ERR, "hvm_copy_from_guest_phys msrpm %u\n", ret);
>> -        return 1;
>> +        return NSVM_ERROR_VVMCB;
>>      }
>>  
>>      /* Check l1 guest io permission map and get a shadow one based on
>>       * if l1 guest intercepts io ports 0x80 and/or 0xED.
>>       */
>> -    svm->ns_oiomap_pa = svm->ns_iomap_pa;
>>      svm->ns_iomap_pa = ns_vmcb->_iopm_base_pa;
>>  
>>      ns_viomap = hvm_map_guest_frame_ro(svm->ns_iomap_pa >> PAGE_SHIFT, 0);
>
>In the description you say "without any sanity checks", yet
>hvm_map_guest_frame_ro() -> _hvm_map_guest_frame() ->
>check_get_page_from_gfn() won't allow unsuitable GFNs to be mapped. Since
>here only the first page is mapped, some extra checking may indeed be
>warranted, but the description then wants updating.
It (the first page) is actually not mapped. It properly fails. However, the Xen
code continues without any issues. The code continues with an internal Xen
allocated memory shadow_io_bitmap in nestedhvm_vcpu_iomap_get.
>
>As to that part of the description, "directly to valid host address" also
>doesn't look to adequately describe what's going on.
I will rephrase the commit message for better clarity in v4.
>--Jan