[PATCH v2 0/3] x86/entry: Clear registers to sanitize speculative usages

[PATCH v2 0/3] x86/entry: Clear registers to sanitize speculative usages

[Dan Williams]

Changes since v1 [1]:
* Move CLEAR_REGS_EXTRA_NOSPEC before TRACE_IRQS_OFF to protect tracing
  users (Andy)

[1]: https://lkml.org/lkml/2018/2/3/397

---

At entry userspace may have populated callee saved registers with values
that could be useful in a speculative execution attack. Clear them to
minimize the kernel's attack surface.

Note, this is done to make it harder to find / manipulate exploitable
sequences in the kernel.

The clearing is limited to the 64-bit 'extra' registers since those are
the most likely to survive with user populated values deep into the call
chain. Normal register pressure likely clobbers values in the lower
registers and the 32-bit case.

As for cycle impact on my Sandy Bridge test system it can handle the xor
sequence at 3.5 instructions per cycle.

---

Andi Kleen (2):
      x86/entry: Clear registers for 64bit exceptions/interrupts
      x86/entry: Clear registers for compat syscalls

Dan Williams (1):
      x86/entry: Clear extra registers beyond syscall arguments for 64bit kernels


 arch/x86/entry/calling.h         |   17 +++++++++++++++++
 arch/x86/entry/entry_64.S        |    6 ++++++
 arch/x86/entry/entry_64_compat.S |    3 +++
 3 files changed, 26 insertions(+)

[PATCH v2 1/3] x86/entry: Clear extra registers beyond syscall arguments for 64bit kernels

[Dan Williams]

At entry userspace may have populated the extra registers outside the
syscall calling convention with values that could be useful in a
speculative execution attack. Clear them to minimize the kernel's attack
surface. Note, this only clears the extra registers and not the unused
registers for syscalls less than 6 arguments since those registers are
likely to be clobbered well before their values could be put to use
under speculation.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: Andy Lutomirski <luto@kernel.org>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Reported-by: Andi Kleen <ak@linux.intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 arch/x86/entry/calling.h  |   17 +++++++++++++++++
 arch/x86/entry/entry_64.S |    1 +
 2 files changed, 18 insertions(+)

diff --git a/arch/x86/entry/calling.h b/arch/x86/entry/calling.h
index 3f48f695d5e6..daee2d19e73d 100644
--- a/arch/x86/entry/calling.h
+++ b/arch/x86/entry/calling.h
@@ -147,6 +147,23 @@ For 32-bit we have the following conventions - kernel is built with
 	UNWIND_HINT_REGS offset=\offset
 	.endm
 
+	/*
+	 * Sanitize extra registers of values that a speculation attack
+	 * might want to exploit. In the CONFIG_FRAME_POINTER=y case,
+	 * the expectation is that %ebp will be clobbered before it
+	 * could be used.
+	 */
+	.macro CLEAR_EXTRA_REGS_NOSPEC
+	xorq %r15, %r15
+	xorq %r14, %r14
+	xorq %r13, %r13
+	xorq %r12, %r12
+	xorl %ebx, %ebx
+#ifndef CONFIG_FRAME_POINTER
+	xorl %ebp, %ebp
+#endif
+	.endm
+
 	.macro POP_EXTRA_REGS
 	popq %r15
 	popq %r14
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index c752abe89d80..5de9a5922026 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -244,6 +244,7 @@ GLOBAL(entry_SYSCALL_64_after_hwframe)
 	pushq	%r15				/* pt_regs->r15 */
 	UNWIND_HINT_REGS
 
+	CLEAR_EXTRA_REGS_NOSPEC
 	TRACE_IRQS_OFF
 
 	/* IRQs are off. */

[PATCH v2 2/3] x86/entry: Clear registers for 64bit exceptions/interrupts

[Dan Williams]

From: Andi Kleen <ak@linux.intel.com>

Clear the 'extra' registers on entering the 64bit kernel for exceptions
and interrupts. The common registers are not cleared since they are
likely clobbered well before they can be exploited in a speculative
execution attack.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 arch/x86/entry/entry_64.S |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index 5de9a5922026..2c3e5e326619 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -563,6 +563,7 @@ END(irq_entries_start)
 	ALLOC_PT_GPREGS_ON_STACK
 	SAVE_C_REGS
 	SAVE_EXTRA_REGS
+	CLEAR_EXTRA_REGS_NOSPEC
 	ENCODE_FRAME_POINTER
 
 	testb	$3, CS(%rsp)
