[stable 1/2] x86-64: Clean up save/restore_i387() usage

[stable 1/2] x86-64: Clean up save/restore_i387() usage

[Suresh Siddha]

[-- Attachment #1: fpu_code_cleanup.patch --]
[-- Type: text/plain, Size: 4241 bytes --]

[Upstream commit: b30f3ae50cd03ef2ff433a5030fbf88dd8323528]

Suresh Siddha wants to fix a possible FPU leakage in error conditions,
but the fact that save/restore_i387() are inlines in a header file makes
that harder to do than necessary.  So start off with an obvious cleanup.

This just moves the x86-64 version of save/restore_i387() out of the
header file, and moves it to the only file that it is actually used in:
arch/x86/kernel/signal_64.c.  So exposing it in a header file was wrong
to begin with.

[ Side note: I'd like to fix up some of the games we play with the
  32-bit version of these functions too, but that's a separate
  matter.  The 32-bit versions are shared - under different names
  at that! - by both the native x86-32 code and the x86-64 32-bit
  compatibility code ]

Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>
Cc: <stable@kernel.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---

Index: linux-2.6.26.2/arch/x86/kernel/signal_64.c
===================================================================
--- linux-2.6.26.2.orig/arch/x86/kernel/signal_64.c	2008-08-06 09:19:01.000000000 -0700
+++ linux-2.6.26.2/arch/x86/kernel/signal_64.c	2008-08-18 11:16:43.000000000 -0700
@@ -53,6 +53,59 @@
 	return do_sigaltstack(uss, uoss, regs->sp);
 }
 
+/*
+ * Signal frame handlers.
+ */
+
+static inline int save_i387(struct _fpstate __user *buf)
+{
+	struct task_struct *tsk = current;
+	int err = 0;
+
+	BUILD_BUG_ON(sizeof(struct user_i387_struct) !=
+			sizeof(tsk->thread.xstate->fxsave));
+
+	if ((unsigned long)buf % 16)
+		printk("save_i387: bad fpstate %p\n", buf);
+
+	if (!used_math())
+		return 0;
+	clear_used_math(); /* trigger finit */
+	if (task_thread_info(tsk)->status & TS_USEDFPU) {
+		err = save_i387_checking((struct i387_fxsave_struct __user *)
+					 buf);
+		if (err)
+			return err;
+		task_thread_info(tsk)->status &= ~TS_USEDFPU;
+		stts();
+	} else {
+		if (__copy_to_user(buf, &tsk->thread.xstate->fxsave,
+				   sizeof(struct i387_fxsave_struct)))
+			return -1;
+	}
+	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);
+}
 
 /*
  * Do a signal return; undo the signal stack.
Index: linux-2.6.26.2/include/asm-x86/i387.h
===================================================================
--- linux-2.6.26.2.orig/include/asm-x86/i387.h	2008-08-06 09:19:01.000000000 -0700
+++ linux-2.6.26.2/include/asm-x86/i387.h	2008-08-18 11:16:43.000000000 -0700
@@ -137,60 +137,6 @@
 	task_thread_info(tsk)->status &= ~TS_USEDFPU;
 }
 
-/*
- * Signal frame handlers.
- */
-
-static inline int save_i387(struct _fpstate __user *buf)
-{
-	struct task_struct *tsk = current;
-	int err = 0;
-
-	BUILD_BUG_ON(sizeof(struct user_i387_struct) !=
-			sizeof(tsk->thread.xstate->fxsave));
-
-	if ((unsigned long)buf % 16)
-		printk("save_i387: bad fpstate %p\n", buf);
-
-	if (!used_math())
-		return 0;
-	clear_used_math(); /* trigger finit */
-	if (task_thread_info(tsk)->status & TS_USEDFPU) {
-		err = save_i387_checking((struct i387_fxsave_struct __user *)
-					 buf);
-		if (err)
-			return err;
-		task_thread_info(tsk)->status &= ~TS_USEDFPU;
-		stts();
-	} else {
-		if (__copy_to_user(buf, &tsk->thread.xstate->fxsave,
-				   sizeof(struct i387_fxsave_struct)))
-			return -1;
-	}
-	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);

-- 

[stable 2/2] x64, fpu: fix possible FPU leakage in error conditions

[Suresh Siddha]

[-- Attachment #1: fix_fpu_leakage.patch --]
[-- Type: text/plain, Size: 3284 bytes --]

[Upstream commit: 6ffac1e90a17ea0aded5c581204397421eec91b6]

On Thu, Jul 24, 2008 at 03:43:44PM -0700, Linus Torvalds wrote:
> 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?

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 commit 92d140e21f1 ("x86: fix taking
DNA during 64bit sigreturn").

Post commit 92d140e21f1, 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.

Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
Cc: <stable@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---

Index: linux-2.6.26.2/arch/x86/kernel/signal_64.c
===================================================================
--- linux-2.6.26.2.orig/arch/x86/kernel/signal_64.c	2008-08-18 11:16:43.000000000 -0700
+++ linux-2.6.26.2/arch/x86/kernel/signal_64.c	2008-08-18 11:16:50.000000000 -0700
@@ -104,7 +104,16 @@
 		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: linux-2.6.26.2/arch/x86/kernel/traps_64.c
===================================================================
--- linux-2.6.26.2.orig/arch/x86/kernel/traps_64.c	2008-08-06 09:19:01.000000000 -0700
+++ linux-2.6.26.2/arch/x86/kernel/traps_64.c	2008-08-18 11:17:43.000000000 -0700
@@ -1141,7 +1141,14 @@
 	}
 
 	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: linux-2.6.26.2/include/asm-x86/i387.h
===================================================================
--- linux-2.6.26.2.orig/include/asm-x86/i387.h	2008-08-18 11:16:43.000000000 -0700
+++ linux-2.6.26.2/include/asm-x86/i387.h	2008-08-18 11:16:50.000000000 -0700
@@ -62,8 +62,6 @@
 #else
 		     : [fx] "cdaSDb" (fx), "m" (*fx), "0" (0));
 #endif
