Re: [patch] x64, fpu: fix possible FPU leakage in error conditions

[Linus Torvalds]

On Thu, 24 Jul 2008, Suresh Siddha wrote:
> > 
> > but the thing is, the only really valid reason for "restore_i387()" to
> > fail is because the read failed.
> 
> Not really. It can cause #GP, if someone sets reserved bits of mxcsr
> in the memory image.

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?

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

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

diff --git a/arch/x86/kernel/signal_64.c b/arch/x86/kernel/signal_64.c
index 47c3d24..c40ddcb 100644
--- a/arch/x86/kernel/signal_64.c
+++ b/arch/x86/kernel/signal_64.c
@@ -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.
diff --git a/include/asm-x86/i387.h b/include/asm-x86/i387.h
index 37672f7..a355264 100644
--- a/include/asm-x86/i387.h
+++ b/include/asm-x86/i387.h
@@ -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);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/