kprobes/x86: RO text code bugfix and cleanup

PATCH In review Masami Hiramatsu 2 participants 5 messages Aug 12, 2017 – Aug 17, 2017 x86

Share & tools

Masami Hiramatsu

author

2017-08-12 00:45 UTC #1 lore raw

PATCH v2 1/2 kprobes/x86: Don't forget to set memory back to RO on failure

Do not forget to set kprobes insn buffer memory back to RO on failure path. Without this fix, if there is an unexpected error on copying instructions, kprobes insn buffer kept RW, which can allow unexpected modifying instruction buffer.

Fixes d0381c81c2f7 ("kprobes/x86: Set kprobes pages read-only")

Signed-off-by Masami Hiramatsu <[email protected]>

---
Changes in v2:
- Use a helper variable instead of using p->ainsn.insn directly.

---
 arch/x86/kernel/kprobes/core.c |   15 +++++++++------
 arch/x86/kernel/kprobes/opt.c  |    1 +
 2 files changed, 10 insertions(+), 6 deletions(-)

arch/x86/kernel/kprobes/core.c +9 -6

index f0153714ddac..5d8194b9a068 100644
@@ -428,15 +428,18 @@ void free_insn_page(void *page)
 
 static int arch_copy_kprobe(struct kprobe *p)
 {
+	kprobe_opcode_t *buf = p->ainsn.insn;
 	struct insn insn;
 	int len;
 
-	set_memory_rw((unsigned long)p->ainsn.insn & PAGE_MASK, 1);
+	set_memory_rw((unsigned long)buf & PAGE_MASK, 1);
 
 	/* Copy an instruction with recovering if other optprobe modifies it.*/
-	len = __copy_instruction(p->ainsn.insn, p->addr, &insn);
-	if (!len)
+	len = __copy_instruction(buf, p->addr, &insn);
+	if (!len) {
+		set_memory_ro((unsigned long)buf & PAGE_MASK, 1);
 		return -EINVAL;
+	}
 
 	/*
 	 * __copy_instruction can modify the displacement of the instruction,

@@ -444,13 +447,13 @@ static int arch_copy_kprobe(struct kprobe *p)
 	 */
 	prepare_boost(p, &insn);
 
-	set_memory_ro((unsigned long)p->ainsn.insn & PAGE_MASK, 1);
+	set_memory_ro((unsigned long)buf & PAGE_MASK, 1);
 
 	/* Check whether the instruction modifies Interrupt Flag or not */
-	p->ainsn.if_modifier = is_IF_modifier(p->ainsn.insn);
+	p->ainsn.if_modifier = is_IF_modifier(buf);
 
 	/* Also, displacement change doesn't affect the first byte */
-	p->opcode = p->ainsn.insn[0];
+	p->opcode = buf[0];
 
 	return 0;
 }

arch/x86/kernel/kprobes/opt.c +1 -0

index 4f98aad38237..86b9a883b712 100644
@@ -371,6 +371,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op,
 	ret = copy_optimized_instructions(buf + TMPL_END_IDX, op->kp.addr);
 	if (ret < 0) {
 		__arch_remove_optimized_kprobe(op, 0);
+		set_memory_ro((unsigned long)buf & PAGE_MASK, 1);
 		return ret;
 	}
 	op->optinsn.size = ret;

Masami Hiramatsu

author

2017-08-12 00:46 UTC #2 lore raw

PATCH v2 2/2 kprobes/x86: Remove addressof operators

In reply to Masami Hiramatsu

Since commit 54a7d50b9205 ("x86: mark kprobe templates as character arrays, not single characters") changes optprobe_template_* to arrays, we can remove addressof operators from those symbols.

Signed-off-by Masami Hiramatsu <[email protected]>

---
 arch/x86/include/asm/kprobes.h |    4 ++--
 arch/x86/kernel/kprobes/opt.c  |    8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

arch/x86/include/asm/kprobes.h +2 -2

index 6cf65437b5e5..9f2e3102e0bb 100644
@@ -58,8 +58,8 @@ extern __visible kprobe_opcode_t optprobe_template_call[];
 extern __visible kprobe_opcode_t optprobe_template_end[];
 #define MAX_OPTIMIZED_LENGTH (MAX_INSN_SIZE + RELATIVE_ADDR_SIZE)
 #define MAX_OPTINSN_SIZE 				\
