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

[PATCH 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.

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       | 20 ++++++++++++--
 xen/arch/x86/xstate.c        | 64 ++++++++++++++++++++++++++++++++------------
 xen/include/asm-x86/xstate.h |  6 +++++
 4 files changed, 112 insertions(+), 40 deletions(-)

-- 
2.1.4


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

[PATCH 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>
---
CC: 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 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 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>
---
CC: 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 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>
---
CC: 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 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.  The current xstate_bv will be less than
xcr0_accum if some bits of xsave state are in their default values.  Work
around this by zeroing the whole rest of the buffer before decompression.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/xstate.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index 6e4a0d3..1973ba0 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -169,6 +169,17 @@ 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;
@@ -176,6 +187,11 @@ void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
     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 +205,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;
 
@@ -205,11 +222,9 @@ void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
 
         if ( src )
         {
-            ASSERT((xstate_offsets[index] + xstate_sizes[index]) <= size);
+            BUG_ON((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]);
 
         valid &= ~feature;
     }
-- 
2.1.4


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

[PATCH 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 equivielent 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>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/arch/x86/domctl.c        | 12 +++++++++---
 xen/arch/x86/hvm/hvm.c       | 20 ++++++++++++++++++--
 xen/arch/x86/xstate.c        |  6 ------
 xen/include/asm-x86/xstate.h |  6 ++++++
 4 files changed, 33 insertions(+), 11 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..a6c6621 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1345,6 +1345,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 +1370,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 1973ba0..f6157f5 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..a8b8751 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 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.

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@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Jan Beulich]

>>> On 12.09.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
> 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-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Jan Beulich]

>>> On 12.09.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
> 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 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.

To be honest, I'm of two minds here. Part of me thinks this is an
okay change. However, in particular ...

> --- 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 )

... the relaxation from != to < looks somewhat fragile to me, going
forward. Did you consider dealing with the issue in the tool stack? It
can't be that hard to repeat the size query in case data retrieval fails.
Such retry logic would be well bounded in terms of iterations it can
potentially take. In fact ...

> @@ -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;

... if this got written on the earlier error path, there wouldn't even
be a need to retry the size query: Data retrieval could be retried
with the new size right after enlarging the buffer.

Jan


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

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

[Jan Beulich]

>>> On 12.09.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
> 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-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Jan Beulich]

>>> On 12.09.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
> @@ -176,6 +187,11 @@ void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
>      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));

Further down I see you convert an ASSERT() to BUG_ON(), but I
wonder why you do that and why the two above can't be ASSERT()
too. xstate_ctxt_size() is not always cheap.

> @@ -189,6 +205,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;
>  
> @@ -205,11 +222,9 @@ void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
>  
>          if ( src )
>          {
> -            ASSERT((xstate_offsets[index] + xstate_sizes[index]) <= size);
> +            BUG_ON((xstate_offsets[index] + xstate_sizes[index]) <= size);

Surely converting an ASSERT() to BUG_ON() means inverting the
relational operator used?

>              memcpy(dest + xstate_offsets[index], src, xstate_sizes[index]);
>          }
> -        else
> -            memset(dest + xstate_offsets[index], 0, xstate_sizes[index]);

So I have difficulty seeing why this memset() wasn't sufficient: It
precisely covers for the respective component being in default
state. Or wait - this was fine if intermediate bits were clear in
xstate_bv, but not if clear-but-valid ones weren't followed by
another set one. Nor would gaps between components have been
taken care of. I think the commit message could be made more
explicit in this regard (of course unless I'm overlooking yet another
aspect).

Jan


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

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

[Andrew Cooper]

On 12/09/16 12:17, Jan Beulich wrote:
>>>> On 12.09.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>> 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 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.
> To be honest, I'm of two minds here. Part of me thinks this is an
> okay change. However, in particular ...
>
>> --- 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 )
> ... the relaxation from != to < looks somewhat fragile to me, going
> forward.

What is fragile about it?  It is a very common pattern in general, and
we use it elsewhere.

> Did you consider dealing with the issue in the tool stack?

Yes, and rejected doing so.

