[PATCH 0/3] Fix small issues in XSAVES

[PATCH 0/3] Fix small issues in XSAVES

[Yu-cheng Yu]

The first two patches in this series are split from my supervisor xstate
patches [1].  The third is to fix a vital issue in __fpu_restore_sig(),
and more RFC than the others.  All three are not directly related to
supervisor xstates or CET, split them out and submit first.  I will
re-submit supervisor xstate patches shortly.

When__fpu_restore_sig() fails, partially cleared FPU registers still belong
to the previous owner task.  That causes that task to use corrupted xregs.
Fix it by doing __cpu_invalidate_fpregs_state() in functions that copy into
fpregs.  Further details are in the commit log of patch #3.

[1] Support XSAVES supervisor states
    https://lkml.kernel.org/r/20190925151022.21688-1-yu-cheng.yu@intel.com/

[2] CET patches:
    https://lkml.kernel.org/r/20190813205225.12032-1-yu-cheng.yu@intel.com/
    https://lkml.kernel.org/r/20190813205359.12196-1-yu-cheng.yu@intel.com/

Yu-cheng Yu (3):
  x86/fpu/xstate: Fix small issues before adding supervisor xstates
  x86/fpu/xstate: Make xfeature_is_supervisor()/xfeature_is_user()
    return bool
  x86/fpu/xstate: Invalidate fpregs when __fpu_restore_sig() fails

 arch/x86/include/asm/fpu/internal.h | 14 ++++++++++++++
 arch/x86/kernel/fpu/core.c          | 15 +++++++++++++--
 arch/x86/kernel/fpu/xstate.c        | 22 ++++++++++------------
 3 files changed, 37 insertions(+), 14 deletions(-)

-- 
2.17.1

[PATCH 1/3] x86/fpu/xstate: Fix small issues before adding supervisor xstates

[Yu-cheng Yu]

In response to earlier comments, fix small issues before introducing XSAVES
supervisor states:
- Add spaces around '*'.
- Fix comments of xfeature_is_supervisor().
- Replace ((u64)1 << 63) with XCOMP_BV_COMPACTED_FORMAT.

No functional changes from this patch.

Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/kernel/fpu/xstate.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index e5cb67d67c03..cfcac7b42e5e 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -60,7 +60,7 @@ u64 xfeatures_mask __read_mostly;
 
 static unsigned int xstate_offsets[XFEATURE_MAX] = { [ 0 ... XFEATURE_MAX - 1] = -1};
 static unsigned int xstate_sizes[XFEATURE_MAX]   = { [ 0 ... XFEATURE_MAX - 1] = -1};
-static unsigned int xstate_comp_offsets[sizeof(xfeatures_mask)*8];
+static unsigned int xstate_comp_offsets[sizeof(xfeatures_mask) * 8];
 
 /*
  * The XSAVE area of kernel can be in standard or compacted format;
@@ -110,12 +110,9 @@ EXPORT_SYMBOL_GPL(cpu_has_xfeatures);
 static int xfeature_is_supervisor(int xfeature_nr)
 {
 	/*
-	 * We currently do not support supervisor states, but if
-	 * we did, we could find out like this.
-	 *
-	 * SDM says: If state component 'i' is a user state component,
-	 * ECX[0] return 0; if state component i is a supervisor
-	 * state component, ECX[0] returns 1.
+	 * Extended State Enumeration Sub-leaves (EAX = 0DH, ECX = n, n > 1)
+	 * returns ECX[0] set to (1) for a supervisor state, and cleared (0)
+	 * for a user state.
 	 */
 	u32 eax, ebx, ecx, edx;
 
