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

[Dave Hansen]

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.