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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.LFD.1.10.0807241431180.8109@nehalem.linux-foundation.org>
Date:	Thu, 24 Jul 2008 14:54:31 -0700 (PDT)
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Suresh Siddha <suresh.b.siddha@...el.com>
cc:	"x86@...nel.org" <x86@...nel.org>,
	"andi@...stfloor.org" <andi@...stfloor.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	"stable@...nel.org" <stable@...nel.org>,
	Ingo Molnar <mingo@...e.hu>
Subject: Re: [patch] x64, fpu: fix possible FPU leakage in error conditions

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/