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

Abdelkareem Abdelsaamad <[email protected]>
Newsgroups org.xenproject.lists.xen-devel
Message-ID <f97fdc50622e4db559ce059091d0ebf2028e2f2e.1786114518.git.abdelkareem.abdelsaamad@citrix.com>
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 <[email protected]>
---
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
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.