[PATCH v4] x86/nSVM: Validate the L1 IOPM physical address range

[PATCH v4] x86/nSVM: Validate the L1 IOPM physical address range

[Abdelkareem Abdelsaamad]

The Xen nested virtualization code does not validate that the physical address
range assigned by the L1 guest, for IOPM, resides within the valid guest
physical memory. Add a sanity check to properly validate the IOPM is in the
valid L1 guest address range. This check also makes the behaviour compliant
with the hardware handling of the assigned address. The hardware is expected to
trigger VMEXIT_INVALID if the address of the last byte in the IOPM is greater
than or equal to the maximum supported physical address, see the APM
volume #2 (40332—Rev. 4.40—July 2026).

While at it, clean up the code. Remove the unused bool viopm and the
svm_vcpu::ns_oiomap_pa. Change nsvm_vmrun_permissionmap return error from
literal 1 to NSVM_ERROR_VVMCB.

Signed-off-by: Abdelkareem Abdelsaamad <abdelkareem.abdelsaamad@citrix.com>
---
Changes in V4:
- Replace the IOPM_PAGES_COUNT constant with an expression calculating the
  actual page span of the IO permission map.
- Rephrase the commit message to better describe the changes.
Changes in V3:
- Switch to using gfn_valid() instead of domain_get_maximum_gpfn() to align
  with what is told to the guest in the CPUID.
- Drop MSRPM validation as it is already validated.
- Change nsvm_vmrun_permissionmap return error from literal 1 to
  NSVM_ERROR_VVMCB.
- Remove parentheses (IOPM_PAGES_COUNT - 1).
Changes in V2:
- Rename IOPM_MAX_PAGES_DIFF and MSRPM_MAX_PAGES_DIFF constants to
  IOPM_PAGES_COUNT and MSRPM_PAGES_COUNT.
- Drop the 2-pages for domain check.
- Change the IOPM and MSRPM boundary checks.
- Use gaddr_to_gfn instead of open-coding >> PAGE_SHIFT.
- Use __func__ instead of hardcoding raw function names.
---
Testing:
 - Using a locally developed XTF nested virt setup, I manually tested VMRUN
   instruction handling with the address value (0xffffffffffffffffUL) assigned
   to VMCB::iopm_base_pa:
   - Without the changes the address is mapped by the Xen code to the address
     0x604b634000 and it completes the execution without any reported errors.
   - With the changes, the VMRUN execution fails and VMEXIT_INVALID is reported
     back in the ns_vmexit.exitcode.
 - CI tests:
https://gitlab.com/xen-project/people/aabdelsa/xen/-/pipelines/2766682638
---
 xen/arch/x86/hvm/svm/nestedsvm.c         | 18 ++++++++++++++----
 xen/arch/x86/include/asm/hvm/svm-types.h |  2 +-
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
index b06124c2c9..b176e6000a 100644
--- a/xen/arch/x86/hvm/svm/nestedsvm.c
+++ b/xen/arch/x86/hvm/svm/nestedsvm.c
@@ -282,7 +282,7 @@ static int nsvm_vcpu_hostrestore(struct vcpu *v, struct cpu_user_regs *regs)
     return 0;
 }
 
-static int nsvm_vmrun_permissionmap(struct vcpu *v, bool viopm)
+static int nsvm_vmrun_permissionmap(struct vcpu *v)
 {
     struct svm_vcpu *arch_svm = &v->arch.hvm.svm;
     struct nestedsvm *svm = &vcpu_nestedsvm(v);
@@ -294,6 +294,17 @@ 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;
+    /* IOPM is structured as a linear array of 64K+3 bits. */
+    const unsigned long nr_iopm_additional_pages = PFN_DOWN((0x10000 + 8) / 8 - 1);
+    gfn_t ns_iopm_end =
+        gfn_add(gaddr_to_gfn(ns_vmcb->_iopm_base_pa), nr_iopm_additional_pages);
+
+    if ( !gfn_valid(v->domain, ns_iopm_end) )
+    {
+        gdprintk(XENLOG_ERR, "%s invalid _iopm_base_pa address (%#"PRIx64")\n",
+                 __func__, ns_vmcb->_iopm_base_pa);
+        return NSVM_ERROR_VVMCB;
+    }
 
     ns_msrpm_ptr = (unsigned long *)svm->ns_cached_msrpm;
 
@@ -302,13 +313,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);
@@ -418,7 +428,7 @@ static int nsvm_vmcb_prepare4vmrun(struct vcpu *v, struct cpu_user_regs *regs)
     n2vmcb->_tsc_offset = n1vmcb->_tsc_offset + ns_vmcb->_tsc_offset;
 
     /* Nested IO permission bitmaps */
-    rc = nsvm_vmrun_permissionmap(v, clean.iopm);
+    rc = nsvm_vmrun_permissionmap(v);
     if ( rc )
         return rc;
 
