kprobes vs __ex_table[]

kprobes vs __ex_table[]

[Peter Zijlstra]

Hi Masami,

I just wondered what would happen if I put a probe on an instruction
that was listed in __ex_table[] or __bug_table[].

And it looks like it will happily do that. It will then run the
instruction out-of-line, and when said instruction traps, the
instruction address will not match the one listed in either __ex_table[]
or __bug_table[] and badness will happen.

If kprobes does indeed not check this, we should probably fix it, if it
does do check this, could you point me to it?

Re: kprobes vs __ex_table[]

[Masami Hiramatsu]

On Thu, 23 Feb 2017 19:30:02 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> Hi Masami,
> 
> I just wondered what would happen if I put a probe on an instruction
> that was listed in __ex_table[] or __bug_table[].

Ah, thanks for reporting, I know __ex_table issue and fixed, but
I didn't care about __bug_table.

> And it looks like it will happily do that. It will then run the
> instruction out-of-line, and when said instruction traps, the
> instruction address will not match the one listed in either __ex_table[]
> or __bug_table[] and badness will happen.

For the __ex_table[], at least on x86, kprobes already handles it in
kprobe_fault_handler, which restore regs->ip to original place when
a pagefault happens on singlestepping.

> If kprobes does indeed not check this, we should probably fix it, if it
> does do check this, could you point me to it?

Yeah, for BUG() case, as far as I can see, there is no check about that.
So, there are 2 ways to fix it up, one is to just reject to put kprobes on
UD2, another is fixup trap address as we did for exceptions_table.
I think latter is better because if there is a divide error happening
on single-step, anyway we should fixup the address...

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: kprobes vs __ex_table[]

[Peter Zijlstra]

On Fri, Feb 24, 2017 at 10:04:51AM +0900, Masami Hiramatsu wrote:
> On Thu, 23 Feb 2017 19:30:02 +0100
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > Hi Masami,
> > 
> > I just wondered what would happen if I put a probe on an instruction
> > that was listed in __ex_table[] or __bug_table[].
> 
> Ah, thanks for reporting, I know __ex_table issue and fixed, but
> I didn't care about __bug_table.
> 
> > And it looks like it will happily do that. It will then run the
> > instruction out-of-line, and when said instruction traps, the
> > instruction address will not match the one listed in either __ex_table[]
> > or __bug_table[] and badness will happen.
> 
> For the __ex_table[], at least on x86, kprobes already handles it in
> kprobe_fault_handler, which restore regs->ip to original place when
> a pagefault happens on singlestepping.

Ah, but that is only #PF, we also use __ex_table on other faults/traps,
like #GP which would need help in do_general_protection(), and I have a
patch (that might not ever go anywhere) that uses it on #UD (but for all
I know we already use #UD to probe existence of instructions).

In any case, we call fixup_exception() from pretty much all exception
vectors, not only #PF.

But see below.

> > If kprobes does indeed not check this, we should probably fix it, if it
> > does do check this, could you point me to it?
> 
> Yeah, for BUG() case, as far as I can see, there is no check about that.

So I've a patch that extends __bug_table[] to WARN (like many other
architectures already have).

http://lkml.kernel.org/r/20170223132813.GB6515@twins.programming.kicks-ass.net

> So, there are 2 ways to fix it up, one is to just reject to put kprobes on
> UD2, another is fixup trap address as we did for exceptions_table.
> I think latter is better because if there is a divide error happening
> on single-step, anyway we should fixup the address...

Right.

So I like the fixup idea, just not sure the current
kprobe_fault_handler() is sufficient or correct.

It looks like it rewrites regs->ip, which would make return from fault
return to the wrong place, no?

Would it not be better to do the fixup in fixup_exception()/fixup_bug()?
Because then we cover all callers, not just #PF.

One more complication with __ex_table and optimized kprobes is that we
need to be careful not to clobber __ex_table[].fixup. It would be very
bad if the optimized probe were to clobber the address we let the fixup
return to -- or that needs fixups too, _after_ running
__ex_table[].handler().

Re: kprobes vs __ex_table[]

[Masami Hiramatsu]

On Fri, 24 Feb 2017 10:26:46 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> On Fri, Feb 24, 2017 at 10:04:51AM +0900, Masami Hiramatsu wrote:
> > On Thu, 23 Feb 2017 19:30:02 +0100
> > Peter Zijlstra <peterz@infradead.org> wrote:
> > 
> > > Hi Masami,
> > > 
> > > I just wondered what would happen if I put a probe on an instruction
> > > that was listed in __ex_table[] or __bug_table[].
> > 
> > Ah, thanks for reporting, I know __ex_table issue and fixed, but
> > I didn't care about __bug_table.
> > 
> > > And it looks like it will happily do that. It will then run the
> > > instruction out-of-line, and when said instruction traps, the
> > > instruction address will not match the one listed in either __ex_table[]
> > > or __bug_table[] and badness will happen.
> > 
> > For the __ex_table[], at least on x86, kprobes already handles it in
> > kprobe_fault_handler, which restore regs->ip to original place when
> > a pagefault happens on singlestepping.
> 
> Ah, but that is only #PF, we also use __ex_table on other faults/traps,
> like #GP which would need help in do_general_protection(),

#GP is also handled via kprobe_exceptions_notify().

> and I have a
> patch (that might not ever go anywhere) that uses it on #UD (but for all
> I know we already use #UD to probe existence of instructions).
> 
> In any case, we call fixup_exception() from pretty much all exception
> vectors, not only #PF.

OK, kprobes actually already has handled notify_die via above function,
so one possible solution is extend it for those exceptions.

> 
> But see below.
> 
> > > If kprobes does indeed not check this, we should probably fix it, if it
> > > does do check this, could you point me to it?
> > 
> > Yeah, for BUG() case, as far as I can see, there is no check about that.
> 
> So I've a patch that extends __bug_table[] to WARN (like many other
> architectures already have).
> 
> http://lkml.kernel.org/r/20170223132813.GB6515@twins.programming.kicks-ass.net

OK, I got it.

> 
> > So, there are 2 ways to fix it up, one is to just reject to put kprobes on
> > UD2, another is fixup trap address as we did for exceptions_table.
> > I think latter is better because if there is a divide error happening
> > on single-step, anyway we should fixup the address...
> 
> Right.
> 
> So I like the fixup idea, just not sure the current
> kprobe_fault_handler() is sufficient or correct.
> 
> It looks like it rewrites regs->ip, which would make return from fault
> return to the wrong place, no?

Hmm, when regs->ip is reset to the original place, kprobe_fault_handler()
returns 0 and normal #PF handler fixup pages etc. and retry from the
original place. This might kick kprobes again and do singlestep.

So, yes, it may not enough for other faults if those will not only check
regs->ip, but read the instruction pointed by regs->ip (as your patch).
In that case you need to use recover_probed_instruction() instead of
probe_kernel_address(). (BTW, recover_probed_instruction() uses memcpy()
without checking kernel_text, it should use probe_kernel_address().)

> 
> Would it not be better to do the fixup in fixup_exception()/fixup_bug()?
> Because then we cover all callers, not just #PF.

As I said above, kprobes already handles notify_die. But yes, we need
a special fixup call before notify_die.

> One more complication with __ex_table and optimized kprobes is that we
> need to be careful not to clobber __ex_table[].fixup. It would be very
> bad if the optimized probe were to clobber the address we let the fixup
> return to -- or that needs fixups too, _after_ running
> __ex_table[].handler().

can_optimize() takes care about that case. If the probe target function
(not only probed address) includes an exception address, it rejects
optimizing probes.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: kprobes vs __ex_table[]

[Peter Zijlstra]

On Sat, Feb 25, 2017 at 01:34:15AM +0900, Masami Hiramatsu wrote:
> > Ah, but that is only #PF, we also use __ex_table on other faults/traps,
> > like #GP which would need help in do_general_protection(),
> 
> #GP is also handled via kprobe_exceptions_notify().

