[PATCH v2 0/6] Fix multiple issues with xsave state handling on migrate

[PATCH v2 0/6] Fix multiple issues with xsave state handling on migrate

[Andrew Cooper]

Patch 5 is the primary bugfix of this series, which is broken in Xen 4.7 as
well as master.  There are multiple latent security issues which would be
exposed at the point support for the first compressed xsave state was added,
but are in currently-dead code.

Changes from v1:
 * Inline and remove get_xsave_addr().  This simplifies the logic.
 * Whitespace & alignment fixes.
 * Use a shadow variable in hvm_load_cpu_xsave_states()
 * Drop the TODO BUG_ON() from compress_xsave_states()

Andrew Cooper (6):
  x86/domctl: Introduce PV_XSAVE_HDR_SIZE and remove its opencoding
  x86/domctl: Fix TOCTOU race with the use of XEN_DOMCTL_getvcpuextstate
  x86/domctl: Simplfy XEN_DOMCTL_getvcpuextstate when xsave is not in use
  x86/xstate: Fix latent bugs in expand_xsave_states()
  x86/domctl: Fix migration of guests which are not using xsave
  x86/xstate: Fix latent bugs in compress_xsave_states()

 xen/arch/x86/domctl.c        | 62 ++++++++++++++++++++-----------
 xen/arch/x86/hvm/hvm.c       | 27 +++++++++++---
 xen/arch/x86/xstate.c        | 87 ++++++++++++++++++++++++++++----------------
 xen/include/asm-x86/xstate.h |  6 +++
 4 files changed, 124 insertions(+), 58 deletions(-)

--
2.1.4


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

[PATCH v2 1/6] x86/domctl: Introduce PV_XSAVE_HDR_SIZE and remove its opencoding

[Andrew Cooper]

Also remove opencoding of PV_XSAVE_SIZE().  Undefine both when they are
done with.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/x86/domctl.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index a904fd6..c9355ce 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1037,7 +1037,8 @@ long arch_do_domctl(
         struct vcpu *v;
         uint32_t offset = 0;
 
-#define PV_XSAVE_SIZE(xcr0) (2 * sizeof(uint64_t) + xstate_ctxt_size(xcr0))
+#define PV_XSAVE_HDR_SIZE (2 * sizeof(uint64_t))
+#define PV_XSAVE_SIZE(xcr0) (PV_XSAVE_HDR_SIZE + xstate_ctxt_size(xcr0))
 
         ret = -ESRCH;
         if ( (evc->vcpu >= d->max_vcpus) ||
@@ -1093,10 +1094,10 @@ long arch_do_domctl(
                 }
 
                 expand_xsave_states(v, xsave_area,
-                                    size - 2 * sizeof(uint64_t));
+                                    size - PV_XSAVE_HDR_SIZE);
 
                 if ( copy_to_guest_offset(evc->buffer, offset, xsave_area,
-                                          size - 2 * sizeof(uint64_t)) )
+                                          size - PV_XSAVE_HDR_SIZE) )
                      ret = -EFAULT;
                 xfree(xsave_area);
            }