-	(((unsigned long)&optprobe_template_end -	\
-	  (unsigned long)&optprobe_template_entry) +	\
+	(((unsigned long)optprobe_template_end -	\
+	  (unsigned long)optprobe_template_entry) +	\
 	 MAX_OPTIMIZED_LENGTH + RELATIVEJUMP_SIZE)
 
 extern const int kretprobe_blacklist_size;

arch/x86/kernel/kprobes/opt.c +4 -4

index 86b9a883b712..d85fac42b512 100644
@@ -142,11 +142,11 @@ void optprobe_template_func(void);
 STACK_FRAME_NON_STANDARD(optprobe_template_func);
 
 #define TMPL_MOVE_IDX \
-	((long)&optprobe_template_val - (long)&optprobe_template_entry)
+	((long)optprobe_template_val - (long)optprobe_template_entry)
 #define TMPL_CALL_IDX \
-	((long)&optprobe_template_call - (long)&optprobe_template_entry)
+	((long)optprobe_template_call - (long)optprobe_template_entry)
 #define TMPL_END_IDX \
-	((long)&optprobe_template_end - (long)&optprobe_template_entry)
+	((long)optprobe_template_end - (long)optprobe_template_entry)
 
 #define INT3_SIZE sizeof(kprobe_opcode_t)

@@ -377,7 +377,7 @@ int arch_prepare_optimized_kprobe(struct optimized_kprobe *op,
 	op->optinsn.size = ret;
 
 	/* Copy arch-dep-instance from template */
-	memcpy(buf, &optprobe_template_entry, TMPL_END_IDX);
+	memcpy(buf, optprobe_template_entry, TMPL_END_IDX);
 
 	/* Set probe information */
 	synthesize_set_arg1(buf + TMPL_MOVE_IDX, (unsigned long)op);

Ingo Molnar

2017-08-17 09:55 UTC #3 lore raw

In reply to Masami Hiramatsu

Quoted 23 lines · Masami Hiramatsu

Hi,
This series fixes a kprobe-x86 bug related to RO text and cleans up addressof operators.
The first one is an obvious bug that misses to set memory RO when the function fails. And the second one is just a cleanup patch to remove addressof operators ("&") since it is meaningless anymore.
V2 has just a following update:
- [1/2] Use a helper variable instead of using p->ainsn.insn directly.
Thanks,
---
Masami Hiramatsu (2):
kprobes/x86: Don't forget to set memory back to RO on failure kprobes/x86: Remove addressof operators

Quoted 4 lines

 arch/x86/include/asm/kprobes.h |    4 ++--
 arch/x86/kernel/kprobes/core.c |   15 +++++++++------
 arch/x86/kernel/kprobes/opt.c  |    9 +++++----
 3 files changed, 16 insertions(+), 12 deletions(-)

So I'm totally opposed to the whole approach of modifying the permissions of the kernel text virtual memory area.

Firstly, it's racy against other kernel subsystems: what happens if some other code patching mechanism does a similar 'mark RWX and then back to RX'? Who provides the synchronization? set_memory_*() certainly does not.

Secondly, it's racy against attackers: if an attacker can time the attack to the time when a kprobe is installed, then the kernel is still vulnerable.

So how about avoiding the problem altogether by patching the kernel not in its virtual text address, but in the direct mappings? Then page permissions won't have to be modified, and the whole solution will be more robust and secure.

Is there anything I'm missing?

Thanks,

Ingo

Masami Hiramatsu

author

2017-08-17 15:07 UTC #4 lore raw

Quoted 31 lines · On Thu, 17 Aug 2017 11:55:30 +0200 Ingo Molnar <[email protected]> wrote:

Quoted 23 lines · Masami Hiramatsu
Hi,
This series fixes a kprobe-x86 bug related to RO text and cleans up addressof operators.
The first one is an obvious bug that misses to set memory RO when the function fails. And the second one is just a cleanup patch to remove addressof operators ("&") since it is meaningless anymore.
V2 has just a following update:
- [1/2] Use a helper variable instead of using p->ainsn.insn directly.
Thanks,
---
Masami Hiramatsu (2):
kprobes/x86: Don't forget to set memory back to RO on failure kprobes/x86: Remove addressof operators
Quoted 4 lines
 arch/x86/include/asm/kprobes.h |    4 ++--
 arch/x86/kernel/kprobes/core.c |   15 +++++++++------
 arch/x86/kernel/kprobes/opt.c  |    9 +++++----
 3 files changed, 16 insertions(+), 12 deletions(-)

Quoted 6 lines

So I'm totally opposed to the whole approach of modifying the permissions of the kernel text virtual memory area.
Firstly, it's racy against other kernel subsystems: what happens if some other code patching mechanism does a similar 'mark RWX and then back to RX'? Who provides the synchronization? set_memory_*() certainly does not.

Hmm, this sounds common problem for set_memory_*() users.

Quoted 2 lines

Secondly, it's racy against attackers: if an attacker can time the attack to the time when a kprobe is installed, then the kernel is still vulnerable.

Right, since this is not against for attackers, but for some unexpected memory corruption bugs. Yes, this is vulnerable against attackers.

Quoted 3 lines

So how about avoiding the problem altogether by patching the kernel not in its virtual text address, but in the direct mappings? Then page permissions won't have to be modified, and the whole solution will be more robust and secure.

So would you mean using text_poke()?
OK, that's a good idea. I'll try to rewrite it again with text_poke().

Thank you!

Quoted 6 lines

Is there anything I'm missing?
Thanks,
Ingo

Signature · 1 line

Masami Hiramatsu <[email protected]>