> It can't be that hard to repeat the size query in case data retrieval fails.

In principle, this is true if the size case was distinguishable from all
the other cases which currently return -EINVAL.

> Such retry logic would be well bounded in terms of iterations it can
> potentially take. In fact ...
>
>> @@ -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;
> ... if this got written on the earlier error path, there wouldn't even
> be a need to retry the size query: Data retrieval could be retried
> with the new size right after enlarging the buffer.

True, but userspace hypercalls are expensive for the domain in question,
and take a global spinlock in Xen which is expensive for the system as a
whole.  Looking forward to PVH control domains, any hypercall will be
far more expensive (a vmexit/entry vs syscall/sysret).

As such, I am not happy introducing unnecessary hypercalls, as they are
entirely detrimental to dom0 and Xen.

~Andrew

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

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

[Jan Beulich]

>>> On 12.09.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
> --- 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. */

Can't this be moved ahead of the xsave_area_compressed() check,
eliminating (as redundant) the check that's there right now?

In any event the tightening to != you do here supports my desire to
not do any relaxation of the size check in patch 2. Or alternatively
you would want to consider restoring the behavior from prior to
the xsaves change, where the <= which you now remove was still
okay.

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1345,6 +1345,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;

This is slightly ugly, as it will prevent eventually constifying dest (which
it really should have been from the beginning).

> --- 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)

This is certainly odd indentation.

Jan


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

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

[Jan Beulich]

>>> On 12.09.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>  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));

Same remark here as on the earlier patch wrt BUG_ON() vs ASSERT().

> +    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);

I don't think this is a valid concern of yours: The function can't be
used to restore features not xcr0_accum anyway (or else that
field would need updating). Afaict validate_xstate() already prevents
this as intended.

> @@ -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);

Are you sure? In compressed format bits in xstate_bv may be clear
while xcomp_bv has them set. In such a case there's nowhere to
copy to, and hence dest is NULL when coming here.

Jan



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

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

[Andrew Cooper]

On 12/09/16 12:41, Jan Beulich wrote:
>>>> On 12.09.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>> @@ -176,6 +187,11 @@ void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
>>      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));
> Further down I see you convert an ASSERT() to BUG_ON(), but I
> wonder why you do that and why the two above can't be ASSERT()
> too. xstate_ctxt_size() is not always cheap.

This isn't a fastpath, and the cpuid work will make xstate_ctxt_size()
into an O(1) operation.

Furthermore, following the investigation of XSA-186, I will not use
assertions for bounds checking.  The potential damage of omitting the
check far outweighs the overhead of the unconditional check.

>
>> @@ -189,6 +205,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;
>>  
>> @@ -205,11 +222,9 @@ void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
>>  
>>          if ( src )
>>          {
>> -            ASSERT((xstate_offsets[index] + xstate_sizes[index]) <= size);
>> +            BUG_ON((xstate_offsets[index] + xstate_sizes[index]) <= size);
> Surely converting an ASSERT() to BUG_ON() means inverting the
> relational operator used?

Very true.  It is unfortunate that all of this is dead code, and
impossible to test.  I also had half a mind to explicitly #if 0 it out
to leave people in no illusion that it ever might have been tested.

>
>>              memcpy(dest + xstate_offsets[index], src, xstate_sizes[index]);
>>          }
>> -        else
>> -            memset(dest + xstate_offsets[index], 0, xstate_sizes[index]);
> So I have difficulty seeing why this memset() wasn't sufficient: It
> precisely covers for the respective component being in default
> state.

No it doesn't.  The loop skips over all bits which are not set in xstate_bv.

I had (erroneously) come to the conclusion that the "if ( src )" check
only caught the case where we had bad comp_offsets[] information, but
rereading the logic, that case would actually corrupt the legacy SSE header.

Overall, it turns out that the "if ( src )" is unconditionally taken.

~Andrew

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

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

[Jan Beulich]

>>> On 12.09.16 at 14:02, <andrew.cooper3@citrix.com> wrote:
> On 12/09/16 12:17, Jan Beulich wrote:
>>>>> On 12.09.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>>> 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 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.
>> To be honest, I'm of two minds here. Part of me thinks this is an
>> okay change. However, in particular ...
>>
>>> --- 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 )
>> ... the relaxation from != to < looks somewhat fragile to me, going
>> forward.
> 
> What is fragile about it?  It is a very common pattern in general, and
> we use it elsewhere.

If we get handed too large a buffer, what meaning do you want to
assign to the extra bytes? Them being meaningless is just one
possible interpretation.

>> Did you consider dealing with the issue in the tool stack?
> 
> Yes, and rejected doing so.
> 
>> It can't be that hard to repeat the size query in case data retrieval fails.
> 
> In principle, this is true if the size case was distinguishable from all
> the other cases which currently return -EINVAL.

It easily is, as long as the caller know what size it used on the most
recent earlier call (since it would then notice it having grown).

>>> @@ -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;
>> ... if this got written on the earlier error path, there wouldn't even
>> be a need to retry the size query: Data retrieval could be retried
>> with the new size right after enlarging the buffer.
> 
> True, but userspace hypercalls are expensive for the domain in question,
> and take a global spinlock in Xen which is expensive for the system as a
> whole.  Looking forward to PVH control domains, any hypercall will be
> far more expensive (a vmexit/entry vs syscall/sysret).
> 
> As such, I am not happy introducing unnecessary hypercalls, as they are
> entirely detrimental to dom0 and Xen.

Let's be realistic: How often do you expect xcr0_accum to change
in practice between the size query and the data retrieval? I'm
perfectly fine fixing the theoretical issue, but I don't think there's
a performance aspect to be considered when deciding how to deal
with it.

Jan

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

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

[Jan Beulich]

>>> On 12.09.16 at 14:29, <andrew.cooper3@citrix.com> wrote:
> On 12/09/16 12:41, Jan Beulich wrote:
>>>>> On 12.09.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>>> @@ -205,11 +222,9 @@ void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
>>>  
>>>          if ( src )
>>>          {
>>> -            ASSERT((xstate_offsets[index] + xstate_sizes[index]) <= size);
>>> +            BUG_ON((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]);
>> So I have difficulty seeing why this memset() wasn't sufficient: It
>> precisely covers for the respective component being in default
>> state.
> 
> No it doesn't.  The loop skips over all bits which are not set in xstate_bv.

Well, yes, I had corrected myself in the following sentence, resulting
in me just asking for the commit message to get clarified.

> I had (erroneously) come to the conclusion that the "if ( src )" check
> only caught the case where we had bad comp_offsets[] information, but
> rereading the logic, that case would actually corrupt the legacy SSE header.
> 
> Overall, it turns out that the "if ( src )" is unconditionally taken.

Oh, I see (same applies to my then wrong comment on patch 6):
We iterate over xstate_bv here, and components with their flag
set in xstate_bv won't see NULL coming back from get_xsave_addr().
I'm sorry for the noise then.

Jan


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

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

[Jan Beulich]

>>> On 12.09.16 at 14:29, <andrew.cooper3@citrix.com> wrote:
> On 12/09/16 12:41, Jan Beulich wrote:
>>>>> On 12.09.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>>> @@ -205,11 +222,9 @@ void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
>>>  
>>>          if ( src )
>>>          {
>>> -            ASSERT((xstate_offsets[index] + xstate_sizes[index]) <= size);
>>> +            BUG_ON((xstate_offsets[index] + xstate_sizes[index]) <= size);
>> Surely converting an ASSERT() to BUG_ON() means inverting the
>> relational operator used?
> 
> Very true.  It is unfortunate that all of this is dead code, and
> impossible to test.  I also had half a mind to explicitly #if 0 it out
> to leave people in no illusion that it ever might have been tested.

So with this correct, the patch is then
Reviewed-by: Jan Beulich <jbeulich@suse.com>


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

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

[Andrew Cooper]

On 12/09/16 13:14, Jan Beulich wrote:
>>>> On 12.09.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>> --- 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. */
> Can't this be moved ahead of the xsave_area_compressed() check,
> eliminating (as redundant) the check that's there right now?

No.  That doesn't catch the case where _xcr0_accum is 0, which will
cause the xsave_area_compressed(_xsave_area) check to wander off the end
of the buffer.

>
> In any event the tightening to != you do here supports my desire to
> not do any relaxation of the size check in patch 2.

The two cases are different, and should not be conflated.

>  Or alternatively
> you would want to consider restoring the behavior from prior to
> the xsaves change, where the <= which you now remove was still
> okay.

I looked back through the history and couldn't find any justification
for the check being <= as opposed to being more strict.  We absolutely
don't want to be in the case where we only restore part of an xstate
component.

>
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -1345,6 +1345,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;
> This is slightly ugly, as it will prevent eventually constifying dest (which
> it really should have been from the beginning).

Hmm yes.  I hadn't considered that it was actually mutating the hvm
domain context buffer.  I will switch to a shadow variable instead.

>
>> --- 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)
> This is certainly odd indentation.

It is where my editor naturally indents to.  It would appear that emacs
is mistaking the __nonnull(1) as the function name, and presuming that
the following line is K&R style parameters.

It is certainly awkward that the attributes need to be that way around.

~Andrew

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

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

[Andrew Cooper]

On 12/09/16 13:27, Jan Beulich wrote:
>>>> On 12.09.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>>  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));
> Same remark here as on the earlier patch wrt BUG_ON() vs ASSERT().

Same answer.

>
>> +    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);
> I don't think this is a valid concern of yours: The function can't be
> used to restore features not xcr0_accum anyway (or else that
> field would need updating). Afaict validate_xstate() already prevents
> this as intended.

This is all currently dead code.  I guess the question really depends on
what we plan to do with compressed states.

Strictly speaking, no XSAVES state can every be present in xcr0, by
design.  If we retroactively consider xcr0_accum to be "all states in
use", then the if condition in context does become relevant when Xen
starts supporting XSAVES-only components.

In such a case, it is definitely wrong to memcpy() the uncompressed
buffer, as Xen will try and use xrstors and corrupt all guest state.

>
>> @@ -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);
> Are you sure? In compressed format bits in xstate_bv may be clear
> while xcomp_bv has them set. In such a case there's nowhere to
> copy to, and hence dest is NULL when coming here.

This logic was based on my wrong analysis.  However, this loop only
executes for bits set in xstate_bv, so "if ( dest )" will never be false.

~Andrew

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

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

[Andrew Cooper]

On 12/09/16 13:33, Jan Beulich wrote:
>>>> On 12.09.16 at 14:02, <andrew.cooper3@citrix.com> wrote:
>> On 12/09/16 12:17, Jan Beulich wrote:
>>>>>> On 12.09.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>>>> 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 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.
>>> To be honest, I'm of two minds here. Part of me thinks this is an
>>> okay change. However, in particular ...
>>>
>>>> --- 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 )
>>> ... the relaxation from != to < looks somewhat fragile to me, going
>>> forward.
>> What is fragile about it?  It is a very common pattern in general, and
>> we use it elsewhere.
> If we get handed too large a buffer, what meaning do you want to
> assign to the extra bytes? Them being meaningless is just one
> possible interpretation.

I am confused, and I think you are too.  There is no meaning to the
bytes; this is getvcpuextstate, not setvcpuextstate.

All it means is that Xen doesn't need to use all the buffer space
provided by the toolstack to write the current state into.


It is exactly the same concept as read(fd, buf, 4096) returning 1024. 
Only some of the potential buffer was actually filled by the call.

>
>>> Did you consider dealing with the issue in the tool stack?
>> Yes, and rejected doing so.
>>
>>> It can't be that hard to repeat the size query in case data retrieval fails.
>> In principle, this is true if the size case was distinguishable from all
>> the other cases which currently return -EINVAL.
> It easily is, as long as the caller know what size it used on the most
> recent earlier call (since it would then notice it having grown).
>
>>>> @@ -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;
>>> ... if this got written on the earlier error path, there wouldn't even
>>> be a need to retry the size query: Data retrieval could be retried
>>> with the new size right after enlarging the buffer.
>> True, but userspace hypercalls are expensive for the domain in question,
>> and take a global spinlock in Xen which is expensive for the system as a
>> whole.  Looking forward to PVH control domains, any hypercall will be
>> far more expensive (a vmexit/entry vs syscall/sysret).
>>
>> As such, I am not happy introducing unnecessary hypercalls, as they are
>> entirely detrimental to dom0 and Xen.
> Let's be realistic: How often do you expect xcr0_accum to change
> in practice between the size query and the data retrieval?

Very very slim, but not 0.

> I'm perfectly fine fixing the theoretical issue, but I don't think there's
> a performance aspect to be considered when deciding how to deal
> with it.

This doesn't mean we should fix it in an inefficient way.

~Andrew

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

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

[Jan Beulich]

>>> On 12.09.16 at 15:09, <andrew.cooper3@citrix.com> wrote:
> On 12/09/16 13:33, Jan Beulich wrote:
>>>>> On 12.09.16 at 14:02, <andrew.cooper3@citrix.com> wrote:
>>> On 12/09/16 12:17, Jan Beulich wrote:
>>>>>>> On 12.09.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>>>>> 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 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.
>>>> To be honest, I'm of two minds here. Part of me thinks this is an
>>>> okay change. However, in particular ...
>>>>
>>>>> --- 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 )
>>>> ... the relaxation from != to < looks somewhat fragile to me, going
>>>> forward.
>>> What is fragile about it?  It is a very common pattern in general, and
>>> we use it elsewhere.
>> If we get handed too large a buffer, what meaning do you want to
>> assign to the extra bytes? Them being meaningless is just one
>> possible interpretation.
> 
> I am confused, and I think you are too.  There is no meaning to the
> bytes; this is getvcpuextstate, not setvcpuextstate.

Oh, indeed. I'm sorry.

Jan


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

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

[Jan Beulich]

>>> On 12.09.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
> 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 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-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Jan Beulich]

>>> On 12.09.16 at 14:46, <andrew.cooper3@citrix.com> wrote:
> On 12/09/16 13:14, Jan Beulich wrote:
>>>>> On 12.09.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>>> --- 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. */
>> Can't this be moved ahead of the xsave_area_compressed() check,
>> eliminating (as redundant) the check that's there right now?
> 
> No.  That doesn't catch the case where _xcr0_accum is 0, which will
> cause the xsave_area_compressed(_xsave_area) check to wander off the end
> of the buffer.

Oh, indeed.

>> In any event the tightening to != you do here supports my desire to
>> not do any relaxation of the size check in patch 2.
> 
> The two cases are different, and should not be conflated.
> 
>>  Or alternatively
>> you would want to consider restoring the behavior from prior to
>> the xsaves change, where the <= which you now remove was still
>> okay.
> 
> I looked back through the history and couldn't find any justification
> for the check being <= as opposed to being more strict.  We absolutely
> don't want to be in the case where we only restore part of an xstate
> component.

Indeed I don't think there was ever any justification provided, it
just happened to be that way (perhaps having in mind that the
FXSAVE layout had unused space at the end).

>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -1345,6 +1345,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;
>> This is slightly ugly, as it will prevent eventually constifying dest (which
>> it really should have been from the beginning).
> 
> Hmm yes.  I hadn't considered that it was actually mutating the hvm
> domain context buffer.  I will switch to a shadow variable instead.
> 
>>
>>> --- 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)
>> This is certainly odd indentation.
> 
> It is where my editor naturally indents to.  It would appear that emacs
> is mistaking the __nonnull(1) as the function name, and presuming that
> the following line is K&R style parameters.

With these two small things taken care of,
Reviewed-by: Jan Beulich <jbeulich@suse.com>


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

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

[Jan Beulich]

>>> On 12.09.16 at 14:59, <andrew.cooper3@citrix.com> wrote:
> On 12/09/16 13:27, Jan Beulich wrote:
>>>>> On 12.09.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>>>  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));
>> Same remark here as on the earlier patch wrt BUG_ON() vs ASSERT().
> 
> Same answer.

Well, it's certainly a matter of taste how much of the above to consider
bounds checking. I for one would take only the middle one as such.

>>> +    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);
>> I don't think this is a valid concern of yours: The function can't be
>> used to restore features not xcr0_accum anyway (or else that
>> field would need updating). Afaict validate_xstate() already prevents
>> this as intended.
> 
> This is all currently dead code.  I guess the question really depends on
> what we plan to do with compressed states.
> 
> Strictly speaking, no XSAVES state can every be present in xcr0, by
> design.  If we retroactively consider xcr0_accum to be "all states in
> use",

I think that's the only viable model, considering how the domctl works:
xcr0_accum needs to represent the combination of features ever
enabled in XCR0 and XSS.

> then the if condition in context does become relevant when Xen
> starts supporting XSAVES-only components.
> 
> In such a case, it is definitely wrong to memcpy() the uncompressed
> buffer, as Xen will try and use xrstors and corrupt all guest state.

How? If the guest never enabled any bit in XSS, how can any such
bit be set in xstate_bv (which is always a subset of XCR0|XSS).

Jan


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

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

[Andrew Cooper]

On 12/09/16 13:43, Jan Beulich wrote:
>>>> On 12.09.16 at 14:29, <andrew.cooper3@citrix.com> wrote:
>> On 12/09/16 12:41, Jan Beulich wrote:
>>>>>> On 12.09.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>>>> @@ -205,11 +222,9 @@ void expand_xsave_states(struct vcpu *v, void *dest, unsigned int size)
>>>>  
>>>>          if ( src )
>>>>          {
>>>> -            ASSERT((xstate_offsets[index] + xstate_sizes[index]) <= size);
>>>> +            BUG_ON((xstate_offsets[index] + xstate_sizes[index]) <= size);
>>> Surely converting an ASSERT() to BUG_ON() means inverting the
>>> relational operator used?
>> Very true.  It is unfortunate that all of this is dead code, and
>> impossible to test.  I also had half a mind to explicitly #if 0 it out
>> to leave people in no illusion that it ever might have been tested.
> So with this correct, the patch is then
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

The discussion on this patch has shown that the "if ( src )" is
unconditionally true, and as such, I would like to remove it.  Would
your R-b stand with this hunk altered similarly to the final hunk in
patch 6 (with the BUG_ON() logic adjustment, and updated wording) ?

~Andrew

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

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

[Jan Beulich]

>>> On 12.09.16 at 15:57, <andrew.cooper3@citrix.com> wrote:
> On 12/09/16 13:43, Jan Beulich wrote:
>>>>> On 12.09.16 at 14:29, <andrew.cooper3@citrix.com> wrote:
>>> On 12/09/16 12:41, Jan Beulich wrote:
>>>>>>> On 12.09.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>>>>> @@ -205,11 +222,9 @@ void expand_xsave_states(struct vcpu *v, void *dest, 
> unsigned int size)
>>>>>  
>>>>>          if ( src )
>>>>>          {
>>>>> -            ASSERT((xstate_offsets[index] + xstate_sizes[index]) <= size);
>>>>> +            BUG_ON((xstate_offsets[index] + xstate_sizes[index]) <= size);
>>>> Surely converting an ASSERT() to BUG_ON() means inverting the
>>>> relational operator used?
>>> Very true.  It is unfortunate that all of this is dead code, and
>>> impossible to test.  I also had half a mind to explicitly #if 0 it out
>>> to leave people in no illusion that it ever might have been tested.
>> So with this correct, the patch is then
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> The discussion on this patch has shown that the "if ( src )" is
> unconditionally true, and as such, I would like to remove it.  Would
> your R-b stand with this hunk altered similarly to the final hunk in
> patch 6 (with the BUG_ON() logic adjustment, and updated wording) ?

Yes.

Jan


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

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

[Andrew Cooper]

On 12/09/16 14:47, Jan Beulich wrote:
>>>> On 12.09.16 at 14:59, <andrew.cooper3@citrix.com> wrote:
>> On 12/09/16 13:27, Jan Beulich wrote:
>>>>>> On 12.09.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>>>>  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));
>>> Same remark here as on the earlier patch wrt BUG_ON() vs ASSERT().
>> Same answer.
> Well, it's certainly a matter of taste how much of the above to consider
> bounds checking. I for one would take only the middle one as such.

The first is necessary if you want the second, and important as the
calling convention involves the caller modifying xcr0* to match the
incoming state.

The compressed check is admittedly different, but it is currently an ABI
requirement that the data is uncompressed.  This could be lifted if we
alter the ABI.

>
>>>> +    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);
>>> I don't think this is a valid concern of yours: The function can't be
>>> used to restore features not xcr0_accum anyway (or else that
>>> field would need updating). Afaict validate_xstate() already prevents
>>> this as intended.
>> This is all currently dead code.  I guess the question really depends on
>> what we plan to do with compressed states.
>>
>> Strictly speaking, no XSAVES state can every be present in xcr0, by
>> design.  If we retroactively consider xcr0_accum to be "all states in
>> use",
> I think that's the only viable model, considering how the domctl works:
> xcr0_accum needs to represent the combination of features ever
> enabled in XCR0 and XSS.

In which case we should really rename it to xstate_accum then.

>
>> then the if condition in context does become relevant when Xen
>> starts supporting XSAVES-only components.
>>
>> In such a case, it is definitely wrong to memcpy() the uncompressed
>> buffer, as Xen will try and use xrstors and corrupt all guest state.
> How? If the guest never enabled any bit in XSS, how can any such
> bit be set in xstate_bv (which is always a subset of XCR0|XSS).

Currently it cant.  This is a preemptive catch for whomever tries
implementing the first XSS state, and doesn't test migration between
older and newer Xen.

~Andrew

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

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

[Jan Beulich]

>>> On 12.09.16 at 17:28, <andrew.cooper3@citrix.com> wrote:
> On 12/09/16 14:47, Jan Beulich wrote:
>>>>> On 12.09.16 at 14:59, <andrew.cooper3@citrix.com> wrote:
>>> On 12/09/16 13:27, Jan Beulich wrote:
>>>>>>> On 12.09.16 at 11:51, <andrew.cooper3@citrix.com> wrote:
>>>>> +    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);
>>>> I don't think this is a valid concern of yours: The function can't be
>>>> used to restore features not xcr0_accum anyway (or else that
>>>> field would need updating). Afaict validate_xstate() already prevents
>>>> this as intended.
>>> This is all currently dead code.  I guess the question really depends on
>>> what we plan to do with compressed states.
>>>
>>> Strictly speaking, no XSAVES state can every be present in xcr0, by
>>> design.  If we retroactively consider xcr0_accum to be "all states in
>>> use",
>> I think that's the only viable model, considering how the domctl works:
>> xcr0_accum needs to represent the combination of features ever
>> enabled in XCR0 and XSS.
> 
> In which case we should really rename it to xstate_accum then.

Right.

>>> then the if condition in context does become relevant when Xen
>>> starts supporting XSAVES-only components.
>>>
>>> In such a case, it is definitely wrong to memcpy() the uncompressed
>>> buffer, as Xen will try and use xrstors and corrupt all guest state.
>> How? If the guest never enabled any bit in XSS, how can any such
>> bit be set in xstate_bv (which is always a subset of XCR0|XSS).
> 
> Currently it cant.  This is a preemptive catch for whomever tries
> implementing the first XSS state, and doesn't test migration between
> older and newer Xen.

I still don't see why you say "currently" when this is an architectural
(of how Xen behaves, not hardware architecture) thing: xcr0_accum,
as its name says, only ever gets bits added to it, and if some bit is
clear there it can't possibly be set in xstate_bv or xcomp_bv (for
the latter of course with the exception of the compressed bit). So by
having made it past 

     if ( !(v->arch.xcr0_accum & XSTATE_XSAVES_ONLY) )
     {

the BUG_ON() you add can't possibly trigger (or else we have much
more severe problems). In particular this has nothing to do with
whether any XSS states are being used.

Jan


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