x64, fpu: fix possible FPU leakage in error conditions

restore_fpu_checking() calls init_fpu() in error conditions. init_fpu() just touches the FPU state in the thread struct and doesn't do anything with the live FPU registers. While this is wrong(as our main intention is to init the live FPU registers aswell), this was benign before the commit 92d140e21f1ce8cf99320afbbcad73879128e6dc.

Post commit 92d140e21f1ce8cf99320afbbcad73879128e6dc, live FPU registers may not belong to this process at this error scenario.

In the error condition for restore_fpu_checking() (especially during the 64bit signal return), we are doing init_fpu(), which saves the live FPU register state (possibly belonging to some other process context) into the thread struct (through unlazy_fpu() in init_fpu()). This is wrong and can leak the FPU data.

Remove the unlazy_fpu() from the init_fpu(). init_fpu() will now always init the FPU data in the thread struct. For the error conditions in restore_fpu_checking(), restore the initialized FPU data from the thread struct.

---

index eb9ddd8..f5c2161 100644
@@ -98,12 +98,6 @@ void __cpuinit fpu_init(void)
  */
 int init_fpu(struct task_struct *tsk)
 {
-	if (tsk_used_math(tsk)) {
-		if (HAVE_HWFP && tsk == current)
-			unlazy_fpu(tsk);
-		return 0;
-	}
-
 	/*
 	 * Memory allocation at the first usage of the FPU and other state.
 	 */

index 37672f7..38af0ed 100644
@@ -45,6 +45,12 @@ static inline void tolerant_fwait(void)
 		     _ASM_EXTABLE(1b, 2b));
 }
 
+static inline void restore_fpu(struct i387_fxsave_struct *fx)
+{
+	__asm__ __volatile__("rex64/fxrstor (%[fx])"
+			     :: [fx] "cdaSDb" (fx), "m" (*fx));
+}
+
 static inline int restore_fpu_checking(struct i387_fxsave_struct *fx)
 {
 	int err;

@@ -62,8 +68,10 @@ static inline int restore_fpu_checking(struct i387_fxsave_struct *fx)
 #else
 		     : [fx] "cdaSDb" (fx), "m" (*fx), "0" (0));
 #endif
-	if (unlikely(err))
+	if (unlikely(err)) {
 		init_fpu(current);
+		restore_fpu(&current->thread.xstate->fxsave);
+	}
 	return err;
 }
 

Why? The thread struct is guaranteed to contain pointless data.

If we cannot restore the FP state from the signal stack, we should not try to restore it from anywhere _else_ either, since nowhere else will have any better results.

I suspect we should just reset the x87 state (which was the _intention_ of the code), possibly by just doing "stts + used_math = 0". The signal handling code already checks for errors, and will force a SIGSEGV if this ever happens.

(Yes, there is also a restore_fpu_checking() in math_state_restore(), but that one _already_ uses &current->thread.xstate->fxsave as the buffer to restore from, so trying to do that _again_ when it fails seems to be really really wrong - we already _did_ that, and that was what failed to begin with)

Linus

init_fpu() will set it to sane init state, from where we can restore.

Yes, this was what I had in mind earlier and should be ok for signal handling case. But as you also noted below:

We are doing init_fpu(), which should make the data sane again.

This is a paranoid case, just to make sure that the next math_state_restore() doesn't cause #GP, after someone sets illegal values through ptrace() or 32bit signal handling (which modifies fpu state in thread struct). I say paranoid, because we already do the necessary checks in the corresponding locations like ptrace/32-bit signal handling.

If we don't do init_fpu() + restore from the sane init state, process has to be killed, in the paranoid failing scenario of math_state_restore()

thanks,
suresh

Umm. I'm still not seeing why the right answer is not just to do "stts + math_used = 0".

And the specific case of math_state_restore(), that will also fix the problem - next time around.

As far as I can tell, your patch causes serious problems in case init_fpu() fails. Which it can do, afaik.

