[RFC v2 1/2] x86/fpu: Fix state corruption in __fpu__restore_sig()

* [RFC v2 1/2] x86/fpu: Fix state corruption in __fpu__restore_sig()
       [not found] <cover.1622351443.git.luto@kernel.org>
@ 2021-05-30  5:12 ` Andy Lutomirski
  2021-05-30 22:02   ` Thomas Gleixner
  2021-05-31 10:01   ` Thomas Gleixner
  0 siblings, 2 replies; 17+ messages in thread
From: Andy Lutomirski @ 2021-05-30  5:12 UTC (permalink / raw)
  To: x86; +Cc: Dave Hansen, Andy Lutomirski, stable, syzbot+2067e764dbcd10721e2e

Prior to adding XSAVES support, some of fpu__clear()'s callers
required that fpu__clear() successfully reset current's FPU state
even if current's XSAVE header was corrupt.  commit b860eb8dce59
("x86/fpu/xstate: Define new functions for clearing fpregs and
xstates") split fpu__clear() into two different functions, neither
of which was capable of correctly handling a corrupt header.

As a practical matter, trying to specify what
fpu__clear_user_states() is supposed to do if current's XSAVE area
is corrupt is difficult -- it's supposed to preverve supervisor
states, but the entire XSAVES buffer may be unusable due to corrupt
XFEATURES and/or XCOMP_BV.

Improve the situation by completely separating fpu__clear_all() and
fpu__clear_user_states().  The former is, once again, a full reset.
The latter requires an intact header and resets only user states.
Update and document __fpu__restore_sig() accordingly.

Cc: stable@vger.kernel.org
Fixes: b860eb8dce59 ("x86/fpu/xstate: Define new functions for clearing fpregs and xstates")
Reported-by: syzbot+2067e764dbcd10721e2e@syzkaller.appspotmail.com
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/kernel/fpu/core.c   | 62 ++++++++++++++++++++++++------------
 arch/x86/kernel/fpu/signal.c | 50 +++++++++++++++++++++++++----
 2 files changed, 84 insertions(+), 28 deletions(-)

diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 571220ac8bea..55792ef6ba6d 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -354,45 +354,65 @@ static inline void copy_init_fpstate_to_fpregs(u64 features_mask)
 }
 
 /*
- * Clear the FPU state back to init state.
- *
- * Called by sys_execve(), by the signal handler code and by various
- * error paths.
+ * Reset current's user FPU states to the init states.  The caller promises
+ * that current's supervisor states (in memory or CPU regs as appropriate)
+ * as well as the XSAVE header in memory are intact.
  */
-static void fpu__clear(struct fpu *fpu, bool user_only)
+void fpu__clear_user_states(struct fpu *fpu)
 {
 	WARN_ON_FPU(fpu != &current->thread.fpu);
 
 	if (!static_cpu_has(X86_FEATURE_FPU)) {
-		fpu__drop(fpu);
-		fpu__initialize(fpu);
+		fpu__clear_all(fpu);
 		return;
 	}
 
 	fpregs_lock();
 
-	if (user_only) {
-		if (!fpregs_state_valid(fpu, smp_processor_id()) &&
-		    xfeatures_mask_supervisor())
-			copy_kernel_to_xregs(&fpu->state.xsave,
-					     xfeatures_mask_supervisor());
-		copy_init_fpstate_to_fpregs(xfeatures_mask_user());
-	} else {
-		copy_init_fpstate_to_fpregs(xfeatures_mask_all);
-	}
+	/*
+	 * Ensure that current's supervisor states are loaded into
+	 * their corresponding registers.
+	 */
+	if (!fpregs_state_valid(fpu, smp_processor_id()) &&
+	    xfeatures_mask_supervisor())
+		copy_kernel_to_xregs(&fpu->state.xsave,
+				     xfeatures_mask_supervisor());
 
+	/*
+	 * Reset user states in registers.
+	 */
+	copy_init_fpstate_to_fpregs(xfeatures_mask_user());
+
+	/*
+	 * Now all FPU registers have their desired values.  Inform the
+	 * FPU state machine that current's FPU registers are in the
+	 * hardware registers.
+	 */
 	fpregs_mark_activate();
+
 	fpregs_unlock();
 }
 
-void fpu__clear_user_states(struct fpu *fpu)
-{
-	fpu__clear(fpu, true);
-}
 
+/*
+ * Reset current's FPU registers (user and supervisor) to their INIT values.
+ * This function does not trust the in-memory XSAVE image to be valid at all;
+ * it is safe to call even if the contents of fpu->state may be entirely
+ * malicious, including the header.
+ *
+ * This means that it must not use XSAVE, as XSAVE reads the header from
+ * memory.
+ *
+ * This does not change the actual hardware registers; when fpu__clear_all()
+ * returns, TIF_NEED_FPU_LOAD will be set, and a subsequent exit to user mode
+ * will reload the hardware registers from memory.
+ */
 void fpu__clear_all(struct fpu *fpu)
 {
-	fpu__clear(fpu, false);
+	fpregs_lock();
+	fpu__drop(fpu);
+	fpu__initialize(fpu);
+	fpregs_unlock();
 }
 
 /*
diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c
index a4ec65317a7f..3ff7e75c7b8f 100644
--- a/arch/x86/kernel/fpu/signal.c
+++ b/arch/x86/kernel/fpu/signal.c
@@ -345,6 +345,7 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 		 */
 		fpregs_lock();
 		pagefault_disable();
+
 		ret = copy_user_to_fpregs_zeroing(buf_fx, user_xfeatures, fx_only);
 		pagefault_enable();
 		if (!ret) {
@@ -382,10 +383,9 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 	}
 
 	/*
-	 * By setting TIF_NEED_FPU_LOAD it is ensured that our xstate is
-	 * not modified on context switch and that the xstate is considered
-	 * to be loaded again on return to userland (overriding last_cpu avoids
-	 * the optimisation).
+	 * By setting TIF_NEED_FPU_LOAD, we ensure that any context switches
+	 * or kernel_fpu_begin() operations (due to scheduling, page faults,
+	 * softirq, etc) do not access fpu->state.
 	 */
 	fpregs_lock();
 
@@ -406,10 +406,18 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 		u64 init_bv = xfeatures_mask_user() & ~user_xfeatures;
 
 		if (using_compacted_format()) {
+			/*
+			 * copy_user_to_xstate() may write complete garbage
+			 * to the user states, but it will not corrupt the
+			 * XSAVE header, nor will it corrupt any supervisor
+			 * states.
+			 */
 			ret = copy_user_to_xstate(&fpu->state.xsave, buf_fx);
 		} else {
 			ret = __copy_from_user(&fpu->state.xsave, buf_fx, state_size);
-
+			/*
+			 * Careful, the XSAVE header may be corrupt now.
+			 */
 			if (!ret && state_size > offsetof(struct xregs_state, header))
 				ret = validate_user_xstate_header(&fpu->state.xsave.header);
 		}
@@ -457,6 +465,15 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 		fpregs_lock();
 		ret = copy_kernel_to_fregs_err(&fpu->state.fsave);
 	}
+
+	/*
+	 * At this point, we have modified the hardware FPU regs.  We must
+	 * either mark them active for current or invalidate them for
+	 * any other task that may have owned them.
+	 *
+	 * Yes, the nonsensically named fpregs_deactivate() function
+	 * ignores its parameter and marks this CPU's regs invalid.
+	 */
 	if (!ret)
 		fpregs_mark_activate();
 	else
@@ -464,8 +481,27 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
 	fpregs_unlock();
 
 err_out:
-	if (ret)
-		fpu__clear_user_states(fpu);
+	if (ret) {
+		if (using_compacted_format()) {
+			/*
+			 * Our failed attempt to load the new state is
+			 * guaranteed to have preserved the XSAVE header
+			 * and all supervisor state.
+			 */
+			fpu__clear_user_states(fpu);
+		} else {
+			/*
+			 * If an error above caused garbage to be written
+			 * to XFEATURES, then XSAVE would preserve that
+			 * garbage in memory, and a subsequent XRSTOR would
+			 * fail or worse.  XSAVES does not have this problem.
+			 *
+			 * Wipe out the FPU state and start over from a clean
+			 * slate.
+			 */
+			fpu__clear_all(fpu);
+		}
+	}
 	return ret;
 }
 
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread