IBRS patch series

cpuid ax=0x7, return rdx bit 26 to indicate presence of this feature IA32_SPEC_CTRL (0x48) and IA32_PRED_CMD (0x49) IA32_SPEC_CTRL, bit0 – Indirect Branch Restricted Speculation (IBRS) IA32_PRED_CMD, bit0 – Indirect Branch Prediction Barrier (IBPB)

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.

Setting of IBPB ensures that earlier code's behavior does not control later indirect branch predictions. It is used when context switching to new untrusted address space. Unlike IBRS, it is a command MSR and does not retain its state.

* Thus a near indirect jump/call/return may be affected by code in a less privileged prediction mode that executed AFTER IBRS mode was last written with a value of 1

* There is no need to clear IBRS before writing it with a value of
1. Unconditionally writing it with a value of 1 after the prediction mode change is sufficient

* Note: IBRS is not required in order to isolate branch predictions for SMM or SGX enclaves

* Code executed by a sibling logical processor cannot control indirect jump/call/return predicted target when IBRS is set

* SMEP will prevent supervisor mode using RSB entries filled by user code; this can reduce the need for software to overwrite RSB entries

* IBRS is not guaranteed to differentiate two applications that use the same CR3 due to recycling. Software can use an IBPB command when recycling a page table base address.

* VMM software can similarly use an IBPB when recycling a controlling VMCS pointer address

CPU performance could be reduced when running with IBRS set.

Signed-off-by Tim Chen <[email protected]>

---
 arch/x86/include/asm/cpufeatures.h | 1 +
 arch/x86/include/asm/msr-index.h   | 7 +++++++
 arch/x86/kernel/cpu/scattered.c    | 1 +
 3 files changed, 9 insertions(+)

arch/x86/include/asm/cpufeatures.h +0 -0

index 86c68cb..431f393 100644

Quoted 57 lines · Tim Chen

cpuid ax=0x7, return rdx bit 26 to indicate presence of this feature IA32_SPEC_CTRL (0x48) and IA32_PRED_CMD (0x49) IA32_SPEC_CTRL, bit0 – Indirect Branch Restricted Speculation (IBRS) IA32_PRED_CMD, bit0 – Indirect Branch Prediction Barrier (IBPB)

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.

Setting of IBPB ensures that earlier code's behavior does not control later indirect branch predictions. It is used when context switching to new untrusted address space. Unlike IBRS, it is a command MSR and does not retain its state.

* Thus a near indirect jump/call/return may be affected by code in a less privileged prediction mode that executed AFTER IBRS mode was last written with a value of 1

* There is no need to clear IBRS before writing it with a value of
1. Unconditionally writing it with a value of 1 after the prediction mode change is sufficient

* Note: IBRS is not required in order to isolate branch predictions for SMM or SGX enclaves

* Code executed by a sibling logical processor cannot control indirect jump/call/return predicted target when IBRS is set

* SMEP will prevent supervisor mode using RSB entries filled by user code; this can reduce the need for software to overwrite RSB entries

* IBRS is not guaranteed to differentiate two applications that use the same CR3 due to recycling. Software can use an IBPB command when recycling a page table base address.

* VMM software can similarly use an IBPB when recycling a controlling VMCS pointer address

CPU performance could be reduced when running with IBRS set.

Signed-off-by Tim Chen <[email protected]>

---
 arch/x86/include/asm/cpufeatures.h | 1 +
 arch/x86/include/asm/msr-index.h   | 7 +++++++
 arch/x86/kernel/cpu/scattered.c    | 1 +
 3 files changed, 9 insertions(+)

arch/x86/include/asm/cpufeatures.h +0 -0

index 86c68cb..431f393 100644
@@ -209,6 +209,7 @@
 #define X86_FEATURE_AVX512_4FMAPS	( 7*32+17) /* AVX-512 Multiply Accumulation Single precision */
 

arch/x86/include/asm/cpufeatures.h line 215

Quoted 5 lines

arch/x86/include/asm/cpufeatures.h +1 -0

 #define X86_FEATURE_MBA			( 7*32+18) /* Memory Bandwidth Allocation */
+#define X86_FEATURE_SPEC_CTRL		( 7*32+19) /* Control Speculation Control */
 
 /* Virtualization flags: Linux defined, word 8 */
 #define X86_FEATURE_TPR_SHADOW		( 8*32+ 0) /* Intel TPR Shadow */

You should have gotten a build warning with just this patch, please also update tools/arch/x86/include/asm/cpufeatures.h to fix that.

And why not use a free slot, (7*32+13) or (7*32+12) is free, right?

Or were you just trying to make backports "easier"? :)

cpuid ax=0x7, return rdx bit 26 to indicate presence of this feature IA32_SPEC_CTRL (0x48) and IA32_PRED_CMD (0x49) IA32_SPEC_CTRL, bit0 – Indirect Branch Restricted Speculation (IBRS) IA32_PRED_CMD,  bit0 – Indirect Branch Prediction Barrier (IBPB)

Quoted 4 lines · Tim Chen

cpuid ax=0x7, return rdx bit 26 to indicate presence of this feature IA32_SPEC_CTRL (0x48) and IA32_PRED_CMD (0x49) IA32_SPEC_CTRL, bit0 – Indirect Branch Restricted Speculation (IBRS) IA32_PRED_CMD,  bit0 – Indirect Branch Prediction Barrier (IBPB)

In a previous iteration of these patches, hadn't you already merged support for the AMD variant? Where'd that go?

Quoted 6 lines · David Woodhouse

Quoted 4 lines · Tim Chen

cpuid ax=0x7, return rdx bit 26 to indicate presence of this feature IA32_SPEC_CTRL (0x48) and IA32_PRED_CMD (0x49) IA32_SPEC_CTRL, bit0 – Indirect Branch Restricted Speculation (IBRS) IA32_PRED_CMD, bit0 – Indirect Branch Prediction Barrier (IBPB)

Quoted 2 lines

In a previous iteration of these patches, hadn't you already merged support for the AMD variant? Where'd that go?

It looks like this series is strictly IBRS related. Even though some of the IBPB definitions are in here, there isn't support later for indicating IBPB is in use or any places where IBPB would be used. I was assuming that there would be another series for IBPB support. Is that the plan?

AMD is covered by the IBRS support as is, but we also have support for the IBPB feature alone, identified by a different CPUID bit. I was waiting for the IBPB series to see if I needed to submit anything or whether the patches were included.

arch/x86/include/asm/msr-index.h +6 -0

+#define MSR_IA32_SPEC_CTRL		0x00000048
+#define SPEC_CTRL_FEATURE_DISABLE_IBRS	(0 << 0)
+#define SPEC_CTRL_FEATURE_ENABLE_IBRS	(1 << 0)
+
+#define MSR_IA32_PRED_CMD		0x00000049
+
 #define MSR_IA32_PERFCTR0		0x000000c1
 #define MSR_IA32_PERFCTR1		0x000000c2
 #define MSR_FSB_FREQ			0x000000cd

Quoted 9 lines · Tim Chen

arch/x86/include/asm/msr-index.h +6 -0

+#define MSR_IA32_SPEC_CTRL		0x00000048
+#define SPEC_CTRL_FEATURE_DISABLE_IBRS	(0 << 0)
+#define SPEC_CTRL_FEATURE_ENABLE_IBRS	(1 << 0)
+
+#define MSR_IA32_PRED_CMD		0x00000049
+
 #define MSR_IA32_PERFCTR0		0x000000c1
 #define MSR_IA32_PERFCTR1		0x000000c2
 #define MSR_FSB_FREQ			0x000000cd

arch/x86/include/asm/msr-index.h line 448

Quoted 5 lines

arch/x86/include/asm/msr-index.h +1 -0

@@ -439,6 +445,7 @@
 #define FEATURE_CONTROL_VMXON_ENABLED_INSIDE_SMX	(1<<1)
 #define FEATURE_CONTROL_VMXON_ENABLED_OUTSIDE_SMX	(1<<2)
 #define FEATURE_CONTROL_LMCE				(1<<20)
+#define FEATURE_SET_IBPB				(1<<0)

So how is that bit related to the control bits above? This file is structured in obvious ways ....

On Fri, Jan 05, 2018 at 02:09:43PM +0100, Thomas Gleixner wrote:
Now about the late microcode my preference is not for static_cpu_has and forcing the early microcode, but my long term preference is to start with this/boot_cpu_has() and then turn static_cpu_has in a true static key so that setup_force_cpu_cap shall also flip the static key for all static_cpu_has(X86_FEATURE_IBPB_SUPPORT) also if run any time after boot and not only if run before the static_cpu_has alternative is patched in.

+.macro ENABLE_IBRS

arch/x86/include/asm/spec_ctrl.h new file +34 -0

+	ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
+	PUSH_MSR_REGS
+	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS
+	POP_MSR_REGS
+10:
+.endm
+
+.macro DISABLE_IBRS
+	ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
+	PUSH_MSR_REGS
+	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS
+	POP_MSR_REGS
+10:
+.endm
+
+.macro ENABLE_IBRS_CLOBBER
+	ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
+	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS
+10:
+.endm
+
+.macro DISABLE_IBRS_CLOBBER
+	ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
+	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS
+10:
+.endm
+
+.macro ENABLE_IBRS_SAVE_AND_CLOBBER save_reg:req
+	ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
+	movl	$MSR_IA32_SPEC_CTRL, %ecx
+	rdmsr
+	movl	%eax, \save_reg
+
+	movl	$0, %edx

arch/x86/include/asm/spec_ctrl.h

Quoted 5 lines · Tim Chen

arch/x86/include/asm/spec_ctrl.h new file +0 -0

new file mode 100644
index 0000000..16fc4f58

Does this really have to live outside of arch/x86/entry/ ?

Quoted 40 lines

+.macro ENABLE_IBRS

arch/x86/include/asm/spec_ctrl.h new file +38 -0

+	ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
+	PUSH_MSR_REGS
+	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS
+	POP_MSR_REGS
+10:
+.endm
+
+.macro DISABLE_IBRS
+	ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
+	PUSH_MSR_REGS
+	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS
+	POP_MSR_REGS
+10:
+.endm
+
+.macro ENABLE_IBRS_CLOBBER
+	ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
+	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS
+10:
+.endm
+
+.macro DISABLE_IBRS_CLOBBER
+	ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
+	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS
+10:
+.endm
+
+.macro ENABLE_IBRS_SAVE_AND_CLOBBER save_reg:req
+	ALTERNATIVE "jmp 10f", "", 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
+10:
+.endm

arch/x86/include/asm/spec_ctrl.h line 73

Quoted 6 lines

arch/x86/include/asm/spec_ctrl.h new file +6 -0

+
+.macro RESTORE_IBRS_CLOBBER save_reg:req
+	ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
+	/* Set IBRS to the value saved in the save_reg */
+	movl    $MSR_IA32_SPEC_CTRL, %ecx
+        movl    $0, %edx

whitespace damage

arch/x86/include/asm/spec_ctrl.h line 77

Quoted 5 lines

arch/x86/include/asm/spec_ctrl.h new file +4 -0

+	movl    \save_reg, %eax
+	wrmsr
+10:
+.endm

Should not all those 10 things look like .Ldone_\@ or something ?

Quoted 2 lines

Does this really have to live outside of arch/x86/entry/ ?

There are some inline C routines later in this file that will be needed by other functions. Want to consolidate them in the same file.

Quoted 5 lines · Tim Chen

Quoted 2 lines

Does this really have to live outside of arch/x86/entry/ ?

There are some inline C routines later in this file that will be needed by other functions. Want to consolidate them in the same file.

We could put all of the assembly into calling.h along with the PTI assembly. Seems as sane a place as anywhere else to put it.

Quoted 7 lines · Dave Hansen

Quoted 5 lines · Tim Chen

Quoted 2 lines

Does this really have to live outside of arch/x86/entry/ ?

There are some inline C routines later in this file that will be needed by other functions. Want to consolidate them in the same file.

Quoted 2 lines

We could put all of the assembly into calling.h along with the PTI assembly. Seems as sane a place as anywhere else to put it.

We should also stop thinking that NMI is at all special. All the paranoid entry paths + NMI should just save and restore it, just like CR3. Otherwise we get nasty corner cases with MCE, kprobes, etc.

Quoted 7 lines · Peter Zijlstra

arch/x86/include/asm/spec_ctrl.h line 77

Quoted 4 lines

arch/x86/include/asm/spec_ctrl.h new file +4 -0

+	movl    \save_reg, %eax
+	wrmsr
+10:
+.endm

Should not all those 10 things look like .Ldone_\@ or something ?

They are removed in patch 6 with .Ldone_\@

That is a minor inefficiency only, but we can eliminate it by saving the MSR when entering the NMI in save_paranoid and restoring it when exiting the NMI.

Quoted 4 lines · Tim Chen

That is a minor inefficiency only, but we can eliminate it by saving the MSR when entering the NMI in save_paranoid and restoring it when exiting the NMI.

Any hints as to what exactly "minor" means in cycles here? :)

If NMI runs when exiting kernel between IBRS_DISABLE and SWAPGS, the NMI would have turned on IBRS bit 0 and then it would have left enabled when exiting the NMI. IBRS bit 0 would then be left enabled in userland until the next enter kernel.

That is a minor inefficiency only, but we can eliminate it by saving the MSR when entering the NMI in save_paranoid and restoring it when exiting the NMI.

Set IBRS upon kernel entrance via syscall and interrupts. Clear it upon exit.

If NMI runs when exiting kernel between IBRS_DISABLE and SWAPGS, the NMI would have turned on IBRS bit 0 and then it would have left enabled when exiting the NMI. IBRS bit 0 would then be left enabled in userland until the next enter kernel.

That is a minor inefficiency only, but we can eliminate it by saving the MSR when entering the NMI in save_paranoid and restoring it when exiting the NMI.

Signed-off-by Andrea Arcangeli <[email protected]>

Signed-off-by Tim Chen <[email protected]>

So not only did we add a CR3 write, we're now adding an MSR write to the entry/exit paths. Please tell me that these are 'fast' MSRs? Given people are already reporting stupid numbers with just the existing PTI/CR3, what kind of pain are we going to get from adding this?

Quoted 2 lines · Tim Chen

Set IBRS upon kernel entrance via syscall and interrupts. Clear it upon exit.

So not only did we add a CR3 write, we're now adding an MSR write to the entry/exit paths. Please tell me that these are 'fast' MSRs? Given people are already reporting stupid numbers with just the existing PTI/CR3, what kind of pain are we going to get from adding this?

Quoted 8 lines · Peter Zijlstra

Quoted 2 lines · Tim Chen

Set IBRS upon kernel entrance via syscall and interrupts. Clear it upon exit.

So not only did we add a CR3 write, we're now adding an MSR write to the entry/exit paths. Please tell me that these are 'fast' MSRs? Given people are already reporting stupid numbers with just the existing PTI/CR3, what kind of pain are we going to get from adding this?

This "dynamic IBRS" that does runtime switching will not be on by default and will be patched around by alternatives unless someone explicitly opts in.

If you decide you want the additional protection that it provides, you can take the performance hit. How much is that? We've been saying that these new MSRs are roughly as expensive as the CR3 writes. How expensive are those? Don't take my word for it, a few folks were talking about it today:

Google says[1]: "We see negligible impact on performance." Amazon says[2]: "We don’t expect meaningful performance impact."

I chopped a few qualifiers out of there, but I think that roughly captures the sentiment.

1.
https://security.googleblog.com/2018/01/more-details-about-mitigations-for-cpu_4.html
2.
http://www.businessinsider.com/google-amazon-performance-hit-meltdown-spectre-fixes-overblown-2018-1

Do we need an arch_prctl() to enable IBRS for user mode?

Quoted 1 line · Andy Lutomirski

Do we need an arch_prctl() to enable IBRS for user mode?

Eventually, once the dust settles. I think there's a spectrum of paranoia here, that is roughly (with increasing paranoia):

1. do nothing
2. do retpoline
3. do IBRS in kernel
4. do IBRS always

I think you're asking for ~3.5.

Set IBRS upon kernel entrance via syscall and interrupts. Clear it upon exit.

arch/x86/include/asm/mwait.h +14 -0

