Re: [PATCH v2 2/8] x86/enter: MACROS to set/clear IBRS

Re: [PATCH v2 2/8] x86/enter: MACROS to set/clear IBRS

[Tim Chen]

On 01/06/2018 03:05 PM, Mark Marshall wrote:
> Hi.
> 
> (I've only just subscribed and can't work out how to reply to a message from before I subscribed (on my phone), sorry)
> 
> In the macro WRMSR_ASM you seem to have lost the wrmsr?
> 
>

Yes.  That's a bug noticed by Thomas and I'll fix for update to IBRS patches.

Tim

[PATCH v2 0/8] IBRS patch series

[Tim Chen]

Thanks to everyone for the feedback on the initial posting.
This is an updated patchset and I hope I've captured all
the review comments.  I've done a lot of code clean up
per everyone's comments. Please let me know if I've missed
something.

The retpoline related changes is moved to the end of the
patch series, so they can be taken out or changed easily
without affecting the other patches.

Many people hate the multi-bits spec_ctrl_ibrs variable so
I got rid of that and replace it with a dynamic_ibrs flag
to indicate if we need to switch IBRS enter/exiting kernel
which is more intuitive and also makes the code cleaner.

Peter/Andrea suggested that we use a static key to control the run time
IBRS enabling/disabling with "STATIC_JUMP_IF_TRUE" kind
of construct.  However, I had some concerns that
JUMP_LABEL config may be disabled and the construct cannot
be used. I also encountered some
OOPs when I'm changing ibrs control state probably
related to changing the jump label branching. I haven't
had time to debug that so I left it out for now.
I will welcome some help here on a patch to get the static key
thing working right.

v2.
1. Added missing feature enumeration in tools/arch/x86/include/asm/cpufeatures.h  
2. Kernel entry macros label cleanup and move them to calling.h
3. Remove unnecessary irqs_diabled check in the mwait
4. Don't use a bit field base sys control variable to make ibrs enabling
   simpler and easier to understand
5. Corrected compile issues for firmware update code
6. Leave IBPB feature bits out from this patch series and will be added
   in its own set of patches later

Tim

---patch series details---
This patch series enables the basic detection and usage of x86 indirect
branch speculation feature.  It enables the indirect branch restricted
speculation (IBRS) on kernel entry and disables it on exit.
It enumerates the indirect branch prediction barrier (IBPB).

The x86 IBRS feature requires corresponding microcode support.
It mitigates the variant 2 vulnerability described in
https://googleprojectzero.blogspot.com/2018/01/reading-privileged-memory-with-side.html

If IBRS is set, near returns and near indirect jumps/calls will not
allow their predicted target address to be controlled by code that
executed in a less privileged prediction mode before the IBRS mode was
last written with a value of 1 or on another logical processor so long
as all RSB entries from the previous less privileged prediction mode
are overwritten.

Both retpoline and IBRS provides mitigation against variant 2 attacks,
with IBRS being the most secured method but could incur more performance
overhead compared to retpoline[1].  If you are very paranoid or you
run on a CPU where IBRS=1 is cheaper, you may also want to run in "IBRS
always" mode.

See: https://docs.google.com/document/d/e/2PACX-1vSMrwkaoSUBAFc6Fjd19F18c1O9pudkfAY-7lGYGOTN8mc9ul-J6pWadcAaBJZcVA7W_3jlLKRtKRbd/pub

More detailed description of IBRS is described in the first patch.

It is applied on top of the page table isolation changes.

A run time and boot time control of the IBRS feature is provided

There are 2 ways to control IBRS

1. At boot time
    noibrs kernel boot parameter will disable IBRS usage

Otherwise if the above parameters are not specified, the system
will enable ibrs and ibpb usage if the cpu supports it.

2. At run time
    echo 0 > /sys/kernel/debug/x86/ibrs_enabled will turn off IBRS
    echo 1 > /sys/kernel/debug/x86/ibrs_enabled will turn on IBRS in kernel
    echo 2 > /sys/kernel/debug/x86/ibrs_enabled will turn on IBRS in both userspace and kernel (IBRS always)

[1] https://lkml.org/lkml/2018/1/4/174


Tim Chen (8):
  x86/feature: Detect the x86 IBRS feature to control Speculation
  x86/enter: MACROS to set/clear IBRS
  x86/enter: Use IBRS on syscall and interrupts
  x86/spec_ctrl: Add sysctl knobs to enable/disable SPEC_CTRL feature
  x86/idle: Disable IBRS entering idle and enable it on wakeup
  x86/microcode: Recheck IBRS features on microcode reload
  x86: Do not use dynamic IBRS if retpoline is enabled
  x86: Use IBRS for firmware update path

 arch/x86/entry/calling.h                 | 104 +++++++++++++++
 arch/x86/entry/entry_64.S                |  23 ++++
 arch/x86/entry/entry_64_compat.S         |   8 ++
 arch/x86/include/asm/apm.h               |   6 +
 arch/x86/include/asm/cpufeatures.h       |   1 +
 arch/x86/include/asm/efi.h               |  17 ++-
 arch/x86/include/asm/msr-index.h         |   4 +
 arch/x86/include/asm/mwait.h             |  13 ++
 arch/x86/include/asm/spec_ctrl.h         |  54 ++++++++
 arch/x86/kernel/cpu/Makefile             |   1 +
 arch/x86/kernel/cpu/microcode/core.c     |   4 +
 arch/x86/kernel/cpu/scattered.c          |   3 +
 arch/x86/kernel/cpu/spec_ctrl.c          | 209 +++++++++++++++++++++++++++++++
 arch/x86/kernel/process.c                |   9 +-
 tools/arch/x86/include/asm/cpufeatures.h |   1 +
 15 files changed, 453 insertions(+), 4 deletions(-)
 create mode 100644 arch/x86/include/asm/spec_ctrl.h
 create mode 100644 arch/x86/kernel/cpu/spec_ctrl.c

-- 
2.9.4

[PATCH v2 2/8] x86/enter: MACROS to set/clear IBRS

[Tim Chen]

Create macros to control IBRS.  Use these macros to enable IBRS on kernel entry
paths and disable IBRS on kernel exit paths.

The registers rax, rcx and rdx are touched when controlling IBRS
so they need to be saved when they can't be clobbered.

Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
---
 arch/x86/entry/calling.h | 74 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)

diff --git a/arch/x86/entry/calling.h b/arch/x86/entry/calling.h
index 45a63e0..09c870d 100644
--- a/arch/x86/entry/calling.h
+++ b/arch/x86/entry/calling.h
@@ -6,6 +6,8 @@
 #include <asm/percpu.h>
 #include <asm/asm-offsets.h>
 #include <asm/processor-flags.h>
+#include <asm/msr-index.h>
+#include <asm/cpufeatures.h>
 
 /*
 
@@ -347,3 +349,75 @@ For 32-bit we have the following conventions - kernel is built with
 .Lafter_call_\@:
 #endif
 .endm
+
+/*
+ * IBRS related macros
+ */
+
+.macro PUSH_MSR_REGS
+	pushq	%rax
+	pushq	%rcx
+	pushq	%rdx
+.endm
+
+.macro POP_MSR_REGS
+	popq	%rdx
+	popq	%rcx
+	popq	%rax
+.endm
+
+.macro WRMSR_ASM msr_nr:req eax_val:req
+	movl	\msr_nr, %ecx
+	movl	$0, %edx
+	movl	\eax_val, %eax
+.endm
+
+.macro ENABLE_IBRS
+	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
+	PUSH_MSR_REGS
+	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS
+	POP_MSR_REGS
+.Lskip_\@:
+.endm
+
+.macro DISABLE_IBRS
+	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
+	PUSH_MSR_REGS
+	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS
+	POP_MSR_REGS
+.Lskip_\@:
+.endm
+
+.macro ENABLE_IBRS_CLOBBER
+	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
+	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS
+.Lskip_\@:
+.endm
+
+.macro DISABLE_IBRS_CLOBBER
+	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
+	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS
+.Lskip_\@:
+.endm
+
+.macro ENABLE_IBRS_SAVE_AND_CLOBBER save_reg:req
+	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
+	movl	$MSR_IA32_SPEC_CTRL, %ecx
+	rdmsr
+	movl	%eax, \save_reg
+
+	movl	$0, %edx
+	movl	$SPEC_CTRL_FEATURE_ENABLE_IBRS, %eax
+	wrmsr
+.Lskip_\@:
+.endm
+
+.macro RESTORE_IBRS_CLOBBER save_reg:req
+	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
+	/* Set IBRS to the value saved in the save_reg */
+	movl    $MSR_IA32_SPEC_CTRL, %ecx
+	movl    $0, %edx
+	movl    \save_reg, %eax
+	wrmsr
+.Lskip_\@:
+.endm
-- 
2.9.4

Re: [PATCH v2 2/8] x86/enter: MACROS to set/clear IBRS

[Borislav Petkov]

On Fri, Jan 05, 2018 at 06:12:17PM -0800, Tim Chen wrote:

> Subject: Re: [PATCH v2 2/8] x86/enter: MACROS to set/clear IBRS

Your subject needs to have a verb and not scream:

Subject: [PATCH v2 2/8] x86/entry: Add macros to set/clear IBRS

> Create macros to control IBRS.  Use these macros to enable IBRS on kernel entry
> paths and disable IBRS on kernel exit paths.
> 
> The registers rax, rcx and rdx are touched when controlling IBRS
> so they need to be saved when they can't be clobbered.
> 
> Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
> ---
>  arch/x86/entry/calling.h | 74 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 74 insertions(+)
> 
> diff --git a/arch/x86/entry/calling.h b/arch/x86/entry/calling.h
> index 45a63e0..09c870d 100644
> --- a/arch/x86/entry/calling.h
> +++ b/arch/x86/entry/calling.h
> @@ -6,6 +6,8 @@
>  #include <asm/percpu.h>
>  #include <asm/asm-offsets.h>
>  #include <asm/processor-flags.h>
> +#include <asm/msr-index.h>
> +#include <asm/cpufeatures.h>
>  
>  /*
>  
> @@ -347,3 +349,75 @@ For 32-bit we have the following conventions - kernel is built with
>  .Lafter_call_\@:
>  #endif
>  .endm
> +
> +/*
> + * IBRS related macros
> + */
> +.macro PUSH_MSR_REGS
> +	pushq	%rax
> +	pushq	%rcx
> +	pushq	%rdx
> +.endm
> +
> +.macro POP_MSR_REGS
> +	popq	%rdx
> +	popq	%rcx
> +	popq	%rax
> +.endm
> +
> +.macro WRMSR_ASM msr_nr:req eax_val:req

WRMSR as a name is good enough.

Also, you need edx_val:req too in case we decide to reuse that macro
for something else later. Which I'm pretty sure we will, once it is out
there.

> +	movl	\msr_nr, %ecx
> +	movl	$0, %edx

... and then

	movl	\edx_val, %edx

> +	movl	\eax_val, %eax
> +.endm
> +
> +.macro ENABLE_IBRS
> +	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
> +	PUSH_MSR_REGS
> +	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS

This is overwriting the previous contents of the MSR. You need to read
it and OR-in its bits [63:2] with SPEC_CTRL_FEATURE_ENABLE_IBRS and
clear bit 0.

Unless the rest of this MSR is not going to be used for anything else.
Then you're fine.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH v2 2/8] x86/enter: MACROS to set/clear IBRS

[Tim Chen]

On 01/07/2018 04:03 AM, Borislav Petkov wrote:
> On Fri, Jan 05, 2018 at 06:12:17PM -0800, Tim Chen wrote:
> 
>> Subject: Re: [PATCH v2 2/8] x86/enter: MACROS to set/clear IBRS
> 
> Your subject needs to have a verb and not scream:
> 
> Subject: [PATCH v2 2/8] x86/entry: Add macros to set/clear IBRS
> 
>> Create macros to control IBRS.  Use these macros to enable IBRS on kernel entry
>> paths and disable IBRS on kernel exit paths.
>>
>> The registers rax, rcx and rdx are touched when controlling IBRS
>> so they need to be saved when they can't be clobbered.
>>
>> Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
>> ---
>>  arch/x86/entry/calling.h | 74 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 74 insertions(+)
>>
>> diff --git a/arch/x86/entry/calling.h b/arch/x86/entry/calling.h
>> index 45a63e0..09c870d 100644
>> --- a/arch/x86/entry/calling.h
>> +++ b/arch/x86/entry/calling.h
>> @@ -6,6 +6,8 @@
>>  #include <asm/percpu.h>
>>  #include <asm/asm-offsets.h>
>>  #include <asm/processor-flags.h>
>> +#include <asm/msr-index.h>
>> +#include <asm/cpufeatures.h>
>>  
>>  /*
>>  
>> @@ -347,3 +349,75 @@ For 32-bit we have the following conventions - kernel is built with
>>  .Lafter_call_\@:
>>  #endif
>>  .endm
>> +
>> +/*
>> + * IBRS related macros
>> + */
>> +.macro PUSH_MSR_REGS
>> +	pushq	%rax
>> +	pushq	%rcx
>> +	pushq	%rdx
>> +.endm
>> +
>> +.macro POP_MSR_REGS
>> +	popq	%rdx
>> +	popq	%rcx
>> +	popq	%rax
>> +.endm
>> +
>> +.macro WRMSR_ASM msr_nr:req eax_val:req
> 
> WRMSR as a name is good enough.
> 
> Also, you need edx_val:req too in case we decide to reuse that macro
> for something else later. Which I'm pretty sure we will, once it is out
> there.
> 
>> +	movl	\msr_nr, %ecx
>> +	movl	$0, %edx
> 
> ... and then
> 
> 	movl	\edx_val, %edx
> 
>> +	movl	\eax_val, %eax
>> +.endm
>> +
>> +.macro ENABLE_IBRS
>> +	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
>> +	PUSH_MSR_REGS
>> +	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS
> 
> This is overwriting the previous contents of the MSR. You need to read
> it and OR-in its bits [63:2] with SPEC_CTRL_FEATURE_ENABLE_IBRS and
> clear bit 0.
> 
> Unless the rest of this MSR is not going to be used for anything else.
> Then you're fine.
> 

Currently we are not using other bits.
When the time comes that we have other bits in this MSR used, we will
change this.

Tim

Re: [PATCH v2 2/8] x86/enter: MACROS to set/clear IBRS

[Borislav Petkov]

On Sun, Jan 07, 2018 at 09:12:21AM -0800, Tim Chen wrote:
> Currently we are not using other bits.
> When the time comes that we have other bits in this MSR used, we will
> change this.

Then please write in a short comment above it that it is ok that the
previous MSR contents get overwritten.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH v2 2/8] x86/enter: MACROS to set/clear IBRS

[Tim Chen]

On 01/07/2018 04:03 AM, Borislav Petkov wrote:

> 
> WRMSR as a name is good enough.
> 

It alias with wrmsr instruction and didn't
build correctly. So I'll keep it as WRMSR_ASM.

Tim