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]>
[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
Fixes: ce004e1cb ("x86/retpoline: Add initial retpoline")
Fixes: f3433c101 ("x86/retpoline/entry: Convert entry")
Suggested-by: Linus Torvalds <[email protected]>
Signed-off-by: Andi Kleen <[email protected]>
---
arch/x86/include/asm/nospec-branch.h | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
index 8ddf8513550e..dc13325a9890 100644
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -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
From: Andi Kleen <[email protected]>
X86_FEATURE_RETPOLINE has been renamed to X86_FEATURE_RETPOLINE_GENERIC.
Convert the sequences using it.
Signed-off-by: Andi Kleen <[email protected]>
---
arch/x86/entry/entry_32.S | 2 +-
arch/x86/entry/entry_64.S | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
index d2ef7f32905b..b3e4a2e35877 100644
--- a/arch/x86/entry/entry_32.S
+++ b/arch/x86/entry/entry_32.S
@@ -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 */
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index 58dbf7a12a05..566734018d76 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -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
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.
Signed-off-by: Andi Kleen <[email protected]>
---
arch/x86/include/asm/cpufeatures.h | 3 ++-
arch/x86/kernel/cpu/bugs.c | 4 +++-
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index f275447862f4..9db5ed0e01a9 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -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 */
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index b957f771a5b7..68b08967a6e1 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -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
Andi,
On Tue, 9 Jan 2018, Andi Kleen wrote:
> Updated fix for the retpoline boot failure.
>
> v3: Now uses X86_FEATURE_RETPOLINE_GENERIC / X86_FEATURE_GENERIC
> as requested by Thomas
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
On Tue, 2018-01-09 at 18:28 -0800, Andi Kleen wrote:
> From: Andi Kleen <[email protected]>
>
> X86_FEATURE_RETPOLINE has been renamed to X86_FEATURE_RETPOLINE_GENERIC.
> Convert the sequences using it.
AMD documentation says they need the RSB fill too, so these should stay
under X86_FEATURE_RETPOLINE I think.
On Tue, 2018-01-09 at 18:28 -0800, Andi Kleen wrote:
> 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.
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.
On 1/10/2018 7:15 AM, Woodhouse, David wrote:
> On Tue, 2018-01-09 at 18:28 -0800, Andi Kleen wrote:
>> From: Andi Kleen <[email protected]>
>>
>> X86_FEATURE_RETPOLINE has been renamed to X86_FEATURE_RETPOLINE_GENERIC.
>> Convert the sequences using it.
>
> 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
>
>
>
> Amazon Web Services UK Limited. Registered in England and Wales with
> registration number 08650665 and which has its registered office at 60
> Holborn Viaduct, London EC1A 2FD, United Kingdom.
On Wed, 2018-01-10 at 08:23 -0600, Tom Lendacky wrote:
> On 1/10/2018 7:15 AM, Woodhouse, David wrote:
> > On Tue, 2018-01-09 at 18:28 -0800, Andi Kleen wrote:
> >> From: Andi Kleen <[email protected]>
> >>
> >> X86_FEATURE_RETPOLINE has been renamed to X86_FEATURE_RETPOLINE_GENERIC.
> >> Convert the sequences using it.
> >
> > 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.
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.
On 1/10/2018 8:46 AM, Woodhouse, David wrote:
> On Wed, 2018-01-10 at 08:23 -0600, Tom Lendacky wrote:
>> On 1/10/2018 7:15 AM, Woodhouse, David wrote:
>>> On Tue, 2018-01-09 at 18:28 -0800, Andi Kleen wrote:
>>>> From: Andi Kleen <[email protected]>
>>>>
>>>> X86_FEATURE_RETPOLINE has been renamed to X86_FEATURE_RETPOLINE_GENERIC.
>>>> Convert the sequences using it.
>>>
>>> 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.
>
> 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
>
>
>
> Amazon Web Services UK Limited. Registered in England and Wales with
> registration number 08650665 and which has its registered office at 60
> Holborn Viaduct, London EC1A 2FD, United Kingdom.
On Wed, 2018-01-10 at 09:18 -0600, Tom Lendacky wrote:
>
> 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.
I think we're dropping that patch anyway. qv¹.
(¹ About to hit 'send' on that...)