Re: [PATCH v3 01/13] x86/retpoline: Add initial retpoline support

[Borislav Petkov <bp@suse.de>]

On Sun, Jan 07, 2018 at 12:21:29PM +0000, David Woodhouse wrote:
> http://git.infradead.org/users/dwmw2/linux-retpoline.git
> 
> In particular, this call site in entry_64.S:
> http://git.infradead.org/users/dwmw2/linux-retpoline.git/blob/0f5c54a36e:/arch/x86/entry/entry_64.S#l270
> 
> It's still just unconditionally calling the out-of-line thunk and not
> using ALTERNATIVE in the CONFIG_RETPOLINE case. I can't just use the
> NOSPEC_CALL macro from
> http://git.infradead.org/users/dwmw2/linux-retpoline.git/blob/0f5c54a36e:/arch/x86/include/asm/nospec-branch.h#l46
> because of that requirement that the return address (on the stack) for
> the CALL instruction must be precisely at the end, in all cases.

Thanks, this makes it all clear.

> Each of the three alternatives *does* end with the CALL, it's just that
> for the two which are shorter than the full retpoline one, they'll get
> padded with NOPs at the end, so the return address on the stack *won't*
> be what's expected.

Right.

> Explicitly padding the alternatives with leading NOPs so that they are
> all precisely the same length would work, and if the alternatives
> mechanism were to pad the shorter ones with leading NOPs instead of
> trailing NOPs that would *also* work (but be fairly difficult
> especially to do that for oldinstr).

Right, so doing this reliably would need adding flags to struct
alt_instr - and this need has arisen before and we've managed not to do
it :). And I'm not really persuaded we need it now either because this
would be a one-off case where we need the padding in the front.

> I'm not sure I *see* a simple answer, and it isn't really that bad to
> just do what GCC is doing and unconditionally call the out-of-line
> thunk. So feel free to just throw your hands up in horror and say "no,
> we can't cope with that" :)

Right, or we can do something like this - diff ontop of yours below. We used to
do this before the automatic padding - explicit NOP padding by hand :-)

First split the ALTERNATIVE_2 into two alternative calls, as Brian suggested:

        ALTERNATIVE "", "lfence", X86_FEATURE_RETPOLINE_AMD

and then the second:

        ALTERNATIVE __stringify(ASM_NOP8; ASM_NOP8; ASM_NOP3; call *\reg), \
                     __stringify(RETPOLINE_CALL \reg), X86_FEATURE_RETPOLINE

with frontal NOP padding.

When compiled, it looks like this:

ffffffff818002e2:       90                      nop
ffffffff818002e3:       90                      nop
ffffffff818002e4:       90                      nop

these first three bytes become LFENCE on AMD. On Intel, it gets optimized:

[    0.042954] ffffffff818002e2: [0:3) optimized NOPs: 0f 1f 00

Then:

ffffffff818002e5:       66 66 66 90             data16 data16 xchg %ax,%ax
ffffffff818002e9:       66 66 66 90             data16 data16 xchg %ax,%ax
ffffffff818002ed:       66 66 66 90             data16 data16 xchg %ax,%ax
ffffffff818002f1:       66 66 66 90             data16 data16 xchg %ax,%ax
ffffffff818002f5:       66 66 90                data16 xchg %ax,%ax
ffffffff818002f8:       ff d3                   callq  *%rbx

This thing remains like this on AMD and on Intel gets turned into:

[    0.044004] apply_alternatives: feat: 7*32+12, old: (ffffffff818002e5, len: 21), repl: (ffffffff82219edc, len: 21), pad: 0
[    0.045967] ffffffff818002e5: old_insn: 66 66 66 90 66 66 66 90 66 66 66 90 66 66 66 90 66 66 90 ff d3
[    0.048006] ffffffff82219edc: rpl_insn: eb 0e e8 04 00 00 00 f3 90 eb fc 48 89 1c 24 c3 e8 ed ff ff ff
[    0.050581] ffffffff818002e5: final_insn: eb 0e e8 04 00 00 00 f3 90 eb fc 48 89 1c 24 c3 e8 ed ff ff ff
[    0.052003] ffffffff8180123b: [0:3) optimized NOPs: 0f 1f 00