Ah! that's where its hidden :-) Thanks!

> > It looks like it rewrites regs->ip, which would make return from fault
> > return to the wrong place, no?
> 
> Hmm, when regs->ip is reset to the original place, kprobe_fault_handler()
> returns 0 and normal #PF handler fixup pages etc. and retry from the
> original place. This might kick kprobes again and do singlestep.
> 
> So, yes, it may not enough for other faults if those will not only check
> regs->ip, but read the instruction pointed by regs->ip (as your patch).
> In that case you need to use recover_probed_instruction() instead of
> probe_kernel_address(). (BTW, recover_probed_instruction() uses memcpy()
> without checking kernel_text, it should use probe_kernel_address().)

> > One more complication with __ex_table and optimized kprobes is that we
> > need to be careful not to clobber __ex_table[].fixup. It would be very
> > bad if the optimized probe were to clobber the address we let the fixup
> > return to -- or that needs fixups too, _after_ running
> > __ex_table[].handler().
> 
> can_optimize() takes care about that case. If the probe target function
> (not only probed address) includes an exception address, it rejects
> optimizing probes.

Ah, so it does search_exception_tables(), which avoids clobbering actual
exception instructions, and most fixups live in .text.fixup and I cannot
actually find if that is excluded.

In any case, thanks for the pointers, I'll see if I can spot any actual
holes.

[RFC PATCH 0/2] kprobes/x86: Handle probing on ex_table cases

[Masami Hiramatsu]

Hi Peter,

Here, I've tried to handle kprobes on exception expected
instructions which are recorded in __ex_table.

1st patch fixes recover_probed_instruction() to use
probe_kernel_read() instead of memcpy kernel text.
So, if you need to get the original instruction for
checking the cause of #UD, you can use it instead
of probe_kernel_read().

2nd patch adds kprobe_exit_singlestep() right before
fixup_exception(), which fixes regs->ip to point original
address and reset current singlestepping process so that
fixup_exeption() can handle the exception correctly.

There seems some die() still not be fixed up. I'm not
sure we should fix that die() messages too. Would we
better fixup regs->ip in those cases?

Thank you,

---

Masami Hiramatsu (2):
      kprobes/x86: Use probe_kernel_read instead of memcpy
      kprobes/x86: Exit single-stepping before trying fixup_exception


 arch/x86/include/asm/kprobes.h |    1 
 arch/x86/kernel/kprobes/core.c |   90 +++++++++++++++++++++++++---------------
 arch/x86/kernel/kprobes/opt.c  |    5 ++
 arch/x86/kernel/traps.c        |   19 ++++++++
 4 files changed, 80 insertions(+), 35 deletions(-)

--
Masami Hiramatsu

[RFC PATCH 1/2] kprobes/x86: Use probe_kernel_read instead of memcpy

[Masami Hiramatsu]

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

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/x86/kernel/kprobes/core.c |    7 +++++--
 arch/x86/kernel/kprobes/opt.c  |    5 ++++-
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 6384eb7..34d3a52 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -264,7 +264,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
@@ -276,7 +279,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)
 {
diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c
index 3d1bee9..06ddd0b 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 2/2] kprobes/x86: Exit single-stepping before trying fixup_exception

[Masami Hiramatsu]

Exit single-stepping out of line and get back regs->ip to original
(probed) address before trying fixup_exception() if the exception
happened on the singlestep buffer, since the fixup_exception()
depends on regs->ip to search an entry on __ex_table.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/x86/include/asm/kprobes.h |    1 
 arch/x86/kernel/kprobes/core.c |   83 +++++++++++++++++++++++++---------------
 arch/x86/kernel/traps.c        |   19 +++++++++
 3 files changed, 71 insertions(+), 32 deletions(-)