@@ -1110,9 +1111,8 @@ long arch_do_domctl(
             const struct xsave_struct *_xsave_area;
 
             ret = -EINVAL;
-            if ( evc->size < 2 * sizeof(uint64_t) ||
-                 evc->size > 2 * sizeof(uint64_t) +
-                             xstate_ctxt_size(xfeature_mask) )
+            if ( evc->size < PV_XSAVE_HDR_SIZE ||
+                 evc->size > PV_XSAVE_SIZE(xfeature_mask) )
                 goto vcpuextstate_out;
 
             receive_buf = xmalloc_bytes(evc->size);
@@ -1131,11 +1131,11 @@ long arch_do_domctl(
 
             _xcr0 = *(uint64_t *)receive_buf;
             _xcr0_accum = *(uint64_t *)(receive_buf + sizeof(uint64_t));
-            _xsave_area = receive_buf + 2 * sizeof(uint64_t);
+            _xsave_area = receive_buf + PV_XSAVE_HDR_SIZE;
 
             if ( _xcr0_accum )
             {
-                if ( evc->size >= 2 * sizeof(uint64_t) + XSTATE_AREA_MIN_SIZE )
+                if ( evc->size >= PV_XSAVE_HDR_SIZE + XSTATE_AREA_MIN_SIZE )
                     ret = validate_xstate(_xcr0, _xcr0_accum,
                                           &_xsave_area->xsave_hdr);
             }
@@ -1155,7 +1155,7 @@ long arch_do_domctl(
                 if ( _xcr0_accum & XSTATE_NONLAZY )
                     v->arch.nonlazy_xstate_used = 1;
                 compress_xsave_states(v, _xsave_area,
-                                      evc->size - 2 * sizeof(uint64_t));
+                                      evc->size - PV_XSAVE_HDR_SIZE);
                 vcpu_unpause(v);
             }
             else
@@ -1164,6 +1164,9 @@ long arch_do_domctl(
             xfree(receive_buf);
         }
 
+#undef PV_XSAVE_HDR_SIZE
+#undef PV_XSAVE_SIZE
+
     vcpuextstate_out:
         if ( domctl->cmd == XEN_DOMCTL_getvcpuextstate )
             copyback = 1;
-- 
2.1.4


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

[PATCH v2 2/6] x86/domctl: Fix TOCTOU race with the use of XEN_DOMCTL_getvcpuextstate

[Andrew Cooper]

A toolstack must call XEN_DOMCTL_getvcpuextstate twice; first to find the size
of the buffer to use, and a second time to get the actual content.

The reported size was based on v->arch.xcr0_accum, but a guest which extends
its xcr0_accum between the two hypercalls will cause the toolstack to fail the
evc->size != size check, as the provided buffer is now too small.  This causes
a hard error during the final phase of migration.

Instead, return a size based on xfeature_mask, which is the maximum size Xen
will ever permit.  The hypercall must now tolerate a toolstack-provided buffer
which is overly large (for the case where a guest isn't using all available
xsave states), and should write back how much data was actually written into
the buffer.

As the query for size now has no dependence on vcpu state, the vcpu_pause()
can be omitted for a small performance improvement.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/x86/domctl.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index c9355ce..815bd33 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1054,19 +1054,25 @@ long arch_do_domctl(
             unsigned int size;
 
             ret = 0;
-            vcpu_pause(v);
 
-            size = PV_XSAVE_SIZE(v->arch.xcr0_accum);
             if ( (!evc->size && !evc->xfeature_mask) ||
                  guest_handle_is_null(evc->buffer) )
             {
+                /*
+                 * A query for the size of buffer to use.  Must return the
+                 * maximum size we ever might hand back to userspace, bearing
+                 * in mind that the vcpu might increase its xcr0_accum between
+                 * this query for size, and the following query for data.
+                 */
                 evc->xfeature_mask = xfeature_mask;
-                evc->size = size;
-                vcpu_unpause(v);
+                evc->size = PV_XSAVE_SIZE(xfeature_mask);
                 goto vcpuextstate_out;
             }
 
-            if ( evc->size != size || evc->xfeature_mask != xfeature_mask )
+            vcpu_pause(v);
+            size = PV_XSAVE_SIZE(v->arch.xcr0_accum);
+
+            if ( evc->size < size || evc->xfeature_mask != xfeature_mask )
                 ret = -EINVAL;
 
             if ( !ret && copy_to_guest_offset(evc->buffer, offset,
@@ -1103,6 +1109,10 @@ long arch_do_domctl(
            }
 
             vcpu_unpause(v);
+
+            /* Specify how much data we actually wrote into the buffer. */
+            if ( !ret )
+                evc->size = size;
         }
         else
         {
-- 
2.1.4


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

[PATCH v2 3/6] x86/domctl: Simplfy XEN_DOMCTL_getvcpuextstate when xsave is not in use

[Andrew Cooper]

Older guests will not use xsave even if it is available.  As such, their
xcr0_accum will be 0 at the point of migrate.

If it is empty, forgo the memory allocation and serialisation into a
zero-length buffer.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/x86/domctl.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 815bd33..5aa9f3a 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1087,11 +1087,13 @@ long arch_do_domctl(
                 ret = -EFAULT;
 
             offset += sizeof(v->arch.xcr0_accum);
-            if ( !ret )
+
+            /* Serialise xsave state, if there is any. */
+            if ( !ret && size > PV_XSAVE_HDR_SIZE )
             {
-                void *xsave_area;
+                unsigned int xsave_size = size - PV_XSAVE_HDR_SIZE;
+                void *xsave_area = xmalloc_bytes(xsave_size);
 
-                xsave_area = xmalloc_bytes(size);
                 if ( !xsave_area )
                 {
                     ret = -ENOMEM;
@@ -1099,11 +1101,10 @@ long arch_do_domctl(
                     goto vcpuextstate_out;
                 }
 
-                expand_xsave_states(v, xsave_area,
-                                    size - PV_XSAVE_HDR_SIZE);
+                expand_xsave_states(v, xsave_area, xsave_size);
 
                 if ( copy_to_guest_offset(evc->buffer, offset, xsave_area,
-                                          size - PV_XSAVE_HDR_SIZE) )
+                                          xsave_size) )
                      ret = -EFAULT;
                 xfree(xsave_area);
            }
-- 
2.1.4


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

[PATCH v2 4/6] x86/xstate: Fix latent bugs in expand_xsave_states()

[Andrew Cooper]

Without checking the size input, the memcpy() for the uncompressed path might
read off the end of the vcpu's xsave_area.  Both callers pass the approprite
size, so hold them to it with a BUG_ON().

The compressed path is currently dead code, but its attempt to avoid leaking
uninitalised data was incomplete.  Work around this by zeroing the whole rest
of the buffer before decompression.

The loop skips all bits which aren't set in xstate_bv, meaning that the
memset() was dead code.  The logic is more obvious with get_xsave_addr()
expanded inline, allowing for quite a lot of simplification, including all the
NULL pointer logic.

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

v2:
 * get_xsave_addr() expanded inline to simplify the logic substantially.
---
 xen/arch/x86/xstate.c | 36 ++++++++++++++++++++++++++++--------
 1 file changed, 28 insertions(+), 8 deletions(-)

diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index 6e4a0d3..2684190 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -169,13 +169,30 @@ static void *get_xsave_addr(struct xsave_struct *xsave,
            (void *)xsave + comp_offsets[xfeature_idx] : NULL;
 }
 
+/*
+ * Serialise a vcpus xsave state into a representation suitable for the
+ * toolstack.
+ *
+ * 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 there is xsave state to
+ * serialise, and that the provided buffer is exactly the right size.
+ */
 void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
 {
     struct xsave_struct *xsave = v->arch.xsave_area;
+    const void *src;
     uint16_t comp_offsets[sizeof(xfeature_mask)*8];
     u64 xstate_bv = xsave->xsave_hdr.xstate_bv;
     u64 valid;
 
+    /* Check there is state to serialise (i.e. at least an XSAVE_HDR) */
+    BUG_ON(!v->arch.xcr0_accum);
+    /* Check there is the correct room to decompress into. */
+    BUG_ON(size != xstate_ctxt_size(v->arch.xcr0_accum));
+
     if ( !(xsave->xsave_hdr.xcomp_bv & XSTATE_COMPACTION_ENABLED) )
     {
         memcpy(dest, xsave, size);
@@ -189,6 +206,7 @@ void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
      * Copy legacy XSAVE area and XSAVE hdr area.
      */
     memcpy(dest, xsave, XSTATE_AREA_MIN_SIZE);
+    memset(dest + XSTATE_AREA_MIN_SIZE, 0, size - XSTATE_AREA_MIN_SIZE);
 
     ((struct xsave_struct *)dest)->xsave_hdr.xcomp_bv =  0;
 
@@ -196,20 +214,22 @@ void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
      * Copy each region from the possibly compacted offset to the
      * non-compacted offset.
      */
+    src = xsave;
     valid = xstate_bv & ~XSTATE_FP_SSE;
     while ( valid )
     {
         u64 feature = valid & -valid;
         unsigned int index = fls(feature) - 1;
-        const void *src = get_xsave_addr(xsave, comp_offsets, index);
 
-        if ( src )
-        {
-            ASSERT((xstate_offsets[index] + xstate_sizes[index]) <= size);
-            memcpy(dest + xstate_offsets[index], src, xstate_sizes[index]);
-        }
-        else
-            memset(dest + xstate_offsets[index], 0, xstate_sizes[index]);
+        /*
+         * We previously verified xstate_bv.  If there isn't valid
+         * comp_offsets[] information, something is very broken.
+         */
+        BUG_ON(!comp_offsets[index]);
+        BUG_ON((xstate_offsets[index] + xstate_sizes[index]) > size);
+
+        memcpy(dest + xstate_offsets[index], src + comp_offsets[index],
+               xstate_sizes[index]);
 
         valid &= ~feature;
     }
-- 
2.1.4


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

[PATCH v2 5/6] x86/domctl: Fix migration of guests which are not using xsave

[Andrew Cooper]

c/s da62246e "x86/xsaves: enable xsaves/xrstors/xsavec in xen" broke migration
of PV guests which were not using xsave.

In such a case, compress_xsave_states() gets passed a zero length buffer.  The
first thing it tries to do is ASSERT() on user-provided data, if it hadn't
already wandered off the end of the buffer to do so.

Perform more verification of the input buffer before passing it to
compress_xsave_states().  This involves making xsave_area_compressed() public.

Similar problems exist on the HVM side, so make equivalent adjustments there.
This doesn't manifest in general, as hvm_save_cpu_xsave_states() elides the
entire record if xsave isn't used, but is a problem if a caller were to
construct an xsave record manually.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <JBeulich@suse.com>
---
v2:
 * Use a shadow variable in hvm_load_cpu_xsave_states() rather than mutating
   the stream itself.  Make desc const while at it.
 * Alter the alignment of xsave_area_compressed()
---
 xen/arch/x86/domctl.c        | 12 +++++++++---
 xen/arch/x86/hvm/hvm.c       | 27 ++++++++++++++++++++++-----
 xen/arch/x86/xstate.c        |  6 ------
 xen/include/asm-x86/xstate.h |  6 ++++++
 4 files changed, 37 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 5aa9f3a..2a2fe04 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1158,7 +1158,15 @@ long arch_do_domctl(
                 goto vcpuextstate_out;
             }
 
-            if ( evc->size <= PV_XSAVE_SIZE(_xcr0_accum) )
+            if ( evc->size == PV_XSAVE_HDR_SIZE )
+                ; /* Nothing to restore. */
+            else if ( evc->size < PV_XSAVE_HDR_SIZE + XSTATE_AREA_MIN_SIZE )
+                ret = -EINVAL; /* Can't be legitimate data. */
+            else if ( xsave_area_compressed(_xsave_area) )
+                ret = -EOPNOTSUPP; /* Don't support compressed data. */
+            else if ( evc->size != PV_XSAVE_SIZE(_xcr0_accum) )
+                ret = -EINVAL; /* Not legitimate data. */
+            else
             {
                 vcpu_pause(v);
                 v->arch.xcr0 = _xcr0;
@@ -1169,8 +1177,6 @@ long arch_do_domctl(
                                       evc->size - PV_XSAVE_HDR_SIZE);
                 vcpu_unpause(v);
             }
-            else
-                ret = -EINVAL;
 
             xfree(receive_buf);
         }
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index ca96643..7bad845 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1265,8 +1265,8 @@ static int hvm_load_cpu_xsave_states(struct domain *d, hvm_domain_context_t *h)
     int err;
     struct vcpu *v;
     struct hvm_hw_cpu_xsave *ctxt;
-    struct hvm_save_descriptor *desc;
-    unsigned int i, desc_start;
+    const struct hvm_save_descriptor *desc;
+    unsigned int i, desc_start, desc_length;
 
     /* Which vcpu is this? */
     vcpuid = hvm_load_instance(h);
@@ -1325,7 +1325,8 @@ static int hvm_load_cpu_xsave_states(struct domain *d, hvm_domain_context_t *h)
         return err;
     }
     size = HVM_CPU_XSAVE_SIZE(ctxt->xcr0_accum);
-    if ( desc->length > size )
+    desc_length = desc->length;
+    if ( desc_length > size )
     {
         /*
          * Xen 4.3.0, 4.2.3 and older used to send longer-than-needed
@@ -1345,6 +1346,23 @@ static int hvm_load_cpu_xsave_states(struct domain *d, hvm_domain_context_t *h)
         printk(XENLOG_G_WARNING
                "HVM%d.%u restore mismatch: xsave length %#x > %#x\n",
                d->domain_id, vcpuid, desc->length, size);
+        /* Rewind desc_length to ignore the extraneous zeros. */
+        desc_length = size;
+    }
+
+    if ( xsave_area_compressed((const void *)&ctxt->save_area) )
+    {
+        printk(XENLOG_G_WARNING
+               "HVM%d.%u restore: compressed xsave state not supported\n",
+               d->domain_id, vcpuid);
+        return -EOPNOTSUPP;
+    }
+    else if ( desc_length != size )
+    {
+        printk(XENLOG_G_WARNING
+               "HVM%d.%u restore mismatch: xsave length %#x != %#x\n",
+               d->domain_id, vcpuid, desc_length, size);
+        return -EINVAL;
     }
     /* Checking finished */
 
@@ -1353,8 +1371,7 @@ static int hvm_load_cpu_xsave_states(struct domain *d, hvm_domain_context_t *h)
     if ( ctxt->xcr0_accum & XSTATE_NONLAZY )
         v->arch.nonlazy_xstate_used = 1;
     compress_xsave_states(v, &ctxt->save_area,
-                          min(desc->length, size) -
-                          offsetof(struct hvm_hw_cpu_xsave,save_area));
+                          size - offsetof(struct hvm_hw_cpu_xsave, save_area));
 
     return 0;
 }
diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index 2684190..ed9c4c7 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -86,12 +86,6 @@ uint64_t get_msr_xss(void)
     return this_cpu(xss);
 }
 
-static bool_t xsave_area_compressed(const struct xsave_struct *xsave_area)
-{
-     return xsave_area && (xsave_area->xsave_hdr.xcomp_bv
-                           & XSTATE_COMPACTION_ENABLED);
-}
-
 static int setup_xstate_features(bool_t bsp)
 {
     unsigned int leaf, eax, ebx, ecx, edx;
diff --git a/xen/include/asm-x86/xstate.h b/xen/include/asm-x86/xstate.h
index 51a9ed4..45b2a9b 100644
--- a/xen/include/asm-x86/xstate.h
+++ b/xen/include/asm-x86/xstate.h
@@ -131,4 +131,10 @@ static inline bool_t xstate_all(const struct vcpu *v)
            (v->arch.xcr0_accum & XSTATE_LAZY & ~XSTATE_FP_SSE);
 }
 
+static inline bool __nonnull(1)
+xsave_area_compressed(const struct xsave_struct *xsave_area)
+{
+    return xsave_area->xsave_hdr.xcomp_bv & XSTATE_COMPACTION_ENABLED;
+}
+
 #endif /* __ASM_XSTATE_H */
-- 
2.1.4


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

[PATCH v2 6/6] x86/xstate: Fix latent bugs in compress_xsave_states()

[Andrew Cooper]

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>
---
CC: Jan Beulich <JBeulich@suse.com>

v2:
 * Inline get_xsave_addr() to simplify the logic
 * Drop the TODO
---
 xen/arch/x86/xstate.c | 45 +++++++++++++++++++++++++++------------------
 1 file changed, 27 insertions(+), 18 deletions(-)

diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index ed9c4c7..9be98e6 100644
--- 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,14 +220,28 @@ 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;
 
-    ASSERT(!xsave_area_compressed(src));
+    BUG_ON(!v->arch.xcr0_accum);
+    BUG_ON(size != xstate_ctxt_size(v->arch.xcr0_accum));
+    BUG_ON(xsave_area_compressed(src));
+
+    xstate_bv = ((const struct xsave_struct *)src)->xsave_hdr.xstate_bv;
 
     if ( !(v->arch.xcr0_accum & XSTATE_XSAVES_ONLY) )
     {
@@ -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;
     }
-- 
2.1.4


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

Re: [PATCH v2 4/6] x86/xstate: Fix latent bugs in expand_xsave_states()

[Jan Beulich]

>>> On 12.09.16 at 18:21, <andrew.cooper3@citrix.com> wrote:
> Without checking the size input, the memcpy() for the uncompressed path might
> read off the end of the vcpu's xsave_area.  Both callers pass the approprite
> size, so hold them to it with a BUG_ON().
> 
> The compressed path is currently dead code, but its attempt to avoid leaking
> uninitalised data was incomplete.  Work around this by zeroing the whole rest
> of the buffer before decompression.
> 
> The loop skips all bits which aren't set in xstate_bv, meaning that the
> memset() was dead code.  The logic is more obvious with get_xsave_addr()
> expanded inline, allowing for quite a lot of simplification, including all the
> NULL pointer logic.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <JBeulich@suse.com>
with one suggestion:

>  void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
>  {
>      struct xsave_struct *xsave = v->arch.xsave_area;
> +    const void *src;

I think with the addition of this variable and the removal of the use of
get_xsave_addr() "xsave" can now also be const.

Jan


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

Re: [PATCH v2 6/6] x86/xstate: Fix latent bugs in compress_xsave_states()

[Jan Beulich]

>>> On 12.09.16 at 18:21, <andrew.cooper3@citrix.com> wrote:
> 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>
albeit, as expressed before I would prefer ...

>  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;
>  
> -    ASSERT(!xsave_area_compressed(src));
> +    BUG_ON(!v->arch.xcr0_accum);
> +    BUG_ON(size != xstate_ctxt_size(v->arch.xcr0_accum));
> +    BUG_ON(xsave_area_compressed(src));

... if at least the ASSERT() remained one.

Jan


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

Re: [PATCH v2 4/6] x86/xstate: Fix latent bugs in expand_xsave_states()

[Andrew Cooper]

On 13/09/16 09:23, Jan Beulich wrote:
>>>> On 12.09.16 at 18:21, <andrew.cooper3@citrix.com> wrote:
>> Without checking the size input, the memcpy() for the uncompressed path might
>> read off the end of the vcpu's xsave_area.  Both callers pass the approprite
>> size, so hold them to it with a BUG_ON().
>>
>> The compressed path is currently dead code, but its attempt to avoid leaking
>> uninitalised data was incomplete.  Work around this by zeroing the whole rest
>> of the buffer before decompression.
>>
>> The loop skips all bits which aren't set in xstate_bv, meaning that the
>> memset() was dead code.  The logic is more obvious with get_xsave_addr()
>> expanded inline, allowing for quite a lot of simplification, including all the
>> NULL pointer logic.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <JBeulich@suse.com>
> with one suggestion:
>
>>  void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
>>  {
>>      struct xsave_struct *xsave = v->arch.xsave_area;
>> +    const void *src;
> I think with the addition of this variable and the removal of the use of
> get_xsave_addr() "xsave" can now also be const.

So it can.  Done.

~Andrew

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

Re: [PATCH v2 6/6] x86/xstate: Fix latent bugs in compress_xsave_states()

[Andrew Cooper]

On 13/09/16 09:27, Jan Beulich wrote:
>>>> On 12.09.16 at 18:21, <andrew.cooper3@citrix.com> wrote:
>> 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>
> albeit, as expressed before I would prefer ...
>
>>  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;
>>  
>> -    ASSERT(!xsave_area_compressed(src));
>> +    BUG_ON(!v->arch.xcr0_accum);
>> +    BUG_ON(size != xstate_ctxt_size(v->arch.xcr0_accum));
>> +    BUG_ON(xsave_area_compressed(src));
> ... if at least the ASSERT() remained one.

Hmm fine.  At least that is only data corruption issue, not an buffer
boundary issue.

~Andrew

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