@@ -342,7 +339,7 @@ static int xfeature_is_aligned(int xfeature_nr)
  */
 static void __init setup_xstate_comp(void)
 {
-	unsigned int xstate_comp_sizes[sizeof(xfeatures_mask)*8];
+	unsigned int xstate_comp_sizes[sizeof(xfeatures_mask) * 8];
 	int i;
 
 	/*
@@ -415,7 +412,8 @@ static void __init setup_init_fpu_buf(void)
 	print_xstate_features();
 
 	if (boot_cpu_has(X86_FEATURE_XSAVES))
-		init_fpstate.xsave.header.xcomp_bv = (u64)1 << 63 | xfeatures_mask;
+		init_fpstate.xsave.header.xcomp_bv = XCOMP_BV_COMPACTED_FORMAT |
+						     xfeatures_mask;
 
 	/*
 	 * Init all the features state with header.xfeatures being 0x0
-- 
2.17.1

[PATCH 2/3] x86/fpu/xstate: Make xfeature_is_supervisor()/xfeature_is_user() return bool

[Yu-cheng Yu]

In the previous patch, xfeature_is_supervisor()'s description is revised,
and since xfeature_is_supervisor()/xfeature_is_user() are used only in
boolean context, make both return bool.

Suggested-by: Borislav Petkov <bp@suse.de>
Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
---
 arch/x86/kernel/fpu/xstate.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index cfcac7b42e5e..912bdfce10dd 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -107,7 +107,7 @@ int cpu_has_xfeatures(u64 xfeatures_needed, const char **feature_name)
 }
 EXPORT_SYMBOL_GPL(cpu_has_xfeatures);
 
-static int xfeature_is_supervisor(int xfeature_nr)
+static bool xfeature_is_supervisor(int xfeature_nr)
 {
 	/*
 	 * Extended State Enumeration Sub-leaves (EAX = 0DH, ECX = n, n > 1)
@@ -117,10 +117,10 @@ static int xfeature_is_supervisor(int xfeature_nr)
 	u32 eax, ebx, ecx, edx;
 
 	cpuid_count(XSTATE_CPUID, xfeature_nr, &eax, &ebx, &ecx, &edx);
-	return !!(ecx & 1);
+	return ecx & 1;
 }
 
-static int xfeature_is_user(int xfeature_nr)
+static bool xfeature_is_user(int xfeature_nr)
 {
 	return !xfeature_is_supervisor(xfeature_nr);
 }
-- 
2.17.1

[PATCH 3/3] x86/fpu/xstate: Invalidate fpregs when __fpu_restore_sig() fails

[Yu-cheng Yu]

In __fpu_restore_sig(),'init_fpstate.xsave' and part of 'fpu->state.xsave'
are restored separately to xregs.  However, as stated in __cpu_invalidate_
fpregs_state(),

  Any code that clobbers the FPU registers or updates the in-memory
  FPU state for a task MUST let the rest of the kernel know that the
  FPU registers are no longer valid for this task.

and this code violates that rule.  Should the restoration fail, the other
task's context is corrupted.

This problem does not occur very often because copy_*_to_xregs() succeeds
most of the time.  It occurs, for instance, in copy_user_to_fpregs_
zeroing() when the first half of the restoration succeeds and the other
half fails.  This can be triggered by running glibc tests, where a non-
present user stack page causes the XRSTOR to fail.

The introduction of supervisor xstates and CET, while not contributing to
the problem, makes it more detectable.  After init_fpstate and the Shadow
Stack pointer have been restored to xregs, the XRSTOR from user stack
fails and fpu_fpregs_owner_ctx is not updated.  The task currently owning
fpregs then uses the corrupted Shadow Stack pointer and triggers a control-
protection fault.

Fix it by adding __cpu_invalidate_fpregs_state() to functions that copy
fpstate to fpregs:
  copy_*_to_xregs_*(), copy_*_to_fxregs_*(), and copy_*_to_fregs_*().
The alternative is to hit all of the call sites themselves.

The function __cpu_invalidate_fpregs_state() is chosen over fpregs_
deactivate() as it is called under fpregs_lock() protection.

In addition to sigreturn, also checked all call sites of these functions:

- copy_init_fpstate_to_fpregs();
- copy_kernel_to_fpregs();
- ex_handler_fprestore();
- fpu__save(); and
- fpu__copy().

In fpu__save() and fpu__copy(), fpregs are re-activated because they are
considered valid in both cases.

Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
---
 arch/x86/include/asm/fpu/internal.h | 14 ++++++++++++++
 arch/x86/kernel/fpu/core.c          | 15 +++++++++++++--
 2 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
index 4c95c365058a..cd380d14e4e2 100644
--- a/arch/x86/include/asm/fpu/internal.h
+++ b/arch/x86/include/asm/fpu/internal.h
@@ -142,6 +142,8 @@ extern void fpstate_sanitize_xstate(struct fpu *fpu);
 		     _ASM_EXTABLE_HANDLE(1b, 2b, ex_handler_fprestore)	\
 		     : output : input)
 
+static inline void __cpu_invalidate_fpregs_state(void);
+
 static inline int copy_fregs_to_user(struct fregs_state __user *fx)
 {
 	return user_insn(fnsave %[fx]; fwait,  [fx] "=m" (*fx), "m" (*fx));
@@ -158,6 +160,8 @@ static inline int copy_fxregs_to_user(struct fxregs_state __user *fx)
 
 static inline void copy_kernel_to_fxregs(struct fxregs_state *fx)
 {
+	__cpu_invalidate_fpregs_state();
+
 	if (IS_ENABLED(CONFIG_X86_32))
 		kernel_insn(fxrstor %[fx], "=m" (*fx), [fx] "m" (*fx));
 	else
@@ -166,6 +170,8 @@ static inline void copy_kernel_to_fxregs(struct fxregs_state *fx)
 
 static inline int copy_kernel_to_fxregs_err(struct fxregs_state *fx)
 {
+	__cpu_invalidate_fpregs_state();
+
 	if (IS_ENABLED(CONFIG_X86_32))
 		return kernel_insn_err(fxrstor %[fx], "=m" (*fx), [fx] "m" (*fx));
 	else
@@ -174,6 +180,8 @@ static inline int copy_kernel_to_fxregs_err(struct fxregs_state *fx)
 
 static inline int copy_user_to_fxregs(struct fxregs_state __user *fx)
 {
+	__cpu_invalidate_fpregs_state();
+
 	if (IS_ENABLED(CONFIG_X86_32))
 		return user_insn(fxrstor %[fx], "=m" (*fx), [fx] "m" (*fx));
 	else
@@ -182,16 +190,19 @@ static inline int copy_user_to_fxregs(struct fxregs_state __user *fx)
 
 static inline void copy_kernel_to_fregs(struct fregs_state *fx)
 {
+	__cpu_invalidate_fpregs_state();
 	kernel_insn(frstor %[fx], "=m" (*fx), [fx] "m" (*fx));
 }
 
 static inline int copy_kernel_to_fregs_err(struct fregs_state *fx)
 {
+	__cpu_invalidate_fpregs_state();
 	return kernel_insn_err(frstor %[fx], "=m" (*fx), [fx] "m" (*fx));
 }
 
 static inline int copy_user_to_fregs(struct fregs_state __user *fx)
 {
+	__cpu_invalidate_fpregs_state();
 	return user_insn(frstor %[fx], "=m" (*fx), [fx] "m" (*fx));
 }
 
@@ -340,6 +351,7 @@ static inline void copy_kernel_to_xregs(struct xregs_state *xstate, u64 mask)
 	u32 lmask = mask;
 	u32 hmask = mask >> 32;
 
+	__cpu_invalidate_fpregs_state();
 	XSTATE_XRESTORE(xstate, lmask, hmask);
 }
 
@@ -382,6 +394,7 @@ static inline int copy_user_to_xregs(struct xregs_state __user *buf, u64 mask)
 	u32 hmask = mask >> 32;
 	int err;
 
+	__cpu_invalidate_fpregs_state();
 	stac();
 	XSTATE_OP(XRSTOR, xstate, lmask, hmask, err);
 	clac();
@@ -399,6 +412,7 @@ static inline int copy_kernel_to_xregs_err(struct xregs_state *xstate, u64 mask)
 	u32 hmask = mask >> 32;
 	int err;
 
+	__cpu_invalidate_fpregs_state();
 	XSTATE_OP(XRSTOR, xstate, lmask, hmask, err);
 
 	return err;
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 12c70840980e..743ff5ea4076 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -127,7 +127,12 @@ void fpu__save(struct fpu *fpu)
 
 	if (!test_thread_flag(TIF_NEED_FPU_LOAD)) {
 		if (!copy_fpregs_to_fpstate(fpu)) {
+			/*
+			 * copy_kernel_to_fpregs deactivates fpregs;
+			 * re-activate fpregs after that.
+			 */
 			copy_kernel_to_fpregs(&fpu->state);
+			fpregs_activate(fpu);
 		}
 	}
 
