Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754042AbeAGOEE (ORCPT + 1 other); Sun, 7 Jan 2018 09:04:04 -0500 Received: from mx2.suse.de ([195.135.220.15]:49068 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753772AbeAGOED (ORCPT ); Sun, 7 Jan 2018 09:04:03 -0500 Date: Sun, 7 Jan 2018 15:03:51 +0100 From: Borislav Petkov To: David Woodhouse Cc: Josh Poimboeuf , "tglx@linutronix.de" , "linux-kernel@vger.kernel.org" , "tim.c.chen@linux.intel.com" , "peterz@infradead.org" , "torvalds@linux-foundation.org" , "ak@linux.intel.com" , "riel@redhat.com" , "keescook@google.com" , "gnomes@lxorguk.ukuu.org.uk" , "pjt@google.com" , "dave.hansen@intel.com" , "luto@amacapital.net" , "jikos@kernel.org" , "gregkh@linux-foundation.org" Subject: Re: [PATCH v3 01/13] x86/retpoline: Add initial retpoline support Message-ID: <20180107140351.iukmbuf2xkqtvyoz@pd.tnic> References: <1515160619.29312.126.camel@amazon.co.uk> <1515170506.29312.149.camel@amazon.co.uk> <20180105164505.xpw5pefxsyu3z56e@pd.tnic> <20180105170806.mtylu2zagfxyj3ry@treble> <20180106003059.jhwx4ouc7xbt7yw6@pd.tnic> <1515227001.29312.205.camel@infradead.org> <20180106170243.ndkn3bfj5ezbijdd@pd.tnic> <1515318042.29312.311.camel@infradead.org> <20180107114645.yydecgoi4x53fyrd@pd.tnic> <1515327689.29312.319.camel@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1515327689.29312.319.camel@infradead.org> User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: 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 #include #include +#include #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) --