ffffffff818002e5:       eb 0e                   jmp ffffffff818002f5
ffffffff818002e7:       e8 04 00 00 00          callq ffffffff818002f0
ffffffff818002ec:       f3 90                   pause
ffffffff818002ee:       eb fc                   jmp ffffffff818002ec
ffffffff818002f0:       48 89 1c 24             mov %rbx,(%rsp)
ffffffff818002f4:       c3                      ret
ffffffff818002f5:       e8 ed ff ff ff          callq ffffffff818002e7

so that in both cases, the CALL remains last.

My fear is if some funky compiler changes the sizes of the insns in
RETPOLINE_CALL/JMP and then the padding becomes wrong. But looking at the
labels, they're all close so you have a 2-byte jmp already and the

call    1112f

should be ok. The MOV is reg,(reg) which should not change size of 4...

But I'm remaining cautious here.

And we'd need to do the respective thing for NOSPEC_JMP.

Btw, I've moved the setting of X86_FEATURE_RETPOLINE and
X86_FEATURE_RETPOLINE_AMD to the respective intel.c and amd.c files. I
think this is what we want.

And if we're doing two different RETPOLINE flavors, then we should call
X86_FEATURE_RETPOLINE
X86_FEATURE_RETPOLINE_INTEL.

Does that whole thing make some sense...?

---
diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h
index c4d08fa29d6c..70825ce909ba 100644
--- a/arch/x86/include/asm/nospec-branch.h
+++ b/arch/x86/include/asm/nospec-branch.h
@@ -6,6 +6,7 @@
 #include <asm/alternative.h>
 #include <asm/alternative-asm.h>
 #include <asm/cpufeatures.h>
+#include <asm/nops.h>
 
 #ifdef __ASSEMBLY__
 
@@ -43,11 +44,15 @@
 #endif
 .endm
 
+/*
+ * RETPOLINE_CALL is 21 bytes so pad the front with 19 bytes + 2 bytes for the
+ * CALL insn so that all CALL instructions remain last.
+ */
 .macro NOSPEC_CALL 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(ASM_NOP8; ASM_NOP8; ASM_NOP3; call *\reg), \
+		     __stringify(RETPOLINE_CALL \reg), X86_FEATURE_RETPOLINE
 #else
 	call	*\reg
 #endif
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index b221fe507640..938111e77dd0 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -554,6 +554,8 @@ static void bsp_init_amd(struct cpuinfo_x86 *c)
 		rdmsrl(MSR_FAM10H_NODE_ID, value);
 		nodes_per_socket = ((value >> 3) & 7) + 1;
 	}
+
+	setup_force_cpu_cap(X86_FEATURE_RETPOLINE_AMD);
 }
 
 static void early_init_amd(struct cpuinfo_x86 *c)
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index cfa5042232a2..372ba3fb400f 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -904,9 +904,6 @@ static void __init early_identify_cpu(struct cpuinfo_x86 *c)
 
 	setup_force_cpu_bug(X86_BUG_SPECTRE_V1);
 	setup_force_cpu_bug(X86_BUG_SPECTRE_V2);
-#ifdef CONFIG_RETPOLINE
-	setup_force_cpu_cap(X86_FEATURE_RETPOLINE);
-#endif
 
 	fpu__init_system(c);
 
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index 35e123e5f413..a9e00edaba46 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -524,6 +524,13 @@ static void init_intel_misc_features(struct cpuinfo_x86 *c)
 	wrmsrl(MSR_MISC_FEATURES_ENABLES, msr);
 }
 
+static void bsp_init_intel(struct cpuinfo_x86 *c)
+{
+#ifdef CONFIG_RETPOLINE
+	setup_force_cpu_cap(X86_FEATURE_RETPOLINE);
+#endif
+}
+
 static void init_intel(struct cpuinfo_x86 *c)
 {
 	unsigned int l2 = 0;
@@ -893,6 +900,7 @@ static const struct cpu_dev intel_cpu_dev = {
 	.c_detect_tlb	= intel_detect_tlb,
 	.c_early_init   = early_init_intel,
 	.c_init		= init_intel,
+	.c_bsp_init	= bsp_init_intel,
 	.c_bsp_resume	= intel_bsp_resume,
 	.c_x86_vendor	= X86_VENDOR_INTEL,
 };

-- 
Regards/Gruss,
    Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--