@@ -191,11 +196,17 @@ int fpu__copy(struct task_struct *dst, struct task_struct *src)
 	 *   register contents so we have to load them back. )
 	 */
 	fpregs_lock();
-	if (test_thread_flag(TIF_NEED_FPU_LOAD))
+	if (test_thread_flag(TIF_NEED_FPU_LOAD)) {
 		memcpy(&dst_fpu->state, &src_fpu->state, fpu_kernel_xstate_size);
 
-	else if (!copy_fpregs_to_fpstate(dst_fpu))
+	} else if (!copy_fpregs_to_fpstate(dst_fpu)) {
+		/*
+		 * copy_kernel_to_fpregs deactivates fpregs;
+		 * re-activate fpregs after that.
+		 */
 		copy_kernel_to_fpregs(&dst_fpu->state);
+		fpregs_activate(src_fpu);
+	}
 
 	fpregs_unlock();
 
-- 
2.17.1

[PATCH v2 3/3] x86/fpu/xstate: Invalidate fpregs when __fpu_restore_sig() fails

[Yu-cheng Yu]

In __fpu_restore_sig(),'init_fpstate.xsave' and part of 'fpu->state.xsave'
are restored separately to xregs.  However, as stated in __cpu_invalidate_
fpregs_state(),

  Any code that clobbers the FPU registers or updates the in-memory
  FPU state for a task MUST let the rest of the kernel know that the
  FPU registers are no longer valid for this task.

and this code violates that rule.  Should the restoration fail, the other
task's context is corrupted.

This problem does not occur very often because copy_*_to_xregs() succeeds
most of the time.  It occurs, for instance, in copy_user_to_fpregs_
zeroing() when the first half of the restoration succeeds and the other
half fails.  This can be triggered by running glibc tests, where a non-
present user stack page causes the XRSTOR to fail.

The introduction of supervisor xstates and CET, while not contributing to
the problem, makes it more detectable.  After init_fpstate and the Shadow
Stack pointer have been restored to xregs, the XRSTOR from user stack
fails and fpu_fpregs_owner_ctx is not updated.  The task currently owning
fpregs then uses the corrupted Shadow Stack pointer and triggers a control-
protection fault.

Fix it by adding __cpu_invalidate_fpregs_state() to functions that copy
fpstate to fpregs:
  copy_*_to_xregs_*(), copy_*_to_fxregs_*(), and copy_*_to_fregs_*().
The alternative is to hit all of the call sites themselves.

The function __cpu_invalidate_fpregs_state() is chosen over fpregs_
deactivate() as it is called under fpregs_lock() protection.

In addition to sigreturn, also checked all call sites of these functions:

- copy_init_fpstate_to_fpregs();
- copy_kernel_to_fpregs();
- ex_handler_fprestore();
- fpu__save(); and
- fpu__copy().

In fpu__save() and fpu__copy(), fpregs are re-activated because they are
considered valid in both cases.

v2:
  Add the missing EXPORT_SYMBOL_GPL(fpu_fpregs_owner_ctx).

Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
---
 arch/x86/include/asm/fpu/internal.h | 14 ++++++++++++++
 arch/x86/kernel/fpu/core.c          | 16 ++++++++++++++--
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
index 4c95c365058a..cd380d14e4e2 100644
--- a/arch/x86/include/asm/fpu/internal.h
+++ b/arch/x86/include/asm/fpu/internal.h
@@ -142,6 +142,8 @@ extern void fpstate_sanitize_xstate(struct fpu *fpu);
 		     _ASM_EXTABLE_HANDLE(1b, 2b, ex_handler_fprestore)	\
 		     : output : input)
 
+static inline void __cpu_invalidate_fpregs_state(void);
+
 static inline int copy_fregs_to_user(struct fregs_state __user *fx)
 {
 	return user_insn(fnsave %[fx]; fwait,  [fx] "=m" (*fx), "m" (*fx));
@@ -158,6 +160,8 @@ static inline int copy_fxregs_to_user(struct fxregs_state __user *fx)
 
 static inline void copy_kernel_to_fxregs(struct fxregs_state *fx)
 {
+	__cpu_invalidate_fpregs_state();
+
 	if (IS_ENABLED(CONFIG_X86_32))
 		kernel_insn(fxrstor %[fx], "=m" (*fx), [fx] "m" (*fx));
 	else
@@ -166,6 +170,8 @@ static inline void copy_kernel_to_fxregs(struct fxregs_state *fx)
 
 static inline int copy_kernel_to_fxregs_err(struct fxregs_state *fx)
 {
+	__cpu_invalidate_fpregs_state();
+
 	if (IS_ENABLED(CONFIG_X86_32))
 		return kernel_insn_err(fxrstor %[fx], "=m" (*fx), [fx] "m" (*fx));
 	else
@@ -174,6 +180,8 @@ static inline int copy_kernel_to_fxregs_err(struct fxregs_state *fx)
 
 static inline int copy_user_to_fxregs(struct fxregs_state __user *fx)
 {
+	__cpu_invalidate_fpregs_state();
+
 	if (IS_ENABLED(CONFIG_X86_32))
 		return user_insn(fxrstor %[fx], "=m" (*fx), [fx] "m" (*fx));
 	else
@@ -182,16 +190,19 @@ static inline int copy_user_to_fxregs(struct fxregs_state __user *fx)
 
 static inline void copy_kernel_to_fregs(struct fregs_state *fx)
 {
+	__cpu_invalidate_fpregs_state();
 	kernel_insn(frstor %[fx], "=m" (*fx), [fx] "m" (*fx));
 }
 
 static inline int copy_kernel_to_fregs_err(struct fregs_state *fx)
 {
+	__cpu_invalidate_fpregs_state();
 	return kernel_insn_err(frstor %[fx], "=m" (*fx), [fx] "m" (*fx));
 }
 
 static inline int copy_user_to_fregs(struct fregs_state __user *fx)
 {
+	__cpu_invalidate_fpregs_state();
 	return user_insn(frstor %[fx], "=m" (*fx), [fx] "m" (*fx));
 }
 
@@ -340,6 +351,7 @@ static inline void copy_kernel_to_xregs(struct xregs_state *xstate, u64 mask)
 	u32 lmask = mask;
 	u32 hmask = mask >> 32;
 
+	__cpu_invalidate_fpregs_state();
 	XSTATE_XRESTORE(xstate, lmask, hmask);
 }
 
@@ -382,6 +394,7 @@ static inline int copy_user_to_xregs(struct xregs_state __user *buf, u64 mask)
 	u32 hmask = mask >> 32;
 	int err;
 
+	__cpu_invalidate_fpregs_state();
 	stac();
 	XSTATE_OP(XRSTOR, xstate, lmask, hmask, err);
 	clac();
@@ -399,6 +412,7 @@ static inline int copy_kernel_to_xregs_err(struct xregs_state *xstate, u64 mask)
 	u32 hmask = mask >> 32;
 	int err;
 
+	__cpu_invalidate_fpregs_state();
 	XSTATE_OP(XRSTOR, xstate, lmask, hmask, err);
 
 	return err;
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 12c70840980e..4e5151e43a2c 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -42,6 +42,7 @@ static DEFINE_PER_CPU(bool, in_kernel_fpu);
  * Track which context is using the FPU on the CPU:
  */
 DEFINE_PER_CPU(struct fpu *, fpu_fpregs_owner_ctx);
+EXPORT_SYMBOL_GPL(fpu_fpregs_owner_ctx);
 
 static bool kernel_fpu_disabled(void)
 {
@@ -127,7 +128,12 @@ void fpu__save(struct fpu *fpu)
 
 	if (!test_thread_flag(TIF_NEED_FPU_LOAD)) {
 		if (!copy_fpregs_to_fpstate(fpu)) {
+			/*
+			 * copy_kernel_to_fpregs deactivates fpregs;
+			 * re-activate fpregs after that.
+			 */
 			copy_kernel_to_fpregs(&fpu->state);
+			fpregs_activate(fpu);
 		}
 	}
 
@@ -191,11 +197,17 @@ int fpu__copy(struct task_struct *dst, struct task_struct *src)
 	 *   register contents so we have to load them back. )
 	 */
 	fpregs_lock();
-	if (test_thread_flag(TIF_NEED_FPU_LOAD))
+	if (test_thread_flag(TIF_NEED_FPU_LOAD)) {
 		memcpy(&dst_fpu->state, &src_fpu->state, fpu_kernel_xstate_size);
 
-	else if (!copy_fpregs_to_fpstate(dst_fpu))
+	} else if (!copy_fpregs_to_fpstate(dst_fpu)) {
+		/*
+		 * copy_kernel_to_fpregs deactivates fpregs;
+		 * re-activate fpregs after that.
+		 */
 		copy_kernel_to_fpregs(&dst_fpu->state);
+		fpregs_activate(src_fpu);
+	}
 
 	fpregs_unlock();
 
-- 
2.17.1

[PATCH v2 0/3] Fix small issues in XSAVES

[Yu-cheng Yu]

Rebase v1 to Linux v5.5-rc1.  This supersedes the previous version, but
does not have any functional changes.

The first two patches in this series are split from my supervisor xstate
patches [2].  The third is to fix a vital issue in __fpu_restore_sig(),
and more RFC than the others.  All three are not directly related to
supervisor xstates or CET, split them out and submit first.  I will
re-submit supervisor xstate patches shortly.

When__fpu_restore_sig() fails, partially cleared FPU registers still belong
to the previous owner task.  That causes that task to use corrupted xregs.
Fix it by doing __cpu_invalidate_fpregs_state() in functions that copy into
fpregs.  Further details are in the commit log of patch #3.

[1] v1 of this series:
    https://lkml.kernel.org/r/20191205182648.32257-1-yu-cheng.yu@intel.com/

[2] Support XSAVES supervisor states
    https://lkml.kernel.org/r/20190925151022.21688-1-yu-cheng.yu@intel.com/

[3] CET patches:
    https://lkml.kernel.org/r/20190813205225.12032-1-yu-cheng.yu@intel.com/
    https://lkml.kernel.org/r/20190813205359.12196-1-yu-cheng.yu@intel.com/

Yu-cheng Yu (3):
  x86/fpu/xstate: Fix small issues before adding supervisor xstates
  x86/fpu/xstate: Make xfeature_is_supervisor()/xfeature_is_user()
    return bool
  x86/fpu/xstate: Invalidate fpregs when __fpu_restore_sig() fails

 arch/x86/include/asm/fpu/internal.h | 14 ++++++++++++++
 arch/x86/kernel/fpu/core.c          | 16 ++++++++++++++--
 arch/x86/kernel/fpu/xstate.c        | 18 ++++++++----------
 3 files changed, 36 insertions(+), 12 deletions(-)

-- 
2.17.1

[PATCH v2 3/3] x86/fpu/xstate: Invalidate fpregs when __fpu_restore_sig() fails

[Yu-cheng Yu]

In __fpu_restore_sig(),'init_fpstate.xsave' and part of 'fpu->state.xsave'
are restored separately to xregs.  However, as stated in __cpu_invalidate_
fpregs_state(),

  Any code that clobbers the FPU registers or updates the in-memory
  FPU state for a task MUST let the rest of the kernel know that the
  FPU registers are no longer valid for this task.

and this code violates that rule.  Should the restoration fail, the other
task's context is corrupted.

This problem does not occur very often because copy_*_to_xregs() succeeds
most of the time.  It occurs, for instance, in copy_user_to_fpregs_
zeroing() when the first half of the restoration succeeds and the other
half fails.  This can be triggered by running glibc tests, where a non-
present user stack page causes the XRSTOR to fail.

The introduction of supervisor xstates and CET, while not contributing to
the problem, makes it more detectable.  After init_fpstate and the Shadow
Stack pointer have been restored to xregs, the XRSTOR from user stack
fails and fpu_fpregs_owner_ctx is not updated.  The task currently owning
fpregs then uses the corrupted Shadow Stack pointer and triggers a control-
protection fault.

Fix it by adding __cpu_invalidate_fpregs_state() to functions that copy
fpstate to fpregs:
  copy_*_to_xregs_*(), copy_*_to_fxregs_*(), and copy_*_to_fregs_*().
The alternative is to hit all of the call sites themselves.

The function __cpu_invalidate_fpregs_state() is chosen over fpregs_
deactivate() as it is called under fpregs_lock() protection.

In addition to sigreturn, also checked all call sites of these functions:

- copy_init_fpstate_to_fpregs();
- copy_kernel_to_fpregs();
- ex_handler_fprestore();
- fpu__save(); and
- fpu__copy().

In fpu__save() and fpu__copy(), fpregs are re-activated because they are
considered valid in both cases.

v2:
  Add the missing EXPORT_SYMBOL_GPL(fpu_fpregs_owner_ctx).

Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
---
 arch/x86/include/asm/fpu/internal.h | 14 ++++++++++++++
 arch/x86/kernel/fpu/core.c          | 16 ++++++++++++++--
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
index 44c48e34d799..f317da2c5ca5 100644
--- a/arch/x86/include/asm/fpu/internal.h
+++ b/arch/x86/include/asm/fpu/internal.h
@@ -142,6 +142,8 @@ extern void fpstate_sanitize_xstate(struct fpu *fpu);
 		     _ASM_EXTABLE_HANDLE(1b, 2b, ex_handler_fprestore)	\
 		     : output : input)
 
+static inline void __cpu_invalidate_fpregs_state(void);
+
 static inline int copy_fregs_to_user(struct fregs_state __user *fx)
 {
 	return user_insn(fnsave %[fx]; fwait,  [fx] "=m" (*fx), "m" (*fx));
@@ -158,6 +160,8 @@ static inline int copy_fxregs_to_user(struct fxregs_state __user *fx)
 
 static inline void copy_kernel_to_fxregs(struct fxregs_state *fx)
 {
+	__cpu_invalidate_fpregs_state();
+
 	if (IS_ENABLED(CONFIG_X86_32))
 		kernel_insn(fxrstor %[fx], "=m" (*fx), [fx] "m" (*fx));
 	else
@@ -166,6 +170,8 @@ static inline void copy_kernel_to_fxregs(struct fxregs_state *fx)
 
 static inline int copy_kernel_to_fxregs_err(struct fxregs_state *fx)
 {
+	__cpu_invalidate_fpregs_state();
+
 	if (IS_ENABLED(CONFIG_X86_32))
 		return kernel_insn_err(fxrstor %[fx], "=m" (*fx), [fx] "m" (*fx));
 	else
@@ -174,6 +180,8 @@ static inline int copy_kernel_to_fxregs_err(struct fxregs_state *fx)
 
 static inline int copy_user_to_fxregs(struct fxregs_state __user *fx)
 {
+	__cpu_invalidate_fpregs_state();
+
 	if (IS_ENABLED(CONFIG_X86_32))
 		return user_insn(fxrstor %[fx], "=m" (*fx), [fx] "m" (*fx));
 	else
@@ -182,16 +190,19 @@ static inline int copy_user_to_fxregs(struct fxregs_state __user *fx)
 
 static inline void copy_kernel_to_fregs(struct fregs_state *fx)
 {
+	__cpu_invalidate_fpregs_state();
 	kernel_insn(frstor %[fx], "=m" (*fx), [fx] "m" (*fx));
 }
 
 static inline int copy_kernel_to_fregs_err(struct fregs_state *fx)
 {
+	__cpu_invalidate_fpregs_state();
 	return kernel_insn_err(frstor %[fx], "=m" (*fx), [fx] "m" (*fx));
 }
 
 static inline int copy_user_to_fregs(struct fregs_state __user *fx)
 {
+	__cpu_invalidate_fpregs_state();
 	return user_insn(frstor %[fx], "=m" (*fx), [fx] "m" (*fx));
 }
 
@@ -340,6 +351,7 @@ static inline void copy_kernel_to_xregs(struct xregs_state *xstate, u64 mask)
 	u32 lmask = mask;
 	u32 hmask = mask >> 32;
 
+	__cpu_invalidate_fpregs_state();
 	XSTATE_XRESTORE(xstate, lmask, hmask);
 }
 
@@ -382,6 +394,7 @@ static inline int copy_user_to_xregs(struct xregs_state __user *buf, u64 mask)
 	u32 hmask = mask >> 32;
 	int err;
 
+	__cpu_invalidate_fpregs_state();
 	stac();
 	XSTATE_OP(XRSTOR, xstate, lmask, hmask, err);
 	clac();
@@ -399,6 +412,7 @@ static inline int copy_kernel_to_xregs_err(struct xregs_state *xstate, u64 mask)
 	u32 hmask = mask >> 32;
 	int err;
 
+	__cpu_invalidate_fpregs_state();
 	XSTATE_OP(XRSTOR, xstate, lmask, hmask, err);
 
 	return err;
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 12c70840980e..4e5151e43a2c 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -42,6 +42,7 @@ static DEFINE_PER_CPU(bool, in_kernel_fpu);
  * Track which context is using the FPU on the CPU:
  */
 DEFINE_PER_CPU(struct fpu *, fpu_fpregs_owner_ctx);
+EXPORT_SYMBOL_GPL(fpu_fpregs_owner_ctx);
 
 static bool kernel_fpu_disabled(void)
 {
@@ -127,7 +128,12 @@ void fpu__save(struct fpu *fpu)
 
 	if (!test_thread_flag(TIF_NEED_FPU_LOAD)) {
 		if (!copy_fpregs_to_fpstate(fpu)) {
+			/*
+			 * copy_kernel_to_fpregs deactivates fpregs;
+			 * re-activate fpregs after that.
+			 */
 			copy_kernel_to_fpregs(&fpu->state);
+			fpregs_activate(fpu);
 		}
 	}
 
@@ -191,11 +197,17 @@ int fpu__copy(struct task_struct *dst, struct task_struct *src)
 	 *   register contents so we have to load them back. )
 	 */
 	fpregs_lock();
-	if (test_thread_flag(TIF_NEED_FPU_LOAD))
+	if (test_thread_flag(TIF_NEED_FPU_LOAD)) {
 		memcpy(&dst_fpu->state, &src_fpu->state, fpu_kernel_xstate_size);
 
-	else if (!copy_fpregs_to_fpstate(dst_fpu))
+	} else if (!copy_fpregs_to_fpstate(dst_fpu)) {
+		/*
+		 * copy_kernel_to_fpregs deactivates fpregs;
+		 * re-activate fpregs after that.
+		 */
 		copy_kernel_to_fpregs(&dst_fpu->state);
+		fpregs_activate(src_fpu);
+	}
 
 	fpregs_unlock();
 
-- 
2.17.1

Re: [PATCH v2 3/3] x86/fpu/xstate: Invalidate fpregs when __fpu_restore_sig() fails

[Sebastian Andrzej Siewior]

On 2019-12-12 13:08:55 [-0800], Yu-cheng Yu wrote:
> In __fpu_restore_sig(),'init_fpstate.xsave' and part of 'fpu->state.xsave'
> are restored separately to xregs.  However, as stated in __cpu_invalidate_
> fpregs_state(),
> 
>   Any code that clobbers the FPU registers or updates the in-memory
>   FPU state for a task MUST let the rest of the kernel know that the
>   FPU registers are no longer valid for this task.
> 
> and this code violates that rule.  Should the restoration fail, the other
> task's context is corrupted.
> 
> This problem does not occur very often because copy_*_to_xregs() succeeds
> most of the time.  

why "most of the time"? It should always succeed. We talk here about
__fpu__restore_sig() correct? Using init_fpstate as part of restore
process isn't the "default" case. If the restore _here_ fails then it
fails.

>                    It occurs, for instance, in copy_user_to_fpregs_
> zeroing() when the first half of the restoration succeeds and the other
> half fails.  This can be triggered by running glibc tests, where a non-
> present user stack page causes the XRSTOR to fail.

So if copy_user_to_fpregs_zeroing() fails then we go to the slowpath.
Then we load the FPU register with copy_kernel_to_xregs_err().
In the end they are either enabled (fpregs_mark_activate()) or cleared
if it failed (fpu__clear()). Don't see here a problem.

Can you tell me which glibc test? I would like to reproduce this.

> The introduction of supervisor xstates and CET, while not contributing to
> the problem, makes it more detectable.  After init_fpstate and the Shadow
> Stack pointer have been restored to xregs, the XRSTOR from user stack
> fails and fpu_fpregs_owner_ctx is not updated.  The task currently owning
> fpregs then uses the corrupted Shadow Stack pointer and triggers a control-
> protection fault.

So I don't need new HW with supervisor and CET? A plain KVM box with
SSE2 and so should be enough?

Sebastian

Re: [PATCH v2 3/3] x86/fpu/xstate: Invalidate fpregs when __fpu_restore_sig() fails

[Yu-cheng Yu]

On Wed, 2019-12-18 at 16:54 +0100, Sebastian Andrzej Siewior wrote:
> On 2019-12-12 13:08:55 [-0800], Yu-cheng Yu wrote:
> > In __fpu_restore_sig(),'init_fpstate.xsave' and part of 'fpu->state.xsave'
> > are restored separately to xregs.  However, as stated in __cpu_invalidate_
> > fpregs_state(),
> > 
> >   Any code that clobbers the FPU registers or updates the in-memory
> >   FPU state for a task MUST let the rest of the kernel know that the
> >   FPU registers are no longer valid for this task.
> > 
> > and this code violates that rule.  Should the restoration fail, the other
> > task's context is corrupted.
> > 
> > This problem does not occur very often because copy_*_to_xregs() succeeds
> > most of the time.  
> 
> why "most of the time"? It should always succeed. We talk here about
> __fpu__restore_sig() correct? Using init_fpstate as part of restore
> process isn't the "default" case. If the restore _here_ fails then it
> fails.
> 
> >                    It occurs, for instance, in copy_user_to_fpregs_
> > zeroing() when the first half of the restoration succeeds and the other
> > half fails.  This can be triggered by running glibc tests, where a non-
> > present user stack page causes the XRSTOR to fail.
> 
> So if copy_user_to_fpregs_zeroing() fails then we go to the slowpath.
> Then we load the FPU register with copy_kernel_to_xregs_err().
> In the end they are either enabled (fpregs_mark_activate()) or cleared
> if it failed (fpu__clear()). Don't see here a problem.

I could have explained this better, sorry!  I will explain the first
case below; other cases are similar.

In copy_user_to_fpregs_zeroing(), we have:

    if (user_xsave()) {
        ...
        if (unlikely(init_bv))
            copy_kernel_to_xregs(&init_fpstate.xsave, init_bv);
        return copy_user_to_xregs(buf, xbv);
        ...
    }

The copy_user_to_xregs() may fail, and when that happens, before going to
the slow path, there is fpregs_unlock() and context switches may happen.
However, at this point, fpu_fpregs_owner_ctx has not been changed; it could
still be another task's FPU.  For this to happen and to be detected, the user
stack page needs to be non-present, fpu_fpregs_owner_ctx need to be another task,
and that other task needs to be able to detect its registers are modified.
The last factor is not easy to reproduce, and a CET control-protection fault
helps.

> 
> Can you tell me which glibc test? I would like to reproduce this.
> 
> > The introduction of supervisor xstates and CET, while not contributing to
> > the problem, makes it more detectable.  After init_fpstate and the Shadow
> > Stack pointer have been restored to xregs, the XRSTOR from user stack
> > fails and fpu_fpregs_owner_ctx is not updated.  The task currently owning
> > fpregs then uses the corrupted Shadow Stack pointer and triggers a control-
> > protection fault.
> 
> So I don't need new HW with supervisor and CET? A plain KVM box with
> SSE2 and so should be enough?

What I do is, clone the whole glibc source, and run mutiple copies of
"make check".  In about 40 minutes or so, there are unexplained seg faults,
or a few control-protection faults (if you enable CET).  Please let me
know if more clarification is needed.

Thanks,
Yu-cheng

Re: [PATCH v2 3/3] x86/fpu/xstate: Invalidate fpregs when __fpu_restore_sig() fails

[Sebastian Andrzej Siewior]

On 2019-12-18 12:53:59 [-0800], Yu-cheng Yu wrote:
> I could have explained this better, sorry!  I will explain the first
> case below; other cases are similar.
> 
> In copy_user_to_fpregs_zeroing(), we have:
> 
>     if (user_xsave()) {
>         ...
>         if (unlikely(init_bv))
>             copy_kernel_to_xregs(&init_fpstate.xsave, init_bv);
>         return copy_user_to_xregs(buf, xbv);
>         ...
>     }
> 
> The copy_user_to_xregs() may fail, and when that happens, before going to
> the slow path, there is fpregs_unlock() and context switches may happen.

The context switch may only happen after fpregs_unlock().

> However, at this point, fpu_fpregs_owner_ctx has not been changed; it could
> still be another task's FPU.

TIF_NEED_FPU_LOAD is set for the task in __fpu__restore_sig() and its
context (__fpu_invalidate_fpregs_state()) has been invalidated. So the
FPU register may contain another task's content and
fpu_fpregs_owner_ctx points to another context.

>                               For this to happen and to be detected, the user
> stack page needs to be non-present, fpu_fpregs_owner_ctx need to be another task,
> and that other task needs to be able to detect its registers are modified.
> The last factor is not easy to reproduce, and a CET control-protection fault
> helps.

So far everything is legal. However. If there is a context switch before
fpregs_lock() then this is bad before we don't account for that.
So that:

diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -352,6 +352,7 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 			fpregs_unlock();
 			return 0;
 		}
+		fpregs_deactivate(fpu);
 		fpregs_unlock();
 	}
 
@@ -403,6 +404,8 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 	}
 	if (!ret)
 		fpregs_mark_activate();
+	else
+		fpregs_deactivate(fpu);
 	fpregs_unlock();
 
 err_out:


Should be enough.

> > Can you tell me which glibc test? I would like to reproduce this.
> > 
> > > The introduction of supervisor xstates and CET, while not contributing to
> > > the problem, makes it more detectable.  After init_fpstate and the Shadow
> > > Stack pointer have been restored to xregs, the XRSTOR from user stack
> > > fails and fpu_fpregs_owner_ctx is not updated.  The task currently owning
> > > fpregs then uses the corrupted Shadow Stack pointer and triggers a control-
> > > protection fault.
> > 
> > So I don't need new HW with supervisor and CET? A plain KVM box with
> > SSE2 and so should be enough?
> 
> What I do is, clone the whole glibc source, and run mutiple copies of
> "make check".  In about 40 minutes or so, there are unexplained seg faults,
> or a few control-protection faults (if you enable CET).  Please let me
> know if more clarification is needed.

Okay. Can you please try the above and if not, I try that glibc thing myself.

> Thanks,
> Yu-cheng

Sebastian

Re: [PATCH v2 3/3] x86/fpu/xstate: Invalidate fpregs when __fpu_restore_sig() fails

[Yu-cheng Yu]

On Thu, 2019-12-19 at 15:22 +0100, Sebastian Andrzej Siewior wrote:
> On 2019-12-18 12:53:59 [-0800], Yu-cheng Yu wrote:
> > I could have explained this better, sorry!  I will explain the first
> > case below; other cases are similar.
> > 
> > In copy_user_to_fpregs_zeroing(), we have:
> > 
> >     if (user_xsave()) {
> >         ...
> >         if (unlikely(init_bv))
> >             copy_kernel_to_xregs(&init_fpstate.xsave, init_bv);
> >         return copy_user_to_xregs(buf, xbv);
> >         ...
> >     }
> > 
> > The copy_user_to_xregs() may fail, and when that happens, before going to
> > the slow path, there is fpregs_unlock() and context switches may happen.
> 
> The context switch may only happen after fpregs_unlock().
> 
> > However, at this point, fpu_fpregs_owner_ctx has not been changed; it could
> > still be another task's FPU.
> 
> TIF_NEED_FPU_LOAD is set for the task in __fpu__restore_sig() and its
> context (__fpu_invalidate_fpregs_state()) has been invalidated. So the
> FPU register may contain another task's content and
> fpu_fpregs_owner_ctx points to another context.
> 
> >                               For this to happen and to be detected, the user
> > stack page needs to be non-present, fpu_fpregs_owner_ctx need to be another task,
> > and that other task needs to be able to detect its registers are modified.
> > The last factor is not easy to reproduce, and a CET control-protection fault
> > helps.
> 
> So far everything is legal. However. If there is a context switch before
> fpregs_lock() then this is bad before we don't account for that.
> So that:
> 
> diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
> --- a/arch/x86/kernel/fpu/signal.c
> +++ b/arch/x86/kernel/fpu/signal.c
> @@ -352,6 +352,7 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
>  			fpregs_unlock();
>  			return 0;
>  		}
> +		fpregs_deactivate(fpu);
>  		fpregs_unlock();
>  	}
>  
> @@ -403,6 +404,8 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
>  	}
>  	if (!ret)
>  		fpregs_mark_activate();
> +	else
> +		fpregs_deactivate(fpu);
>  	fpregs_unlock();
>  
>  err_out:
> 
> 
> Should be enough.

