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

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

[Suresh Siddha]

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.

Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
---

diff --git a/arch/x86/kernel/i387.c b/arch/x86/kernel/i387.c
index eb9ddd8..f5c2161 100644
--- a/arch/x86/kernel/i387.c
+++ b/arch/x86/kernel/i387.c
@@ -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.
 	 */
diff --git a/include/asm-x86/i387.h b/include/asm-x86/i387.h
index 37672f7..38af0ed 100644
--- a/include/asm-x86/i387.h
+++ b/include/asm-x86/i387.h
@@ -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;
 }
 

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

[Linus Torvalds]

On Thu, 24 Jul 2008, Suresh Siddha wrote:
> 
> 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.

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

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

[Suresh Siddha]

On Thu, Jul 24, 2008 at 11:31:42AM -0700, Linus Torvalds wrote:
> 
> 
> On Thu, 24 Jul 2008, Suresh Siddha wrote:
> >
> > 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.
> 
> Why? The thread struct is guaranteed to contain pointless data.

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

> 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, this was what I had in mind earlier and should be ok for signal handling
case. But as you also noted below:

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

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

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

[Linus Torvalds]

On Thu, 24 Jul 2008, Suresh Siddha wrote:
> 
> 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()

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

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

[Suresh Siddha]

On Thu, Jul 24, 2008 at 11:59:03AM -0700, Linus Torvalds wrote:
> 
> On Thu, 24 Jul 2008, Suresh Siddha wrote:
> >
> > 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()
> 
> 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.

Ok.

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

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

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

[Linus Torvalds]

On Thu, 24 Jul 2008, Suresh Siddha wrote:
>
> Only the first init_fpu() can fail (memory allocation failure). But here,
> it is def not the first time.

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

		Linus

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

[Suresh Siddha]

On Thu, Jul 24, 2008 at 01:30:51PM -0700, Linus Torvalds wrote:
> 
> 
> On Thu, 24 Jul 2008, Suresh Siddha wrote:
> >
> > Only the first init_fpu() can fail (memory allocation failure). But here,
> > it is def not the first time.
> 
> Ahh. Agreed. I missed that. But yes, clear_fpu() is still preferable, I
> think.

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.

Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
---

diff --git a/arch/x86/kernel/signal_64.c b/arch/x86/kernel/signal_64.c
index bf87684..672f35a 100644
--- a/arch/x86/kernel/signal_64.c
+++ b/arch/x86/kernel/signal_64.c
@@ -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();
diff --git a/arch/x86/kernel/traps_64.c b/arch/x86/kernel/traps_64.c
index 3f18d73..6abf39c 100644
--- a/arch/x86/kernel/traps_64.c
+++ b/arch/x86/kernel/traps_64.c
@@ -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++;
 }
diff --git a/include/asm-x86/i387.h b/include/asm-x86/i387.h
index 37672f7..d46d591 100644
--- a/include/asm-x86/i387.h
+++ b/include/asm-x86/i387.h
@@ -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;
 }
 

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

[Linus Torvalds]

On Thu, 24 Jul 2008, Suresh Siddha wrote:
> 
> Meanwhile, I wanted to keep this patch simple, so that it can be easily
> applied to -stable series aswell.

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

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

[Suresh Siddha]

On Thu, Jul 24, 2008 at 02:54:31PM -0700, Linus Torvalds wrote:
> 
> 
> On Thu, 24 Jul 2008, Suresh Siddha wrote:
> >
> > Meanwhile, I wanted to keep this patch simple, so that it can be easily
> > applied to -stable series aswell.
> 
> 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.

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

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

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

> 
> 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?

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

> 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?

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

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

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

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

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

[Suresh Siddha]

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

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

> > 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?

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

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

[Suresh Siddha]

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

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

thanks,
suresh

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

[Linus Torvalds]

On Thu, 24 Jul 2008, Suresh Siddha wrote:
> 
> BTW, while you are at it, please move save_i387() aswell, to be consistent
> and clean.

Ack. Will do.

		Linus

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

[Suresh Siddha]

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?

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.

Signed-off-by: Suresh Siddha <suresh.b.siddha@intel.com>
---

diff --git a/arch/x86/kernel/signal_64.c b/arch/x86/kernel/signal_64.c
index b45ef8d..ca316b5 100644
--- a/arch/x86/kernel/signal_64.c
+++ b/arch/x86/kernel/signal_64.c
@@ -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;
 }
 
 /*
diff --git a/arch/x86/kernel/traps_64.c b/arch/x86/kernel/traps_64.c
index 3f18d73..513caac 100644
--- a/arch/x86/kernel/traps_64.c
+++ b/arch/x86/kernel/traps_64.c
@@ -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++;
 }
diff --git a/include/asm-x86/i387.h b/include/asm-x86/i387.h
index 96fa844..0048fb7 100644
--- a/include/asm-x86/i387.h
+++ b/include/asm-x86/i387.h
@@ -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;
 }
 

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

[Ingo Molnar]

* Suresh Siddha <suresh.b.siddha@intel.com> wrote:

> 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?
> 
> 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.

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