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

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

[Abdelkareem Abdelsaamad]

The Xen nested virtualization code maps the physical address assigned by the L1
guests, for IOPM_BASE, directly to valid host address without sanity checks.
Add sanity checks to verify the L1 assigned address is a valid guest address.
This check also makes the bahavior compliant with the Hardware handling of the
assigned addresses. The hardware is expected to trigger VMEXIT_INVALID with
IOPM_BASE address greater than or qual 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 V3:
- Switch to using gfn_valid() instead of domain_get_maximum_gpfn() to align
  with what is told to the guest in the CPUID.
- 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/2741283982
---
 xen/arch/x86/hvm/svm/nestedsvm.c         | 17 +++++++++++++----
 xen/arch/x86/include/asm/hvm/svm-types.h |  2 +-
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
index b06124c2c9..e895cf12c4 100644
--- 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
 
 int nestedsvm_vmcb_map(struct vcpu *v, uint64_t vmcbaddr)
 {
@@ -282,7 +283,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 +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);
+
+    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 +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);
@@ -418,7 +427,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 v3] x86/nSVM: Check the L1 IOPM_BASE assigned physical address

[Jan Beulich]

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)?

> +    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 +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.

As to that part of the description, "directly to valid host address" also
doesn't look to adequately describe what's going on.

Jan

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

[Abdelkareem Abdelsaamad]

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

[Abdelkareem Abdelsaamad]

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