Yes, this works.  But then everywhere that calls copy_*_to_xregs_*() etc. needs to be checked.
Are there other alternatives?

Yu-cheng

Re: [PATCH v2 3/3] x86/fpu/xstate: Invalidate fpregs when __fpu_restore_sig() fails

[Sebastian Andrzej Siewior]

On 2019-12-19 08:44:08 [-0800], Yu-cheng Yu wrote:
> Yes, this works.  But then everywhere that calls copy_*_to_xregs_*() etc. needs to be checked.
> Are there other alternatives?

I don't like the big hammer approach of your very much. It might make
all it "correct" but then it might lead to more "invalids" then needed.
It also required to export the symbol which I would like to avoid.

So if this patch works for you and you don't find anything else where it
falls apart then I will audit tomorrow all callers which got the
"invalidator" added and check for that angle.

Unless someone here complains big tyme and wants this instead…

> Yu-cheng

Sebastian

Re: [PATCH v2 3/3] x86/fpu/xstate: Invalidate fpregs when __fpu_restore_sig() fails

[Yu-cheng Yu]

On Thu, 2019-12-19 at 18:16 +0100, Sebastian Andrzej Siewior wrote:
> On 2019-12-19 08:44:08 [-0800], Yu-cheng Yu wrote:
> > Yes, this works.  But then everywhere that calls copy_*_to_xregs_*() etc. needs to be checked.
> > Are there other alternatives?
> 
> I don't like the big hammer approach of your very much. It might make
> all it "correct" but then it might lead to more "invalids" then needed.
> It also required to export the symbol which I would like to avoid.

