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

[Suresh Siddha <suresh.b.siddha@intel.com>]

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