@@ -100,15 +101,33 @@ static inline void __sti_mwait(unsigned long eax, unsigned long ecx)
 static inline void mwait_idle_with_hints(unsigned long eax, unsigned long ecx)
 {
 	if (static_cpu_has_bug(X86_BUG_MONITOR) || !current_set_polling_and_test()) {
+		bool can_toggle_ibrs = false;
 		if (static_cpu_has_bug(X86_BUG_CLFLUSH_MONITOR)) {
 			mb();
 			clflush((void *)&current_thread_info()->flags);
 			mb();
 		}
 
+		if (irqs_disabled()) {
+		       /*
+			* CPUs run faster with speculation protection
+			* disabled.  All CPU threads in a core must
+			* disable speculation protection for it to be
+			* disabled.  Disable it while we are idle so the
+			* other hyperthread can run fast.
+			*
+			* nmi uses the save_paranoid model which
+			* always enables ibrs on exception entry
+			* before any indirect jump can run.
+			*/
+			can_toggle_ibrs = true;

arch/x86/include/asm/spec_ctrl.h +10 -0

index 16fc4f58..28b0314 100644
@@ -76,5 +76,42 @@
 10:
 .endm
 
+#else
+#include <asm/microcode.h>
+
+static inline void __disable_indirect_speculation(void)
+{
+        native_wrmsrl(MSR_IA32_SPEC_CTRL, SPEC_CTRL_FEATURE_ENABLE_IBRS);
+}
+
+static inline void __enable_indirect_speculation(void)
+{

arch/x86/include/asm/spec_ctrl.h +5 -0

+	if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
+		__enable_indirect_speculation();
+}
+
+static inline void unprotected_speculation_end(void)

Argh.. no. Who is calling this with IRQs enabled? And why can't we frob the MSR with IRQs enabled? That comment doesn't seem to explain anything.

Quoted 30 lines · Tim Chen

arch/x86/include/asm/mwait.h +17 -0

@@ -100,15 +101,33 @@ static inline void __sti_mwait(unsigned long eax, unsigned long ecx)
 static inline void mwait_idle_with_hints(unsigned long eax, unsigned long ecx)
 {
 	if (static_cpu_has_bug(X86_BUG_MONITOR) || !current_set_polling_and_test()) {
+		bool can_toggle_ibrs = false;
 		if (static_cpu_has_bug(X86_BUG_CLFLUSH_MONITOR)) {
 			mb();
 			clflush((void *)&current_thread_info()->flags);
 			mb();
 		}
 
+		if (irqs_disabled()) {
+		       /*
+			* CPUs run faster with speculation protection
+			* disabled.  All CPU threads in a core must
+			* disable speculation protection for it to be
+			* disabled.  Disable it while we are idle so the
+			* other hyperthread can run fast.
+			*
+			* nmi uses the save_paranoid model which
+			* always enables ibrs on exception entry
+			* before any indirect jump can run.
+			*/
+			can_toggle_ibrs = true;
+			unprotected_speculation_begin();
+		}
 		__monitor((void *)&current_thread_info()->flags, 0, 0);
 		if (!need_resched())
 			__mwait(eax, ecx);
+		if (can_toggle_ibrs)

arch/x86/include/asm/mwait.h static inline void __sti_mwait(unsigned long eax, unsigned long ecx) · line 133

Quoted 4 lines

arch/x86/include/asm/mwait.h +1 -0

+			unprotected_speculation_end();
 	}
 	current_clr_polling();
 }

Argh.. no. Who is calling this with IRQs enabled? And why can't we frob the MSR with IRQs enabled? That comment doesn't seem to explain anything.

Quoted 23 lines

arch/x86/include/asm/spec_ctrl.h +14 -0

index 16fc4f58..28b0314 100644
@@ -76,5 +76,42 @@
 10:
 .endm
 
+#else
+#include <asm/microcode.h>
+
+static inline void __disable_indirect_speculation(void)
+{
+        native_wrmsrl(MSR_IA32_SPEC_CTRL, SPEC_CTRL_FEATURE_ENABLE_IBRS);
+}
+
+static inline void __enable_indirect_speculation(void)
+{
+        native_wrmsrl(MSR_IA32_SPEC_CTRL, SPEC_CTRL_FEATURE_DISABLE_IBRS);
+}
+
+/*

arch/x86/include/asm/spec_ctrl.h line 98

Quoted 6 lines

arch/x86/include/asm/spec_ctrl.h +6 -0

+ * Interrupts must be disabled to begin unprotected speculation.
+ * Otherwise interrupts could be running in unprotected mode.
+ */
+static inline void unprotected_speculation_begin(void)
+{
+	WARN_ON_ONCE(!irqs_disabled());

lockdep_assert_irqs_disabled()

Quoted 11 lines

arch/x86/include/asm/spec_ctrl.h +10 -0

+	if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
+		__enable_indirect_speculation();
+}
+
+static inline void unprotected_speculation_end(void)
+{
+	if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
+		__disable_indirect_speculation();
+	else
+		/*

arch/x86/include/asm/spec_ctrl.h line 113

Quoted 5 lines

arch/x86/include/asm/spec_ctrl.h +5 -0

+		 * If we intended to disable indirect speculation
+		 * but come here due to mis-speculation, we need
+		 * to stop the mis-speculation with rmb.
+		 */
+		rmb();

Code is lacking {}, also the comment doesn't make sense. If we don't have the MSR, why are we doing an LFENCE?

And why are these boot_cpu_has() and not static_cpu_has() ?

No one should be calling this with IRQs enabled. This check is probably just paranoid. I can get rid of it.

It probably doesn't matter as we will be switching the check to the spec_ctrl_ibrs a couple of patches later.

Quoted 2 lines · Tim Chen

No one should be calling this with IRQs enabled. This check is probably just paranoid. I can get rid of it.

Yes, confirmed.

Quoted 2 lines

It probably doesn't matter as we will be switching the check to the spec_ctrl_ibrs a couple of patches later.

The other problem with static_cpu_has is later the code intends to support the cpuid rescan post late microcode update which will set bits in boot_cpu_data (and transfer them to each cpu_has). If we'd only support early microcode update static_cpu_has should have been ok too.

What's the problem to make the early update mandatory for this?

Quoted 6 lines

Quoted 6 lines

arch/x86/include/asm/spec_ctrl.h +6 -0

+static inline void unprotected_speculation_end(void)
+{
+	if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
+		__disable_indirect_speculation();
+	else
+		/*

Quoted 8 lines

arch/x86/include/asm/spec_ctrl.h line 113

Quoted 5 lines

arch/x86/include/asm/spec_ctrl.h +5 -0

+		 * If we intended to disable indirect speculation
+		 * but come here due to mis-speculation, we need
+		 * to stop the mis-speculation with rmb.
+		 */
+		rmb();

Code is lacking {}, also the comment doesn't make sense. If we don't have the MSR, why are we doing an LFENCE?

The reason for lfence is because we intend to put up IBRS and disable speculation. But if the CPU mis-speculates into the else portion, we could be speculating without IBRS. The lfence stop the mis-speculation.

Quoted 2 lines

And why are these boot_cpu_has() and not static_cpu_has() ?

It probably doesn't matter as we will be switching the check to the spec_ctrl_ibrs a couple of patches later.

From: David Woodhouse <[email protected]>

We are impervious to the indirect branch prediction attack with retpoline but firmware won't be, so we still need to set IBRS to protect firmware code execution when calling into firmware at runtime.

From: David Woodhouse <[email protected]>

We are impervious to the indirect branch prediction attack with retpoline but firmware won't be, so we still need to set IBRS to protect firmware code execution when calling into firmware at runtime.

Quoted 7 lines · Tim Chen

From: David Woodhouse <[email protected]>

We are impervious to the indirect branch prediction attack with retpoline
but firmware won't be, so we still need to set IBRS to protect firmware code execution when calling into firmware at runtime.

Wait, what?

Maybe it's just the wine from dinner talking, but if the firmware has issues, we have bigger things to worry about here, right?  It already handed over the "chain of trust" to us, so we have already implicitly trusted that the firmware was correct here.  So why do we need to do anything about firmware calls in this manner?

Quoted 8 lines · Greg KH

Quoted 7 lines · Tim Chen

From: David Woodhouse <[email protected]>

We are impervious to the indirect branch prediction attack with retpoline
but firmware won't be, so we still need to set IBRS to protect firmware code execution when calling into firmware at runtime.

Quoted 7 lines

Wait, what?

Maybe it's just the wine from dinner talking, but if the firmware has issues, we have bigger things to worry about here, right?  It already handed over the "chain of trust" to us, so we have already implicitly trusted that the firmware was correct here.  So why do we need to do anything about firmware calls in this manner?

In the ideal world, firmware exists to boot the kernel and then it gets out of the way, never to be thought of again.

In the Intel world, firmware idiocy permeates everything and we sometimes end up making calls to it at runtime.

If an attacker can poison the BTB for an indirect branch in EFI firmware, then reliably do something which calls EFI runtime calls, that might lead to an exploit. So if we're using retpoline for the kernel, then we should be setting IBRS before any firmware calls.

Quoted 20 lines · David

Quoted 8 lines · Greg KH

Quoted 7 lines · Tim Chen

From: David Woodhouse <[email protected]>

We are impervious to the indirect branch prediction attack with retpoline
but firmware won't be, so we still need to set IBRS to protect firmware code execution when calling into firmware at runtime.

Quoted 7 lines

Wait, what?

Maybe it's just the wine from dinner talking, but if the firmware has issues, we have bigger things to worry about here, right?  It already handed over the "chain of trust" to us, so we have already implicitly trusted that the firmware was correct here.  So why do we need to do anything about firmware calls in this manner?

In the ideal world, firmware exists to boot the kernel and then it gets out of the way, never to be thought of again.

Quoted 7 lines

In the Intel world, firmware idiocy permeates everything and we sometimes end up making calls to it at runtime.

If an attacker can poison the BTB for an indirect branch in EFI firmware, then reliably do something which calls EFI runtime calls, that might lead to an exploit. So if we're using retpoline for the kernel, then we should be setting IBRS before any firmware calls.

Ugh, ok, seems a bit far-fetched to me, but I will not object anymore.

Except that the patch doesn't actually build, which means no one has actually tested it at all :(

Quoted 5 lines · Tim Chen

From: David Woodhouse <[email protected]>

We are impervious to the indirect branch prediction attack with retpoline but firmware won't be, so we still need to set IBRS to protect firmware code execution when calling into firmware at runtime.

Wait, what?

Maybe it's just the wine from dinner talking, but if the firmware has issues, we have bigger things to worry about here, right? It already handed over the "chain of trust" to us, so we have already implicitly trusted that the firmware was correct here. So why do we need to do anything about firmware calls in this manner?

Or am I totally missing something else here?

Quoted 5 lines · Tim Chen

From: David Woodhouse <[email protected]>

We are impervious to the indirect branch prediction attack with retpoline but firmware won't be, so we still need to set IBRS to protect firmware code execution when calling into firmware at runtime.

Wait, what?

Maybe it's just the wine from dinner talking, but if the firmware has issues, we have bigger things to worry about here, right? It already handed over the "chain of trust" to us, so we have already implicitly trusted that the firmware was correct here. So why do we need to do anything about firmware calls in this manner?

Or am I totally missing something else here?

(quoted diff) +0 -0

 }
 

static inline bool apm_bios_call_simple_asm(u32 func, u32 ebx_in,

arch/x86/include/asm/apm.h +1 -0

@@ -52,6 +56,7 @@ static inline bool apm_bios_call_simple_asm(u32 func, u32
 ebx_in,
        int     cx, dx, si;
        bool    error;
 
+       unprotected_firmware_begin();
        /*
         * N.B. We do NOT need a cld after the BIOS call

* because we always save and restore the flags.

arch/x86/include/asm/apm.h static inline void apm_bios_call_asm(u32 func, u32 ebx_in, · line 51

Quoted 6 lines · Tim Chen

arch/x86/include/asm/apm.h +1 -0

@@ -44,6 +47,7 @@ static inline void apm_bios_call_asm(u32 func, u32 ebx_in,
 u32 ecx_in,
                  "=S" (*esi)
                : "a" (func), "b" (ebx_in), "c" (ecx_in)
                : "memory", "cc");
+       unprotected_formware_end();

Just an obvious typo (form/firm) but I thought I'd report it.

Quoted 8 lines

(quoted diff) +0 -0

 }
 

static inline bool apm_bios_call_simple_asm(u32 func, u32 ebx_in,

arch/x86/include/asm/apm.h +0 -0

@@ -52,6 +56,7 @@ static inline bool apm_bios_call_simple_asm(u32 func, u32
 ebx_in,
        int     cx, dx, si;
        bool    error;
 

arch/x86/include/asm/apm.h static inline bool apm_bios_call_simple_asm(u32 func, u32 · line 79

Quoted 12 lines

arch/x86/include/asm/apm.h +1 -0

+       unprotected_firmware_begin();
        /*
         * N.B. We do NOT need a cld after the BIOS call

* because we always save and restore the flags.

arch/x86/include/asm/apm.h +1 -0

@@ -68,6 +73,7 @@ static inline bool apm_bios_call_simple_asm(u32 func, u32
 ebx_in,
                  "=S" (si)
                : "a" (func), "b" (ebx_in), "c" (ecx_in)
                : "memory", "cc");
+       unprotected_formware_end();
        return error;
 }

Same here.

arch/x86/include/asm/spec_ctrl.h +8 -0

index 28b0314..23b2804 100644
@@ -113,5 +113,42 @@ static inline void unprotected_speculation_end(void)
 		rmb();
 }
 
+
+#if defined(RETPOLINE)
+/*
+ * RETPOLINE does not protect against indirect speculation
+ * in firmware code.  Enable IBRS to protect firmware execution.
+ */
+static inline void unprotected_firmware_begin(void)
+{

arch/x86/include/asm/spec_ctrl.h +7 -0

+
+static inline void unprotected_firmware_end(void)
+{
+	if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
+                __enable_indirect_speculation();
+}
+

arch/x86/include/asm/spec_ctrl.h +2 -0

+#endif
+
 #endif /* __ASSEMBLY__ */
 #endif /* _ASM_X86_SPEC_CTRL_H */

Signature · 1 line

2.9.4

#ifdef APM_ZERO_SEGS
# define APM_DO_ZERO_SEGS \

arch/x86/include/asm/apm.h +1 -0

 		"pushl %%ds\n\t" \

@@ -28,6 +30,7 @@ static inline void apm_bios_call_asm(u32 func, u32 ebx_in, u32 ecx_in,
 					u32 *eax, u32 *ebx, u32 *ecx,
 					u32 *edx, u32 *esi)
 {
+	unprotected_firmware_begin();
 	/*
 	 * N.B. We do NOT need a cld after the BIOS call
 	 * because we always save and restore the flags.

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/ibrs_enabled will turn off IBRS
echo 1 > /sys/kernel/debug/ibrs_enabled will turn on IBRS in kernel echo 2 > /sys/kernel/debug/ibrs_enabled will turn on IBRS in both userspace and kernel

Quoted 4 lines

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

I am not sure that tristate is really needed. What's wrong with on/off only?

Distros have the tri-state already deployed.

Paranoid people want "IBRS always" aka "ibrs 2".

So why not "IBRS always" or off? No need for the "IBRS only in the kernel" setting.

Quoted 1 line

Paranoid people want "IBRS always" aka "ibrs 2".

So why not "IBRS always" or off? No need for the "IBRS only in the kernel" setting.

Quoted 4 lines · Borislav Petkov

Quoted 1 line

Paranoid people want "IBRS always" aka "ibrs 2".

So why not "IBRS always" or off? No need for the "IBRS only in the kernel" setting.

IBRS=1 slows execution down. If it's on all the time, you pay a performance cost in userspace. The assumption is that the user/kernel boundary switching cost is below the cost of having it on all the time.

Quoted 6 lines · Borislav Petkov

Quoted 4 lines

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

I am not sure that tristate is really needed. What's wrong with on/off only?

Lots of things:

Distros have the tri-state already deployed.

Paranoid people want "IBRS always" aka "ibrs 2".
Future CPUs may have cheap-enough IBRS to not be worth switching it.

Quoted 8 lines · Tim Chen

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.

Quoted 4 lines

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

I am not sure that tristate is really needed. What's wrong with on/off only?

Also, stuff like that:

arch/x86/kernel/cpu/spec_ctrl.c new file +3 -0

+/* mutex to serialize IBRS control changes */
+DEFINE_MUTEX(spec_ctrl_mutex);
+EXPORT_SYMBOL(spec_ctrl_mutex);

looks ugly.

Wrapper functions which everything calls and they hide internals are much more preferrable.

And then, if at all, this needs to be connected to the retpolines fun, methinks, so that it can be decided at boot what to use.

Quoted 2 lines

And then, if at all, this needs to be connected to the retpolines fun, methinks, so that it can be decided at boot what to use.

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

Quoted 2 lines · Andrea Arcangeli

Quoted 2 lines

And then, if at all, this needs to be connected to the retpolines fun, methinks, so that it can be decided at boot what to use.

Yes, we need to reconcile this with the retpoline. I've tried to capture what we do where in here:

Quoted 1 line

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

Any feedback on this, especially on the CPUs that aren't specifically covered would be welcome.

Consolidating in arch/x86/kernel/spec_ctrl.c would allow removing that export.

Here I've got:

static DEFINE_MUTEX(spec_ctrl_mutex);

Turning this off at any time is very easy, making reptoline runtime disabled is more difficult.

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/ibrs_enabled will turn off IBRS
echo 1 > /sys/kernel/debug/ibrs_enabled will turn on IBRS in kernel echo 2 > /sys/kernel/debug/ibrs_enabled will turn on IBRS in both userspace and kernel

Quoted 5 lines · Tim Chen

There are 2 ways to control IBRS

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

Quoted 7 lines

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/ibrs_enabled will turn off IBRS
echo 1 > /sys/kernel/debug/ibrs_enabled will turn on IBRS in kernel echo 2 > /sys/kernel/debug/ibrs_enabled will turn on IBRS in both userspace and kernel

debug/x86/?

Reviewed-by Andrea Arcangeli <[email protected]>

Thanks,
Andrea

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/ibrs_enabled will turn off IBRS
echo 1 > /sys/kernel/debug/ibrs_enabled will turn on IBRS in kernel echo 2 > /sys/kernel/debug/ibrs_enabled will turn on IBRS in both userspace and kernel

The implementation was updated with input from Andrea Arcangeli.

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/ibrs_enabled will turn off IBRS
echo 1 > /sys/kernel/debug/ibrs_enabled will turn on IBRS in kernel echo 2 > /sys/kernel/debug/ibrs_enabled will turn on IBRS in both userspace and kernel

The implementation was updated with input from Andrea Arcangeli.

Signed-off-by Tim Chen <[email protected]>

---
 Documentation/admin-guide/kernel-parameters.txt |   4 +
 arch/x86/include/asm/spec_ctrl.h                | 163 +++++++++++++++++++-----
 arch/x86/kernel/cpu/Makefile                    |   1 +
 arch/x86/kernel/cpu/scattered.c                 |  10 ++
 arch/x86/kernel/cpu/spec_ctrl.c                 | 124 ++++++++++++++++++
 5 files changed, 270 insertions(+), 32 deletions(-)
 create mode 100644 arch/x86/kernel/cpu/spec_ctrl.c

Documentation/admin-guide/kernel-parameters.txt +4 -0

index 5dfd262..d64f49f 100644
@@ -2573,6 +2573,10 @@
 			noexec=on: enable non-executable mappings (default)
 			noexec=off: disable non-executable mappings
 
+	noibrs          [X86]
+			Don't use indirect branch restricted speculation (IBRS)
+			feature.
+
 	nosmap		[X86]
 			Disable SMAP (Supervisor Mode Access Prevention)
 			even if it is supported by processor.

arch/x86/include/asm/spec_ctrl.h +65 -14

index 23b2804..2c35571 100644
@@ -1,13 +1,17 @@
 #ifndef _ASM_X86_SPEC_CTRL_H
 #define _ASM_X86_SPEC_CTRL_H
 
-#include <linux/stringify.h>
 #include <asm/msr-index.h>
 #include <asm/cpufeatures.h>
-#include <asm/alternative-asm.h>
+
+#define SPEC_CTRL_IBRS_INUSE      	(1<<0)	/* OS enables IBRS usage */
+#define SPEC_CTRL_IBRS_SUPPORTED	(1<<1)  /* System supports IBRS */
+#define SPEC_CTRL_IBRS_ADMIN_DISABLED	(1<<2)  /* Admin disables IBRS */
 
 #ifdef __ASSEMBLY__
 
+.extern spec_ctrl_ibrs
+
 .macro PUSH_MSR_REGS
 	pushq %rax
 	pushq %rcx

@@ -27,35 +31,63 @@
 .endm
 
 .macro ENABLE_IBRS
-	ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
+	testl $SPEC_CTRL_IBRS_INUSE, spec_ctrl_ibrs
+	jz .Lskip_\@
+
 	PUSH_MSR_REGS
 	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS
 	POP_MSR_REGS
-10:
+
+	jmp .Ldone_\@
+.Lskip_\@:
+	/*
+	 * prevent speculation beyond here as we could want to
+	 * stop speculation by enabling IBRS
+	 */
+	lfence
+.Ldone_\@:
 .endm
 
 .macro DISABLE_IBRS
-	ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
+	testl $SPEC_CTRL_IBRS_INUSE, spec_ctrl_ibrs
+	jz .Lskip_\@
+
 	PUSH_MSR_REGS
 	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS
 	POP_MSR_REGS
-10:
+
+.Lskip_\@:
 .endm
 
 .macro ENABLE_IBRS_CLOBBER
-	ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
+	testl $SPEC_CTRL_IBRS_INUSE, spec_ctrl_ibrs
+	jz .Lskip_\@
+
 	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS
-10:
+
+	jmp .Ldone_\@
+.Lskip_\@:
+	/*
+	 * prevent speculation beyond here as we could want to
+	 * stop speculation by enabling IBRS
+	 */
+	lfence
+.Ldone_\@:
 .endm
 
 .macro DISABLE_IBRS_CLOBBER
-	ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
+	testl $SPEC_CTRL_IBRS_INUSE, spec_ctrl_ibrs
+	jz .Lskip_\@
+
 	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS
-10:
+
+.Lskip_\@:
 .endm
 
 .macro ENABLE_IBRS_SAVE_AND_CLOBBER save_reg:req
-	ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
+	testl $SPEC_CTRL_IBRS_INUSE, spec_ctrl_ibrs
+	jz .Lskip_\@
+
 	movl	$MSR_IA32_SPEC_CTRL, %ecx
 	rdmsr
 	movl	%eax, \save_reg

@@ -63,22 +95,103 @@
 	movl	$0, %edx
 	movl	$SPEC_CTRL_FEATURE_ENABLE_IBRS, %eax
 	wrmsr
-10:
+
+	jmp .Ldone_\@
+.Lskip_\@:
+	/*
+	 * prevent speculation beyond here as we could want to
+	 * stop speculation by enabling IBRS
+	 */
+	lfence
+.Ldone_\@:
 .endm
 
 .macro RESTORE_IBRS_CLOBBER save_reg:req
-	ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
+	testl $SPEC_CTRL_IBRS_INUSE, spec_ctrl_ibrs
+	jz .Lskip_\@
+
 	/* Set IBRS to the value saved in the save_reg */
 	movl    $MSR_IA32_SPEC_CTRL, %ecx
         movl    $0, %edx
 	movl    \save_reg, %eax
 	wrmsr
-10:
+
+	jmp .Ldone_\@
+.Lskip_\@:
+	/*
+	 * prevent speculation beyond here as we could want to
+	 * stop speculation by enabling IBRS
+	 */
+	lfence
+.Ldone_\@:
 .endm
 
 #else
 #include <asm/microcode.h>
 
+extern int spec_ctrl_ibrs;

arch/x86/include/asm/spec_ctrl.h +56 -18

+};
+
+
+static inline void set_ibrs_inuse(void)
+{
+	if (spec_ctrl_ibrs & SPEC_CTRL_IBRS_SUPPORTED)
+		spec_ctrl_ibrs |= SPEC_CTRL_IBRS_INUSE;
+}
+
+static inline void clear_ibrs_inuse(void)
+{
+	spec_ctrl_ibrs &= ~SPEC_CTRL_IBRS_INUSE;
+}
+
+static inline int ibrs_inuse(void)
+{
+	if (spec_ctrl_ibrs & SPEC_CTRL_IBRS_INUSE)
+		return 1;
+	else
+		/*
+		 * prevent speculation beyond here as we could want to
+		 * stop speculation by enabling IBRS with this check
+		 */
+		rmb();
+	return 0;
+}
+
+static inline void set_ibrs_supported(void)
+{
+	spec_ctrl_ibrs |= SPEC_CTRL_IBRS_SUPPORTED;
+	if (!(spec_ctrl_ibrs & SPEC_CTRL_IBRS_ADMIN_DISABLED))
+		set_ibrs_inuse();
+	else
+		/*
+		 * prevent speculation beyond here as we could want to
+		 * stop speculation by enabling IBRS
+		 */
+		rmb();
+}
+
+static inline void set_ibrs_disabled(void)
+{
+	spec_ctrl_ibrs |= SPEC_CTRL_IBRS_ADMIN_DISABLED;
+	if (ibrs_inuse())
+		clear_ibrs_inuse();
+}
+
+static inline void clear_ibrs_disabled(void)
+{
+	spec_ctrl_ibrs &= ~SPEC_CTRL_IBRS_ADMIN_DISABLED;
+}
+
 static inline void __disable_indirect_speculation(void)
 {
         native_wrmsrl(MSR_IA32_SPEC_CTRL, SPEC_CTRL_FEATURE_ENABLE_IBRS);

@@ -96,21 +209,14 @@ static inline void __enable_indirect_speculation(void)
 static inline void unprotected_speculation_begin(void)
 {
 	WARN_ON_ONCE(!irqs_disabled());
-	if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
+	if (ibrs_inuse())
 		__enable_indirect_speculation();
 }
 
 static inline void unprotected_speculation_end(void)
 {
-	if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
+	if (ibrs_inuse())
 		__disable_indirect_speculation();
-	else
-		/*
-		 * If we intended to disable indirect speculation
-		 * but come here due to mis-speculation, we need
-		 * to stop the mis-speculation with rmb.
-		 */
-		rmb();
 }
 
 

@@ -121,20 +227,13 @@ static inline void unprotected_speculation_end(void)
  */
 static inline void unprotected_firmware_begin(void)
 {
-	if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
+	if (ibrs_inuse())
                 __disable_indirect_speculation();
-	else
-		/*
-		 * If we intended to disable indirect speculation
-		 * but come here due to mis-speculation, we need
-		 * to stop the mis-speculation with rmb.
-		 */
-		rmb();
 }
 
 static inline void unprotected_firmware_end(void)
 {
-	if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
+	if (ibrs_inuse())
                 __enable_indirect_speculation();
 }
 

arch/x86/kernel/cpu/Makefile +1 -0

index 90cb82d..a25f1ab 100644
@@ -24,6 +24,7 @@ obj-y			+= match.o
 obj-y			+= bugs.o
 obj-$(CONFIG_CPU_FREQ)	+= aperfmperf.o
 obj-y			+= cpuid-deps.o
+obj-y			+= spec_ctrl.o
 
 obj-$(CONFIG_PROC_FS)	+= proc.o
 obj-$(CONFIG_X86_FEATURE_NAMES) += capflags.o powerflags.o

arch/x86/kernel/cpu/scattered.c +10 -0

index bc50c40..2d23a2fe 100644
@@ -8,6 +8,7 @@
 #include <asm/processor.h>
 
 #include <asm/apic.h>
+#include <asm/spec_ctrl.h>
 
 struct cpuid_bit {
 	u16 feature;

@@ -56,6 +57,15 @@ void init_scattered_cpuid_features(struct cpuinfo_x86 *c)
 
 		if (regs[cb->reg] & (1 << cb->bit))
 			set_cpu_cap(c, cb->feature);
+
+	}
+	if (!c->cpu_index) {
+		if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) {
+			printk(KERN_INFO "FEATURE SPEC_CTRL Present\n");
+			set_ibrs_supported();
+			if (ibrs_inuse())
+				ibrs_enabled = IBRS_ENABLED;
+		}
 	}
 }
 

arch/x86/kernel/cpu/spec_ctrl.c new file +0 -0

new file mode 100644
index 0000000..6946678

arch/x86/kernel/cpu/spec_ctrl.c new file +14 -0

+
+static int __init noibrs(char *str)
+{
+        set_ibrs_disabled();
+
+        return 0;
+}
+early_param("noibrs", ex);
+
+static ssize_t __enabled_read(struct file *file, char __user *user_buf,
+                              size_t count, loff_t *ppos, unsigned int *field)
+{
+	char buf[32];
+	unsigned int len;

arch/x86/kernel/cpu/spec_ctrl.c new file +14 -0

+static void spec_ctrl_flush_all_cpus(u32 msr_nr, u64 val)
+{
+	int cpu;
+	get_online_cpus();
+	for_each_online_cpu(cpu)
+		wrmsrl_on_cpu(cpu, msr_nr, val);
+	put_online_cpus();
+}
+
+static ssize_t ibrs_enabled_write(struct file *file,
+                                  const char __user *user_buf,
+                                  size_t count, loff_t *ppos)
+{
+        char buf[32];

arch/x86/kernel/cpu/spec_ctrl.c new file +17 -0

+
+	if (enable == IBRS_DISABLED) {
+		/* disable IBRS usage */
+		set_ibrs_disabled();
+		if (spec_ctrl_ibrs & SPEC_CTRL_IBRS_SUPPORTED)
+			spec_ctrl_flush_all_cpus(MSR_IA32_SPEC_CTRL, SPEC_CTRL_FEATURE_DISABLE_IBRS);
+	} else if (enable == IBRS_ENABLED) {
+		/* enable IBRS usage in kernel */
+		clear_ibrs_disabled();
+		if (spec_ctrl_ibrs & SPEC_CTRL_IBRS_SUPPORTED)
+			set_ibrs_inuse();
+		else
+			/* Platform don't support IBRS */
+			enable = IBRS_DISABLED;
+	} else if (enable == IBRS_ENABLED_USER) {
+		/* enable IBRS usage in both userspace and kernel */
+		clear_ibrs_disabled();

Quoted 179 lines · Tim Chen

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/ibrs_enabled will turn off IBRS
echo 1 > /sys/kernel/debug/ibrs_enabled will turn on IBRS in kernel echo 2 > /sys/kernel/debug/ibrs_enabled will turn on IBRS in both userspace and kernel

The implementation was updated with input from Andrea Arcangeli.

Signed-off-by Tim Chen <[email protected]>

---
 Documentation/admin-guide/kernel-parameters.txt |   4 +
 arch/x86/include/asm/spec_ctrl.h                | 163 +++++++++++++++++++-----
 arch/x86/kernel/cpu/Makefile                    |   1 +
 arch/x86/kernel/cpu/scattered.c                 |  10 ++
 arch/x86/kernel/cpu/spec_ctrl.c                 | 124 ++++++++++++++++++
 5 files changed, 270 insertions(+), 32 deletions(-)
 create mode 100644 arch/x86/kernel/cpu/spec_ctrl.c

Documentation/admin-guide/kernel-parameters.txt +4 -0

index 5dfd262..d64f49f 100644
@@ -2573,6 +2573,10 @@
 			noexec=on: enable non-executable mappings (default)
 			noexec=off: disable non-executable mappings
 
+	noibrs          [X86]
+			Don't use indirect branch restricted speculation (IBRS)
+			feature.
+
 	nosmap		[X86]
 			Disable SMAP (Supervisor Mode Access Prevention)
 			even if it is supported by processor.

arch/x86/include/asm/spec_ctrl.h +65 -14

index 23b2804..2c35571 100644
@@ -1,13 +1,17 @@
 #ifndef _ASM_X86_SPEC_CTRL_H
 #define _ASM_X86_SPEC_CTRL_H
 
-#include <linux/stringify.h>
 #include <asm/msr-index.h>
 #include <asm/cpufeatures.h>
-#include <asm/alternative-asm.h>
+
+#define SPEC_CTRL_IBRS_INUSE      	(1<<0)	/* OS enables IBRS usage */
+#define SPEC_CTRL_IBRS_SUPPORTED	(1<<1)  /* System supports IBRS */
+#define SPEC_CTRL_IBRS_ADMIN_DISABLED	(1<<2)  /* Admin disables IBRS */
 
 #ifdef __ASSEMBLY__
 
+.extern spec_ctrl_ibrs
+
 .macro PUSH_MSR_REGS
 	pushq %rax
 	pushq %rcx

@@ -27,35 +31,63 @@
 .endm
 
 .macro ENABLE_IBRS
-	ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
+	testl $SPEC_CTRL_IBRS_INUSE, spec_ctrl_ibrs
+	jz .Lskip_\@
+
 	PUSH_MSR_REGS
 	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS
 	POP_MSR_REGS
-10:
+
+	jmp .Ldone_\@
+.Lskip_\@:
+	/*
+	 * prevent speculation beyond here as we could want to
+	 * stop speculation by enabling IBRS
+	 */
+	lfence
+.Ldone_\@:
 .endm
 
 .macro DISABLE_IBRS
-	ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
+	testl $SPEC_CTRL_IBRS_INUSE, spec_ctrl_ibrs
+	jz .Lskip_\@
+
 	PUSH_MSR_REGS
 	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS
 	POP_MSR_REGS
-10:
+
+.Lskip_\@:
 .endm
 
 .macro ENABLE_IBRS_CLOBBER
-	ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
+	testl $SPEC_CTRL_IBRS_INUSE, spec_ctrl_ibrs
+	jz .Lskip_\@
+
 	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS
-10:
+
+	jmp .Ldone_\@
+.Lskip_\@:
+	/*
+	 * prevent speculation beyond here as we could want to
+	 * stop speculation by enabling IBRS
+	 */
+	lfence
+.Ldone_\@:
 .endm
 
 .macro DISABLE_IBRS_CLOBBER
-	ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
+	testl $SPEC_CTRL_IBRS_INUSE, spec_ctrl_ibrs
+	jz .Lskip_\@
+
 	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS
-10:
+
+.Lskip_\@:
 .endm
 
 .macro ENABLE_IBRS_SAVE_AND_CLOBBER save_reg:req
-	ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
+	testl $SPEC_CTRL_IBRS_INUSE, spec_ctrl_ibrs
+	jz .Lskip_\@
+
 	movl	$MSR_IA32_SPEC_CTRL, %ecx
 	rdmsr
 	movl	%eax, \save_reg

@@ -63,22 +95,103 @@
 	movl	$0, %edx
 	movl	$SPEC_CTRL_FEATURE_ENABLE_IBRS, %eax
 	wrmsr
-10:
+
+	jmp .Ldone_\@
+.Lskip_\@:
+	/*
+	 * prevent speculation beyond here as we could want to
+	 * stop speculation by enabling IBRS
+	 */
+	lfence
+.Ldone_\@:
 .endm
 
 .macro RESTORE_IBRS_CLOBBER save_reg:req
-	ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
+	testl $SPEC_CTRL_IBRS_INUSE, spec_ctrl_ibrs
+	jz .Lskip_\@
+
 	/* Set IBRS to the value saved in the save_reg */
 	movl    $MSR_IA32_SPEC_CTRL, %ecx
         movl    $0, %edx
 	movl    \save_reg, %eax
 	wrmsr
-10:
+
+	jmp .Ldone_\@
+.Lskip_\@:
+	/*
+	 * prevent speculation beyond here as we could want to
+	 * stop speculation by enabling IBRS
+	 */
+	lfence
+.Ldone_\@:
 .endm
 
 #else
 #include <asm/microcode.h>
 
+extern int spec_ctrl_ibrs;

arch/x86/include/asm/spec_ctrl.h line 142

Quoted 10 lines

arch/x86/include/asm/spec_ctrl.h +10 -0

+extern struct mutex spec_ctrl_mutex;
+extern unsigned int ibrs_enabled;
+
+enum {
+        IBRS_DISABLED,
+        /* in host kernel, disabled in guest and userland */
+        IBRS_ENABLED,
+        /* in host kernel and host userland, disabled in guest */
+        IBRS_ENABLED_USER,
+        IBRS_MAX = IBRS_ENABLED_USER,

Yup, not a bit field, your documentation is wrong :(

Quoted 148 lines

arch/x86/include/asm/spec_ctrl.h +56 -18

+};
+
+
+static inline void set_ibrs_inuse(void)
+{
+	if (spec_ctrl_ibrs & SPEC_CTRL_IBRS_SUPPORTED)
+		spec_ctrl_ibrs |= SPEC_CTRL_IBRS_INUSE;
+}
+
+static inline void clear_ibrs_inuse(void)
+{
+	spec_ctrl_ibrs &= ~SPEC_CTRL_IBRS_INUSE;
+}
+
+static inline int ibrs_inuse(void)
+{
+	if (spec_ctrl_ibrs & SPEC_CTRL_IBRS_INUSE)
+		return 1;
+	else
+		/*
+		 * prevent speculation beyond here as we could want to
+		 * stop speculation by enabling IBRS with this check
+		 */
+		rmb();
+	return 0;
+}
+
+static inline void set_ibrs_supported(void)
+{
+	spec_ctrl_ibrs |= SPEC_CTRL_IBRS_SUPPORTED;
+	if (!(spec_ctrl_ibrs & SPEC_CTRL_IBRS_ADMIN_DISABLED))
+		set_ibrs_inuse();
+	else
+		/*
+		 * prevent speculation beyond here as we could want to
+		 * stop speculation by enabling IBRS
+		 */
+		rmb();
+}
+
+static inline void set_ibrs_disabled(void)
+{
+	spec_ctrl_ibrs |= SPEC_CTRL_IBRS_ADMIN_DISABLED;
+	if (ibrs_inuse())
+		clear_ibrs_inuse();
+}
+
+static inline void clear_ibrs_disabled(void)
+{
+	spec_ctrl_ibrs &= ~SPEC_CTRL_IBRS_ADMIN_DISABLED;
+}
+
 static inline void __disable_indirect_speculation(void)
 {
         native_wrmsrl(MSR_IA32_SPEC_CTRL, SPEC_CTRL_FEATURE_ENABLE_IBRS);

@@ -96,21 +209,14 @@ static inline void __enable_indirect_speculation(void)
 static inline void unprotected_speculation_begin(void)
 {
 	WARN_ON_ONCE(!irqs_disabled());
-	if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
+	if (ibrs_inuse())
 		__enable_indirect_speculation();
 }
 
 static inline void unprotected_speculation_end(void)
 {
-	if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
+	if (ibrs_inuse())
 		__disable_indirect_speculation();
-	else
-		/*
-		 * If we intended to disable indirect speculation
-		 * but come here due to mis-speculation, we need
-		 * to stop the mis-speculation with rmb.
-		 */
-		rmb();
 }
 
 

@@ -121,20 +227,13 @@ static inline void unprotected_speculation_end(void)
  */
 static inline void unprotected_firmware_begin(void)
 {
-	if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
+	if (ibrs_inuse())
                 __disable_indirect_speculation();
-	else
-		/*
-		 * If we intended to disable indirect speculation
-		 * but come here due to mis-speculation, we need
-		 * to stop the mis-speculation with rmb.
-		 */
-		rmb();
 }
 
 static inline void unprotected_firmware_end(void)
 {
-	if (boot_cpu_has(X86_FEATURE_SPEC_CTRL))
+	if (ibrs_inuse())
                 __enable_indirect_speculation();
 }
 

arch/x86/kernel/cpu/Makefile +1 -0

index 90cb82d..a25f1ab 100644
@@ -24,6 +24,7 @@ obj-y			+= match.o
 obj-y			+= bugs.o
 obj-$(CONFIG_CPU_FREQ)	+= aperfmperf.o
 obj-y			+= cpuid-deps.o
+obj-y			+= spec_ctrl.o
 
 obj-$(CONFIG_PROC_FS)	+= proc.o
 obj-$(CONFIG_X86_FEATURE_NAMES) += capflags.o powerflags.o

arch/x86/kernel/cpu/scattered.c +10 -0

index bc50c40..2d23a2fe 100644
@@ -8,6 +8,7 @@
 #include <asm/processor.h>
 
 #include <asm/apic.h>
+#include <asm/spec_ctrl.h>
 
 struct cpuid_bit {
 	u16 feature;

@@ -56,6 +57,15 @@ void init_scattered_cpuid_features(struct cpuinfo_x86 *c)
 
 		if (regs[cb->reg] & (1 << cb->bit))
 			set_cpu_cap(c, cb->feature);
+
+	}
+	if (!c->cpu_index) {
+		if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) {
+			printk(KERN_INFO "FEATURE SPEC_CTRL Present\n");
+			set_ibrs_supported();
+			if (ibrs_inuse())
+				ibrs_enabled = IBRS_ENABLED;
+		}
 	}
 }
 

arch/x86/kernel/cpu/spec_ctrl.c new file +0 -0

new file mode 100644
index 0000000..6946678

arch/x86/kernel/cpu/spec_ctrl.c line 1

Quoted 1 line

arch/x86/kernel/cpu/spec_ctrl.c new file +1 -0

@@ -0,0 +1,124 @@
+#include <linux/export.h>

No copyright or SPDX line?

At least it doesn't have the old horrid Intel header boilerplate, so I should be thankful of that. But it isn't ok like this either, sorry.

arch/x86/kernel/cpu/spec_ctrl.c line 14

Quoted 13 lines

arch/x86/kernel/cpu/spec_ctrl.c new file +13 -0

+#include <linux/init.h>
+#include <linux/mutex.h>
+#include <linux/debugfs.h>
+#include <linux/uaccess.h>
+
+#include <asm/spec_ctrl.h>
+#include <asm/cpufeature.h>
+
+/*
+ * spec_ctrl_ibrs
+ * bit 0 = indicate if ibrs is currently in use
+ * bit 1 = indicate if system supports ibrs
+ * bit 2 = indicate if admin disables ibrs

Why bits and not integer values? Can you mix them?

arch/x86/kernel/cpu/spec_ctrl.c line 18

Quoted 5 lines

arch/x86/kernel/cpu/spec_ctrl.c new file +4 -0

+ */
+
+int spec_ctrl_ibrs;
+EXPORT_SYMBOL(spec_ctrl_ibrs);

Why is this exported? What module will ever need this?

And horrid global symbol name, it doesn't say what it is in an obvious way just by looking at it.

arch/x86/kernel/cpu/spec_ctrl.c line 22

Quoted 4 lines

arch/x86/kernel/cpu/spec_ctrl.c new file +4 -0

+
+/* mutex to serialize IBRS control changes */
+DEFINE_MUTEX(spec_ctrl_mutex);
+EXPORT_SYMBOL(spec_ctrl_mutex);

Why export this? You only ever use it in one place? Again, what module needs it?

And I have to ask, EXPORT_SYMBOL_GPL() for this and spec_ctrl_ibrs as well?

arch/x86/kernel/cpu/spec_ctrl.c line 25

Quoted 2 lines

arch/x86/kernel/cpu/spec_ctrl.c new file +2 -0

+unsigned int ibrs_enabled __read_mostly;
+EXPORT_SYMBOL(ibrs_enabled);

Again, what module needs this?

And did __read_mostly really matter in performance tests?

Quoted 14 lines

arch/x86/kernel/cpu/spec_ctrl.c new file +14 -0

+
+static int __init noibrs(char *str)
+{
+        set_ibrs_disabled();
+
+        return 0;
+}
+early_param("noibrs", ex);
+
+static ssize_t __enabled_read(struct file *file, char __user *user_buf,
+                              size_t count, loff_t *ppos, unsigned int *field)
+{
+	char buf[32];
+	unsigned int len;

arch/x86/kernel/cpu/spec_ctrl.c line 49

Quoted 10 lines

arch/x86/kernel/cpu/spec_ctrl.c new file +10 -0

+
+	len = sprintf(buf, "%d\n", READ_ONCE(*field));
+	return simple_read_from_buffer(user_buf, count, ppos, buf, len);
+}
+
+static ssize_t ibrs_enabled_read(struct file *file, char __user *user_buf,
+                                 size_t count, loff_t *ppos)
+{
+	return __enabled_read(file, user_buf, count, ppos, &ibrs_enabled);
+}

It's a single variable, why such complex debugfs for that? This should be handled by a helper macro already, right?

Quoted 18 lines

arch/x86/kernel/cpu/spec_ctrl.c new file +18 -0

+static void spec_ctrl_flush_all_cpus(u32 msr_nr, u64 val)
+{
+	int cpu;
+	get_online_cpus();
+	for_each_online_cpu(cpu)
+		wrmsrl_on_cpu(cpu, msr_nr, val);
+	put_online_cpus();
+}
+
+static ssize_t ibrs_enabled_write(struct file *file,
+                                  const char __user *user_buf,
+                                  size_t count, loff_t *ppos)
+{
+        char buf[32];
+        ssize_t len;
+        unsigned int enable;
+
+        len = min(count, sizeof(buf) - 1);

arch/x86/kernel/cpu/spec_ctrl.c line 74

Quoted 6 lines

arch/x86/kernel/cpu/spec_ctrl.c new file +6 -0

+        if (copy_from_user(buf, user_buf, len))
+                return -EFAULT;
+
+        buf[len] = '\0';
+        if (kstrtouint(buf, 0, &enable))
+                return -EINVAL;

3 value "flags" are horrid :(

arch/x86/kernel/cpu/spec_ctrl.c line 79

Quoted 6 lines

arch/x86/kernel/cpu/spec_ctrl.c new file +5 -0

+
+        if (enable > IBRS_MAX)
+                return -EINVAL;
+
+	mutex_lock(&spec_ctrl_mutex);

Always run checkpatch.pl so you don't get grumpy kernel maintainers telling you to run checkpatch.pl :(

Quoted 21 lines

arch/x86/kernel/cpu/spec_ctrl.c new file +21 -0

+
+	if (enable == IBRS_DISABLED) {
+		/* disable IBRS usage */
+		set_ibrs_disabled();
+		if (spec_ctrl_ibrs & SPEC_CTRL_IBRS_SUPPORTED)
+			spec_ctrl_flush_all_cpus(MSR_IA32_SPEC_CTRL, SPEC_CTRL_FEATURE_DISABLE_IBRS);
+	} else if (enable == IBRS_ENABLED) {
+		/* enable IBRS usage in kernel */
+		clear_ibrs_disabled();
+		if (spec_ctrl_ibrs & SPEC_CTRL_IBRS_SUPPORTED)
+			set_ibrs_inuse();
+		else
+			/* Platform don't support IBRS */
+			enable = IBRS_DISABLED;
+	} else if (enable == IBRS_ENABLED_USER) {
+		/* enable IBRS usage in both userspace and kernel */
+		clear_ibrs_disabled();
+		/* don't change IBRS value once we set it to always on */
+		clear_ibrs_inuse();
+		if (spec_ctrl_ibrs & SPEC_CTRL_IBRS_SUPPORTED)
+			spec_ctrl_flush_all_cpus(MSR_IA32_SPEC_CTRL, SPEC_CTRL_FEATURE_ENABLE_IBRS);

arch/x86/kernel/cpu/spec_ctrl.c line 106

Quoted 6 lines

arch/x86/kernel/cpu/spec_ctrl.c new file +6 -0

+		else
+			/* Platform don't support IBRS */
+			enable = IBRS_DISABLED;
+	}
+
+	WRITE_ONCE(ibrs_enabled, enable);

It's a debugfs write callback, why do you care about WRITE_ONCE()?

thanks,

greg k-h

.macro ENABLE_IBRS

arch/x86/include/asm/spec_ctrl.h +3 -2

-	ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
+	testl $SPEC_CTRL_IBRS_INUSE, spec_ctrl_ibrs
+	jz .Lskip_\@
+
 	PUSH_MSR_REGS
 	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS
 	POP_MSR_REGS
-10:

Quoted 15 lines · Tim Chen

.macro ENABLE_IBRS

arch/x86/include/asm/spec_ctrl.h +9 -2

-	ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
+	testl $SPEC_CTRL_IBRS_INUSE, spec_ctrl_ibrs
+	jz .Lskip_\@
+
 	PUSH_MSR_REGS
 	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS
 	POP_MSR_REGS
-10:
+
+	jmp .Ldone_\@
+.Lskip_\@:
+	/*
+	 * prevent speculation beyond here as we could want to
+	 * stop speculation by enabling IBRS

arch/x86/include/asm/spec_ctrl.h line 49

Quoted 4 lines

arch/x86/include/asm/spec_ctrl.h +3 -0

+	 */
+	lfence
+.Ldone_\@:
 .endm

Yeah no. We have jump labels for this stuff. There is no reason what so ever to do dynamic tests for a variable that _never_ changes.

Quoted 16 lines · Peter Zijlstra

Quoted 15 lines · Tim Chen

.macro ENABLE_IBRS

arch/x86/include/asm/spec_ctrl.h +9 -2

-	ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL
+	testl $SPEC_CTRL_IBRS_INUSE, spec_ctrl_ibrs
+	jz .Lskip_\@
+
 	PUSH_MSR_REGS
 	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS
 	POP_MSR_REGS
-10:
+
+	jmp .Ldone_\@
+.Lskip_\@:
+	/*
+	 * prevent speculation beyond here as we could want to
+	 * stop speculation by enabling IBRS

Quoted 9 lines

arch/x86/include/asm/spec_ctrl.h line 49

Quoted 4 lines

arch/x86/include/asm/spec_ctrl.h +3 -0

+	 */
+	lfence
+.Ldone_\@:
 .endm

Yeah no. We have jump labels for this stuff. There is no reason what so ever to do dynamic tests for a variable that _never_ changes.

Admin can change spec_ctrl_ibrs value at run time, or when we scan new microcode. So it doesn't often change, but it could.

There may be time when the admin wants to run the system in a more secure mode, and time when it is okay to leave out IBRS.

In older kernels there's not even the boilerplate to check the static key in asm,

Quoted 2 lines · Andrea Arcangeli

In older kernels there's not even the boilerplate to check the static key in asm,

This is the reason why I didn't use a static_key too.

+       mutex_lock(&spec_ctrl_mutex);
+
+       if (enable == IBRS_DISABLED) {
+               /* disable IBRS usage */
+               set_ibrs_disabled();
+               if (spec_ctrl_ibrs & SPEC_CTRL_IBRS_SUPPORTED)
+                       spec_ctrl_flush_all_cpus(MSR_IA32_SPEC_CTRL, SPEC_CTRL_FEATURE_DISABLE_IBRS); +       } else if (enable == IBRS_ENABLED) {
+               /* enable IBRS usage in kernel */
+               clear_ibrs_disabled();
+               if (spec_ctrl_ibrs & SPEC_CTRL_IBRS_SUPPORTED)
+                       set_ibrs_inuse();
+               else
+                       /* Platform don't support IBRS */
+                       enable = IBRS_DISABLED;
+       } else if (enable == IBRS_ENABLED_USER) {
+               /* enable IBRS usage in both userspace and kernel */
+               clear_ibrs_disabled();

+               /* don't change IBRS value once we set it to always on */
+               clear_ibrs_inuse();
+               if (spec_ctrl_ibrs & SPEC_CTRL_IBRS_SUPPORTED)
+                       spec_ctrl_flush_all_cpus(MSR_IA32_SPEC_CTRL, SPEC_CTRL_FEATURE_ENABLE_IBRS); +               else
+                       /* Platform don't support IBRS */
+                       enable = IBRS_DISABLED;
+       }

Quoted 17 lines · Tim Chen

arch/x86/kernel/cpu/spec_ctrl.c new file +16 -0

+       mutex_lock(&spec_ctrl_mutex);
+
+       if (enable == IBRS_DISABLED) {
+               /* disable IBRS usage */
+               set_ibrs_disabled();
+               if (spec_ctrl_ibrs & SPEC_CTRL_IBRS_SUPPORTED)
+                       spec_ctrl_flush_all_cpus(MSR_IA32_SPEC_CTRL, SPEC_CTRL_FEATURE_DISABLE_IBRS);
+       } else if (enable == IBRS_ENABLED) {
+               /* enable IBRS usage in kernel */
+               clear_ibrs_disabled();
+               if (spec_ctrl_ibrs & SPEC_CTRL_IBRS_SUPPORTED)
+                       set_ibrs_inuse();
+               else
+                       /* Platform don't support IBRS */
+                       enable = IBRS_DISABLED;
+       } else if (enable == IBRS_ENABLED_USER) {

arch/x86/kernel/cpu/spec_ctrl.c line 104

Quoted 10 lines

arch/x86/kernel/cpu/spec_ctrl.c new file +10 -0

+               /* enable IBRS usage in both userspace and kernel */
+               clear_ibrs_disabled();
+               /* don't change IBRS value once we set it to always on */
+               clear_ibrs_inuse();
+               if (spec_ctrl_ibrs & SPEC_CTRL_IBRS_SUPPORTED)
+                       spec_ctrl_flush_all_cpus(MSR_IA32_SPEC_CTRL, SPEC_CTRL_FEATURE_ENABLE_IBRS);
+               else
+                       /* Platform don't support IBRS */
+                       enable = IBRS_DISABLED;
+       }

This doesn't take the retpoline status into account. If we have retpoline, we don't need IBRS in the kernel.

On new microcode write, check whether IBRS
is present by rescanning scattered CPU features.

Signed-off-by Tim Chen <[email protected]>

---
 arch/x86/kernel/cpu/microcode/core.c | 6 ++++++
 1 file changed, 6 insertions(+)

arch/x86/kernel/cpu/microcode/core.c +1 -0

index c4fa4a8..44b9355 100644
@@ -40,6 +40,7 @@
 #include <asm/processor.h>
 #include <asm/cmdline.h>
 #include <asm/setup.h>
+#include <asm/spec_ctrl.h>
 
 #define DRIVER_VERSION	"2.2"
 

Quoted 24 lines · Tim Chen

On new microcode write, check whether IBRS
is present by rescanning scattered CPU features.

Signed-off-by Tim Chen <[email protected]>

---
 arch/x86/kernel/cpu/microcode/core.c | 6 ++++++
 1 file changed, 6 insertions(+)

arch/x86/kernel/cpu/microcode/core.c +1 -0

index c4fa4a8..44b9355 100644
@@ -40,6 +40,7 @@
 #include <asm/processor.h>
 #include <asm/cmdline.h>
 #include <asm/setup.h>
+#include <asm/spec_ctrl.h>
 
 #define DRIVER_VERSION	"2.2"
 

@@ -444,6 +445,11 @@ static ssize_t microcode_write(struct file *file, const char __user *buf,
 	if (ret > 0)
 		perf_check_microcode();
 

arch/x86/kernel/cpu/microcode/core.c static ssize_t microcode_write(struct file *file, const char __user *buf, · line 450

Quoted 3 lines

arch/x86/kernel/cpu/microcode/core.c +3 -0

+	/* check spec_ctrl capabilities */
+	mutex_lock(&spec_ctrl_mutex);
+	init_scattered_cpuid_features(&boot_cpu_data);

No need for that - make a specific function like perf_check_microcode() which checks only the IBRS bit and updates stuff accordingly.

void spec_ctrl_rescan_cpuid(void)

{
	int cpu;

	if (use_ibp_disable)
		return;
	mutex_lock(&spec_ctrl_mutex);
	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ||
	    boot_cpu_data.x86_vendor == X86_VENDOR_AMD) {
		/* detect spec ctrl related cpuid additions */
		init_scattered_cpuid_features(&boot_cpu_data);

Quoted 11 lines · Andrea Arcangeli

void spec_ctrl_rescan_cpuid(void)

{
	int cpu;

	if (use_ibp_disable)
		return;
	mutex_lock(&spec_ctrl_mutex);
	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ||
	    boot_cpu_data.x86_vendor == X86_VENDOR_AMD) {
		/* detect spec ctrl related cpuid additions */
		init_scattered_cpuid_features(&boot_cpu_data);

You don't need to noodle through all the scattered features - just the two bits.

Quoted 11 lines · Andrea Arcangeli

void spec_ctrl_rescan_cpuid(void)

{
	int cpu;

	if (use_ibp_disable)
		return;
	mutex_lock(&spec_ctrl_mutex);
	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ||
	    boot_cpu_data.x86_vendor == X86_VENDOR_AMD) {
		/* detect spec ctrl related cpuid additions */
		init_scattered_cpuid_features(&boot_cpu_data);

You don't need to noodle through all the scattered features - just the two bits.

Does it really matter? Rescanning everything can't hurt here, and it shouldn't be noticable in any boot time chart. I feel like arguing about tiny stuff like this takes away from the obvious other problems this whole patch series had :(

Quoted 4 lines · Greg KH

Does it really matter? Rescanning everything can't hurt here, and it shouldn't be noticable in any boot time chart. I feel like arguing about tiny stuff like this takes away from the obvious other problems this whole patch series had :(

It does because others will come and copy this and then we're stuck with cleaning up the mess. The whole CPUID parsing needs a much desired cleanup but there's other more pressing shit going on..

Quoted 13 lines · Borislav Petkov

Quoted 11 lines · Andrea Arcangeli

void spec_ctrl_rescan_cpuid(void)

{
	int cpu;

	if (use_ibp_disable)
		return;
	mutex_lock(&spec_ctrl_mutex);
	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL ||
	    boot_cpu_data.x86_vendor == X86_VENDOR_AMD) {
		/* detect spec ctrl related cpuid additions */
		init_scattered_cpuid_features(&boot_cpu_data);

Quoted 2 lines

You don't need to noodle through all the scattered features - just the two bits.

Does it really matter? Rescanning everything can't hurt here, and it shouldn't be noticable in any boot time chart. I feel like arguing about tiny stuff like this takes away from the obvious other problems this whole patch series had :(

Speculation on Skylake and later requires these patches ("dynamic IBRS") be used instead of retpoline[1].

Quoted 5 lines · Tim Chen

Speculation on Skylake and later requires these patches ("dynamic IBRS")
be used instead of retpoline[1].

Can somebody explain this part?

I was assuming that retpoline would work around this issue on all uarchs.

This seems to say "retpoline does nothing on Skylake+"

On Skylake the target for a 'ret' instruction may also come from the BTB. So if you ever let the RSB (which remembers where the 'call's came from get empty, you end up vulnerable.

Quoted 4 lines · David Woodhouse

On Skylake the target for a 'ret' instruction may also come from the BTB. So if you ever let the RSB (which remembers where the 'call's came from get empty, you end up vulnerable.

That sounds like it could cause mispredicts, but it doesn't sound _exploitable_.

Sure, interrupts in between the call instruction and the 'ret' could overflow the return stack. And we could migrate to another CPU. And so apparently SMM clears the return stack too.

... but again, none of them sound even remotely _exploitable_.

Quoted 3 lines · David Woodhouse

On Skylake the target for a 'ret' instruction may also come from the BTB. So if you ever let the RSB (which remembers where the 'call's came from get empty, you end up vulnerable.

That sounds like it could cause mispredicts, but it doesn't sound _exploitable_.

Sure, interrupts in between the call instruction and the 'ret' could overflow the return stack. And we could migrate to another CPU. And so apparently SMM clears the return stack too.

... but again, none of them sound even remotely _exploitable_.

Remember: it's not mispredicts that leak information. It's *exploits" that use forced very specific mispredicts to leak information.

There's a big difference there. And I think patch authors should keep that difference in mind.

For example, flushing the BTB at kernel entry doesn't mean that later in-kernel indirect branches don't get predicted, and doesn't even mean that they don't get mis-predicted. It only means that an exploit can't pre-populate those things and use them for exploits.

Also remember that sibling threads share a BTB, so you can't rely on isolated straight-line codepath on the current cpu for safety. (e.g. by issuing an IBPB on every entry to supervisor mode).

Quoted 4 lines · David Woodhouse

On Skylake the target for a 'ret' instruction may also come from the BTB. So if you ever let the RSB (which remembers where the 'call's came from get empty, you end up vulnerable.

That sounds like it could cause mispredicts, but it doesn't sound _exploitable_.

Sure, interrupts in between the call instruction and the 'ret' could overflow the return stack. And we could migrate to another CPU. And so apparently SMM clears the return stack too.

... but again, none of them sound even remotely _exploitable_.

this is about a level of paranoia you are comfortable with.

Retpoline on Skylake raises the bar for the issue enormously, but there are a set of corner cases that exist and that are not trivial to prove you dealt with them.

personally I am comfortable with retpoline on Skylake, but I would like to have IBRS as an opt in for the paranoid.

Quoted 6 lines · Arjan

this is about a level of paranoia you are comfortable with.

Retpoline on Skylake raises the bar for the issue enormously, but there are a set of corner cases that exist and that are not trivial to prove you dealt with them.

Quoted 3 lines

personally I am comfortable with retpoline on Skylake, but I would like to have IBRS as an opt in for the paranoid.

IBRS actually seems to perform more than decently on Skylake with the microcode updates we're shipping in RHEL---much more so than Broadwell or Haswell. Can you confirm that this is expected?