linux-kernel - Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC

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 for Android: free password hash cracker in your pocket

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

Message-ID: <1517063185.6624.25.camel@infradead.org>
Date:   Sat, 27 Jan 2018 14:26:25 +0000
From:   David Woodhouse <dwmw2@...radead.org>
To:     Konrad Rzeszutek Wilk <konrad@...nok.org>,
        Peter Zijlstra <peterz@...radead.org>
Cc:     Dave Hansen <dave.hansen@...el.com>,
        Tim Chen <tim.c.chen@...ux.intel.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Andy Lutomirski <luto@...nel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Greg KH <gregkh@...uxfoundation.org>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Andi Kleen <ak@...ux.intel.com>,
        Arjan Van De Ven <arjan.van.de.ven@...el.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/8] x86/spec_ctrl: Add sysctl knobs to
 enable/disable SPEC_CTRL feature

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(). 
Download attachment "smime.p7s" of type "application/x-pkcs7-signature" (5213 bytes)