x86-64: Clean up save/restore_i387() usage

PATCH In review Suresh Siddha 2 participants 4 messages Aug 18, 2008

lore

Share & tools

lore

Suresh Siddha

author

2008-08-18 18:41 UTC #0 lore raw

PATCH 1/2 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 <[email protected]>

Cc <[email protected]>

Signed-off-by Linus Torvalds <[email protected]>

---

arch/x86/kernel/signal_64.c +53 -0

Index: linux-2.6.26.2/arch/x86/kernel/signal_64.c
===================================================================
@@ -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.

include/asm-x86/i387.h +0 ~~-54~~

60 lines not on this page · raw message

Suresh Siddha

author

2008-08-18 18:40 UTC #1 lore raw

PATCH 2/2 x64, fpu: fix possible FPU leakage in error conditions

[Upstream commit: 6ffac1e90a17ea0aded5c581204397421eec91b6]

Quoted 3 lines · Linus Torvalds

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 <[email protected]>

Cc <[email protected]>

Cc Linus Torvalds <[email protected]>

Signed-off-by Ingo Molnar <[email protected]>

---

arch/x86/kernel/signal_64.c +10 -1

Index: linux-2.6.26.2/arch/x86/kernel/signal_64.c
===================================================================
@@ -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;
 }
 
 /*

arch/x86/kernel/traps_64.c +8 -1

Index: linux-2.6.26.2/arch/x86/kernel/traps_64.c
===================================================================
@@ -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++;
 }

include/asm-x86/i387.h +0 -2

Index: linux-2.6.26.2/include/asm-x86/i387.h
===================================================================
@@ -62,8 +62,6 @@
 #else
 		     : [fx] "cdaSDb" (fx), "m" (*fx), "0" (0));
 #endif
-	if (unlikely(err))
-		init_fpu(current);
 	return err;
 }

Linus Torvalds

2008-08-18 18:49 UTC #2 lore raw

In reply to Suresh Siddha

Quoted 2 lines · Suresh Siddha

[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

Suresh Siddha

author Acked-by

2008-08-18 19:33 UTC #3 lore raw

Quoted 10 lines · Linus Torvalds

Quoted 2 lines · Suresh Siddha
[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 <[email protected]>
Subject: x86-64: Clean up 'save/restore_i387()' usage

[ Upstream commit b30f3ae50cd03ef2ff433a5030fbf88dd8323528]

Acked-by Suresh Siddha <[email protected]>

Signed-off-by Linus Torvalds <[email protected]>

arch/x86/kernel/signal_64.c +53 -0

index 47c3d24..b45ef8d 100644
@@ -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.

include/asm-x86/i387.h +0 ~~-54~~

60 lines not on this page · raw message