Copying to registers invalids current fpregs context.  It might not cause
extra register loading, because registers are in fact already invalidated
and any task owning the context needs to reload anyway.  Setting
fpu_fpregs_owner_ctx is only to let the rest of the kernel know the
fact that already happened.

But, I agree with you the patch does look biggish.

> 
> So if this patch works for you and you don't find anything else where it
> falls apart then I will audit tomorrow all callers which got the
> "invalidator" added and check for that angle.

Yes, that works for me.  Also, most of these call sites are under fpregs_lock(),
and we could use __cpu_invalidate_fpregs_state().

I was also thinking maybe add warnings when any new code re-introduces the issue,
but not sure where to add that.  Do you think that is needed?

Thanks,
Yu-cheng

Re: [PATCH v2 3/3] x86/fpu/xstate: Invalidate fpregs when __fpu_restore_sig() fails

[Sebastian Andrzej Siewior]

On 2019-12-19 09:40:06 [-0800], Yu-cheng Yu wrote:
> On Thu, 2019-12-19 at 18:16 +0100, Sebastian Andrzej Siewior wrote:
> > On 2019-12-19 08:44:08 [-0800], Yu-cheng Yu wrote:
> > > Yes, this works.  But then everywhere that calls copy_*_to_xregs_*() etc. needs to be checked.
> > > Are there other alternatives?
> > 
> > I don't like the big hammer approach of your very much. It might make
> > all it "correct" but then it might lead to more "invalids" then needed.
> > It also required to export the symbol which I would like to avoid.
> 
> Copying to registers invalids current fpregs context.  It might not cause
> extra register loading, because registers are in fact already invalidated
> and any task owning the context needs to reload anyway.  Setting
> fpu_fpregs_owner_ctx is only to let the rest of the kernel know the
> fact that already happened.
> 
> But, I agree with you the patch does look biggish.