Linus

Ok.

Only the first init_fpu() can fail (memory allocation failure). But here, it is def not the first time.

Anyhow, your suggestion is simple and clean. Will post the patch shortly.

I have to do clear_fpu() though(stts + math_used = 0 may not be enough).

thanks,
suresh

Ahh. Agreed. I missed that. But yes, clear_fpu() is still preferable, I think.

Linus

Appended the patch(tested) with modified changelog. restore_i387() for 64bit is in the header file, which complicates calling clear_fpu().

To simplify, I am doing the error check at the caller sites in c files.

I am planning to clean (move the 64-bit save/restore_i387() to c files etc) some of this up in the upcoming xsave/xrstor patchset anyhow.

Meanwhile, I wanted to keep this patch simple, so that it can be easily applied to -stable series aswell.

thanks,
suresh
---

restore_fpu_checking() calls init_fpu() in error conditions. While this is wrong(as our main intention is to clear the fpu state of the thread), this was benign before the commit 92d140e21f1ce8cf99320afbbcad73879128e6dc.

Post commit 92d140e21f1ce8cf99320afbbcad73879128e6dc, live FPU registers may not belong to this process at this error scenario.

In the error condition for restore_fpu_checking() (especially during the 64bit signal return), we are doing init_fpu(), which saves the live FPU register state (possibly belonging to some other process context) into the thread struct (through unlazy_fpu() in init_fpu()). This is wrong and can leak the FPU data.

For the error conditions in restore_fpu_checking(), clear the fpu state present in the thread struct.

---

index bf87684..672f35a 100644
@@ -96,15 +96,25 @@ restore_sigcontext(struct pt_regs *regs, struct sigcontext __user *sc,
 	}
 
 	{
+		int ret = 0;
 		struct _fpstate __user * buf;
 		err |= __get_user(buf, &sc->fpstate);
 
 		if (buf) {
 			if (!access_ok(VERIFY_READ, buf, sizeof(*buf)))
 				goto badframe;
-			err |= restore_i387(buf);
-		} else {
+			ret = restore_i387(buf);
+		}
+		
+		/*
+		 * clear the fpu state if there is no math info found in the
+		 * sigcontext, or we encountered an error while restoring
+		 * it.
+		 */
+		if (!buf || ret) {
 			struct task_struct *me = current;
+
+			err |= ret;
 			if (used_math()) {
 				clear_fpu(me);
 				clear_used_math();

index 3f18d73..6abf39c 100644
@@ -1131,7 +1131,18 @@ asmlinkage void math_state_restore(void)
 	}
 
 	clts();				/* Allow maths ops (or we recurse) */
-	restore_fpu_checking(&me->thread.xstate->fxsave);
+	/*
+	 * Paranoid restore. If it fails, we return with math state cleared.
+	 * Next attempt to restore will init the state and continue.
+	 */
+	if (restore_fpu_checking(&me->thread.xstate->fxsave)) {
+		stts();		/* TS_USEDFPU is still not set, hence
+				 * clear_fpu() doesn't achieve what we want
+				 * here. stts() is enough.
+				 */
+		clear_used_math();
+		return;
+	}
 	task_thread_info(me)->status |= TS_USEDFPU;
 	me->fpu_counter++;
 }

index 37672f7..d46d591 100644
@@ -62,8 +62,6 @@ static inline int restore_fpu_checking(struct i387_fxsave_struct *fx)
 #else
 		     : [fx] "cdaSDb" (fx), "m" (*fx), "0" (0));
 #endif
-	if (unlikely(err))
-		init_fpu(current);
 	return err;
 }
 

Hmm. There's somethign more fundamentally wrong, it really shouldn't be this ugly.

For example, the signal handler path right now does

	if (!access_ok(VERIFY_READ, buf, sizeof(*buf)))
		goto badframe;
	err |= restore_i387(buf);

but the thing is, the only really valid reason for "restore_i387()" to fail is because the read failed.

