linux-kernel - Re: [PATCH] x86/retpoline: Also fill return buffer after idle

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: <1515506318.22302.46.camel@infradead.org>
Date:   Tue, 09 Jan 2018 13:58:38 +0000
From:   David Woodhouse <dwmw2@...radead.org>
To:     Peter Zijlstra <peterz@...radead.org>,
        Andi Kleen <andi@...stfloor.org>
Cc:     pjt@...gle.com, linux-kernel@...r.kernel.org,
        torvalds@...ux-foundation.org, gregkh@...ux-foundation.org,
        tim.c.chen@...ux.intel.com, dave.hansen@...el.com,
        tglx@...utronix.de, luto@...capital.net,
        Andi Kleen <ak@...ux.intel.com>
Subject: Re: [PATCH] x86/retpoline: Also fill return buffer after idle

On Tue, 2018-01-09 at 10:37 +0100, Peter Zijlstra wrote:
> On Mon, Jan 08, 2018 at 03:51:26PM -0800, Andi Kleen wrote:
> 
> > 
> > @@ -107,8 +109,15 @@ static inline void mwait_idle_with_hints(unsigned long eax, unsigned long ecx)
> >  		}
> >  
> >  		__monitor((void *)¤t_thread_info()->flags, 0, 0);
> > -		if (!need_resched())
> > +		if (!need_resched()) {
> >  			__mwait(eax, ecx);
> > +			/*
> > +			 * idle could have cleared the return buffer,
> > +			 * so fill it to prevent uncontrolled
> > +			 * speculation.
> > +			 */
> > +			fill_return_buffer();
> wouldn't something like:
> 
> 			if (static_cpu_has(X86_FEATURE_RETPOLINE))
> 				fill_return_buffer();
> 
> be much saner? Then we avoid the entire call when not needed and you
> don't have to muck with the asm either.

Hm...

The background, of course, that that we need to be careful when doing
things like this. If you end up with a conditional branch there, then
processor can speculate right past it. There's a reason a lot of the
IBRS-setting code has, effectively, an 'else lfence' in the cases where
it isn't being done with ALTERNATIVEs.

We had a *beautiful* case of that in the early IBRS patch set, on the
syscall path, where the conditional branch opened up a path for
speculative execution all the way to the jmp *sys_call_table(…).

Now, as discussed on IRC, we can see that the current implementation of
static_cpu_has using asm goto *is* generally doing the right thing and
turning it into a straight unconditional jump over the
fill_return_buffer() code. Clever GCC, have biscuit.

However, you are suggesting that we turn the static_cpu_has() trick
from a "nice to have" optimisation which is all very well when it pans
out, to something we *rely* on for secure operation of the system.

It never ends well when we rely on all versions of GCC optimising
things precisely how we want.

If you can build in a sanity check to ensure that the build will *fail*
when GCC doesn't do what we want, I suppose we could live with that.
But we don't have such a sanity check at the moment, do we?

Download attachment "smime.p7s" of type "application/x-pkcs7-signature" (5213 bytes)