Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature

[David Woodhouse <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 1707 bytes --]

On Sat, 2018-01-27 at 08:59 -0500, Konrad Rzeszutek Wilk wrote:
> On Mon, Jan 08, 2018 at 05:14:37PM +0100, Peter Zijlstra wrote:
> > 
> > On Mon, Jan 08, 2018 at 01:47:07PM +0100, Peter Zijlstra wrote:
> > > 
> > > > 
> > > > a good suggestion, but we encountered some issues with it either
> > > > crashing the kernel at boot or not properly turning on/off.
> > The below boots, but I lack stuff to test the enabling.
> ..snip..
> > 
> > --- a/arch/x86/entry/calling.h
> > +++ b/arch/x86/entry/calling.h
> > @@ -373,22 +373,17 @@ For 32-bit we have the following convent
> >  .endm
> >  
> >  .macro ENABLE_IBRS
> > -	testl	$1, dynamic_ibrs
> > -	jz	.Lskip_\@
> > +	STATIC_JUMP_IF_FALSE .Lskip_\@, ibrs_key, def=0
> >  
> >  	PUSH_MSR_REGS
> >  	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS
> >  	POP_MSR_REGS
> > -	jmp	.Ldone_\@
> >  
> >  .Lskip_\@:
> > -	lfence
> > -.Ldone_\@:
> >  .endm
> I know that this particular patchset is now obsolete as the retpoline
> along with stuffing the RSB half or full is the preferred way.
> 
> But I am wondering - why was the 'lfence' added in the first place
> if dynamic_ibrs was zero?

This was in the system call path, just before the jmp *sys_call_table()
indirect jump. If an attacker could cause that 'jz .Lskip_\@' over the
wrmsr to be predicted *taken*, speculative execution would happily
proceed all the way to the indirect jump. Oops :)

Hence the 'else lfence' so that the branch-taken code path still stops
to wait. Hence also the subsequent insistence on using ALTERNATIVE for
it, and my paranoia about relying on GCC not missing optimisations if
we're using static_cpu_has(). 

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5213 bytes --]