@@ -1121,6 +1122,7 @@ ENTRY(xen_failsafe_callback)
 	ALLOC_PT_GPREGS_ON_STACK
 	SAVE_C_REGS
 	SAVE_EXTRA_REGS
+	CLEAR_EXTRA_REGS_NOSPEC
 	ENCODE_FRAME_POINTER
 	jmp	error_exit
 END(xen_failsafe_callback)
@@ -1166,6 +1168,7 @@ ENTRY(paranoid_entry)
 	cld
 	SAVE_C_REGS 8
 	SAVE_EXTRA_REGS 8
+	CLEAR_EXTRA_REGS_NOSPEC
 	ENCODE_FRAME_POINTER 8
 	movl	$1, %ebx
 	movl	$MSR_GS_BASE, %ecx
@@ -1218,6 +1221,7 @@ ENTRY(error_entry)
 	cld
 	SAVE_C_REGS 8
 	SAVE_EXTRA_REGS 8
+	CLEAR_EXTRA_REGS_NOSPEC
 	ENCODE_FRAME_POINTER 8
 	xorl	%ebx, %ebx
 	testb	$3, CS+8(%rsp)
@@ -1416,6 +1420,7 @@ ENTRY(nmi)
 	pushq	%r14		/* pt_regs->r14 */
 	pushq	%r15		/* pt_regs->r15 */
 	UNWIND_HINT_REGS
+	CLEAR_EXTRA_REGS_NOSPEC
 	ENCODE_FRAME_POINTER
 
 	/*

[PATCH v2 3/3] x86/entry: Clear registers for compat syscalls

[Dan Williams]

From: Andi Kleen <ak@linux.intel.com>

At entry userspace may have populated registers with values that could
be useful in a speculative execution attack. Clear them to minimize the
kernel's attack surface.

[djbw: rename the macro, only clear the extra registers]
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org
Cc: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Andi Kleen <ak@linux.intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 arch/x86/entry/entry_64_compat.S |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/entry/entry_64_compat.S b/arch/x86/entry/entry_64_compat.S
index 98d5358e4041..f55b018a580b 100644
--- a/arch/x86/entry/entry_64_compat.S
+++ b/arch/x86/entry/entry_64_compat.S
@@ -95,6 +95,7 @@ ENTRY(entry_SYSENTER_compat)
 	pushq   $0			/* pt_regs->r14 = 0 */
 	pushq   $0			/* pt_regs->r15 = 0 */
 	cld
+	CLEAR_EXTRA_REGS_NOSPEC
 
 	/*
 	 * SYSENTER doesn't filter flags, so we need to clear NT and AC
@@ -223,6 +224,7 @@ GLOBAL(entry_SYSCALL_compat_after_hwframe)
 	pushq   $0			/* pt_regs->r13 = 0 */
 	pushq   $0			/* pt_regs->r14 = 0 */
 	pushq   $0			/* pt_regs->r15 = 0 */
+	CLEAR_EXTRA_REGS_NOSPEC
 
 	/*
 	 * User mode is traced as though IRQs are on, and SYSENTER
@@ -348,6 +350,7 @@ ENTRY(entry_INT80_compat)
 	pushq   %r14                    /* pt_regs->r14 */
 	pushq   %r15                    /* pt_regs->r15 */
 	cld
+	CLEAR_EXTRA_REGS_NOSPEC
 
 	/*
 	 * User mode is traced as though IRQs are on, and the interrupt

Re: [PATCH v2 1/3] x86/entry: Clear extra registers beyond syscall arguments for 64bit kernels

[Ingo Molnar]

* Dan Williams <dan.j.williams@intel.com> wrote:

> +	/*
> +	 * Sanitize extra registers of values that a speculation attack
> +	 * might want to exploit. In the CONFIG_FRAME_POINTER=y case,
> +	 * the expectation is that %ebp will be clobbered before it
> +	 * could be used.
> +	 */
> +	.macro CLEAR_EXTRA_REGS_NOSPEC
> +	xorq %r15, %r15
> +	xorq %r14, %r14
> +	xorq %r13, %r13
> +	xorq %r12, %r12
> +	xorl %ebx, %ebx
> +#ifndef CONFIG_FRAME_POINTER
> +	xorl %ebp, %ebp
> +#endif
> +	.endm