-	if (unlikely(err))
-		init_fpu(current);
 	return err;
 }
 

-- 

Re: [stable 1/2] x86-64: Clean up save/restore_i387() usage

[Linus Torvalds]

On Mon, 18 Aug 2008, Suresh Siddha wrote:
>
> [Upstream commit: b30f3ae50cd03ef2ff433a5030fbf88dd8323528]

And it's authorship got destroyed by you.

Please guys, add teh proper "From:" line at the top. Don't forward patches 
like this unattributed. It doesn't matter if they are trivial or not, it's 
just really bad form. 

			Linus

Re: [stable 1/2] x86-64: Clean up save/restore_i387() usage

[Suresh Siddha]

On Mon, Aug 18, 2008 at 11:48:12AM -0700, Linus Torvalds wrote:
> 
> On Mon, 18 Aug 2008, Suresh Siddha wrote:
> >
> > [Upstream commit: b30f3ae50cd03ef2ff433a5030fbf88dd8323528]
> 
> And it's authorship got destroyed by you.
> 
> Please guys, add teh proper "From:" line at the top. Don't forward patches
> like this unattributed. It doesn't matter if they are trivial or not, it's
> just really bad form.

Sorry! Noticed it while sending but ignored. Will fix up my scripts
accordingly.

Greg, please ignore the earlier patch titled,
	[stable 1/2] x86-64: Clean up save/restore_i387() usage

And accept the appended one, instead: Thanks.
---
From: Linus Torvalds <torvalds@linux-foundation.org>
Subject: x86-64: Clean up 'save/restore_i387()' usage

    [ Upstream commit b30f3ae50cd03ef2ff433a5030fbf88dd8323528]

    Suresh Siddha wants to fix a possible FPU leakage in error conditions,
    but the fact that save/restore_i387() are inlines in a header file makes
    that harder to do than necessary.  So start off with an obvious cleanup.
    
    This just moves the x86-64 version of save/restore_i387() out of the
    header file, and moves it to the only file that it is actually used in:
    arch/x86/kernel/signal_64.c.  So exposing it in a header file was wrong
    to begin with.
    
    [ Side note: I'd like to fix up some of the games we play with the
      32-bit version of these functions too, but that's a separate
      matter.  The 32-bit versions are shared - under different names
      at that! - by both the native x86-32 code and the x86-64 32-bit
      compatibility code ]
    
    Acked-by: Suresh Siddha <suresh.b.siddha@intel.com>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

diff --git a/arch/x86/kernel/signal_64.c b/arch/x86/kernel/signal_64.c
index 47c3d24..b45ef8d 100644
--- a/arch/x86/kernel/signal_64.c
+++ b/arch/x86/kernel/signal_64.c
@@ -53,6 +53,59 @@ sys_sigaltstack(const stack_t __user *uss, stack_t __user *uoss,
 	return do_sigaltstack(uss, uoss, regs->sp);
 }
 
+/*
+ * Signal frame handlers.
+ */
+
+static inline int save_i387(struct _fpstate __user *buf)
+{
+	struct task_struct *tsk = current;
+	int err = 0;
+
+	BUILD_BUG_ON(sizeof(struct user_i387_struct) !=
+			sizeof(tsk->thread.xstate->fxsave));
+
+	if ((unsigned long)buf % 16)
+		printk("save_i387: bad fpstate %p\n", buf);
+
+	if (!used_math())
+		return 0;
+	clear_used_math(); /* trigger finit */
+	if (task_thread_info(tsk)->status & TS_USEDFPU) {
+		err = save_i387_checking((struct i387_fxsave_struct __user *)
+					 buf);
+		if (err)
+			return err;
+		task_thread_info(tsk)->status &= ~TS_USEDFPU;
+		stts();
+	} else {
+		if (__copy_to_user(buf, &tsk->thread.xstate->fxsave,
+				   sizeof(struct i387_fxsave_struct)))
+			return -1;
+	}
+	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);
+}
 
 /*
  * Do a signal return; undo the signal stack.
diff --git a/include/asm-x86/i387.h b/include/asm-x86/i387.h
index 37672f7..96fa844 100644
--- a/include/asm-x86/i387.h
+++ b/include/asm-x86/i387.h
@@ -137,60 +137,6 @@ static inline void __save_init_fpu(struct task_struct *tsk)
 	task_thread_info(tsk)->status &= ~TS_USEDFPU;
 }
 
-/*
- * Signal frame handlers.
- */
-
-static inline int save_i387(struct _fpstate __user *buf)
-{
-	struct task_struct *tsk = current;
-	int err = 0;
-
-	BUILD_BUG_ON(sizeof(struct user_i387_struct) !=
-			sizeof(tsk->thread.xstate->fxsave));
-
-	if ((unsigned long)buf % 16)
-		printk("save_i387: bad fpstate %p\n", buf);
-
-	if (!used_math())
-		return 0;
-	clear_used_math(); /* trigger finit */
-	if (task_thread_info(tsk)->status & TS_USEDFPU) {
-		err = save_i387_checking((struct i387_fxsave_struct __user *)
-					 buf);
-		if (err)
-			return err;
-		task_thread_info(tsk)->status &= ~TS_USEDFPU;
-		stts();
-	} else {
-		if (__copy_to_user(buf, &tsk->thread.xstate->fxsave,
-				   sizeof(struct i387_fxsave_struct)))
-			return -1;
-	}
-	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);