[PATCH -tip v2 0/2] kprobes/x86: RO text code bugfix and cleanup

[PATCH -tip v2 0/2] kprobes/x86: RO text code bugfix and cleanup

[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


 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(-)

--
Masami Hiramatsu <mhiramat@kernel.org>

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

[Masami Hiramatsu]

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 <mhiramat@kernel.org>
---
  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(-)

diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index f0153714ddac..5d8194b9a068 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -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;
 }
diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c
index 4f98aad38237..86b9a883b712 100644
--- a/arch/x86/kernel/kprobes/opt.c
+++ b/arch/x86/kernel/kprobes/opt.c
@@ -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;

[PATCH -tip v2 2/2] kprobes/x86: Remove addressof operators

[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 <mhiramat@kernel.org>
---
 arch/x86/include/asm/kprobes.h |    4 ++--
 arch/x86/kernel/kprobes/opt.c  |    8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/x86/include/asm/kprobes.h b/arch/x86/include/asm/kprobes.h
index 6cf65437b5e5..9f2e3102e0bb 100644
--- a/arch/x86/include/asm/kprobes.h
+++ b/arch/x86/include/asm/kprobes.h
@@ -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;
diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c
index 86b9a883b712..d85fac42b512 100644
--- a/arch/x86/kernel/kprobes/opt.c
+++ b/arch/x86/kernel/kprobes/opt.c
@@ -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);

Re: [PATCH -tip v2 0/2] kprobes/x86: RO text code bugfix and cleanup

[Ingo Molnar]

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

> 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
> 
> 
>  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

Re: [PATCH -tip v2 0/2] kprobes/x86: RO text code bugfix and cleanup

[Masami Hiramatsu]

On Thu, 17 Aug 2017 11:55:30 +0200
Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > 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
> > 
> > 
> >  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.

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

> 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.

> 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!

> 
> Is there anything I'm missing?
> 
> Thanks,
> 
> 	Ingo


-- 
Masami Hiramatsu <mhiramat@kernel.org>