[Xen-devel] [PATCH 1/2] x86/xstate: Use the CPUID policy in valid_xcr0(), rather than allowing all features

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

It turns out that Xen has never enforced that a domain remain within the
xstate features advertised in CPUID.

The check of new_bv against xfeature_mask ensures that a domain stays within
the set of features that Xen has enabled in hardware (and therefore isn't a
security problem), but this does means that attempts to level a guest for
migration safety might not be effective if the guest ignores CPUID.

Plumb a domain pointer down into valid_xcr0(), and check the proposed xcr0
against the policy maximum.  This subsumes the PKRU check for PV guests in
handle_xsetbv() (and also demonstrates that I should have spotted this problem
while reviewing c/s fbf9971241f).

Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
---
CC: Jan Beulich <JBeulich@xxxxxxxx>

Backporting notes: This is safe in the restore case, but only back as far as
the introduction of cpuid_policy infrastructure.  Before then, a restore
boolean needs to be pumbed down as well, and used to select between the
hardware maximum value and calls to {hvm,pv}_cpuid() to find the
toolstack-chosen level.
---
 xen/arch/x86/domctl.c        |  2 +-
 xen/arch/x86/hvm/hvm.c       |  2 +-
 xen/arch/x86/xstate.c        | 23 ++++++++++++++---------
 xen/include/asm-x86/xstate.h |  5 +++--
 4 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index b973629..0423a0c 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1170,7 +1170,7 @@ long arch_do_domctl(
             if ( _xcr0_accum )
             {
                 if ( evc->size >= PV_XSAVE_HDR_SIZE + XSTATE_AREA_MIN_SIZE )
-                    ret = validate_xstate(_xcr0, _xcr0_accum,
+                    ret = validate_xstate(d, _xcr0, _xcr0_accum,
                                           &_xsave_area->xsave_hdr);
             }
             else if ( !_xcr0 )
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index f9408e1..d57a942 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1254,7 +1254,7 @@ static int hvm_load_cpu_xsave_states(struct domain *d, 
hvm_domain_context_t *h)
     ctxt = (struct hvm_hw_cpu_xsave *)&h->data[h->cur];
     h->cur += desc->length;
 
-    err = validate_xstate(ctxt->xcr0, ctxt->xcr0_accum,
+    err = validate_xstate(d, ctxt->xcr0, ctxt->xcr0_accum,
                           (const void *)&ctxt->save_area.xsave_hdr);
     if ( err )
     {
diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index b4aea4b..44b993f 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -645,8 +645,16 @@ void xstate_init(struct cpuinfo_x86 *c)
         BUG();
 }
 
-static bool valid_xcr0(u64 xcr0)
+static bool valid_xcr0(const struct domain *d, uint64_t xcr0)
 {
+    const struct cpuid_policy *cp = d->arch.cpuid;
+    uint64_t xcr0_max =
+        ((uint64_t)cp->xstate.xcr0_high << 32) | cp->xstate.xcr0_low;
+
+    /* Check xcr0 against the CPUID policy. */
+    if ( xcr0 & ~xcr0_max )
+        return false;
+
     /* FP must be unconditionally set. */
     if ( !(xcr0 & X86_XCR0_FP) )
         return false;
@@ -670,14 +678,15 @@ static bool valid_xcr0(u64 xcr0)
     return !(xcr0 & X86_XCR0_BNDREGS) == !(xcr0 & X86_XCR0_BNDCSR);
 }
 
-int validate_xstate(u64 xcr0, u64 xcr0_accum, const struct xsave_hdr *hdr)
+int validate_xstate(const struct domain *d, uint64_t xcr0, uint64_t xcr0_accum,
+                    const struct xsave_hdr *hdr)
 {
     unsigned int i;
 
     if ( (hdr->xstate_bv & ~xcr0_accum) ||
          (xcr0 & ~xcr0_accum) ||
-         !valid_xcr0(xcr0) ||
-         !valid_xcr0(xcr0_accum) )
+         !valid_xcr0(d, xcr0) ||
+         !valid_xcr0(d, xcr0_accum) )
         return -EINVAL;
 
     if ( (xcr0_accum & ~xfeature_mask) ||
@@ -699,13 +708,9 @@ int handle_xsetbv(u32 index, u64 new_bv)
     if ( index != XCR_XFEATURE_ENABLED_MASK )
         return -EOPNOTSUPP;
 
-    if ( (new_bv & ~xfeature_mask) || !valid_xcr0(new_bv) )
+    if ( (new_bv & ~xfeature_mask) || !valid_xcr0(curr->domain, new_bv) )
         return -EINVAL;
 
-    /* XCR0.PKRU is disabled on PV mode. */
-    if ( is_pv_vcpu(curr) && (new_bv & X86_XCR0_PKRU) )
-        return -EOPNOTSUPP;
-
     if ( !set_xcr0(new_bv) )
         return -EFAULT;
 
diff --git a/xen/include/asm-x86/xstate.h b/xen/include/asm-x86/xstate.h
index 86a4a1f..47f602b 100644
--- a/xen/include/asm-x86/xstate.h
+++ b/xen/include/asm-x86/xstate.h
@@ -97,8 +97,9 @@ void xsave(struct vcpu *v, uint64_t mask);
 void xrstor(struct vcpu *v, uint64_t mask);
 void xstate_set_init(uint64_t mask);
 bool xsave_enabled(const struct vcpu *v);
-int __must_check validate_xstate(u64 xcr0, u64 xcr0_accum,
-                                 const struct xsave_hdr *);
+int __must_check validate_xstate(const struct domain *d,
+                                 uint64_t xcr0, uint64_t xcr0_accum,
+                                 const struct xsave_hdr *hdr);
 int __must_check handle_xsetbv(u32 index, u64 new_bv);
 void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size);
 void compress_xsave_states(struct vcpu *v, const void *src, unsigned int size);
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel