Re: [PATCH v3 01/13] x86/retpoline: Add initial retpoline support
From: Borislav Petkov
Date: Sat Jan 06 2018 - 12:02:54 EST
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 <bp@xxxxxxx>
Date: Fri, 5 Jan 2018 20:32:58 +0100
Subject: [PATCH] WIP
Signed-off-by: Borislav Petkov <bp@xxxxxxx>
---
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 <asm/tlbflush.h>
#include <asm/io.h>
#include <asm/fixmap.h>
+#include <asm/insn.h>
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)
--