Update retpoline boot fix

Updated fix for the retpoline boot failure.

v3: Now uses X86_FEATURE_RETPOLINE_GENERIC / X86_FEATURE_GENERIC as requested by Thomas

From: Andi Kleen <[email protected]>

On 64bit we want to split X86_FEATURE_RETPOLINE and X86_FEATURE_RETPOLINE_AMD, so that only one is set. This enables new simpler ALTERNATIVE sequences. But 32bit requires a common flag because it uses common code for both.

Rename the existing X86_FEATURE_RETPOLINE to X86_FEATURE_GENERIC Use the existing X86_FEATURE_RETPOLINE as a common flag that is set for both the AMD and generic Retpoline.

Then don't set the generic RETPOLINE flag for AMD.

---
 arch/x86/include/asm/cpufeatures.h | 3 ++-
 arch/x86/kernel/cpu/bugs.c         | 4 +++-
 2 files changed, 5 insertions(+), 2 deletions(-)

index f275447862f4..9db5ed0e01a9 100644
@@ -203,7 +203,7 @@
 #define X86_FEATURE_PROC_FEEDBACK	( 7*32+ 9) /* AMD ProcFeedbackInterface */
 #define X86_FEATURE_SME			( 7*32+10) /* AMD Secure Memory Encryption */
 #define X86_FEATURE_PTI			( 7*32+11) /* Kernel Page Table Isolation enabled */
-#define X86_FEATURE_RETPOLINE		( 7*32+12) /* Generic Retpoline mitigation for Spectre variant 2 */
+#define X86_FEATURE_RETPOLINE_GENERIC	( 7*32+12) /* Generic Retpoline mitigation for Spectre variant 2 */
 #define X86_FEATURE_RETPOLINE_AMD	( 7*32+13) /* AMD Retpoline mitigation for Spectre variant 2 */
 #define X86_FEATURE_INTEL_PPIN		( 7*32+14) /* Intel Processor Inventory Number */
 #define X86_FEATURE_INTEL_PT		( 7*32+15) /* Intel Processor Trace */

@@ -211,6 +211,7 @@
 #define X86_FEATURE_AVX512_4FMAPS	( 7*32+17) /* AVX-512 Multiply Accumulation Single precision */
 
 #define X86_FEATURE_MBA			( 7*32+18) /* Memory Bandwidth Allocation */
+#define X86_FEATURE_RETPOLINE		( 7*32+19) /* Common code for both RETPOLINE_GENERIC and RETPOLINE_AMD */
 
 /* Virtualization flags: Linux defined, word 8 */
 #define X86_FEATURE_TPR_SHADOW		( 8*32+ 0) /* Intel TPR Shadow */

index b957f771a5b7..68b08967a6e1 100644
@@ -155,6 +155,7 @@ void __init spectre_v2_check_boottime_disable(void)
 	retpoline_generic:
 		spectre_v2_enabled = retp_compiler() ?
 			SPECTRE_V2_RETPOLINE_GENERIC : SPECTRE_V2_RETPOLINE_MINIMAL;
+		setup_force_cpu_cap(X86_FEATURE_RETPOLINE_GENERIC);
 	}
 	setup_force_cpu_cap(X86_FEATURE_RETPOLINE);
 	return;

@@ -165,8 +166,9 @@ void __init spectre_v2_check_boottime_disable(void)
 	pr_err("kernel not compiled with retpoline; no mitigation available!");
 #endif
 disable:
-	setup_clear_cpu_cap(X86_FEATURE_RETPOLINE);
+	setup_clear_cpu_cap(X86_FEATURE_RETPOLINE_GENERIC);
 	setup_clear_cpu_cap(X86_FEATURE_RETPOLINE_AMD);
+	setup_clear_cpu_cap(X86_FEATURE_RETPOLINE);
 	return;
 }
 

2.14.3

From: Andi Kleen <[email protected]>

[This fixes a boot failure in the earlier patches so may want to be moved earlier to keep git bisect happy]

With the latest tip x86/pti I get oopses when booting a 64bit VM in qemu with RETPOLINE/gcc7 and PTI enabled. Something is wrong with the ALTERNATIVE_2 sequence used in NOSPEC_JMP

Linus suggested a better sequence that is shorter and simpler and avoids the problem.

It requires excluding X86_FEATURE_RETPOLINE and X86_FEATURE_RETPOLINE_AMD, but that has been done in the previous patch.

Use the new sequence for NOSPEC_CALL and NOSPEC_JMP.

v2: Use new sequence for CALL/RET. Add extra patch for _COMMON.
v3: Use RETPOLINE_GENERIC for Intel

---
 arch/x86/include/asm/nospec-branch.h | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

