[Xen-devel] [PATCH 6/6] x86/xstate: Fix latent bugs in compress_xsave_states()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

compress_xsave_states() mustn't read xstate_bv or xcomp_bv before first
confirming that the input buffer is large enough.  It also doesn't cope with
compressed input.  Make all of these problems the callers responsbility to
ensure.

The logic cant cope with an xstate change which would force the use of xrstors
when the vcpu is unpaused.  Leave a TODO and BUG_ON() to make this obvious to
whomever is first to implement an xsaves-only state, rather than causing data
corruption.

Finally, avoid silently discarding incoming states if something ends up wrong
with comp_offsets[].  This case shouldn't be able to happen if the preceeding
verification is correct.

Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
---
CC: Jan Beulich <JBeulich@xxxxxxxx>
---
 xen/arch/x86/xstate.c | 37 +++++++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index f6157f5..937abc6 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -224,17 +224,36 @@ void expand_xsave_states(struct vcpu *v, void *dest, 
unsigned int size)
     }
 }
 
+/*
+ * Deserialise a toolstack's xsave state representation suitably for a vcpu.
+ *
+ * Internally a vcpus xsave state may be compressed or uncompressed, depending
+ * on the features in use, but the ABI with the toolstack is strictly
+ * uncompressed.
+ *
+ * It is the callers responsibility to ensure that the source buffer contains
+ * xsave state, is uncompressed, and is exactly the right size.
+ */
 void compress_xsave_states(struct vcpu *v, const void *src, unsigned int size)
 {
     struct xsave_struct *xsave = v->arch.xsave_area;
     uint16_t comp_offsets[sizeof(xfeature_mask)*8];
-    u64 xstate_bv = ((const struct xsave_struct *)src)->xsave_hdr.xstate_bv;
-    u64 valid;
+    u64 xstate_bv, valid;
+
+    BUG_ON(!v->arch.xcr0_accum);
+    BUG_ON(size != xstate_ctxt_size(v->arch.xcr0_accum));
+    BUG_ON(xsave_area_compressed(src));
 
-    ASSERT(!xsave_area_compressed(src));
+    xstate_bv = ((const struct xsave_struct *)src)->xsave_hdr.xstate_bv;
 
     if ( !(v->arch.xcr0_accum & XSTATE_XSAVES_ONLY) )
     {
+        /*
+         * TODO: This logic doesn't currently handle restoration of xsave
+         * state which would force the vcpu from uncompressed to compressed.
+         */
+        BUG_ON(xstate_bv & XSTATE_XSAVES_ONLY);
+
         memcpy(xsave, src, size);
         return;
     }
@@ -262,11 +281,13 @@ void compress_xsave_states(struct vcpu *v, const void 
*src, unsigned int size)
         unsigned int index = fls(feature) - 1;
         void *dest = get_xsave_addr(xsave, comp_offsets, index);
 
-        if ( dest )
-        {
-            ASSERT((xstate_offsets[index] + xstate_sizes[index]) <= size);
-            memcpy(dest, src + xstate_offsets[index], xstate_sizes[index]);
-        }
+        /*
+         * We previously verified xstate_bv.  If we don't have valid
+         * comp_offset[] information, something is very broken.
+         */
+        BUG_ON(!dest);
+        BUG_ON((xstate_offsets[index] + xstate_sizes[index]) <= size);
+        memcpy(dest, src + xstate_offsets[index], xstate_sizes[index]);
 
         valid &= ~feature;
     }
-- 
2.1.4


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