x86/xstate: Fix latent bugs in compress_xsave_states()

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.

Simplify the decompression logic by inlining get_xsave_addr().  As xstate_bv
is previously validated, dest won't ever been NULL.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>

diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index 6739c8310b47eba4968e8f734d1360e14494b7f2..85a0116f5ec55af8d4361f2151b95a2dfd0e08ce 100644 (file)
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -154,15 +154,6 @@ static void setup_xstate_comp(uint16_t *comp_offsets,
     ASSERT(offset <= xsave_cntxt_size);
 }
 
-static void *get_xsave_addr(struct xsave_struct *xsave,
-                            const uint16_t *comp_offsets,
-                            unsigned int xfeature_idx)
-{
-    ASSERT(xsave_area_compressed(xsave));
-    return (1ul << xfeature_idx) & xsave->xsave_hdr.xstate_bv ?
-           (void *)xsave + comp_offsets[xfeature_idx] : NULL;
-}
-
 /*
  * Serialise a vcpus xsave state into a representation suitable for the
  * toolstack.
@@ -229,15 +220,29 @@ 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;
+    void *dest;
     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));
     ASSERT(!xsave_area_compressed(src));
 
+    xstate_bv = ((const struct xsave_struct *)src)->xsave_hdr.xstate_bv;
+
     if ( !(v->arch.xcr0_accum & XSTATE_XSAVES_ONLY) )
     {
         memcpy(xsave, src, size);
@@ -260,18 +265,22 @@ void compress_xsave_states(struct vcpu *v, const void *src, unsigned int size)
      * Copy each region from the non-compacted offset to the
      * possibly compacted offset.
      */
+    dest = xsave;
     valid = xstate_bv & ~XSTATE_FP_SSE;
     while ( valid )
     {
         u64 feature = valid & -valid;
         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(!comp_offsets[index]);
+        BUG_ON((xstate_offsets[index] + xstate_sizes[index]) > size);
+
+        memcpy(dest + comp_offsets[index], src + xstate_offsets[index],
+               xstate_sizes[index]);
 
         valid &= ~feature;
     }