[PATCH] x86/shadow: Correct the behaviour of a guests view of maxphysaddr

[Andrew Cooper <andrew.cooper3@citrix.com>]

XSA-173 (c/s 8b1764833) introduces gfn_bits, and an upper limit which might be
lower than the real maxphysaddr, to avoid overflowing the superpage shadow
backpointer.

However, plenty of hardware has a physical address width less that 44 bits,
and the code added in shadow_domain_init() is a straight asignment.  This
causes gfn_bits to be increased beyond the physical address width on most
Intel consumer hardware (which has a width of 39).

It also means that the effective maxphysaddr for shadowed guests differs from
the value reported to the guest in CPUID.  This means that a guest can create
PTEs which either should fault but don't, or shouldn't fault but do.

Remove gfn_bits and rework its logic in terms of a guests maxphysaddr.
recalculate_cpuid_policy() is updated to properly clamp maxphysaddr between
the host maximum, a possibly-smaller shadow restriction, and 32 as a minimum.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
---
 xen/arch/x86/cpuid.c            | 20 ++++++++++++++------
 xen/arch/x86/hvm/vmx/vvmx.c     |  2 +-
 xen/arch/x86/mm/guest_walk.c    |  3 ++-
 xen/arch/x86/mm/hap/hap.c       |  2 --
 xen/arch/x86/mm/p2m.c           |  3 ++-
 xen/arch/x86/mm/shadow/common.c | 10 ----------
 xen/arch/x86/mm/shadow/multi.c  |  3 ++-
 xen/include/asm-x86/domain.h    |  3 ---
 8 files changed, 21 insertions(+), 25 deletions(-)

diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index e0a387e..02a3451 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -1,4 +1,5 @@
 #include <xen/init.h>
+#include <xen/kconfig.h>
 #include <xen/lib.h>
 #include <xen/sched.h>
 #include <asm/cpuid.h>
@@ -6,6 +7,7 @@
 #include <asm/hvm/nestedhvm.h>
 #include <asm/hvm/svm/svm.h>
 #include <asm/hvm/vmx/vmcs.h>
+#include <asm/paging.h>
 #include <asm/processor.h>
 #include <asm/xstate.h>
 
@@ -424,7 +426,7 @@ void recalculate_cpuid_policy(struct domain *d)
     const struct cpuid_policy *max =
         is_pv_domain(d) ? &pv_max_policy : &hvm_max_policy;
     uint32_t fs[FSCAPINTS], max_fs[FSCAPINTS];
-    unsigned int i;
+    unsigned int i, maxphysaddr_limit;
 
     p->x86_vendor = get_cpu_vendor(p->basic.vendor_ebx, p->basic.vendor_ecx,
                                    p->basic.vendor_edx, gcv_guest);
@@ -502,11 +504,17 @@ void recalculate_cpuid_policy(struct domain *d)
 
     cpuid_featureset_to_policy(fs, p);
 
-    p->extd.maxphysaddr = min(p->extd.maxphysaddr, max->extd.maxphysaddr);
-    p->extd.maxphysaddr = min_t(uint8_t, p->extd.maxphysaddr,
-                                d->arch.paging.gfn_bits + PAGE_SHIFT);
-    p->extd.maxphysaddr = max_t(uint8_t, p->extd.maxphysaddr,
-                                (p->basic.pae || p->basic.pse36) ? 36 : 32);
+    maxphysaddr_limit = paging_mode_hap(d) ? hap_paddr_bits : paddr_bits;
+
+    if ( !IS_ENABLED(BIGMEM) && paging_mode_shadow(d) &&
+         (!is_pv_domain(d) || opt_allow_superpage) )
+    {
+        /* Shadowed superpages store GFNs in 32-bit page_info fields. */
+        maxphysaddr_limit = min(maxphysaddr_limit, 32U + PAGE_SHIFT);
+    }
+
+    p->extd.maxphysaddr = min_t(uint8_t, p->extd.maxphysaddr, maxphysaddr_limit);
+    p->extd.maxphysaddr = max_t(uint8_t, p->extd.maxphysaddr, 32);
 
     p->extd.maxlinaddr = p->extd.lm ? 48 : 32;
 
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 5acb88a..4ea8a6b 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1380,7 +1380,7 @@ int nvmx_handle_vmxon(struct cpu_user_regs *regs)
     }
 
     if ( (gpa & ~PAGE_MASK) ||
-         (gpa >> (v->domain->arch.paging.gfn_bits + PAGE_SHIFT)) )
+         (gpa >> v->domain->arch.cpuid->extd.maxphysaddr) )
     {
         vmfail_invalid(regs);
         return X86EMUL_OKAY;
diff --git a/xen/arch/x86/mm/guest_walk.c b/xen/arch/x86/mm/guest_walk.c
index a67fd5a..5ad8cf6 100644
--- a/xen/arch/x86/mm/guest_walk.c
+++ b/xen/arch/x86/mm/guest_walk.c
@@ -435,7 +435,8 @@ guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m,
     /* If this guest has a restricted physical address space then the
      * target GFN must fit within it. */
     if ( !(rc & _PAGE_PRESENT)
-         && gfn_x(guest_l1e_get_gfn(gw->l1e)) >> d->arch.paging.gfn_bits )
+         && gfn_x(guest_l1e_get_gfn(gw->l1e)) >>
+         (d->arch.cpuid->extd.maxphysaddr - PAGE_SHIFT) )
         rc |= _PAGE_INVALID_BITS;
 
     return rc;
diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index 6dbb3cc..928cd5e 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -447,8 +447,6 @@ void hap_domain_init(struct domain *d)
 {
     INIT_PAGE_LIST_HEAD(&d->arch.paging.hap.freelist);
 
-    d->arch.paging.gfn_bits = hap_paddr_bits - PAGE_SHIFT;
-
     /* Use HAP logdirty mechanism. */
     paging_log_dirty_init(d, hap_enable_log_dirty,
                           hap_disable_log_dirty,
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 6548e9f..fc706c6 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -1790,7 +1790,8 @@ void *map_domain_gfn(struct p2m_domain *p2m, gfn_t gfn, mfn_t *mfn,
 {
     struct page_info *page;
 
-    if ( gfn_x(gfn) >> p2m->domain->arch.paging.gfn_bits )
+    if ( gfn_x(gfn) >>
+         (p2m->domain->arch.cpuid->extd.maxphysaddr - PAGE_SHIFT) )
     {
         *rc = _PAGE_INVALID_BIT;
         return NULL;
diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index a619d65..2235a0a 100644
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -52,16 +52,6 @@ int shadow_domain_init(struct domain *d, unsigned int domcr_flags)
     INIT_PAGE_LIST_HEAD(&d->arch.paging.shadow.freelist);
     INIT_PAGE_LIST_HEAD(&d->arch.paging.shadow.pinned_shadows);
 
-    d->arch.paging.gfn_bits = paddr_bits - PAGE_SHIFT;
-#ifndef CONFIG_BIGMEM
-    /*
-     * Shadowed superpages store GFNs in 32-bit page_info fields.
-     * Note that we cannot use guest_supports_superpages() here.
-     */
-    if ( !is_pv_domain(d) || opt_allow_superpage )
-        d->arch.paging.gfn_bits = 32;
-#endif
-
     /* Use shadow pagetables for log-dirty support */
     paging_log_dirty_init(d, sh_enable_log_dirty,
                           sh_disable_log_dirty, sh_clean_dirty_bitmap);
diff --git a/xen/arch/x86/mm/shadow/multi.c b/xen/arch/x86/mm/shadow/multi.c
index d4090d7..e951daf 100644
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -537,7 +537,8 @@ _sh_propagate(struct vcpu *v,
 
     /* Check there's something for the shadows to map to */
     if ( (!p2m_is_valid(p2mt) && !p2m_is_grant(p2mt))
-         || gfn_x(target_gfn) >> d->arch.paging.gfn_bits )
+         || gfn_x(target_gfn) >>
+         (d->arch.cpuid->extd.maxphysaddr - PAGE_SHIFT) )
     {
         *sp = shadow_l1e_empty();
         goto done;
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index e6c7e13..2270e96 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -195,9 +195,6 @@ struct paging_domain {
     /* log dirty support */
     struct log_dirty_domain log_dirty;
 
-    /* Number of valid bits in a gfn. */
-    unsigned int gfn_bits;
-
     /* preemption handling */
     struct {
         const struct domain *dom;
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel