xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: "Jan Beulich" <JBeulich@suse.com>
To: xen-devel <xen-devel@lists.xenproject.org>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
	Wei Liu <wei.liu2@citrix.com>,
	Shuai Ruan <shuai.ruan@linux.intel.com>
Subject: [PATCH] x86/xstate: don't clobber or leak state when using XSAVES
Date: Mon, 25 Apr 2016 01:07:54 -0600	[thread overview]
Message-ID: <571DDE6A02000078000E51D3@prv-mh.provo.novell.com> (raw)

[-- Attachment #1: Type: text/plain, Size: 4006 bytes --]

Commit 4d27280572 ("x86/xsaves: fix overwriting between non-lazy/lazy
xsaves") switched to always saving full state when using compacted
format (which is the only one XSAVES allows). It didn't, however, also
adjust the restore side: In order to save full state, we also need to
make sure we always load full state, or else the subject vCPU's state
would get clobbered by that of the vCPU which happened to last have in
use the respective component(s).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
As to inclusion in 4.7: This is a fiy to a _latent_ bug, i.e. not one
currently exposed (since XSTATE_XSAVES_ONLY is zero right now).
Nevertheless I think we should avoid releasing code with such an issue.

--- a/xen/arch/x86/i387.c
+++ b/xen/arch/x86/i387.c
@@ -129,13 +129,8 @@ static inline uint64_t vcpu_xsave_mask(c
      * has ever used lazy states (checking xcr0_accum excluding
      * XSTATE_FP_SSE), vcpu_xsave_mask will return XSTATE_ALL. Otherwise
      * return XSTATE_NONLAZY.
-     * XSTATE_FP_SSE may be excluded, because the offsets of XSTATE_FP_SSE
-     * (in the legacy region of xsave area) are fixed, so saving
-     * XSTATE_FP_SSE will not cause overwriting problem.
      */
-    return (v->arch.xsave_area->xsave_hdr.xcomp_bv & XSTATE_COMPACTION_ENABLED)
-           && (v->arch.xcr0_accum & XSTATE_LAZY & ~XSTATE_FP_SSE)
-           ? XSTATE_ALL : XSTATE_NONLAZY;
+    return xstate_all(v) ? XSTATE_ALL : XSTATE_NONLAZY;
 }
 
 /* Save x87 extended state */
@@ -215,11 +210,26 @@ void vcpu_restore_fpu_eager(struct vcpu
 {
     ASSERT(!is_idle_vcpu(v));
     
-    /* save the nonlazy extended state which is not tracked by CR0.TS bit */
-    if ( v->arch.nonlazy_xstate_used )
+    /* Restore nonlazy extended state (i.e. parts not tracked by CR0.TS). */
+    if ( !v->arch.nonlazy_xstate_used )
+        return;
+
+    /* Avoid recursion */
+    clts();
+
+    /*
+     * When saving full state even with !v->fpu_dirtied (see vcpu_xsave_mask()
+     * above) we also need to restore full state, to prevent subsequently
+     * saving state belonging to another vCPU.
+     */
+    if ( xstate_all(v) )
+    {
+        fpu_xrstor(v, XSTATE_ALL);
+        v->fpu_initialised = 1;
+        v->fpu_dirtied = 1;
+    }
+    else
     {
-        /* Avoid recursion */
-        clts();        
         fpu_xrstor(v, XSTATE_NONLAZY);
         stts();
     }
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -681,6 +681,14 @@ int handle_xsetbv(u32 index, u64 new_bv)
         clts();
         if ( curr->fpu_dirtied )
             asm ( "stmxcsr %0" : "=m" (curr->arch.xsave_area->fpu_sse.mxcsr) );
+        else if ( xstate_all(curr) )
+        {
+            /* See the comment in i387.c:vcpu_restore_fpu_eager(). */
+            mask |= XSTATE_LAZY;
+            curr->fpu_initialised = 1;
+            curr->fpu_dirtied = 1;
+            cr0 &= ~X86_CR0_TS;
+        }
         xrstor(curr, mask);
         if ( cr0 & X86_CR0_TS )
             write_cr0(cr0);
--- a/xen/include/asm-x86/xstate.h
+++ b/xen/include/asm-x86/xstate.h
@@ -8,7 +8,7 @@
 #ifndef __ASM_XSTATE_H
 #define __ASM_XSTATE_H
 
-#include <xen/types.h>
+#include <xen/sched.h>
 #include <asm/cpufeature.h>
 
 #define FCW_DEFAULT               0x037f
@@ -107,4 +107,16 @@ int xstate_alloc_save_area(struct vcpu *
 void xstate_init(struct cpuinfo_x86 *c);
 unsigned int xstate_ctxt_size(u64 xcr0);
 
+static inline bool_t xstate_all(const struct vcpu *v)
+{
+    /*
+     * XSTATE_FP_SSE may be excluded, because the offsets of XSTATE_FP_SSE
+     * (in the legacy region of xsave area) are fixed, so saving
+     * XSTATE_FP_SSE will not cause overwriting problem with XSAVES/XSAVEC.
+     */
+    return (v->arch.xsave_area->xsave_hdr.xcomp_bv &
+            XSTATE_COMPACTION_ENABLED) &&
+           (v->arch.xcr0_accum & XSTATE_LAZY & ~XSTATE_FP_SSE);
+}
+
 #endif /* __ASM_XSTATE_H */




[-- Attachment #2: x86-XSAVES-save-restore-all.patch --]
[-- Type: text/plain, Size: 4061 bytes --]

x86/xstate: don't clobber or leak state when using XSAVES

Commit 4d27280572 ("x86/xsaves: fix overwriting between non-lazy/lazy
xsaves") switched to always saving full state when using compacted
format (which is the only one XSAVES allows). It didn't, however, also
adjust the restore side: In order to save full state, we also need to
make sure we always load full state, or else the subject vCPU's state
would get clobbered by that of the vCPU which happened to last have in
use the respective component(s).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
As to inclusion in 4.7: This is a fiy to a _latent_ bug, i.e. not one
currently exposed (since XSTATE_XSAVES_ONLY is zero right now).
Nevertheless I think we should avoid releasing code with such an issue.

--- a/xen/arch/x86/i387.c
+++ b/xen/arch/x86/i387.c
@@ -129,13 +129,8 @@ static inline uint64_t vcpu_xsave_mask(c
      * has ever used lazy states (checking xcr0_accum excluding
      * XSTATE_FP_SSE), vcpu_xsave_mask will return XSTATE_ALL. Otherwise
      * return XSTATE_NONLAZY.
-     * XSTATE_FP_SSE may be excluded, because the offsets of XSTATE_FP_SSE
-     * (in the legacy region of xsave area) are fixed, so saving
-     * XSTATE_FP_SSE will not cause overwriting problem.
      */
-    return (v->arch.xsave_area->xsave_hdr.xcomp_bv & XSTATE_COMPACTION_ENABLED)
-           && (v->arch.xcr0_accum & XSTATE_LAZY & ~XSTATE_FP_SSE)
-           ? XSTATE_ALL : XSTATE_NONLAZY;
+    return xstate_all(v) ? XSTATE_ALL : XSTATE_NONLAZY;
 }
 
 /* Save x87 extended state */
@@ -215,11 +210,26 @@ void vcpu_restore_fpu_eager(struct vcpu
 {
     ASSERT(!is_idle_vcpu(v));
     
-    /* save the nonlazy extended state which is not tracked by CR0.TS bit */
-    if ( v->arch.nonlazy_xstate_used )
+    /* Restore nonlazy extended state (i.e. parts not tracked by CR0.TS). */
+    if ( !v->arch.nonlazy_xstate_used )
+        return;
+
+    /* Avoid recursion */
+    clts();
+
+    /*
+     * When saving full state even with !v->fpu_dirtied (see vcpu_xsave_mask()
+     * above) we also need to restore full state, to prevent subsequently
+     * saving state belonging to another vCPU.
+     */
+    if ( xstate_all(v) )
+    {
+        fpu_xrstor(v, XSTATE_ALL);
+        v->fpu_initialised = 1;
+        v->fpu_dirtied = 1;
+    }
+    else
     {
-        /* Avoid recursion */
-        clts();        
         fpu_xrstor(v, XSTATE_NONLAZY);
         stts();
     }
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -681,6 +681,14 @@ int handle_xsetbv(u32 index, u64 new_bv)
         clts();
         if ( curr->fpu_dirtied )
             asm ( "stmxcsr %0" : "=m" (curr->arch.xsave_area->fpu_sse.mxcsr) );
+        else if ( xstate_all(curr) )
+        {
+            /* See the comment in i387.c:vcpu_restore_fpu_eager(). */
+            mask |= XSTATE_LAZY;
+            curr->fpu_initialised = 1;
+            curr->fpu_dirtied = 1;
+            cr0 &= ~X86_CR0_TS;
+        }
         xrstor(curr, mask);
         if ( cr0 & X86_CR0_TS )
             write_cr0(cr0);
--- a/xen/include/asm-x86/xstate.h
+++ b/xen/include/asm-x86/xstate.h
@@ -8,7 +8,7 @@
 #ifndef __ASM_XSTATE_H
 #define __ASM_XSTATE_H
 
-#include <xen/types.h>
+#include <xen/sched.h>
 #include <asm/cpufeature.h>
 
 #define FCW_DEFAULT               0x037f
@@ -107,4 +107,16 @@ int xstate_alloc_save_area(struct vcpu *
 void xstate_init(struct cpuinfo_x86 *c);
 unsigned int xstate_ctxt_size(u64 xcr0);
 
+static inline bool_t xstate_all(const struct vcpu *v)
+{
+    /*
+     * XSTATE_FP_SSE may be excluded, because the offsets of XSTATE_FP_SSE
+     * (in the legacy region of xsave area) are fixed, so saving
+     * XSTATE_FP_SSE will not cause overwriting problem with XSAVES/XSAVEC.
+     */
+    return (v->arch.xsave_area->xsave_hdr.xcomp_bv &
+            XSTATE_COMPACTION_ENABLED) &&
+           (v->arch.xcr0_accum & XSTATE_LAZY & ~XSTATE_FP_SSE);
+}
+
 #endif /* __ASM_XSTATE_H */

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

             reply	other threads:[~2016-04-25  7:07 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-04-25  7:07 Jan Beulich [this message]
2016-04-25 11:39 ` [PATCH] x86/xstate: don't clobber or leak state when using XSAVES Wei Liu
2016-04-26  9:25 ` Andrew Cooper
2016-04-29  1:21 ` Shuai Ruan
     [not found] ` <20160429012139.GA4359@shuai.ruan@linux.intel.com>
2016-04-29  9:13   ` Wei Liu
2016-05-05  1:20     ` Shuai Ruan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=571DDE6A02000078000E51D3@prv-mh.provo.novell.com \
    --to=jbeulich@suse.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=shuai.ruan@linux.intel.com \
    --cc=wei.liu2@citrix.com \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).