Now that I looked at it:
All kernel loads don't fail. If they fail we end up in the handler and
restore to init-state. So no need to reset `fpu_fpregs_owner_ctx' in this
case. The variable is actually set to task's FPU state so resetting is
not required.
fpu__save() invokes copy_kernel_to_fpregs() (on older boxes) and by
resetting `fpu_fpregs_owner_ctx' we would load it twice (in fpu__save()
and on return to userland).

So far I can tell, the only problematic case is the signal code because
here the state restore *may* fail and we *may* do it in two steps. The
error happens only if both `may' are true.

> > So if this patch works for you and you don't find anything else where it
> > falls apart then I will audit tomorrow all callers which got the
> > "invalidator" added and check for that angle.
> 
> Yes, that works for me.  Also, most of these call sites are under fpregs_lock(),
> and we could use __cpu_invalidate_fpregs_state().
> I was also thinking maybe add warnings when any new code re-introduces the issue,
> but not sure where to add that.  Do you think that is needed?

I was thinking about it. So the `read-FPU-state' function must be
invoked within the fpregs_lock() section. This could be easily
enforced. At fpregs_unlock() time `fpu_fpregs_owner_ctx' must be NULL or
pointing to task's FPU.
My brain is fried for today so I'm sure if this is a sane approach. But
it might be a start.

> Thanks,
> Yu-cheng

Sebastian

Re: [PATCH v2 3/3] x86/fpu/xstate: Invalidate fpregs when __fpu_restore_sig() fails

[Yu-cheng Yu]

On Fri, 2019-12-20 at 21:16 +0100, Sebastian Andrzej Siewior wrote:
> [...]
> Now that I looked at it:
> All kernel loads don't fail. If they fail we end up in the handler and
> restore to init-state. So no need to reset `fpu_fpregs_owner_ctx' in this
> case. The variable is actually set to task's FPU state so resetting is
> not required.

Agree.

> fpu__save() invokes copy_kernel_to_fpregs() (on older boxes) and by
> resetting `fpu_fpregs_owner_ctx' we would load it twice (in fpu__save()
> and on return to userland).

That is true.

> So far I can tell, the only problematic case is the signal code because
> here the state restore *may* fail and we *may* do it in two steps. The
> error happens only if both `may' are true.
> 
> > > So if this patch works for you and you don't find anything else where it
> > > falls apart then I will audit tomorrow all callers which got the
> > > "invalidator" added and check for that angle.
> > 
> > Yes, that works for me.  Also, most of these call sites are under fpregs_lock(),
> > and we could use __cpu_invalidate_fpregs_state().
> > I was also thinking maybe add warnings when any new code re-introduces the issue,
> > but not sure where to add that.  Do you think that is needed?
> 
> I was thinking about it. So the `read-FPU-state' function must be
> invoked within the fpregs_lock() section. This could be easily
> enforced. At fpregs_unlock() time `fpu_fpregs_owner_ctx' must be NULL or
> pointing to task's FPU.
> My brain is fried for today so I'm sure if this is a sane approach. But
> it might be a start.

I will also think about it.  Thanks!

Yu-cheng