Which in turn implies that if it fails, it should just do the same thing as that access_ok() failure did!

So why doesn't it just do

	if (!access_ok(VERIFY_READ, buf, sizeof(*buf)))
		goto badframe;
	if (restore_i387(buf))
		goto badframe:

because I don't see why that path should even _care_ about the i387 details? Especially since it doesn't even try to do that if the buffer pointer is totally bogus..

What am I missing? This code looks unnecessarily complex, and your patch makes it even harder to follow. Is this complexity really needed and worth it?

Also, looking at that "math_state_restore()" thing some more, I can't for the life of me convince myself that even just initializing the state is enough. We've used math before, and if we cannot restore it from the fxsave area, why would we _ever_ say that it's ok to try to continue with some _other_ state?

IOW, rather than resetting it, shouldn't we force a SIGFPE or something?

Sorry for being difficult, but I'd much rather get the x87 state handling _right_ and make it logically consistent than paper over yet another mistake we've done in this area. For example, regular 32-bit x86 doesn't do any of this crap. It just does "restore_fpu()" in math_state_restore().

Why does x86-64 need to do anythign else? It's not even a user address, it cannot take page faults. So exactly what are we protecting against?

I may well be missing something here, so please fill me in..

Linus

Not really. It can cause #GP, if someone sets reserved bits of mxcsr in the memory image.

But restore_i387() may be in an insane state (we did clts() but doesn't have the proper state in its live registers etc) when it failed to restore fpu. Ideally we should fix this inside restore_i387(). But restore_i387() is in header file and I have to re-arrange some of the code
in the header file, to call clear_fpu() from restore_i387().

does the above explain? but you are correct, it doesn't look clean :(

Probably SIGSEGV is the right thing (because that's we do for other general-protection faults).

Andi was paranoid I think. Just in case, if we miss some future reserved bit handling in xfpregs_set() etc, kernel shouldn't die.

thanks,
suresh

Ahh, ok, I can imagine that. And I guess we might copy the data from user space into the memory image without validating it at points (signal handler restore and/or ptrace). Do we?

Ok, how about we just move restore_i387() out of the header file? I realize that the x86 code plays some games with this whole thing (that whole '#define restore_i387_ia32 restore_i387'), but that is 32-bit specific, and the restore_i387() in the header file is 64-bit specific.

Hmm. In fact, I think that x86-64 version is actually only used in a single place - arch/x86/kernel/signal_64.c. So it's actively *wrong* to have that thing in a header file to begin with!

So how about this patch as a starting point? This is the RightThing(tm) to do regardless, and if it then makes it easier to do some other cleanups, we should do it first. What do you think?

Linus

---
 arch/x86/kernel/signal_64.c |   20 ++++++++++++++++++++
 include/asm-x86/i387.h      |   21 ---------------------
 2 files changed, 20 insertions(+), 21 deletions(-)

index 47c3d24..c40ddcb 100644
@@ -53,6 +53,26 @@ sys_sigaltstack(const stack_t __user *uss, stack_t __user *uoss,
 	return do_sigaltstack(uss, uoss, regs->sp);
 }
 
+/*
+ * This restores directly out of user space. Exceptions are handled.
+ */
+static inline int restore_i387(struct _fpstate __user *buf)
+{
+	struct task_struct *tsk = current;
+	int err;
+
+	if (!used_math()) {
+		err = init_fpu(tsk);
+		if (err)
+			return err;
+	}
+
+	if (!(task_thread_info(current)->status & TS_USEDFPU)) {
+		clts();
+		task_thread_info(current)->status |= TS_USEDFPU;
+	}
+	return restore_fpu_checking((__force struct i387_fxsave_struct *)buf);
+}
 
 /*
  * Do a signal return; undo the signal stack.

index 37672f7..a355264 100644
@@ -170,27 +170,6 @@ static inline int save_i387(struct _fpstate __user *buf)
 	return 1;
 }
 
-/*
- * This restores directly out of user space. Exceptions are handled.
- */
-static inline int restore_i387(struct _fpstate __user *buf)
-{
-	struct task_struct *tsk = current;
-	int err;
-
-	if (!used_math()) {
-		err = init_fpu(tsk);
-		if (err)
-			return err;
-	}
-
-	if (!(task_thread_info(current)->status & TS_USEDFPU)) {
-		clts();
-		task_thread_info(current)->status |= TS_USEDFPU;
-	}
-	return restore_fpu_checking((__force struct i387_fxsave_struct *)buf);
-}
-
 #else  /* CONFIG_X86_32 */
 
 extern void finit(void);

Today in ptrace (and in 32bit signal handling), we copy the user data and then clear the reserved bits blindly again ;)

In 64bit signal handling, we do a fxrstor from the live user buffer and scream then itself if we find an issue.

Andi being paranoid, added more checks for 64bit math_state_restore().

Sure. You have my Ack. I will request -stable folks to pickup multiple patches (second patch, I will post shortly on top of yours).

thanks,
suresh

BTW, while you are at it, please move save_i387() aswell, to be consistent and clean.

thanks,
suresh

Ack. Will do.

Linus

Linus, Attached is my last attempt for the day. Please review. Thanks.
---

restore_fpu_checking() calls init_fpu() in error conditions. While this is wrong(as our main intention is to clear the fpu state of the thread), this was benign before the commit 92d140e21f1ce8cf99320afbbcad73879128e6dc.

Post commit 92d140e21f1ce8cf99320afbbcad73879128e6dc, live FPU registers may not belong to this process at this error scenario.

In the error condition for restore_fpu_checking() (especially during the 64bit signal return), we are doing init_fpu(), which saves the live FPU register state (possibly belonging to some other process context) into the thread struct (through unlazy_fpu() in init_fpu()). This is wrong and can leak the FPU data.

For the signal handler restore error condition in restore_i387(), clear the fpu state present in the thread struct(before ultimately sending a SIGSEGV for badframe).

For the paranoid error condition check in math_state_restore(), send a SIGSEGV, if we fail to restore the state.

---

index b45ef8d..ca316b5 100644
@@ -104,7 +104,16 @@ static inline int restore_i387(struct _fpstate __user *buf)
 		clts();
 		task_thread_info(current)->status |= TS_USEDFPU;
 	}
-	return restore_fpu_checking((__force struct i387_fxsave_struct *)buf);
+	err = restore_fpu_checking((__force struct i387_fxsave_struct *)buf);
+	if (unlikely(err)) {
+		/*
+		 * Encountered an error while doing the restore from the
+		 * user buffer, clear the fpu state.
+		 */
+		clear_fpu(tsk);
+		clear_used_math();
+	}
+	return err;
 }
 
 /*

index 3f18d73..513caac 100644
@@ -1131,7 +1131,14 @@ asmlinkage void math_state_restore(void)
 	}
 
 	clts();				/* Allow maths ops (or we recurse) */
-	restore_fpu_checking(&me->thread.xstate->fxsave);
+	/*
+	 * Paranoid restore. send a SIGSEGV if we fail to restore the state.
+	 */
+	if (unlikely(restore_fpu_checking(&me->thread.xstate->fxsave))) {
+		stts();
+		force_sig(SIGSEGV, me);
+		return;
+	}
 	task_thread_info(me)->status |= TS_USEDFPU;
 	me->fpu_counter++;
 }

index 96fa844..0048fb7 100644
@@ -62,8 +62,6 @@ static inline int restore_fpu_checking(struct i387_fxsave_struct *fx)
 #else
 		     : [fx] "cdaSDb" (fx), "m" (*fx), "0" (0));
 #endif
-	if (unlikely(err))
-		init_fpu(current);
 	return err;
 }
 

i've applied your patch to tip/x86/fpu for further testing. I suspect once it proves reliable we can merge it into v2.6.27, after -rc1.

Ingo