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

[David Woodhouse]

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?

Attachment: smime.p7s
Description: S/MIME cryptographic signature