index 8ddf8513550e..dc13325a9890 100644
@@ -46,9 +46,8 @@
  */
 .macro JMP_NOSPEC reg:req
 #ifdef CONFIG_RETPOLINE
-	ALTERNATIVE_2 __stringify(jmp *\reg),				\
-		__stringify(RETPOLINE_JMP \reg), X86_FEATURE_RETPOLINE,	\
-		__stringify(lfence; jmp *\reg), X86_FEATURE_RETPOLINE_AMD
+	ALTERNATIVE "", "lfence", X86_FEATURE_RETPOLINE_AMD
+	ALTERNATIVE __stringify(jmp *\reg), __stringify(RETPOLINE_JMP \reg), X86_FEATURE_RETPOLINE_GENERIC
 #else
 	jmp	*\reg
 #endif

@@ -56,9 +55,8 @@
 
 .macro CALL_NOSPEC reg:req
 #ifdef CONFIG_RETPOLINE
-	ALTERNATIVE_2 __stringify(call *\reg),				\
-		__stringify(RETPOLINE_CALL \reg), X86_FEATURE_RETPOLINE,\
-		__stringify(lfence; call *\reg), X86_FEATURE_RETPOLINE_AMD
+	ALTERNATIVE "", "lfence", X86_FEATURE_RETPOLINE_AMD
+	ALTERNATIVE __stringify(call *\reg), __stringify(RETPOLINE_CALL \reg), X86_FEATURE_RETPOLINE_GENERIC
 #else
 	call	*\reg
 #endif

2.14.3

This is just masking a problem which has now been fixed properly elsewhere — by removing the NOPs from the start of the underlying RETPOLINE_JMP sequence, *and* by fixing the alternatives mechanism not to get confused when the altinstr sequence starts with a NOP.

I'm not really convinced by the alternative. It's actually *longer*, because the lfence can no longer be tucked away in the space that the full retpoline implementation would have taken. You've prepended a three-byte 'nop' to the full retpoline.

And I'm not really sure it's simpler either. We go from "do <this>, or <this> or <this>", with each alternative being a complete and equivalent way to branch to the register, to a more complex matrix based on two conditions.

On the whole, given that the actual bug is fixed already, I am inclined to dismiss this — albeit carefully, since it was partly Linus' suggestion — as bikeshedding.

From: Andi Kleen <[email protected]>

X86_FEATURE_RETPOLINE has been renamed to X86_FEATURE_RETPOLINE_GENERIC. Convert the sequences using it.

---
 arch/x86/entry/entry_32.S | 2 +-
 arch/x86/entry/entry_64.S | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

index d2ef7f32905b..b3e4a2e35877 100644
@@ -258,7 +258,7 @@ ENTRY(__switch_to_asm)
 	 * any calls on the new stack, and this can be difficult to
 	 * ensure in a complex C function like __switch_to.
 	 */
-	ALTERNATIVE "", "FILL_RETURN_BUFFER %ebx", X86_FEATURE_RETPOLINE
+	ALTERNATIVE "", "FILL_RETURN_BUFFER %ebx", X86_FEATURE_RETPOLINE_GENERIC
 #endif
 
 	/* restore callee-saved registers */

index 58dbf7a12a05..566734018d76 100644
@@ -501,7 +501,7 @@ ENTRY(__switch_to_asm)
 	 * any calls on the new stack, and this can be difficult to
 	 * ensure in a complex C function like __switch_to.
 	 */
-	ALTERNATIVE "", "FILL_RETURN_BUFFER %r12", X86_FEATURE_RETPOLINE
+	ALTERNATIVE "", "FILL_RETURN_BUFFER %r12", X86_FEATURE_RETPOLINE_GENERIC
 #endif
 
 	/* restore callee-saved registers */

2.14.3

AMD documentation says they need the RSB fill too, so these should stay under X86_FEATURE_RETPOLINE I think.

AMD processors don't fall back to the indirect predictor on an underflow so this isn't needed on AMD if it is purely for underflow prevention.

Thanks,
Tom

It isn't purely for underflow protection. This is needed (see the AMD doc) for protecting against RSB entries that actually point to userspace addresses. It's only not needed if we have SMEP.

Ok, hence my caveat on the underflow in the reply. If it's to eliminate userspace addresses, then yes, it needs to be applied for AMD as well.

In that case maybe the comments in arch/x86/entry/entry_{32,64}.S need to be updated since they only talk about underflow.

Thanks,
Tom

I think we're dropping that patch anyway. qv¹.

(¹ About to hit 'send' on that...)

Andi,

Thanks for looking into that.

Can I ask a favour? Can you please have the cover letter contain something like:

[patch v3 0/N] x86/retpoline: Rework and fix retpoline alternatives

or something like that?

It makes the quick parsing of folded threads so much simpler and I really would appreciate this.

Thanks,

tglx