diff --git a/arch/x86/include/asm/kprobes.h b/arch/x86/include/asm/kprobes.h
index d1d1e50..79e121a 100644
--- a/arch/x86/include/asm/kprobes.h
+++ b/arch/x86/include/asm/kprobes.h
@@ -111,6 +111,7 @@ struct kprobe_ctlblk {
 	struct prev_kprobe prev_kprobe;
 };
 
+extern int kprobe_exit_singlestep(struct pt_regs *regs);
 extern int kprobe_fault_handler(struct pt_regs *regs, int trapnr);
 extern int kprobe_exceptions_notify(struct notifier_block *self,
 				    unsigned long val, void *data);
diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 34d3a52..f2a3f3b 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -949,43 +949,62 @@ int kprobe_debug_handler(struct pt_regs *regs)
 }
 NOKPROBE_SYMBOL(kprobe_debug_handler);
 
-int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
+/* Fixup current ip register and reset current kprobe, if needed. */
+int kprobe_exit_singlestep(struct pt_regs *regs)
 {
-	struct kprobe *cur = kprobe_running();
 	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
+	struct kprobe *cur = kprobe_running();
 
-	if (unlikely(regs->ip == (unsigned long)cur->ainsn.insn)) {
-		/* This must happen on single-stepping */
-		WARN_ON(kcb->kprobe_status != KPROBE_HIT_SS &&
-			kcb->kprobe_status != KPROBE_REENTER);
-		/*
-		 * We are here because the instruction being single
-		 * stepped caused a page fault. We reset the current
-		 * kprobe and the ip points back to the probe address
-		 * and allow the page fault handler to continue as a
-		 * normal page fault.
-		 */
-		regs->ip = (unsigned long)cur->addr;
-		/*
-		 * Trap flag (TF) has been set here because this fault
-		 * happened where the single stepping will be done.
-		 * So clear it by resetting the current kprobe:
-		 */
-		regs->flags &= ~X86_EFLAGS_TF;
+	if (unlikely(regs->ip != (unsigned long)cur->ainsn.insn))
+		return 0;
 
-		/*
-		 * If the TF flag was set before the kprobe hit,
-		 * don't touch it:
-		 */
-		regs->flags |= kcb->kprobe_old_flags;
+	/* This must happen on single-stepping */
+	WARN_ON(kcb->kprobe_status != KPROBE_HIT_SS &&
+		kcb->kprobe_status != KPROBE_REENTER);
+	/*
+	 * We are here because the instruction being single
+	 * stepped caused a page fault. We reset the current
+	 * kprobe and the ip points back to the probe address
+	 * and allow the page fault handler to continue as a
+	 * normal page fault.
+	 */
+	regs->ip = (unsigned long)cur->addr;
+	/*
+	 * Trap flag (TF) has been set here because this fault
+	 * happened where the single stepping will be done.
+	 * So clear it by resetting the current kprobe:
+	 */
+	regs->flags &= ~X86_EFLAGS_TF;
 
-		if (kcb->kprobe_status == KPROBE_REENTER)
-			restore_previous_kprobe(kcb);
-		else
-			reset_current_kprobe();
-		preempt_enable_no_resched();
-	} else if (kcb->kprobe_status == KPROBE_HIT_ACTIVE ||
-		   kcb->kprobe_status == KPROBE_HIT_SSDONE) {
+	/*
+	 * If the TF flag was set before the kprobe hit,
+	 * don't touch it:
+	 */
+	regs->flags |= kcb->kprobe_old_flags;
+
+	if (kcb->kprobe_status == KPROBE_REENTER)
+		restore_previous_kprobe(kcb);
+	else
+		reset_current_kprobe();
+
+	/* Preempt has been disabled before single stepping */
+	preempt_enable_no_resched();
+
+	return 1;
+}
+NOKPROBE_SYMBOL(kprobe_exit_singlestep);
+
+int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
+{
+	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
+	struct kprobe *cur = kprobe_running();
+
+	/* If the fault happened on singlestep, finish it and retry */
+	if (kprobe_exit_singlestep(regs))
+		return 0;
+
+	if (kcb->kprobe_status == KPROBE_HIT_ACTIVE ||
+	    kcb->kprobe_status == KPROBE_HIT_SSDONE) {
 		/*
 		 * We increment the nmissed count for accounting,
 		 * we can also use npre/npostfault count for accounting
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 948443e..7ac1baf 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -187,6 +187,14 @@ do_trap_no_signal(struct task_struct *tsk, int trapnr, char *str,
 	}
 
 	if (!user_mode(regs)) {
+		/*
+		 * Exit from the kprobe's single-stepping before trying
+		 * fixup_exception() because the fixup routine is based on
+		 * trapped address (regs->ip). Single-stepping out of line
+		 * executes an instruction in different place, so it should
+		 * be fixed.
+		 */
+		kprobe_exit_singlestep(regs);
 		if (!fixup_exception(regs, trapnr)) {
 			tsk->thread.error_code = error_code;
 			tsk->thread.trap_nr = trapnr;
@@ -500,6 +508,12 @@ do_general_protection(struct pt_regs *regs, long error_code)
 
 	tsk = current;
 	if (!user_mode(regs)) {
+		/*
+		 * Exit from the kprobe's single-stepping before trying
+		 * fixup_exception(). Note that if the GPF occurred in
+		 * kprobe user handlers, it is handled in notify_die.
+		 */
+		kprobe_exit_singlestep(regs);
 		if (fixup_exception(regs, X86_TRAP_GP))
 			return;
 
@@ -799,6 +813,11 @@ static void math_error(struct pt_regs *regs, int error_code, int trapnr)
 	cond_local_irq_enable(regs);
 
 	if (!user_mode(regs)) {
+		/*
+		 * Exit from the kprobe's single-stepping before trying
+		 * fixup_exception().
+		 */
+		kprobe_exit_singlestep(regs);
 		if (!fixup_exception(regs, trapnr)) {
 			task->thread.error_code = error_code;
 			task->thread.trap_nr = trapnr;

Re: kprobes vs __ex_table[]

[Masami Hiramatsu]

Hi Peter,

On Fri, 24 Feb 2017 10:26:46 +0100
Peter Zijlstra <peterz@infradead.org> wrote:

> One more complication with __ex_table and optimized kprobes is that we
> need to be careful not to clobber __ex_table[].fixup. It would be very
> bad if the optimized probe were to clobber the address we let the fixup
> return to -- or that needs fixups too, _after_ running
> __ex_table[].handler().

This gave me a chance to read closer current code, and I found that
I made a mistake 5 years ago on kprobe-booster. The commit 464846888d9a
("x86/kprobes: Fix a bug which can modify kernel code permanently")
introduced another bug -- which passed the address of copied instruction
instead of probing address to search_exception_tables() when preparing
kprobe-booster (skips singlestep.)

I'll send a fix patch.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

[PATCH] [BUGFIX] kprobes/x86: Fix to check __ex_table entry by probed address

[Masami Hiramatsu]

Fix to check exception table entry by using probed address
instead of the address of copied instruction. This may cause
unexpected kernel panic if user probe an address where an
exception can happen which should be fixup by __ex_table
(e.g. copy_from_user.)

Unless user puts a kprobe on such address, this doesn't
cause any problem.

This bug has been introduced by commit 464846888d9a
("x86/kprobes: Fix a bug which can modify kernel code permanently").

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Fixes: 464846888d9a ("x86/kprobes: Fix a bug which can modify kernel code permanently")
---
 arch/x86/kernel/kprobes/common.h |    2 +-
 arch/x86/kernel/kprobes/core.c   |    6 +++---
 arch/x86/kernel/kprobes/opt.c    |    2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/kprobes/common.h b/arch/x86/kernel/kprobes/common.h
index c6ee63f..d688826 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);
+extern int can_boost(kprobe_opcode_t *instruction, void *addr);
 /* Recover instruction if given address is probed */
 extern unsigned long recover_probed_instruction(kprobe_opcode_t *buf,
 					 unsigned long addr);
diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 6384eb7..993fa4f 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -167,12 +167,12 @@ NOKPROBE_SYMBOL(skip_prefixes);
  * Returns non-zero if opcode is boostable.
  * RIP relative instructions are adjusted at copying time in 64 bits mode
  */
-int can_boost(kprobe_opcode_t *opcodes)
+int can_boost(kprobe_opcode_t *opcodes, void *addr)
 {
 	kprobe_opcode_t opcode;
 	kprobe_opcode_t *orig_opcodes = opcodes;
 
-	if (search_exception_tables((unsigned long)opcodes))
+	if (search_exception_tables((unsigned long)addr))
 		return 0;	/* Page fault may occur on this address. */
 
 retry:
@@ -417,7 +417,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.
 	 */
-	if (can_boost(p->ainsn.insn))
+	if (can_boost(p->ainsn.insn, p->addr))
 		p->ainsn.boostable = 0;
 	else
 		p->ainsn.boostable = -1;
diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c
index 3d1bee9..3e7c6e5 100644
--- a/arch/x86/kernel/kprobes/opt.c
+++ b/arch/x86/kernel/kprobes/opt.c
@@ -178,7 +178,7 @@ static int copy_optimized_instructions(u8 *dest, u8 *src)
 
 	while (len < RELATIVEJUMP_SIZE) {
 		ret = __copy_instruction(dest + len, src + len);
-		if (!ret || !can_boost(dest + len))
+		if (!ret || !can_boost(dest + len, src + len))
 			return -EINVAL;
 		len += ret;
 	}

[tip:perf/urgent] kprobes/x86: Fix kernel panic when certain exception-handling addresses are probed

[tip-bot for Masami Hiramatsu]

Commit-ID:  75013fb16f8484898eaa8d0b08fed942d790f029
Gitweb:     http://git.kernel.org/tip/75013fb16f8484898eaa8d0b08fed942d790f029
Author:     Masami Hiramatsu <mhiramat@kernel.org>
AuthorDate: Wed, 1 Mar 2017 01:23:24 +0900
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Wed, 1 Mar 2017 09:56:13 +0100

kprobes/x86: Fix kernel panic when certain exception-handling addresses are probed

Fix to the exception table entry check by using probed address
instead of the address of copied instruction.

This bug may cause unexpected kernel panic if user probe an address
where an exception can happen which should be fixup by __ex_table
(e.g. copy_from_user.)

Unless user puts a kprobe on such address, this doesn't
cause any problem.

This bug has been introduced years ago, by commit:

  464846888d9a ("x86/kprobes: Fix a bug which can modify kernel code permanently").

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Fixes: 464846888d9a ("x86/kprobes: Fix a bug which can modify kernel code permanently")
Link: http://lkml.kernel.org/r/148829899399.28855.12581062400757221722.stgit@devbox
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/kprobes/common.h | 2 +-
 arch/x86/kernel/kprobes/core.c   | 6 +++---
 arch/x86/kernel/kprobes/opt.c    | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/kprobes/common.h b/arch/x86/kernel/kprobes/common.h
index c6ee63f..d688826 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);
+extern int can_boost(kprobe_opcode_t *instruction, void *addr);
 /* Recover instruction if given address is probed */
 extern unsigned long recover_probed_instruction(kprobe_opcode_t *buf,
 					 unsigned long addr);
diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 520b8df..88b3c94 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -166,12 +166,12 @@ NOKPROBE_SYMBOL(skip_prefixes);
  * Returns non-zero if opcode is boostable.
  * RIP relative instructions are adjusted at copying time in 64 bits mode
  */
-int can_boost(kprobe_opcode_t *opcodes)
+int can_boost(kprobe_opcode_t *opcodes, void *addr)
 {
 	kprobe_opcode_t opcode;
 	kprobe_opcode_t *orig_opcodes = opcodes;
 
-	if (search_exception_tables((unsigned long)opcodes))
+	if (search_exception_tables((unsigned long)addr))
 		return 0;	/* Page fault may occur on this address. */
 
 retry:
@@ -416,7 +416,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.
 	 */
-	if (can_boost(p->ainsn.insn))
+	if (can_boost(p->ainsn.insn, p->addr))
 		p->ainsn.boostable = 0;
 	else
 		p->ainsn.boostable = -1;
diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c
index 3d1bee9..3e7c6e5 100644
--- a/arch/x86/kernel/kprobes/opt.c
+++ b/arch/x86/kernel/kprobes/opt.c
@@ -178,7 +178,7 @@ static int copy_optimized_instructions(u8 *dest, u8 *src)
 
 	while (len < RELATIVEJUMP_SIZE) {
 		ret = __copy_instruction(dest + len, src + len);
-		if (!ret || !can_boost(dest + len))
+		if (!ret || !can_boost(dest + len, src + len))
 			return -EINVAL;
 		len += ret;
 	}

Re: [RFC PATCH 2/2] kprobes/x86: Exit single-stepping before trying fixup_exception

[Masami Hiramatsu]

On Tue, 28 Feb 2017 01:14:38 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Exit single-stepping out of line and get back regs->ip to original
> (probed) address before trying fixup_exception() if the exception
> happened on the singlestep buffer, since the fixup_exception()
> depends on regs->ip to search an entry on __ex_table.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  arch/x86/include/asm/kprobes.h |    1 
>  arch/x86/kernel/kprobes/core.c |   83 +++++++++++++++++++++++++---------------
>  arch/x86/kernel/traps.c        |   19 +++++++++
>  3 files changed, 71 insertions(+), 32 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kprobes.h b/arch/x86/include/asm/kprobes.h
> index d1d1e50..79e121a 100644
> --- a/arch/x86/include/asm/kprobes.h
> +++ b/arch/x86/include/asm/kprobes.h
> @@ -111,6 +111,7 @@ struct kprobe_ctlblk {
>  	struct prev_kprobe prev_kprobe;
>  };
>  
> +extern int kprobe_exit_singlestep(struct pt_regs *regs);
>  extern int kprobe_fault_handler(struct pt_regs *regs, int trapnr);
>  extern int kprobe_exceptions_notify(struct notifier_block *self,
>  				    unsigned long val, void *data);
> diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
> index 34d3a52..f2a3f3b 100644
> --- a/arch/x86/kernel/kprobes/core.c
> +++ b/arch/x86/kernel/kprobes/core.c
> @@ -949,43 +949,62 @@ int kprobe_debug_handler(struct pt_regs *regs)
>  }
>  NOKPROBE_SYMBOL(kprobe_debug_handler);
>  
> -int kprobe_fault_handler(struct pt_regs *regs, int trapnr)
> +/* Fixup current ip register and reset current kprobe, if needed. */
> +int kprobe_exit_singlestep(struct pt_regs *regs)
>  {
> -	struct kprobe *cur = kprobe_running();
>  	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
> +	struct kprobe *cur = kprobe_running();
>  
> -	if (unlikely(regs->ip == (unsigned long)cur->ainsn.insn)) {
> -		/* This must happen on single-stepping */
> -		WARN_ON(kcb->kprobe_status != KPROBE_HIT_SS &&
> -			kcb->kprobe_status != KPROBE_REENTER);
> -		/*
> -		 * We are here because the instruction being single
> -		 * stepped caused a page fault. We reset the current
> -		 * kprobe and the ip points back to the probe address
> -		 * and allow the page fault handler to continue as a
> -		 * normal page fault.
> -		 */
> -		regs->ip = (unsigned long)cur->addr;
> -		/*
> -		 * Trap flag (TF) has been set here because this fault
> -		 * happened where the single stepping will be done.
> -		 * So clear it by resetting the current kprobe:
> -		 */
> -		regs->flags &= ~X86_EFLAGS_TF;
> +	if (unlikely(regs->ip != (unsigned long)cur->ainsn.insn))

Oops, this is not unlikely, this is likely case (since I inverted the condition).

Thanks,


-- 
Masami Hiramatsu <mhiramat@kernel.org>