diff --git a/xen/arch/x86/include/asm/hvm/svm-types.h b/xen/arch/x86/include/asm/hvm/svm-types.h
index 8acadb9dcc..beab9a3af2 100644
--- a/xen/arch/x86/include/asm/hvm/svm-types.h
+++ b/xen/arch/x86/include/asm/hvm/svm-types.h
@@ -51,7 +51,7 @@ struct nestedsvm {
     unsigned long *ns_merged_msrpm;
 
     /* guest physical address of virtual io permission map */
-    paddr_t ns_iomap_pa, ns_oiomap_pa;
+    paddr_t ns_iomap_pa;
     /* Shadow io permission map */
     unsigned long *ns_iomap;
 
-- 
2.53.0

Re: [PATCH v4] x86/nSVM: Validate the L1 IOPM physical address range

[Jan Beulich]

On 17.08.2026 18:59, Abdelkareem Abdelsaamad wrote:
> --- a/xen/arch/x86/hvm/svm/nestedsvm.c
> +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
> @@ -282,7 +282,7 @@ static int nsvm_vcpu_hostrestore(struct vcpu *v, struct cpu_user_regs *regs)
>      return 0;
>  }
>  
> -static int nsvm_vmrun_permissionmap(struct vcpu *v, bool viopm)
> +static int nsvm_vmrun_permissionmap(struct vcpu *v)
>  {
>      struct svm_vcpu *arch_svm = &v->arch.hvm.svm;
>      struct nestedsvm *svm = &vcpu_nestedsvm(v);
> @@ -294,6 +294,17 @@ 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;
> +    /* IOPM is structured as a linear array of 64K+3 bits. */
> +    const unsigned long nr_iopm_additional_pages = PFN_DOWN((0x10000 + 8) / 8 - 1);

Hm, the expression I did suggest was indeed off by one, yet yours doesn't fit
the comment very well. What's wrong with PFN_DOWN((0x10000 + 3) / 8) or
PFN_DOWN((0xffff + 4) / 8)?

> +    gfn_t ns_iopm_end =
> +        gfn_add(gaddr_to_gfn(ns_vmcb->_iopm_base_pa), nr_iopm_additional_pages);

I'm also inclined to suggest to drop the local variable, as it's used just
here. The overall result would be

    /* IOPM is structured as a linear array of 64K+3 bits. */
    gfn_t ns_iopm_end = gfn_add(gaddr_to_gfn(ns_vmcb->_iopm_base_pa),
                                PFN_DOWN((0x10000 + 3) / 8));

which imo is a little easier to follow. Preferably with those adjustments
(happy to carry out while committing, but please confirm):
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: Re: [PATCH v4] x86/nSVM: Validate the L1 IOPM physical address range

[Abdelkareem Abdelsaamad]

On 18.08.2026 11:23, Jan Beulich wrote:
>On 17.08.2026 18:59, Abdelkareem Abdelsaamad wrote:
>> --- a/xen/arch/x86/hvm/svm/nestedsvm.c
>> +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
>> @@ -282,7 +282,7 @@ static int nsvm_vcpu_hostrestore(struct vcpu *v, struct cpu_user_regs *regs)
>>      return 0;
>>  }
>>  
>> -static int nsvm_vmrun_permissionmap(struct vcpu *v, bool viopm)
>> +static int nsvm_vmrun_permissionmap(struct vcpu *v)
>>  {
>>      struct svm_vcpu *arch_svm = &v->arch.hvm.svm;
>>      struct nestedsvm *svm = &vcpu_nestedsvm(v);
>> @@ -294,6 +294,17 @@ 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;
>> +    /* IOPM is structured as a linear array of 64K+3 bits. */
>> +    const unsigned long nr_iopm_additional_pages = PFN_DOWN((0x10000 + 8) / 8 - 1);
>
>Hm, the expression I did suggest was indeed off by one, yet yours doesn't fit
>the comment very well. What's wrong with PFN_DOWN((0x10000 + 3) / 8) or
>PFN_DOWN((0xffff + 4) / 8)?
In the expression I wrote, I was trying to maintain maximum fidelity to the
exact byte count and fetch the last byte index, even though PFN_DOWN ultimately
maps both 8192 and 8193 to 2 pages to have more fidelity to the original
calculation. It probaly looks a bit over-engineered but I wanted to keep as
much fidelity to the actual expression.
> +    gfn_t ns_iopm_end =
> +        gfn_add(gaddr_to_gfn(ns_vmcb->_iopm_base_pa), nr_iopm_additional_pages);
>
>I'm also inclined to suggest to drop the local variable, as it's used just
>here. The overall result would be
>
>    /* IOPM is structured as a linear array of 64K+3 bits. */
>    gfn_t ns_iopm_end = gfn_add(gaddr_to_gfn(ns_vmcb->_iopm_base_pa),
>                                PFN_DOWN((0x10000 + 3) / 8));
>
>which imo is a little easier to follow. Preferably with those adjustments
>(happy to carry out while committing, but please confirm):
Sounds good to me! Please go ahead and drop it while committing. Thank you.
>Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
>Jan