Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753723AbeAFRCv (ORCPT + 1 other); Sat, 6 Jan 2018 12:02:51 -0500 Received: from mx2.suse.de ([195.135.220.15]:50912 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753580AbeAFRCt (ORCPT ); Sat, 6 Jan 2018 12:02:49 -0500 Date: Sat, 6 Jan 2018 18:02:43 +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: <20180106170243.ndkn3bfj5ezbijdd@pd.tnic> References: <1515058213.12987.89.camel@amazon.co.uk> <20180104143710.8961-1-dwmw@amazon.co.uk> <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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1515227001.29312.205.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 Sat, Jan 06, 2018 at 08:23:21AM +0000, David Woodhouse wrote: > Thanks. From code inspection, I couldn't see that it was smart enough > *not* to process a relative jump in the 'altinstr' section which was > jumping to a target *within* that same altinstr section, and thus > didn't need to be touched at all. Does this work? > > alternative("", "xor %%rdi, %%rdi; jmp 2f; 2: jmp startup_64", X86_FEATURE_K8); So this is fine because it gets turned into a two-byte jump: [ 0.816005] apply_alternatives: feat: 3*32+4, old: (ffffffff810273c9, len: 10), repl: (ffffffff824759d2, len: 10), pad: 10 [ 0.820001] ffffffff810273c9: old_insn: 90 90 90 90 90 90 90 90 90 90 [ 0.821247] ffffffff824759d2: rpl_insn: 48 31 ff eb 00 e9 24 a6 b8 fe [ 0.822455] process_jumps: insn start 0x48, at 0, len: 3 [ 0.823496] process_jumps: insn start 0xeb, at 3, len: 2 [ 0.824002] process_jumps: insn start 0xe9, at 5, len: 5 [ 0.825120] recompute_jump: target RIP: ffffffff81000000, new_displ: 0xfffd8c37 [ 0.826567] recompute_jump: final displ: 0xfffd8c32, JMP 0xffffffff81000000 [ 0.828001] ffffffff810273c9: final_insn: e9 32 8c fd ff e9 24 a6 b8 fe i.e., notice the "eb 00" thing. Which, when copied into the kernel proper, will simply work as it is a small offset which, when referring to other code which gets copied *together* with it, should work. I.e., we're not changing the offsets during the copy so all good. It becomes more tricky when you force a 5-byte jump: alternative("", "xor %%rdi, %%rdi; .byte 0xe9; .long 2f - .altinstr_replacement; 2: jmp startup_64", X86_FEATURE_K8); because then you need to know whether the offset is within the .altinstr_replacement section itself or it is meant to be an absolute offset like jmp startup_64 or within another section. On that I need to sleep more to figure out what a reliable way to do it, would be. I mean, not that we need it now. If all we care is two-byte offsets, those should work now. The stress being on "should". The current version: --- From: Borislav Petkov Date: Fri, 5 Jan 2018 20:32:58 +0100 Subject: [PATCH] WIP Signed-off-by: Borislav Petkov --- diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c index dbaf14d69ebd..0cb4f886e6d7 100644 --- a/arch/x86/kernel/alternative.c +++ b/arch/x86/kernel/alternative.c @@ -21,6 +21,7 @@ #include #include #include +#include int __read_mostly alternatives_patched; @@ -280,25 +281,35 @@ static inline bool is_jmp(const u8 opcode) return opcode == 0xeb || opcode == 0xe9; } +/** + * @orig_insn: pointer to the original insn + * @repl_insn: pointer to the replacement insn + * @repl_len: length of the replacement insn + * @insnbuf: buffer we're working on massaging the insns + * @i_off: offset within the buffer + */ static void __init_or_module -recompute_jump(struct alt_instr *a, u8 *orig_insn, u8 *repl_insn, u8 *insnbuf) +recompute_jump(struct alt_instr *a, u8 *orig_insn, u8 *repl_insn, u8 repl_len, + u8 *insnbuf, u8 i_off) { u8 *next_rip, *tgt_rip; s32 n_dspl, o_dspl; - int repl_len; - if (a->replacementlen != 5) + if (repl_len != 5) return; - o_dspl = *(s32 *)(insnbuf + 1); + o_dspl = *(s32 *)(repl_insn + 1); + + DPRINTK("o_dspl: 0x%x, orig_insn: %px", o_dspl, orig_insn); /* next_rip of the replacement JMP */ - next_rip = repl_insn + a->replacementlen; + next_rip = repl_insn + repl_len; + /* target rip of the replacement JMP */ tgt_rip = next_rip + o_dspl; n_dspl = tgt_rip - orig_insn; - DPRINTK("target RIP: %p, new_displ: 0x%x", tgt_rip, n_dspl); + DPRINTK("target RIP: %px, new_displ: 0x%x", tgt_rip, n_dspl); if (tgt_rip - orig_insn >= 0) { if (n_dspl - 2 <= 127) @@ -316,8 +327,8 @@ recompute_jump(struct alt_instr *a, u8 *orig_insn, u8 *repl_insn, u8 *insnbuf) two_byte_jmp: n_dspl -= 2; - insnbuf[0] = 0xeb; - insnbuf[1] = (s8)n_dspl; + insnbuf[i_off] = 0xeb; + insnbuf[i_off + 1] = (s8)n_dspl; add_nops(insnbuf + 2, 3); repl_len = 2; @@ -326,8 +337,8 @@ recompute_jump(struct alt_instr *a, u8 *orig_insn, u8 *repl_insn, u8 *insnbuf) five_byte_jmp: n_dspl -= 5; - insnbuf[0] = 0xe9; - *(s32 *)&insnbuf[1] = n_dspl; + insnbuf[i_off] = 0xe9; + *(s32 *)&insnbuf[i_off + 1] = n_dspl; repl_len = 5; @@ -337,6 +348,32 @@ recompute_jump(struct alt_instr *a, u8 *orig_insn, u8 *repl_insn, u8 *insnbuf) n_dspl, (unsigned long)orig_insn + n_dspl + repl_len); } +static void __init_or_module process_jumps(struct alt_instr *a, u8 *insnbuf) +{ + u8 *repl = (u8 *)&a->repl_offset + a->repl_offset; + u8 *instr = (u8 *)&a->instr_offset + a->instr_offset; + struct insn insn; + int i = 0; + + if (!a->replacementlen) + return; + + while (i < a->replacementlen) { + kernel_insn_init(&insn, repl, a->replacementlen); + + insn_get_length(&insn); + + DPRINTK("insn start 0x%x, at %d, len: %d", repl[0], i, insn.length); + + if (is_jmp(repl[0])) + recompute_jump(a, instr, repl, insn.length, insnbuf, i); + + i += insn.length; + repl += insn.length; + instr += insn.length; + } +} + /* * "noinline" to cause control flow change and thus invalidate I$ and * cause refetch after modification. @@ -352,7 +389,7 @@ static void __init_or_module noinline optimize_nops(struct alt_instr *a, u8 *ins add_nops(instr + (a->instrlen - a->padlen), a->padlen); local_irq_restore(flags); - DUMP_BYTES(instr, a->instrlen, "%p: [%d:%d) optimized NOPs: ", + DUMP_BYTES(instr, a->instrlen, "%px: [%d:%d) optimized NOPs: ", instr, a->instrlen - a->padlen, a->padlen); } @@ -373,7 +410,7 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start, u8 *instr, *replacement; u8 insnbuf[MAX_PATCH_LEN]; - DPRINTK("alt table %p -> %p", start, end); + DPRINTK("alt table %px -> %px", start, end); /* * The scan order should be from start to end. A later scanned * alternative code can overwrite previously scanned alternative code. @@ -397,14 +434,14 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start, continue; } - DPRINTK("feat: %d*32+%d, old: (%p, len: %d), repl: (%p, len: %d), pad: %d", + DPRINTK("feat: %d*32+%d, old: (%px, len: %d), repl: (%px, len: %d), pad: %d", a->cpuid >> 5, a->cpuid & 0x1f, instr, a->instrlen, replacement, a->replacementlen, a->padlen); - DUMP_BYTES(instr, a->instrlen, "%p: old_insn: ", instr); - DUMP_BYTES(replacement, a->replacementlen, "%p: rpl_insn: ", replacement); + DUMP_BYTES(instr, a->instrlen, "%px: old_insn: ", instr); + DUMP_BYTES(replacement, a->replacementlen, "%px: rpl_insn: ", replacement); memcpy(insnbuf, replacement, a->replacementlen); insnbuf_sz = a->replacementlen; @@ -422,15 +459,14 @@ void __init_or_module noinline apply_alternatives(struct alt_instr *start, (unsigned long)instr + *(s32 *)(insnbuf + 1) + 5); } - if (a->replacementlen && is_jmp(replacement[0])) - recompute_jump(a, instr, replacement, insnbuf); + process_jumps(a, insnbuf); if (a->instrlen > a->replacementlen) { add_nops(insnbuf + a->replacementlen, a->instrlen - a->replacementlen); insnbuf_sz += a->instrlen - a->replacementlen; } - DUMP_BYTES(insnbuf, insnbuf_sz, "%p: final_insn: ", instr); + DUMP_BYTES(insnbuf, insnbuf_sz, "%px: final_insn: ", instr); text_poke_early(instr, insnbuf, insnbuf_sz); } -- 2.13.0 SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) --