Re: [RFC PATCH v2 5/6] x86/entry/pti: avoid setting CR3 when it's already correct

From: Willy Tarreau <w@1wt.eu>
To: Dave Hansen <dave.hansen@linux.intel.com>
Cc: linux-kernel@vger.kernel.org, x86@kernel.org,
	Andy Lutomirski <luto@kernel.org>, Borislav Petkov <bp@alien8.de>,
	Brian Gerst <brgerst@gmail.com>, Ingo Molnar <mingo@kernel.org>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Josh Poimboeuf <jpoimboe@redhat.com>,
	"H. Peter Anvin" <hpa@zytor.com>,
	Kees Cook <keescook@chromium.org>
Subject: Re: [RFC PATCH v2 5/6] x86/entry/pti: avoid setting CR3 when it's already correct
Date: Thu, 11 Jan 2018 07:46:31 +0100	[thread overview]
Message-ID: <20180111064631.GD14920@1wt.eu> (raw)
In-Reply-To: <ae5cc387-f56e-4d8c-365a-8238e7e7d7bd@linux.intel.com>

On Wed, Jan 10, 2018 at 12:29:17PM -0800, Dave Hansen wrote:
> On 01/09/2018 04:56 AM, Willy Tarreau wrote:
> > --- a/arch/x86/entry/calling.h
> > +++ b/arch/x86/entry/calling.h
> > @@ -214,6 +214,11 @@
> >  .macro SWITCH_TO_KERNEL_CR3 scratch_reg:req
> >  	ALTERNATIVE "jmp .Lend_\@", "", X86_FEATURE_PTI
> >  	mov	%cr3, \scratch_reg
> > +
> > +	/* if we're already on the kernel PGD, we don't switch */
> > +	testq $(PTI_SWITCH_PGTABLES_MASK), \scratch_reg
> > +	jz .Lend_\@
> > +
> >  	ADJUST_KERNEL_CR3 \scratch_reg
> 
> Willy, this is not specific to your patch, but it is one of the first
> *changes* to this code since Spectre, thus I'm bringing it up in this
> thread.
> 
> The code already has some, but new conditional branches give me the
> willies.  None of them take the form that can be used to exploit
> Spectre/Variant1 that I can see, but I do think we need to start talking
> about why this is not vulnerable and probably documenting it in the
> entry code.
> 
> Spectre/V1 (conditional branches)
>  * Data reads in entry/exit when you have the user CR3 must be in
>    cpu_entry_area and readable to a Meltdown exploit, anyway.  That
>    implies that there is no data to be leaked in the address space
>    at all at this point.
>  * Conditional branches in the entry/exit code with a kernel CR3 value
>    are the only concern.  It is safe, however, if the data being checked
>    is not user-controllable.
> 
> Spectre/V2 (indirect branches)
>  * Indirect Branches in the entry code are forbidden because of
>    Spectre/V2.  Retpolines or other mitigations (IBRS) must be used
>    instead.
> 
> Anybody disagree?
> 
> In this case, the data being checked (CR3) is not user-controllable and
> there are no indirect branches.  So this code is OK.

Just let me know what comment you'd like me to add there. I suppose
something like "This code has code has been checked against Spectre/v1/v2
attacks ; it's safe as CR3 is not user-controllable and there's no
indirect branches" ?

Thanks,
Willy