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

[Borislav Petkov <bp@suse.de>]

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@suse.de>
Date: Fri, 5 Jan 2018 20:32:58 +0100
Subject: [PATCH] WIP

Signed-off-by: Borislav Petkov <bp@suse.de>
---
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)
--