Yeah, so this series look pretty good to me, but there's one small detail: I think 
RBP should be cleared unconditionally here, even in the CONFIG_FRAME_POINTERS=y 
case, because:

 - It's much easier to think about the validity of this code if we _know_ that
   these particular registers are cleared, so they cannot be used for deep 
   speculation. While typically on frame-pointer kernels most regular C function
   entry sequences will set RBP, there's exceptions:

     - There's various conditional pieces of entry code that run before any 
       RBP-clobbering C function is called. While none of them has an exploitable 
       Spectre 'gadget' at the moment, we'd have to consider this for every future
       change.

     - There's various compiler instrumentation that might run before RBP 
       clobbering of the typical C function prologue: -mfentry is one such case 
       (used by all distro kernels) compiler plugins might be another case. The 
       instrumentation is often hand written in assembly - which would thus have 
       to be 'RBP safe' as well.

     - Sanitizing RBP is not a hard requirement on the compiler: there's versions 
       of GCC where it won't set RBP, such as leaf functions - and other 
       compilers might have different defaults. This fact makes it harder to 
       ascertain that various C functions from low level assembly that we are 
       verifying for 'RBP safety' do indeed sanitize RBP under all circumstances.

   I.e. we cannot universally rely on RBP being sanitized. In _practice_ it will 
   be sanitized, but we don't know for sure without expending quite some effort to 
   think through all the cases.

 - CONFIG_FRAME_POINTERS=y is a non-default debug option with a significant 
   runtime cost that will get less and less testing as time goes forward. So 
   we are complicating the code path and are micro-optimizing an already 
   significantly slower debug build of the kernel.

So all things considered removing the #ifndef would make this angle easier to 
think about: let's just clear all the extra registers.

If you agree then there's no need to resend the series for this reason alone, I'll 
remove the #ifndef when applying the patches.

Thanks,

	Ingo

Re: [PATCH v2 1/3] x86/entry: Clear extra registers beyond syscall arguments for 64bit kernels

[Ingo Molnar]

* Dan Williams <dan.j.williams@intel.com> wrote:

> +	/*
> +	 * Sanitize extra registers of values that a speculation attack
> +	 * might want to exploit. In the CONFIG_FRAME_POINTER=y case,
> +	 * the expectation is that %ebp will be clobbered before it
> +	 * could be used.
> +	 */
> +	.macro CLEAR_EXTRA_REGS_NOSPEC
> +	xorq %r15, %r15
> +	xorq %r14, %r14
> +	xorq %r13, %r13
> +	xorq %r12, %r12
> +	xorl %ebx, %ebx
> +#ifndef CONFIG_FRAME_POINTER
> +	xorl %ebp, %ebp
> +#endif

BTW., is there any reason behind the order of the clearing of these registers? 
This ordering seems rather random:

 - The canonical register order is: RBX, RBP, R12, R13, R14, R15, which is also 
   their push-order on the stack.

 - The CLEAR_EXTRA_REGS_NOSPEC order appears to be the reverse order (pop-order), 
   but with RBX and RBP reversed.

So since this is a 'push side' primitive I'd use the regular (push-) ordering 
instead:

	.macro CLEAR_EXTRA_REGS_NOSPEC
	xorl %ebx, %ebx
	xorl %ebp, %ebp
	xorq %r12, %r12
	xorq %r13, %r13
	xorq %r14, %r14
	xorq %r15, %r15

It obviously doesn't matter to correctness - only to readability.

There's also a (very) small micro-optimization argument in favor of the regular 
order: the earlier registers are more likely to be utilized by C functions, so the 
sooner we clear them, the less potential interaction these clearing instructions 
are going to have with any later use.

Thanks,

	Ingo

Re: [PATCH v2 1/3] x86/entry: Clear extra registers beyond syscall arguments for 64bit kernels

[Andy Lutomirski]

> On Feb 5, 2018, at 3:42 AM, Ingo Molnar <mingo@kernel.org> wrote:
> 
> 
> * Dan Williams <dan.j.williams@intel.com> wrote:
> 
>> +    /*
>> +     * Sanitize extra registers of values that a speculation attack
>> +     * might want to exploit. In the CONFIG_FRAME_POINTER=y case,
>> +     * the expectation is that %ebp will be clobbered before it
>> +     * could be used.
>> +     */
>> +    .macro CLEAR_EXTRA_REGS_NOSPEC
>> +    xorq %r15, %r15
>> +    xorq %r14, %r14
>> +    xorq %r13, %r13
>> +    xorq %r12, %r12
>> +    xorl %ebx, %ebx
>> +#ifndef CONFIG_FRAME_POINTER
>> +    xorl %ebp, %ebp
>> +#endif
>> +    .endm
> 
> Yeah, so this series look pretty good to me, but there's one small detail: I think 
> RBP should be cleared unconditionally here, even in the CONFIG_FRAME_POINTERS=y 
> case, because:

ENCODE_FRAME_POINTER should take care of rbp, though.

Re: [PATCH v2 1/3] x86/entry: Clear extra registers beyond syscall arguments for 64bit kernels

[Andi Kleen]

>      - There's various conditional pieces of entry code that run before any 
>        RBP-clobbering C function is called. While none of them has an exploitable 
>        Spectre 'gadget' at the moment, we'd have to consider this for every future
>        change.

The Frame Pointer is always set up in assembler too, just in another macro.

There should be only a few pushes between this clear and the set up,
which are not exploitable.

But yes in the end it doesn't matter much either way so the ifdef
could be removed for slightly nicer looking code.

-Andi

Re: [PATCH v2 1/3] x86/entry: Clear extra registers beyond syscall arguments for 64bit kernels

[Ingo Molnar]

* Andy Lutomirski <luto@amacapital.net> wrote:

> 
> 
> > On Feb 5, 2018, at 3:42 AM, Ingo Molnar <mingo@kernel.org> wrote:
> > 
> > 
> > * Dan Williams <dan.j.williams@intel.com> wrote:
> > 
> >> +    /*
> >> +     * Sanitize extra registers of values that a speculation attack
> >> +     * might want to exploit. In the CONFIG_FRAME_POINTER=y case,
> >> +     * the expectation is that %ebp will be clobbered before it
> >> +     * could be used.
> >> +     */
> >> +    .macro CLEAR_EXTRA_REGS_NOSPEC
> >> +    xorq %r15, %r15
> >> +    xorq %r14, %r14
> >> +    xorq %r13, %r13
> >> +    xorq %r12, %r12
> >> +    xorl %ebx, %ebx
> >> +#ifndef CONFIG_FRAME_POINTER
> >> +    xorl %ebp, %ebp
> >> +#endif
> >> +    .endm
> > 
> > Yeah, so this series look pretty good to me, but there's one small detail: I think 
> > RBP should be cleared unconditionally here, even in the CONFIG_FRAME_POINTERS=y 
> > case, because:
> 
> ENCODE_FRAME_POINTER should take care of rbp, though.

AFAICS there's various entry paths where it's not used I think: for example the 
compat system calls in entry_64_compat.S don't seem to encode RBP in such a 
fashion (unless I missed some macro side effect).

Basically I'd iterate these things the following way:

 - where there's justified, demonstrable doubt we should be clearing input values 
   in a simple, robust fashion (as it seems to be in this particular case)

 - then we can micro-optimize again when we are convinced that it's safe

Otherwise I'm worried about the speed with which we converge to Spectre 
correctness.

Thanks,

	Ingo

Re: [PATCH v2 1/3] x86/entry: Clear extra registers beyond syscall arguments for 64bit kernels

[Andy Lutomirski]

On Mon, Feb 5, 2018 at 4:30 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Andy Lutomirski <luto@amacapital.net> wrote:
>
>>
>>
>> > On Feb 5, 2018, at 3:42 AM, Ingo Molnar <mingo@kernel.org> wrote:
>> >
>> >
>> > * Dan Williams <dan.j.williams@intel.com> wrote:
>> >
>> >> +    /*
>> >> +     * Sanitize extra registers of values that a speculation attack
>> >> +     * might want to exploit. In the CONFIG_FRAME_POINTER=y case,
>> >> +     * the expectation is that %ebp will be clobbered before it
>> >> +     * could be used.
>> >> +     */
>> >> +    .macro CLEAR_EXTRA_REGS_NOSPEC
>> >> +    xorq %r15, %r15
>> >> +    xorq %r14, %r14
>> >> +    xorq %r13, %r13
>> >> +    xorq %r12, %r12
>> >> +    xorl %ebx, %ebx
>> >> +#ifndef CONFIG_FRAME_POINTER
>> >> +    xorl %ebp, %ebp
>> >> +#endif
>> >> +    .endm
>> >
>> > Yeah, so this series look pretty good to me, but there's one small detail: I think
>> > RBP should be cleared unconditionally here, even in the CONFIG_FRAME_POINTERS=y
>> > case, because:
>>
>> ENCODE_FRAME_POINTER should take care of rbp, though.
>
> AFAICS there's various entry paths where it's not used I think: for example the
> compat system calls in entry_64_compat.S don't seem to encode RBP in such a
> fashion (unless I missed some macro side effect).

Then that's a separate bug that should be fixed.  Josh?

Re: [PATCH v2 1/3] x86/entry: Clear extra registers beyond syscall arguments for 64bit kernels

[Josh Poimboeuf]

On Mon, Feb 05, 2018 at 04:37:26PM +0000, Andy Lutomirski wrote:
> On Mon, Feb 5, 2018 at 4:30 PM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * Andy Lutomirski <luto@amacapital.net> wrote:
> >
> >>
> >>
> >> > On Feb 5, 2018, at 3:42 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >> >
> >> >
> >> > * Dan Williams <dan.j.williams@intel.com> wrote:
> >> >
> >> >> +    /*
> >> >> +     * Sanitize extra registers of values that a speculation attack
> >> >> +     * might want to exploit. In the CONFIG_FRAME_POINTER=y case,
> >> >> +     * the expectation is that %ebp will be clobbered before it
> >> >> +     * could be used.
> >> >> +     */
> >> >> +    .macro CLEAR_EXTRA_REGS_NOSPEC
> >> >> +    xorq %r15, %r15
> >> >> +    xorq %r14, %r14
> >> >> +    xorq %r13, %r13
> >> >> +    xorq %r12, %r12
> >> >> +    xorl %ebx, %ebx
> >> >> +#ifndef CONFIG_FRAME_POINTER
> >> >> +    xorl %ebp, %ebp
> >> >> +#endif
> >> >> +    .endm
> >> >
> >> > Yeah, so this series look pretty good to me, but there's one small detail: I think
> >> > RBP should be cleared unconditionally here, even in the CONFIG_FRAME_POINTERS=y
> >> > case, because:
> >>
> >> ENCODE_FRAME_POINTER should take care of rbp, though.
> >
> > AFAICS there's various entry paths where it's not used I think: for example the
> > compat system calls in entry_64_compat.S don't seem to encode RBP in such a
> > fashion (unless I missed some macro side effect).
> 
> Then that's a separate bug that should be fixed.  Josh?

We don't encode the frame pointer on syscalls, because "fast path"
(though that's obviously no longer a consideration).

-- 
Josh

Re: [PATCH v2 1/3] x86/entry: Clear extra registers beyond syscall arguments for 64bit kernels

[Andy Lutomirski]

On Mon, Feb 5, 2018 at 5:26 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> On Mon, Feb 05, 2018 at 04:37:26PM +0000, Andy Lutomirski wrote:
>> On Mon, Feb 5, 2018 at 4:30 PM, Ingo Molnar <mingo@kernel.org> wrote:
>> >
>> > * Andy Lutomirski <luto@amacapital.net> wrote:
>> >
>> >>
>> >>
>> >> > On Feb 5, 2018, at 3:42 AM, Ingo Molnar <mingo@kernel.org> wrote:
>> >> >
>> >> >
>> >> > * Dan Williams <dan.j.williams@intel.com> wrote:
>> >> >
>> >> >> +    /*
>> >> >> +     * Sanitize extra registers of values that a speculation attack
>> >> >> +     * might want to exploit. In the CONFIG_FRAME_POINTER=y case,
>> >> >> +     * the expectation is that %ebp will be clobbered before it
>> >> >> +     * could be used.
>> >> >> +     */
>> >> >> +    .macro CLEAR_EXTRA_REGS_NOSPEC
>> >> >> +    xorq %r15, %r15
>> >> >> +    xorq %r14, %r14
>> >> >> +    xorq %r13, %r13
>> >> >> +    xorq %r12, %r12
>> >> >> +    xorl %ebx, %ebx
>> >> >> +#ifndef CONFIG_FRAME_POINTER
>> >> >> +    xorl %ebp, %ebp
>> >> >> +#endif
>> >> >> +    .endm
>> >> >
>> >> > Yeah, so this series look pretty good to me, but there's one small detail: I think
>> >> > RBP should be cleared unconditionally here, even in the CONFIG_FRAME_POINTERS=y
>> >> > case, because:
>> >>
>> >> ENCODE_FRAME_POINTER should take care of rbp, though.
>> >
>> > AFAICS there's various entry paths where it's not used I think: for example the
>> > compat system calls in entry_64_compat.S don't seem to encode RBP in such a
>> > fashion (unless I missed some macro side effect).
>>
>> Then that's a separate bug that should be fixed.  Josh?
>
> We don't encode the frame pointer on syscalls, because "fast path"
> (though that's obviously no longer a consideration).

Should we start encoding the frame pointer?

--Andy

Re: [PATCH v2 1/3] x86/entry: Clear extra registers beyond syscall arguments for 64bit kernels

[Josh Poimboeuf]

On Mon, Feb 05, 2018 at 05:31:39PM +0000, Andy Lutomirski wrote:
> On Mon, Feb 5, 2018 at 5:26 PM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > On Mon, Feb 05, 2018 at 04:37:26PM +0000, Andy Lutomirski wrote:
> >> On Mon, Feb 5, 2018 at 4:30 PM, Ingo Molnar <mingo@kernel.org> wrote:
> >> >
> >> > * Andy Lutomirski <luto@amacapital.net> wrote:
> >> >
> >> >>
> >> >>
> >> >> > On Feb 5, 2018, at 3:42 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >> >> >
> >> >> >
> >> >> > * Dan Williams <dan.j.williams@intel.com> wrote:
> >> >> >
> >> >> >> +    /*
> >> >> >> +     * Sanitize extra registers of values that a speculation attack
> >> >> >> +     * might want to exploit. In the CONFIG_FRAME_POINTER=y case,
> >> >> >> +     * the expectation is that %ebp will be clobbered before it
> >> >> >> +     * could be used.
> >> >> >> +     */
> >> >> >> +    .macro CLEAR_EXTRA_REGS_NOSPEC
> >> >> >> +    xorq %r15, %r15
> >> >> >> +    xorq %r14, %r14
> >> >> >> +    xorq %r13, %r13
> >> >> >> +    xorq %r12, %r12
> >> >> >> +    xorl %ebx, %ebx
> >> >> >> +#ifndef CONFIG_FRAME_POINTER
> >> >> >> +    xorl %ebp, %ebp
> >> >> >> +#endif
> >> >> >> +    .endm
> >> >> >
> >> >> > Yeah, so this series look pretty good to me, but there's one small detail: I think
> >> >> > RBP should be cleared unconditionally here, even in the CONFIG_FRAME_POINTERS=y
> >> >> > case, because:
> >> >>
> >> >> ENCODE_FRAME_POINTER should take care of rbp, though.
> >> >
> >> > AFAICS there's various entry paths where it's not used I think: for example the
> >> > compat system calls in entry_64_compat.S don't seem to encode RBP in such a
> >> > fashion (unless I missed some macro side effect).
> >>
> >> Then that's a separate bug that should be fixed.  Josh?
> >
> > We don't encode the frame pointer on syscalls, because "fast path"
> > (though that's obviously no longer a consideration).
> 
> Should we start encoding the frame pointer?

Perhaps, but I should clarify it's not a bug.  For syscalls, we instead
just standardized the location of the last stack frame.  But that's a
bit fragile and I was never too happy with it.

Encoding the frame pointer would be a lot more straightforward, though
we'd still need to figure out a way to detect the "end" of the stack for
kthreads.  We could probably encode that information as well: "here's
the end, but there are no pt_regs".  Could just be an "encoded" NULL.

-- 
Josh

Re: [PATCH v2 1/3] x86/entry: Clear extra registers beyond syscall arguments for 64bit kernels

[Dan Williams]

On Mon, Feb 5, 2018 at 3:58 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Dan Williams <dan.j.williams@intel.com> wrote:
>
>> +     /*
>> +      * Sanitize extra registers of values that a speculation attack
>> +      * might want to exploit. In the CONFIG_FRAME_POINTER=y case,
>> +      * the expectation is that %ebp will be clobbered before it
>> +      * could be used.
>> +      */
>> +     .macro CLEAR_EXTRA_REGS_NOSPEC
>> +     xorq %r15, %r15
>> +     xorq %r14, %r14
>> +     xorq %r13, %r13
>> +     xorq %r12, %r12
>> +     xorl %ebx, %ebx
>> +#ifndef CONFIG_FRAME_POINTER
>> +     xorl %ebp, %ebp
>> +#endif
>
> BTW., is there any reason behind the order of the clearing of these registers?
> This ordering seems rather random:
>
>  - The canonical register order is: RBX, RBP, R12, R13, R14, R15, which is also
>    their push-order on the stack.
>
>  - The CLEAR_EXTRA_REGS_NOSPEC order appears to be the reverse order (pop-order),
>    but with RBX and RBP reversed.
>
> So since this is a 'push side' primitive I'd use the regular (push-) ordering
> instead:
>
>         .macro CLEAR_EXTRA_REGS_NOSPEC
>         xorl %ebx, %ebx
>         xorl %ebp, %ebp
>         xorq %r12, %r12
>         xorq %r13, %r13
>         xorq %r14, %r14
>         xorq %r15, %r15
>
> It obviously doesn't matter to correctness - only to readability.

Sure, will do.

>
> There's also a (very) small micro-optimization argument in favor of the regular
> order: the earlier registers are more likely to be utilized by C functions, so the
> sooner we clear them, the less potential interaction these clearing instructions
> are going to have with any later use.

On a suggestion from Arjan it also appears worthwhile to interleave
'mov' with 'xor'. Perf stat says that this test gets 3.45 instructions
per cycle:

        for (i = 0; i < INT_MAX/1024; i++)
                asm(".rept 1024\n"
                    "xorl %%ebx, %%ebx\n"
                    "movq $0,    %%r10\n"
                    "xorq %%r11, %%r11\n"
                    "movq $0,    %%r12\n"
                    "xorq %%r13, %%r13\n"
                    "movq $0,    %%r14\n"
                    "xorq %%r15, %%r15\n"
                    ".endr"
                    : : : "r15", "r14", "r13", "r12",
                        "ebx", "r11", "r10");

...the 'rept' is there to try to minimize micro-op caching effects.
The straight xor version in comparisons gets 2.88 instructions per
cycle:

        for (i = 0; i < INT_MAX/1024; i++)
                asm(".rept 1024\n"
                    "xorl %%ebx, %%ebx\n"
                    "xorq %%r10, %%r10\n"
                    "xorq %%r11, %%r11\n"
                    "xorq %%r12, %%r12\n"
                    "xorq %%r13, %%r13\n"
                    "xorq %%r14, %%r14\n"
                    "xorq %%r15, %%r15\n"
                    ".endr"
                    : : : "r15", "r14", "r13", "r12",
                        "ebx", "r11", "r10");

Re: [PATCH v2 1/3] x86/entry: Clear extra registers beyond syscall arguments for 64bit kernels

[Linus Torvalds]

On Mon, Feb 5, 2018 at 1:33 PM, Dan Williams <dan.j.williams@intel.com> wrote:
>
> On a suggestion from Arjan it also appears worthwhile to interleave
> 'mov' with 'xor'. Perf stat says that this test gets 3.45 instructions
> per cycle:

Ugh.

A "xor %reg/reg" is two bytes (three for the high regs due to REX
prefix). A "mov $0" is 7 bytes because unlike most of the ALU ops,
"mov" doesn't have a 8-bit expanding immediate.

So replacing those xors with movq's will add at least four bytes per
replacement.  So you may well end up adding an L1 cache miss.

At which point "3.45 ipc" vs "2.88 ipc" is pretty much a non-issue.

I suspect that a bigger win would be if you try to interleave those
"xor" instructions with the "pushq" instructions in the entry code.
Because those push instructions tend to be limited by the LSU store
bandwidth, so you can probably put in xor instructions almost for free
in there.

                   Linus

Re: [PATCH v2 1/3] x86/entry: Clear extra registers beyond syscall arguments for 64bit kernels

[Andy Lutomirski]

On Mon, Feb 5, 2018 at 9:58 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Mon, Feb 5, 2018 at 1:33 PM, Dan Williams <dan.j.williams@intel.com> wrote:
>>
>> On a suggestion from Arjan it also appears worthwhile to interleave
>> 'mov' with 'xor'. Perf stat says that this test gets 3.45 instructions
>> per cycle:
>
> Ugh.
>
> A "xor %reg/reg" is two bytes (three for the high regs due to REX
> prefix). A "mov $0" is 7 bytes because unlike most of the ALU ops,
> "mov" doesn't have a 8-bit expanding immediate.
>
> So replacing those xors with movq's will add at least four bytes per
> replacement.  So you may well end up adding an L1 cache miss.
>
> At which point "3.45 ipc" vs "2.88 ipc" is pretty much a non-issue.
>
> I suspect that a bigger win would be if you try to interleave those
> "xor" instructions with the "pushq" instructions in the entry code.
> Because those push instructions tend to be limited by the LSU store
> bandwidth, so you can probably put in xor instructions almost for free
> in there.
>

At the risk of over-optimizing a dead horse, what about:

xorl %ebx, %ebx
movq %ebx, %r10
xorl %r11, %r11
movq %ebx, %r12

etc.

We'll have a cycle of latency from xor to mov, but I'd be rather
surprised if the CPU can't hide that.

Re: [PATCH v2 1/3] x86/entry: Clear extra registers beyond syscall arguments for 64bit kernels

[Dan Williams]

On Mon, Feb 5, 2018 at 2:10 PM, Andy Lutomirski <luto@kernel.org> wrote:
> On Mon, Feb 5, 2018 at 9:58 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>> On Mon, Feb 5, 2018 at 1:33 PM, Dan Williams <dan.j.williams@intel.com> wrote:
>>>
>>> On a suggestion from Arjan it also appears worthwhile to interleave
>>> 'mov' with 'xor'. Perf stat says that this test gets 3.45 instructions
>>> per cycle:
>>
>> Ugh.
>>
>> A "xor %reg/reg" is two bytes (three for the high regs due to REX
>> prefix). A "mov $0" is 7 bytes because unlike most of the ALU ops,
>> "mov" doesn't have a 8-bit expanding immediate.
>>
>> So replacing those xors with movq's will add at least four bytes per
>> replacement.  So you may well end up adding an L1 cache miss.
>>
>> At which point "3.45 ipc" vs "2.88 ipc" is pretty much a non-issue.
>>
>> I suspect that a bigger win would be if you try to interleave those
>> "xor" instructions with the "pushq" instructions in the entry code.
>> Because those push instructions tend to be limited by the LSU store
>> bandwidth, so you can probably put in xor instructions almost for free
>> in there.
>>
>
> At the risk of over-optimizing a dead horse, what about:
>
> xorl %ebx, %ebx
> movq %ebx, %r10
> xorl %r11, %r11
> movq %ebx, %r12
>
> etc.
>
> We'll have a cycle of latency from xor to mov, but I'd be rather
> surprised if the CPU can't hide that.

Hmm, this again gets 2.88 ipc:

        for (i = 0; i < INT_MAX/1024; i++)
                asm(".rept 1024\n"
                    "xorl %%ebx, %%ebx\n"
                    "movq %%rbx, %%r10\n"
                    "xorq %%r11, %%r11\n"
                    "movq %%rbx, %%r12\n"
                    "xorq %%r13, %%r13\n"
                    "movq %%rbx, %%r14\n"
                    "xorq %%r15, %%r15\n"
                    ".endr"
                    : : : "r15", "r14", "r13", "r12",
                        "ebx", "r11", "r10");

Re: [PATCH v2 1/3] x86/entry: Clear extra registers beyond syscall arguments for 64bit kernels

[Linus Torvalds]

On Mon, Feb 5, 2018 at 2:10 PM, Andy Lutomirski <luto@kernel.org> wrote:
> At the risk of over-optimizing a dead horse, what about:
>
> xorl %ebx, %ebx
> movq %ebx, %r10
> xorl %r11, %r11
> movq %ebx, %r12
>
> etc.
>
> We'll have a cycle of latency from xor to mov, but I'd be rather
> surprised if the CPU can't hide that.

Ugh. xor really is nice because it breaks all dependencies.

Really, it's much more likely that we can just hide the xors in the
pushes. Small, simple, easy.

But I'm not timing it.

               Linus

Re: [PATCH v2 1/3] x86/entry: Clear extra registers beyond syscall arguments for 64bit kernels

[Linus Torvalds]

On Mon, Feb 5, 2018 at 2:22 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> But I'm not timing it.

I lied.

I did this:

        for (i = 0; i < 100000; i++)
                asm(".rept 16384\n"
                "subq $128,%rsp\n\t"
                "pushq %rbx\n\t"
                "pushq %r10\n\t"
                "pushq %r11\n\t"
                "pushq %r12\n\t"
                "pushq %r13\n\t"
                "pushq %r14\n\t"
                "pushq %r15\n\t"

                "popq %r15\n\t"
                "popq %r14\n\t"
                "popq %r13\n\t"
                "popq %r12\n\t"
                "popq %r11\n\t"
                "popq %r10\n\t"
                "popq %rbx\n\t"
                "addq $128,%rsp\n\t"
                ".endr");

and then I timed it like that, and with "xorq" of the register after
each "pushq".

And the timings came out the same, to within the (bad) timing I did.

So I really do think you can just put the xor right next to the push,
and it will be effectively free.

            Linus

Re: [PATCH v2 1/3] x86/entry: Clear extra registers beyond syscall arguments for 64bit kernels

[Ingo Molnar]

* Andi Kleen <ak@linux.intel.com> wrote:

> >      - There's various conditional pieces of entry code that run before any 
> >        RBP-clobbering C function is called. While none of them has an exploitable 
> >        Spectre 'gadget' at the moment, we'd have to consider this for every future
> >        change.
> 
> The Frame Pointer is always set up in assembler too, just in another macro.

As I replied to Andy, that's not universally true: there are code paths where RBP 
is not set before calling C code or going into the more complex parts of the 
kernel entry code.

This RBP value leak in fact demonstrates the validity of my robustness argument:

> >   I.e. we cannot universally rely on RBP being sanitized. In _practice_ it 
> >   will be sanitized, but we don't know for sure without expending quite some 
> >   effort to think through all the cases.

Thanks,

	Ingo