[RFC PATCH tip/master V2 0/8] kprobes/x86: Make kprobes instruction buffers read-only

[RFC PATCH tip/master V2 0/8] kprobes/x86: Make kprobes instruction buffers read-only

[Masami Hiramatsu]

Hi,

This is the 2nd version of the series, which has been updated
only 8/8, built and tested correctly. (Sorry, in previous version
I missed to build and tested different kernel.)

This series tries to make kprobes instruction buffers read-only
pages. Since those buffers are used for trampoline code, those
are a part of "text area" and it should be marked as ro for
avoiding unexpected modification. And this actually fix a warning
rodata sanity check reported by lkp-robot.

https://lkml.org/lkml/2017/2/27/161

This change requires changing the kprobe-booster at first
because it can modify the instruction buffer to add a jump while
resuming from single-stepping. Of course after we make the buffer
readonly, we may not be able to modify it while probing.

So, at first this series checks the current bootable instructions
and fixes a missed instruction (call far), modifies can_boost to
use x86 instruction decoder, and inserts "booster" jump while
preparing instruction buffer instead of resuming from single-stepping.
At last, it makes the buffers for kprobes and optprobe readonly.

This series also has some cleanup patches related to above 
changes.

Changes from V1:
 - [8/8]: Fix build errors.

---

Masami Hiramatsu (8):
      kprobes/x86: Fix not to boost call far instruction
      kprobes/x86: Fix the description of __copy_instruction()
      kprobes/x86: Use instruction decoder for booster
      kprobes/x86: Do not modify singlestep buffer while resuming
      kprobes/x86: Make boostable flag boolean
      kprobes/x86: Set kprobes pages readonly
      kprobes/x86: Use probe_kernel_read instead of memcpy
      kprobes/x86: Consolidate insn decoder users for copying code


 arch/x86/include/asm/kprobes.h   |    7 +-
 arch/x86/kernel/kprobes/common.h |    4 +
 arch/x86/kernel/kprobes/core.c   |  148 +++++++++++++++++++-------------------
 arch/x86/kernel/kprobes/ftrace.c |    2 -
 arch/x86/kernel/kprobes/opt.c    |   13 +++
 5 files changed, 89 insertions(+), 85 deletions(-)

--
Masami Hiramatsu (Linaro) <mhiramat@kernel.org>

[RFC PATCH tip/master V2 1/8] kprobes/x86: Fix not to boost call far instruction

[Masami Hiramatsu]

Fix kprobe-booster not to boost call far instruction,
because call may store the address in singlestep
execution buffer to the stack, which should be modified
after single stepping.

Currently, this instruction will be filtered as not
boostable in resume_execution(), so this is not a
critical issue.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/x86/kernel/kprobes/core.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 993fa4f..9eae5a6 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -200,6 +200,8 @@ int can_boost(kprobe_opcode_t *opcodes, void *addr)
 		return (opcode != 0x62 && opcode != 0x67);
 	case 0x70:
 		return 0; /* can't boost conditional jump */
+	case 0x90:
+		return opcode != 0x9a;	/* can't boost call far */
 	case 0xc0:
 		/* can't boost software-interruptions */
 		return (0xc1 < opcode && opcode < 0xcc) || opcode == 0xcf;

[RFC PATCH tip/master V2 2/8] kprobes/x86: Fix the description of __copy_instruction()

[Masami Hiramatsu]

Fix the description comment of __copy_instruction() function
since it has already been changed to return the length of
copied instruction.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/x86/kernel/kprobes/core.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 9eae5a6..81d4dc7 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -350,11 +350,10 @@ static int is_IF_modifier(kprobe_opcode_t *insn)
 }
 
 /*
- * Copy an instruction and adjust the displacement if the instruction
- * uses the %rip-relative addressing mode.
- * If it does, Return the address of the 32-bit displacement word.
- * If not, return null.
- * Only applicable to 64-bit x86.
+ * Copy an instruction with recovering modified instruction by kprobes
+ * and adjust the displacement if the instruction uses the %rip-relative
+ * addressing mode.
+ * This returns the length of copied instruction, or 0 if it has an error.
  */
 int __copy_instruction(u8 *dest, u8 *src)
 {
@@ -376,6 +375,7 @@ int __copy_instruction(u8 *dest, u8 *src)
 	memcpy(dest, insn.kaddr, length);
 
 #ifdef CONFIG_X86_64
+	/* Only x86_64 has RIP relative instructions */
 	if (insn_rip_relative(&insn)) {
 		s64 newdisp;
 		u8 *disp;

[RFC PATCH tip/master V2 3/8] kprobes/x86: Use instruction decoder for booster

[Masami Hiramatsu]

Use x86 instruction decoder for checking whether the probed
instruction is able to boost or not, instead of hand-written
code.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/x86/kernel/kprobes/core.c |   39 ++++++++++++++++-----------------------
 1 file changed, 16 insertions(+), 23 deletions(-)

diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 81d4dc7..6327f95 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -169,35 +169,33 @@ NOKPROBE_SYMBOL(skip_prefixes);
  */
 int can_boost(kprobe_opcode_t *opcodes, void *addr)
 {
+	struct insn insn;
 	kprobe_opcode_t opcode;
-	kprobe_opcode_t *orig_opcodes = opcodes;
 
 	if (search_exception_tables((unsigned long)addr))
 		return 0;	/* Page fault may occur on this address. */
 
-retry:
-	if (opcodes - orig_opcodes > MAX_INSN_SIZE - 1)
-		return 0;
-	opcode = *(opcodes++);
+	kernel_insn_init(&insn, (void *)opcodes, MAX_INSN_SIZE);
+	insn_get_opcode(&insn);
 
 	/* 2nd-byte opcode */
-	if (opcode == 0x0f) {
-		if (opcodes - orig_opcodes > MAX_INSN_SIZE - 1)
-			return 0;
-		return test_bit(*opcodes,
+	if (insn.opcode.nbytes == 2)
+		return test_bit(insn.opcode.bytes[1],
 				(unsigned long *)twobyte_is_boostable);
-	}
+
+	if (insn.opcode.nbytes != 1)
+		return 0;
+
+	/* Can't boost Address-size override prefix */
+	if (unlikely(inat_is_address_size_prefix(insn.attr)))
+		return 0;
+
+	opcode = insn.opcode.bytes[0];
 
 	switch (opcode & 0xf0) {
-#ifdef CONFIG_X86_64
-	case 0x40:
-		goto retry; /* REX prefix is boostable */
-#endif
 	case 0x60:
-		if (0x63 < opcode && opcode < 0x67)
-			goto retry; /* prefixes */
-		/* can't boost Address-size override and bound */
-		return (opcode != 0x62 && opcode != 0x67);
+		/* can't boost "bound" */
+		return (opcode != 0x62);
 	case 0x70:
 		return 0; /* can't boost conditional jump */
 	case 0x90:
@@ -212,14 +210,9 @@ int can_boost(kprobe_opcode_t *opcodes, void *addr)
 		/* can boost in/out and absolute jmps */
 		return ((opcode & 0x04) || opcode == 0xea);
 	case 0xf0:
-		if ((opcode & 0x0c) == 0 && opcode != 0xf1)
-			goto retry; /* lock/rep(ne) prefix */
 		/* clear and set flags are boostable */
 		return (opcode == 0xf5 || (0xf7 < opcode && opcode < 0xfe));
 	default:
-		/* segment override prefixes are boostable */
-		if (opcode == 0x26 || opcode == 0x36 || opcode == 0x3e)
-			goto retry; /* prefixes */
 		/* CS override prefix and call are not boostable */
 		return (opcode != 0x2e && opcode != 0x9a);
 	}

[RFC PATCH tip/master V2 4/8] kprobes/x86: Do not modify singlestep buffer while resuming

[Masami Hiramatsu]

Do not modify singlestep execution buffer (kprobe.ainsn.insn)
while resuming from single-stepping, instead, modifies
the buffer to add a jump back instruction at preparing
buffer.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/x86/kernel/kprobes/core.c |   42 +++++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 22 deletions(-)

diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 6327f95..ea3b8e5 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -399,23 +399,36 @@ int __copy_instruction(u8 *dest, u8 *src)
 	return length;
 }
 
+/* Prepare reljump right after instruction to boost */
+static void prepare_boost(struct kprobe *p, int length)
+{
+	if (can_boost(p->ainsn.insn, p->addr) &&
+	    MAX_INSN_SIZE - length >= RELATIVEJUMP_SIZE) {
+		/*
+		 * These instructions can be executed directly if it
+		 * jumps back to correct address.
+		 */
+		synthesize_reljump((void *)p->ainsn.insn + length,
+				   (void *)p->addr + length);
+		p->ainsn.boostable = 1;
+	} else
+		p->ainsn.boostable = -1;
+}
+
 static int arch_copy_kprobe(struct kprobe *p)
 {
-	int ret;
+	int len;
 
 	/* Copy an instruction with recovering if other optprobe modifies it.*/
-	ret = __copy_instruction(p->ainsn.insn, p->addr);
-	if (!ret)
+	len = __copy_instruction(p->ainsn.insn, p->addr);
+	if (!len)
 		return -EINVAL;
 
 	/*
 	 * __copy_instruction can modify the displacement of the instruction,
 	 * but it doesn't affect boostable check.
 	 */
-	if (can_boost(p->ainsn.insn, p->addr))
-		p->ainsn.boostable = 0;
-	else
-		p->ainsn.boostable = -1;
+	prepare_boost(p, len);
 
 	/* Check whether the instruction modifies Interrupt Flag or not */
 	p->ainsn.if_modifier = is_IF_modifier(p->ainsn.insn);
@@ -878,21 +891,6 @@ static void resume_execution(struct kprobe *p, struct pt_regs *regs,
 		break;
 	}
 
-	if (p->ainsn.boostable == 0) {
-		if ((regs->ip > copy_ip) &&
-		    (regs->ip - copy_ip) + 5 < MAX_INSN_SIZE) {
-			/*
-			 * These instructions can be executed directly if it
-			 * jumps back to correct address.
-			 */
-			synthesize_reljump((void *)regs->ip,
-				(void *)orig_ip + (regs->ip - copy_ip));
-			p->ainsn.boostable = 1;
-		} else {
-			p->ainsn.boostable = -1;
-		}
-	}
-
 	regs->ip += orig_ip - copy_ip;
 
 no_change:

[RFC PATCH tip/master V2 5/8] kprobes/x86: Make boostable flag boolean

[Masami Hiramatsu]

Make arch_specific_insn.boostable to boolean, since it has
only 2 states, boostable or not. So it is better to use
boolean from the viewpoint of code readability.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/x86/include/asm/kprobes.h   |    7 +++----
 arch/x86/kernel/kprobes/core.c   |   12 ++++++------
 arch/x86/kernel/kprobes/ftrace.c |    2 +-
 3 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/arch/x86/include/asm/kprobes.h b/arch/x86/include/asm/kprobes.h
index 2005816..34b984c 100644
--- a/arch/x86/include/asm/kprobes.h
+++ b/arch/x86/include/asm/kprobes.h
@@ -72,14 +72,13 @@ struct arch_specific_insn {
 	/* copy of the original instruction */
 	kprobe_opcode_t *insn;
 	/*
-	 * boostable = -1: This instruction type is not boostable.
-	 * boostable = 0: This instruction type is boostable.
-	 * boostable = 1: This instruction has been boosted: we have
+	 * boostable = false: This instruction type is not boostable.
+	 * boostable = true: This instruction has been boosted: we have
 	 * added a relative jump after the instruction copy in insn,
 	 * so no single-step and fixup are needed (unless there's
 	 * a post_handler or break_handler).
 	 */
-	int boostable;
+	bool boostable;
 	bool if_modifier;
 };
 
diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index ea3b8e5..b358c84 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -410,9 +410,9 @@ static void prepare_boost(struct kprobe *p, int length)
 		 */
 		synthesize_reljump((void *)p->ainsn.insn + length,
 				   (void *)p->addr + length);
-		p->ainsn.boostable = 1;
+		p->ainsn.boostable = true;
 	} else
-		p->ainsn.boostable = -1;
+		p->ainsn.boostable = false;
 }
 
 static int arch_copy_kprobe(struct kprobe *p)
@@ -467,7 +467,7 @@ void arch_disarm_kprobe(struct kprobe *p)
 void arch_remove_kprobe(struct kprobe *p)
 {
 	if (p->ainsn.insn) {
-		free_insn_slot(p->ainsn.insn, (p->ainsn.boostable == 1));
+		free_insn_slot(p->ainsn.insn, p->ainsn.boostable);
 		p->ainsn.insn = NULL;
 	}
 }
@@ -539,7 +539,7 @@ static void setup_singlestep(struct kprobe *p, struct pt_regs *regs,
 		return;
 
 #if !defined(CONFIG_PREEMPT)
-	if (p->ainsn.boostable == 1 && !p->post_handler) {
+	if (p->ainsn.boostable && !p->post_handler) {
 		/* Boost up -- we can execute copied instructions directly */
 		if (!reenter)
 			reset_current_kprobe();
@@ -859,7 +859,7 @@ static void resume_execution(struct kprobe *p, struct pt_regs *regs,
 	case 0xcf:
 	case 0xea:	/* jmp absolute -- ip is correct */
 		/* ip is already adjusted, no more changes required */
-		p->ainsn.boostable = 1;
+		p->ainsn.boostable = true;
 		goto no_change;
 	case 0xe8:	/* call relative - Fix return addr */
 		*tos = orig_ip + (*tos - copy_ip);
@@ -884,7 +884,7 @@ static void resume_execution(struct kprobe *p, struct pt_regs *regs,
 			 * jmp near and far, absolute indirect
 			 * ip is correct. And this is boostable
 			 */
-			p->ainsn.boostable = 1;
+			p->ainsn.boostable = true;
 			goto no_change;
 		}
 	default:
diff --git a/arch/x86/kernel/kprobes/ftrace.c b/arch/x86/kernel/kprobes/ftrace.c
index 5f8f0b3..041f7b6 100644
--- a/arch/x86/kernel/kprobes/ftrace.c
+++ b/arch/x86/kernel/kprobes/ftrace.c
@@ -94,6 +94,6 @@ NOKPROBE_SYMBOL(kprobe_ftrace_handler);
 int arch_prepare_kprobe_ftrace(struct kprobe *p)
 {
 	p->ainsn.insn = NULL;
-	p->ainsn.boostable = -1;
+	p->ainsn.boostable = false;
 	return 0;
 }

[RFC PATCH tip/master V2 6/8] kprobes/x86: Set kprobes pages readonly

[Masami Hiramatsu]

Set the pages which is used for kprobes' singlestep buffer
and optprobe's trampoline instruction buffer to readonly.
This can prevent unexpected (or unintended) instruction
modification.

This also passes rodata_test as below.

Without this patch, rodata_test shows a warning:

[   10.041310] ------------[ cut here ]------------
[   10.041717] WARNING: CPU: 0 PID: 1 at arch/x86/mm/dump_pagetables.c:235 note_page+0x7a9/0xa20
[   10.042469] x86/mm: Found insecure W+X mapping at address ffffffffa0000000/0xffffffffa0000000
[   10.043073] Modules linked in:
[   10.043317] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G    B           4.11.0-rc1-00282-gc4bf2f7 #6
[   10.043935] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.10.1-0-g8891697-prebuilt.qemu-project.org 04/01/2014
[   10.044721] Call Trace:
[   10.044898]  dump_stack+0x63/0x8d
[   10.045131]  __warn+0x107/0x130
[   10.045352]  warn_slowpath_fmt+0x92/0xb0
[   10.045623]  ? __warn+0x130/0x130
[   10.045856]  ? 0xffffffffa0000000
[   10.046104]  ? 0xffffffffa0000000
[   10.046337]  note_page+0x7a9/0xa20
[   10.046577]  ptdump_walk_pgd_level_core+0x511/0x540
[   10.046914]  ? note_page+0xa20/0xa20
[   10.047163]  ? do_raw_spin_unlock+0x92/0x120
[   10.047458]  ? 0xffffffffa0000000
[   10.047691]  ? 0xffffffff81000000
[   10.047924]  ptdump_walk_pgd_level_checkwx+0x12/0x20
[   10.048266]  mark_rodata_ro+0x10e/0x120
[   10.048533]  ? rest_init+0xe0/0xe0
[   10.048771]  kernel_init+0x2a/0x120
[   10.049016]  ? rest_init+0xe0/0xe0
[   10.049254]  ret_from_fork+0x2c/0x40
[   10.049503] ---[ end trace 1e4cda1ee3314caa ]---
[   10.049861] x86/mm: Checked W+X mappings: FAILED, 2 W+X pages found.
[   10.050349] rodata_test: all tests were successful

With this fix, no W+X pages found:

[   10.130919] x86/mm: Checked W+X mappings: passed, no W+X pages found.
[   10.131624] rodata_test: all tests were successful

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Reported-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
---
 arch/x86/kernel/kprobes/core.c |    4 ++++
 arch/x86/kernel/kprobes/opt.c  |    3 +++
 2 files changed, 7 insertions(+)

diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index b358c84..3238752 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -419,6 +419,8 @@ static int arch_copy_kprobe(struct kprobe *p)
 {
 	int len;
 
+	set_memory_rw((unsigned long)p->ainsn.insn & PAGE_MASK, 1);
+
 	/* Copy an instruction with recovering if other optprobe modifies it.*/
 	len = __copy_instruction(p->ainsn.insn, p->addr);
 	if (!len)
@@ -430,6 +432,8 @@ static int arch_copy_kprobe(struct kprobe *p)
 	 */
 	prepare_boost(p, len);
 
+	set_memory_ro((unsigned long)p->ainsn.insn & PAGE_MASK, 1);
+
 	/* Check whether the instruction modifies Interrupt Flag or not */
 	p->ainsn.if_modifier = is_IF_modifier(p->ainsn.insn);
 
diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c
index 3e7c6e5..b121037 100644
--- a/arch/x86/kernel/kprobes/opt.c
+++ b/arch/x86/kernel/kprobes/opt.c
@@ -350,6 +350,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op,
 	}
 
 	buf = (u8 *)op->optinsn.insn;
+	set_memory_rw((unsigned long)buf & PAGE_MASK, 1);
 
 	/* Copy instructions into the out-of-line buffer */
 	ret = copy_optimized_instructions(buf + TMPL_END_IDX, op->kp.addr);
@@ -372,6 +373,8 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op,
 	synthesize_reljump(buf + TMPL_END_IDX + op->optinsn.size,
 			   (u8 *)op->kp.addr + op->optinsn.size);
 
+	set_memory_ro((unsigned long)buf & PAGE_MASK, 1);
+
 	flush_icache_range((unsigned long) buf,
 			   (unsigned long) buf + TMPL_END_IDX +
 			   op->optinsn.size + RELATIVEJUMP_SIZE);

[RFC PATCH tip/master V2 7/8] kprobes/x86: Use probe_kernel_read instead of memcpy

[Masami Hiramatsu]

Use probe_kernel_read() for avoiding unexpected faults while
copying kernel text in __recover_probed_insn(),
__recover_optprobed_insn() and __copy_instruction().

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
  Note that this is just an update patch which I had been
  sent to LKML last month ( https://lkml.org/lkml/2017/2/27/294 )
---
 arch/x86/kernel/kprobes/core.c |   12 +++++++++---
 arch/x86/kernel/kprobes/opt.c  |    5 ++++-
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 3238752..a9ae61a 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -259,7 +259,10 @@ __recover_probed_insn(kprobe_opcode_t *buf, unsigned long addr)
 	 * Fortunately, we know that the original code is the ideal 5-byte
 	 * long NOP.
 	 */
-	memcpy(buf, (void *)addr, MAX_INSN_SIZE * sizeof(kprobe_opcode_t));
+	if (probe_kernel_read(buf, (void *)addr,
+		MAX_INSN_SIZE * sizeof(kprobe_opcode_t)))
+		return 0UL;
+
 	if (faddr)
 		memcpy(buf, ideal_nops[NOP_ATOMIC5], 5);
 	else
@@ -271,7 +274,7 @@ __recover_probed_insn(kprobe_opcode_t *buf, unsigned long addr)
  * Recover the probed instruction at addr for further analysis.
  * Caller must lock kprobes by kprobe_mutex, or disable preemption
  * for preventing to release referencing kprobes.
- * Returns zero if the instruction can not get recovered.
+ * Returns zero if the instruction can not get recovered (or access failed).
  */
 unsigned long recover_probed_instruction(kprobe_opcode_t *buf, unsigned long addr)
 {
@@ -365,7 +368,10 @@ int __copy_instruction(u8 *dest, u8 *src)
 	/* Another subsystem puts a breakpoint, failed to recover */
 	if (insn.opcode.bytes[0] == BREAKPOINT_INSTRUCTION)
 		return 0;
-	memcpy(dest, insn.kaddr, length);
+
+	/* This can access kernel text if given address is not recovered */
+	if (kernel_probe_read(dest, insn.kaddr, length))
+		return 0;
 
 #ifdef CONFIG_X86_64
 	/* Only x86_64 has RIP relative instructions */
diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c
index b121037..5b52334 100644
--- a/arch/x86/kernel/kprobes/opt.c
+++ b/arch/x86/kernel/kprobes/opt.c
@@ -65,7 +65,10 @@ unsigned long __recover_optprobed_insn(kprobe_opcode_t *buf, unsigned long addr)
 	 * overwritten by jump destination address. In this case, original
 	 * bytes must be recovered from op->optinsn.copied_insn buffer.
 	 */
-	memcpy(buf, (void *)addr, MAX_INSN_SIZE * sizeof(kprobe_opcode_t));
+	if (probe_kernel_read(buf, (void *)addr,
+		MAX_INSN_SIZE * sizeof(kprobe_opcode_t)))
+		return 0UL;
+
 	if (addr == (unsigned long)kp->addr) {
 		buf[0] = kp->opcode;
 		memcpy(buf + 1, op->optinsn.copied_insn, RELATIVE_ADDR_SIZE);

[RFC PATCH tip/master V2 8/8] kprobes/x86: Consolidate insn decoder users for copying code

[Masami Hiramatsu]

Consolidate x86 instruction decoder users on the path of
copying original code for kprobes.

Kprobes decodes same instruction 3 times in maximum when
preparing instruction buffer. The first time for getting the
length of instruction, the 2nd for adjusting displacement,
and the 3rd for checking whether the instruction is boostable
or not. For each time, actually decoding target address is
slightly different (1st is original address or recovered
instruction buffer, 2nd and 3rd are copied buffer), but
basically those must have same instruction.
Thus, this patch also changes the target address to copied
buffer at first and reuses the decoded "insn" for displacement
adjusting and checking boostable.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 Changes in v2:
   - Fix several build errors.
---
 arch/x86/kernel/kprobes/common.h |    4 +-
 arch/x86/kernel/kprobes/core.c   |   67 ++++++++++++++++++--------------------
 arch/x86/kernel/kprobes/opt.c    |    5 ++-
 3 files changed, 36 insertions(+), 40 deletions(-)

diff --git a/arch/x86/kernel/kprobes/common.h b/arch/x86/kernel/kprobes/common.h
index d688826..db2182d 100644
--- a/arch/x86/kernel/kprobes/common.h
+++ b/arch/x86/kernel/kprobes/common.h
@@ -67,7 +67,7 @@
 #endif
 
 /* Ensure if the instruction can be boostable */
-extern int can_boost(kprobe_opcode_t *instruction, void *addr);
+extern int can_boost(struct insn *insn, void *orig_addr);
 /* Recover instruction if given address is probed */
 extern unsigned long recover_probed_instruction(kprobe_opcode_t *buf,
 					 unsigned long addr);
@@ -75,7 +75,7 @@ extern unsigned long recover_probed_instruction(kprobe_opcode_t *buf,
  * Copy an instruction and adjust the displacement if the instruction
  * uses the %rip-relative addressing mode.
  */
-extern int __copy_instruction(u8 *dest, u8 *src);
+extern int __copy_instruction(u8 *dest, u8 *src, struct insn *insn);
 
 /* Generate a relative-jump/call instruction */
 extern void synthesize_reljump(void *from, void *to);
diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index a9ae61a..f2ad51c 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -164,33 +164,29 @@ static kprobe_opcode_t *skip_prefixes(kprobe_opcode_t *insn)
 NOKPROBE_SYMBOL(skip_prefixes);
 
 /*
- * Returns non-zero if opcode is boostable.
+ * Returns non-zero if INSN is boostable.
  * RIP relative instructions are adjusted at copying time in 64 bits mode
  */
-int can_boost(kprobe_opcode_t *opcodes, void *addr)
+int can_boost(struct insn *insn, void *addr)
 {
-	struct insn insn;
 	kprobe_opcode_t opcode;
 
 	if (search_exception_tables((unsigned long)addr))
 		return 0;	/* Page fault may occur on this address. */
 
-	kernel_insn_init(&insn, (void *)opcodes, MAX_INSN_SIZE);
-	insn_get_opcode(&insn);
-
 	/* 2nd-byte opcode */
-	if (insn.opcode.nbytes == 2)
-		return test_bit(insn.opcode.bytes[1],
+	if (insn->opcode.nbytes == 2)
+		return test_bit(insn->opcode.bytes[1],
 				(unsigned long *)twobyte_is_boostable);
 
-	if (insn.opcode.nbytes != 1)
+	if (insn->opcode.nbytes != 1)
 		return 0;
 
 	/* Can't boost Address-size override prefix */
-	if (unlikely(inat_is_address_size_prefix(insn.attr)))
+	if (unlikely(inat_is_address_size_prefix(insn->attr)))
 		return 0;
 
-	opcode = insn.opcode.bytes[0];
+	opcode = insn->opcode.bytes[0];
 
 	switch (opcode & 0xf0) {
 	case 0x60:
@@ -351,35 +347,31 @@ static int is_IF_modifier(kprobe_opcode_t *insn)
  * addressing mode.
  * This returns the length of copied instruction, or 0 if it has an error.
  */
-int __copy_instruction(u8 *dest, u8 *src)
+int __copy_instruction(u8 *dest, u8 *src, struct insn *insn)
 {
-	struct insn insn;
 	kprobe_opcode_t buf[MAX_INSN_SIZE];
-	int length;
 	unsigned long recovered_insn =
 		recover_probed_instruction(buf, (unsigned long)src);
 
-	if (!recovered_insn)
+	if (!recovered_insn || !insn)
 		return 0;
-	kernel_insn_init(&insn, (void *)recovered_insn, MAX_INSN_SIZE);
-	insn_get_length(&insn);
-	length = insn.length;
 
-	/* Another subsystem puts a breakpoint, failed to recover */
-	if (insn.opcode.bytes[0] == BREAKPOINT_INSTRUCTION)
+	/* This can access kernel text if given address is not recovered */
+	if (probe_kernel_read(dest, (void *)recovered_insn, MAX_INSN_SIZE))
 		return 0;
 
-	/* This can access kernel text if given address is not recovered */
-	if (kernel_probe_read(dest, insn.kaddr, length))
+	kernel_insn_init(insn, dest, MAX_INSN_SIZE);
+	insn_get_length(insn);
+
+	/* Another subsystem puts a breakpoint, failed to recover */
+	if (insn->opcode.bytes[0] == BREAKPOINT_INSTRUCTION)
 		return 0;
 
 #ifdef CONFIG_X86_64
 	/* Only x86_64 has RIP relative instructions */
-	if (insn_rip_relative(&insn)) {
+	if (insn_rip_relative(insn)) {
 		s64 newdisp;
 		u8 *disp;
-		kernel_insn_init(&insn, dest, length);
-		insn_get_displacement(&insn);
 		/*
 		 * The copied instruction uses the %rip-relative addressing
 		 * mode.  Adjust the displacement for the difference between
@@ -392,30 +384,32 @@ int __copy_instruction(u8 *dest, u8 *src)
 		 * extension of the original signed 32-bit displacement would
 		 * have given.
 		 */
-		newdisp = (u8 *) src + (s64) insn.displacement.value - (u8 *) dest;
+		newdisp = (u8 *) src + (s64) insn->displacement.value
+			  - (u8 *) dest;
 		if ((s64) (s32) newdisp != newdisp) {
 			pr_err("Kprobes error: new displacement does not fit into s32 (%llx)\n", newdisp);
-			pr_err("\tSrc: %p, Dest: %p, old disp: %x\n", src, dest, insn.displacement.value);
+			pr_err("\tSrc: %p, Dest: %p, old disp: %x\n",
+				src, dest, insn->displacement.value);
 			return 0;
 		}
-		disp = (u8 *) dest + insn_offset_displacement(&insn);
+		disp = (u8 *) dest + insn_offset_displacement(insn);
 		*(s32 *) disp = (s32) newdisp;
 	}
 #endif
-	return length;
+	return insn->length;
 }
 
 /* Prepare reljump right after instruction to boost */
-static void prepare_boost(struct kprobe *p, int length)
+static void prepare_boost(struct kprobe *p, struct insn *insn)
 {
-	if (can_boost(p->ainsn.insn, p->addr) &&
-	    MAX_INSN_SIZE - length >= RELATIVEJUMP_SIZE) {
+	if (can_boost(insn, p->addr) &&
+	    MAX_INSN_SIZE - insn->length >= RELATIVEJUMP_SIZE) {
 		/*
 		 * These instructions can be executed directly if it
 		 * jumps back to correct address.
 		 */
-		synthesize_reljump((void *)p->ainsn.insn + length,
-				   (void *)p->addr + length);
+		synthesize_reljump((void *)p->ainsn.insn + insn->length,
+				   (void *)p->addr + insn->length);
 		p->ainsn.boostable = true;
 	} else
 		p->ainsn.boostable = false;
@@ -423,12 +417,13 @@ static void prepare_boost(struct kprobe *p, int length)
 
 static int arch_copy_kprobe(struct kprobe *p)
 {
+	struct insn insn;
 	int len;
 
 	set_memory_rw((unsigned long)p->ainsn.insn & PAGE_MASK, 1);
 
 	/* Copy an instruction with recovering if other optprobe modifies it.*/
-	len = __copy_instruction(p->ainsn.insn, p->addr);
+	len = __copy_instruction(p->ainsn.insn, p->addr, &insn);
 	if (!len)
 		return -EINVAL;
 
@@ -436,7 +431,7 @@ static int arch_copy_kprobe(struct kprobe *p)
 	 * __copy_instruction can modify the displacement of the instruction,
 	 * but it doesn't affect boostable check.
 	 */
-	prepare_boost(p, len);
+	prepare_boost(p, &insn);
 
 	set_memory_ro((unsigned long)p->ainsn.insn & PAGE_MASK, 1);
 
diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c
index 5b52334..9aadff3 100644
--- a/arch/x86/kernel/kprobes/opt.c
+++ b/arch/x86/kernel/kprobes/opt.c
@@ -177,11 +177,12 @@ NOKPROBE_SYMBOL(optimized_callback);
 
 static int copy_optimized_instructions(u8 *dest, u8 *src)
 {
+	struct insn insn;
 	int len = 0, ret;
 
 	while (len < RELATIVEJUMP_SIZE) {
-		ret = __copy_instruction(dest + len, src + len);
-		if (!ret || !can_boost(dest + len, src + len))
+		ret = __copy_instruction(dest + len, src + len, &insn);
+		if (!ret || !can_boost(&insn, src + len))
 			return -EINVAL;
 		len += ret;
 	}

Re: [RFC PATCH tip/master V2 4/8] kprobes/x86: Do not modify singlestep buffer while resuming

[Ingo Molnar]

* Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Do not modify singlestep execution buffer (kprobe.ainsn.insn)
> while resuming from single-stepping, instead, modifies
> the buffer to add a jump back instruction at preparing
> buffer.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  arch/x86/kernel/kprobes/core.c |   42 +++++++++++++++++++---------------------
>  1 file changed, 20 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> index 6327f95..ea3b8e5 100644
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -399,23 +399,36 @@ int __copy_instruction(u8 *dest, u8 *src)
>  	return length;
>  }
>  
> +/* Prepare reljump right after instruction to boost */
> +static void prepare_boost(struct kprobe *p, int length)
> +{
> +	if (can_boost(p->ainsn.insn, p->addr) &&
> +	    MAX_INSN_SIZE - length >= RELATIVEJUMP_SIZE) {
> +		/*
> +		 * These instructions can be executed directly if it
> +		 * jumps back to correct address.
> +		 */
> +		synthesize_reljump((void *)p->ainsn.insn + length,
> +				   (void *)p->addr + length);
> +		p->ainsn.boostable = 1;
> +	} else
> +		p->ainsn.boostable = -1;

Those imbalanced curly braces are not proper kernel style.

Also, is the (void *) cast required? arch.insns ought to be void * already, right? 
(I haven't checking whether that's true for all architectures.)

Btw., the original code had the curly braces right:

> -	if (p->ainsn.boostable == 0) {
> -		if ((regs->ip > copy_ip) &&
> -		    (regs->ip - copy_ip) + 5 < MAX_INSN_SIZE) {
> -			/*
> -			 * These instructions can be executed directly if it
> -			 * jumps back to correct address.
> -			 */
> -			synthesize_reljump((void *)regs->ip,
> -				(void *)orig_ip + (regs->ip - copy_ip));
> -			p->ainsn.boostable = 1;
> -		} else {
> -			p->ainsn.boostable = -1;
> -		}
> -	}

Thanks,

	Ingo

Re: [RFC PATCH tip/master V2 4/8] kprobes/x86: Do not modify singlestep buffer while resuming

[Masami Hiramatsu]

On Tue, 28 Mar 2017 09:04:19 +0200
Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > Do not modify singlestep execution buffer (kprobe.ainsn.insn)
> > while resuming from single-stepping, instead, modifies
> > the buffer to add a jump back instruction at preparing
> > buffer.
> > 
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > ---
> >  arch/x86/kernel/kprobes/core.c |   42 +++++++++++++++++++---------------------
> >  1 file changed, 20 insertions(+), 22 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> > index 6327f95..ea3b8e5 100644
> > --- a/arch/x86/kernel/kprobes/core.c
> > +++ b/arch/x86/kernel/kprobes/core.c
> > @@ -399,23 +399,36 @@ int __copy_instruction(u8 *dest, u8 *src)
> >  	return length;
> >  }
> >  
> > +/* Prepare reljump right after instruction to boost */
> > +static void prepare_boost(struct kprobe *p, int length)
> > +{
> > +	if (can_boost(p->ainsn.insn, p->addr) &&
> > +	    MAX_INSN_SIZE - length >= RELATIVEJUMP_SIZE) {
> > +		/*
> > +		 * These instructions can be executed directly if it
> > +		 * jumps back to correct address.
> > +		 */
> > +		synthesize_reljump((void *)p->ainsn.insn + length,
> > +				   (void *)p->addr + length);
> > +		p->ainsn.boostable = 1;
> > +	} else
> > +		p->ainsn.boostable = -1;
> 
> Those imbalanced curly braces are not proper kernel style.

Ah, OK, I'll fix that. 

> 
> Also, is the (void *) cast required? arch.insns ought to be void * already, right? 

Hmm, it is kprobe_opcode_t *, but anyway, on x86 it is same,, since kprobe_opcode_t
on x86 is u8.

> (I haven't checking whether that's true for all architectures.)

It is OK because this is x86 very specific code.

Thank you,

> 
> Btw., the original code had the curly braces right:
> 
> > -	if (p->ainsn.boostable == 0) {
> > -		if ((regs->ip > copy_ip) &&
> > -		    (regs->ip - copy_ip) + 5 < MAX_INSN_SIZE) {
> > -			/*
> > -			 * These instructions can be executed directly if it
> > -			 * jumps back to correct address.
> > -			 */
> > -			synthesize_reljump((void *)regs->ip,
> > -				(void *)orig_ip + (regs->ip - copy_ip));
> > -			p->ainsn.boostable = 1;
> > -		} else {
> > -			p->ainsn.boostable = -1;
> > -		}
> > -	}
> 
> Thanks,
> 
> 	Ingo


-- 
Masami Hiramatsu <mhiramat@kernel.org>