* [stable 1/2] x86-64: Clean up save/restore_i387() usage
@ 2008-08-18 18:35 Suresh Siddha
2008-08-18 18:35 ` [stable 2/2] x64, fpu: fix possible FPU leakage in error conditions Suresh Siddha
2008-08-18 18:48 ` [stable 1/2] x86-64: Clean up save/restore_i387() usage Linus Torvalds
0 siblings, 2 replies; 4+ messages in thread
From: Suresh Siddha @ 2008-08-18 18:35 UTC (permalink / raw)
To: greg, mingo; +Cc: stable, linux-kernel, Suresh Siddha, Linus Torvalds
[-- 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);
--
^ permalink raw reply [flat|nested] 4+ messages in thread
* [stable 2/2] x64, fpu: fix possible FPU leakage in error conditions
2008-08-18 18:35 [stable 1/2] x86-64: Clean up save/restore_i387() usage Suresh Siddha
@ 2008-08-18 18:35 ` Suresh Siddha
2008-08-18 18:48 ` [stable 1/2] x86-64: Clean up save/restore_i387() usage Linus Torvalds
1 sibling, 0 replies; 4+ messages in thread
From: Suresh Siddha @ 2008-08-18 18:35 UTC (permalink / raw)
To: greg, mingo; +Cc: stable, linux-kernel, Suresh Siddha, Linus Torvalds
[-- 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;
}
--
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [stable 1/2] x86-64: Clean up save/restore_i387() usage
2008-08-18 18:35 [stable 1/2] x86-64: Clean up save/restore_i387() usage Suresh Siddha
2008-08-18 18:35 ` [stable 2/2] x64, fpu: fix possible FPU leakage in error conditions Suresh Siddha
@ 2008-08-18 18:48 ` Linus Torvalds
2008-08-18 19:32 ` Suresh Siddha
1 sibling, 1 reply; 4+ messages in thread
From: Linus Torvalds @ 2008-08-18 18:48 UTC (permalink / raw)
To: Suresh Siddha; +Cc: greg, mingo, stable, linux-kernel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [stable 1/2] x86-64: Clean up save/restore_i387() usage
2008-08-18 18:48 ` [stable 1/2] x86-64: Clean up save/restore_i387() usage Linus Torvalds
@ 2008-08-18 19:32 ` Suresh Siddha
0 siblings, 0 replies; 4+ messages in thread
From: Suresh Siddha @ 2008-08-18 19:32 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Siddha, Suresh B, greg, mingo, stable, linux-kernel
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);
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2008-08-18 19:33 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-08-18 18:35 [stable 1/2] x86-64: Clean up save/restore_i387() usage Suresh Siddha
2008-08-18 18:35 ` [stable 2/2] x64, fpu: fix possible FPU leakage in error conditions Suresh Siddha
2008-08-18 18:48 ` [stable 1/2] x86-64: Clean up save/restore_i387() usage Linus Torvalds
2008-08-18 19:32 ` Suresh Siddha
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).