[BUGFIX PATCH V2 0/3] kprobes/arm: Improve kprobes implementation on arm

[BUGFIX PATCH V2 0/3] kprobes/arm: Improve kprobes implementation on arm

[Masami Hiramatsu]

Hi,

Here is the 2nd version of the patches which improve kprobe
on arm implementation (a kind of bugfix). Version 1 is here;

https://lkml.org/lkml/2017/2/13/538

In this version I didn't update the code, just update the
patch description according to Tixy's comment and add his Ack.

Thank you,

---

Masami Hiramatsu (3):
      kprobes/arm: Allow to handle reentered kprobe on single-stepping
      kprobes/arm: Skip single-stepping in recursing path if possible
      kprobes/arm: Fix the return address of multiple kretprobes


 arch/arm/probes/kprobes/core.c |   49 +++++++++++++++++++++++++++++++---------
 1 file changed, 38 insertions(+), 11 deletions(-)

--
Masami Hiramatsu

[BUGFIX PATCH V2 1/3] kprobes/arm: Allow to handle reentered kprobe on single-stepping

[Masami Hiramatsu]

This is arm port of commit 6a5022a56ac3 ("kprobes/x86: Allow to
handle reentered kprobe on single-stepping")

Since the FIQ handlers can interrupt in the single stepping
(or preparing the single stepping, do_debug etc.), we should
consider a kprobe is hit in the NMI handler. Even in that
case, the kprobe is allowed to be reentered as same as the
kprobes is hit in kprobe handlers (KPROBE_HIT_ACTIVE or
KPROBE_HIT_SSDONE.)

The real issue will happen when a kprobe hit while another
reentered kprobe is processing (KPROBE_REENTER), because
we already consumed a saved-area for the previous kprobe.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Acked-by: Jon Medhurst <tixy@linaro.org>
Reported-by: Russell King <linux@armlinux.org.uk>
---
 arch/arm/probes/kprobes/core.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/probes/kprobes/core.c b/arch/arm/probes/kprobes/core.c
index a4ec240..264fedb 100644
--- a/arch/arm/probes/kprobes/core.c
+++ b/arch/arm/probes/kprobes/core.c
@@ -270,6 +270,7 @@ void __kprobes kprobe_handler(struct pt_regs *regs)
 			switch (kcb->kprobe_status) {
 			case KPROBE_HIT_ACTIVE:
 			case KPROBE_HIT_SSDONE:
+			case KPROBE_HIT_SS:
 				/* A pre- or post-handler probe got us here. */
 				kprobes_inc_nmissed_count(p);
 				save_previous_kprobe(kcb);
@@ -278,6 +279,11 @@ void __kprobes kprobe_handler(struct pt_regs *regs)
 				singlestep(p, regs, kcb);
 				restore_previous_kprobe(kcb);
 				break;
+			case KPROBE_REENTER:
+				/* A nested probe was hit in FIQ, it is a BUG */
+				pr_warn("Unrecoverable kprobe detected at %p.\n",
+					p->addr);
+				/* fall through */
 			default:
 				/* impossible cases */
 				BUG();

[BUGFIX PATCH V2 2/3] kprobes/arm: Skip single-stepping in recursing path if possible

[Masami Hiramatsu]

Kprobes/arm skips single-stepping (moreover handling the event)
if the conditional instruction must not be executed. This
also applies that rule when we hit a recursing kprobe, so
that the nmissed count isn't incremented in that case.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Acked-by: Jon Medhurst <tixy@linaro.org>
---
 arch/arm/probes/kprobes/core.c |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/arch/arm/probes/kprobes/core.c b/arch/arm/probes/kprobes/core.c
index 264fedb..84989ae 100644
--- a/arch/arm/probes/kprobes/core.c
+++ b/arch/arm/probes/kprobes/core.c
@@ -265,7 +265,15 @@ void __kprobes kprobe_handler(struct pt_regs *regs)
 #endif
 
 	if (p) {
-		if (cur) {
+		if (!p->ainsn.insn_check_cc(regs->ARM_cpsr)) {
+			/*
+			 * Probe hit but conditional execution check failed,
+			 * so just skip the instruction and continue as if
+			 * nothing had happened.
+			 * In this case, we can skip recursing check too.
+			 */
+			singlestep_skip(p, regs);
+		} else if (cur) {
 			/* Kprobe is pending, so we're recursing. */
 			switch (kcb->kprobe_status) {
 			case KPROBE_HIT_ACTIVE:
@@ -288,7 +296,7 @@ void __kprobes kprobe_handler(struct pt_regs *regs)
 				/* impossible cases */
 				BUG();
 			}
-		} else if (p->ainsn.insn_check_cc(regs->ARM_cpsr)) {
+		} else {
 			/* Probe hit and conditional execution check ok. */
 			set_current_kprobe(p);
 			kcb->kprobe_status = KPROBE_HIT_ACTIVE;
@@ -309,13 +317,6 @@ void __kprobes kprobe_handler(struct pt_regs *regs)
 				}
 				reset_current_kprobe();
 			}
-		} else {
-			/*
-			 * Probe hit but conditional execution check failed,
-			 * so just skip the instruction and continue as if
-			 * nothing had happened.
-			 */
-			singlestep_skip(p, regs);
 		}
 	} else if (cur) {
 		/* We probably hit a jprobe.  Call its break handler. */

[BUGFIX PATCH V2 3/3] kprobes/arm: Fix the return address of multiple kretprobes

[Masami Hiramatsu]

This is arm port of commit 737480a0d525 ("kprobes/x86:
Fix the return address of multiple kretprobes").

Fix the return address of subsequent kretprobes when multiple
kretprobes are set on the same function.

For example:

  # cd /sys/kernel/debug/tracing
  # echo "r:event1 sys_symlink" > kprobe_events
  # echo "r:event2 sys_symlink" >> kprobe_events
  # echo 1 > events/kprobes/enable
  # ln -s /tmp/foo /tmp/bar

 (without this patch)

  # cat trace | grep -v ^#
              ln-82    [000] dn.2    68.446525: event1: (kretprobe_trampoline+0x0/0x18 <- SyS_symlink)
              ln-82    [000] dn.2    68.447831: event2: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)

 (with this patch)

  # cat trace | grep -v ^#
              ln-81    [000] dn.1    39.463469: event1: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)
              ln-81    [000] dn.1    39.464701: event2: (ret_fast_syscall+0x0/0x1c <- SyS_symlink)

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Acked-by: Jon Medhurst <tixy@linaro.org>
Cc: KUMANO Syuhei <kumano.prog@gmail.com>
---
 arch/arm/probes/kprobes/core.c |   24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/arch/arm/probes/kprobes/core.c b/arch/arm/probes/kprobes/core.c
index 84989ae..023800a 100644
--- a/arch/arm/probes/kprobes/core.c
+++ b/arch/arm/probes/kprobes/core.c
@@ -440,6 +440,7 @@ static __used __kprobes void *trampoline_handler(struct pt_regs *regs)
 	struct hlist_node *tmp;
 	unsigned long flags, orig_ret_address = 0;
 	unsigned long trampoline_address = (unsigned long)&kretprobe_trampoline;
+	kprobe_opcode_t *correct_ret_addr = NULL;
 
 	INIT_HLIST_HEAD(&empty_rp);
 	kretprobe_hash_lock(current, &head, &flags);
@@ -462,14 +463,34 @@ static __used __kprobes void *trampoline_handler(struct pt_regs *regs)
 			/* another task is sharing our hash bucket */
 			continue;
 
+		orig_ret_address = (unsigned long)ri->ret_addr;
+
+		if (orig_ret_address != trampoline_address)
+			/*
+			 * This is the real return address. Any other
+			 * instances associated with this task are for
+			 * other calls deeper on the call stack
+			 */
+			break;
+	}
+
+	kretprobe_assert(ri, orig_ret_address, trampoline_address);
+
+	correct_ret_addr = ri->ret_addr;
+	hlist_for_each_entry_safe(ri, tmp, head, hlist) {
+		if (ri->task != current)
+			/* another task is sharing our hash bucket */
+			continue;
+
+		orig_ret_address = (unsigned long)ri->ret_addr;
 		if (ri->rp && ri->rp->handler) {
 			__this_cpu_write(current_kprobe, &ri->rp->kp);
 			get_kprobe_ctlblk()->kprobe_status = KPROBE_HIT_ACTIVE;
+			ri->ret_addr = correct_ret_addr;
 			ri->rp->handler(ri, regs);
 			__this_cpu_write(current_kprobe, NULL);
 		}
 
-		orig_ret_address = (unsigned long)ri->ret_addr;
 		recycle_rp_inst(ri, &empty_rp);
 
 		if (orig_ret_address != trampoline_address)
@@ -481,7 +502,6 @@ static __used __kprobes void *trampoline_handler(struct pt_regs *regs)
 			break;
 	}
 
-	kretprobe_assert(ri, orig_ret_address, trampoline_address);
 	kretprobe_hash_unlock(current, &flags);
 
 	hlist_for_each_entry_safe(ri, tmp, &empty_rp, hlist) {

Re: [BUGFIX PATCH V2 0/3] kprobes/arm: Improve kprobes implementation on arm

[Jon Medhurst (Tixy)]

On Wed, 2017-02-15 at 09:27 +0900, Masami Hiramatsu wrote:
> Hi,
> 
> Here is the 2nd version of the patches which improve kprobe
> on arm implementation (a kind of bugfix). Version 1 is here;
> 
> https://lkml.org/lkml/2017/2/13/538
> 
> In this version I didn't update the code, just update the
> patch description according to Tixy's comment and add his Ack.
> 
> Thank you,
> 
> ---
> 
> Masami Hiramatsu (3):
>       kprobes/arm: Allow to handle reentered kprobe on single-stepping
>       kprobes/arm: Skip single-stepping in recursing path if possible
>       kprobes/arm: Fix the return address of multiple kretprobes
> 

Thanks for doing these. Am I correct in assuming we don't need to
consider these fixes urgent or critical? Only the first looks like it
could be serious, and the x86 fix for that is 3 years old and ARM has
gone without it all this time. So I'm guessing it's fine to wait for the
normal development process and deal with it after the about to open
merge window is completed?

If so, I propose that I put the patches in a branch for Russell to pull
later (unless he pipes up with objections or says otherwise). Meantime
I'll investigate the kprobes test failures I see (which actually looks
like cache/TLB issues and not test code problems after all).

BTW, I added the ARM kernel list to the CC. I spotted you didn't add it
to you patch postings, which means people interested in ARM (other than
Russell) wouldn't have seen them.

Thanks

-- 
Tixy

Re: [BUGFIX PATCH V2 0/3] kprobes/arm: Improve kprobes implementation on arm

[Masami Hiramatsu]

On Fri, 17 Feb 2017 11:01:10 +0000
"Jon Medhurst (Tixy)" <tixy@linaro.org> wrote:

> On Wed, 2017-02-15 at 09:27 +0900, Masami Hiramatsu wrote:
> > Hi,
> > 
> > Here is the 2nd version of the patches which improve kprobe
> > on arm implementation (a kind of bugfix). Version 1 is here;
> > 
> > https://lkml.org/lkml/2017/2/13/538
> > 
> > In this version I didn't update the code, just update the
> > patch description according to Tixy's comment and add his Ack.
> > 
> > Thank you,
> > 
> > ---
> > 
> > Masami Hiramatsu (3):
> >       kprobes/arm: Allow to handle reentered kprobe on single-stepping
> >       kprobes/arm: Skip single-stepping in recursing path if possible
> >       kprobes/arm: Fix the return address of multiple kretprobes
> > 
> 
> Thanks for doing these. Am I correct in assuming we don't need to
> consider these fixes urgent or critical? Only the first looks like it
> could be serious, and the x86 fix for that is 3 years old and ARM has
> gone without it all this time. So I'm guessing it's fine to wait for the
> normal development process and deal with it after the about to open
> merge window is completed?

Agreed. I'm not sure how frequently FIQ is used in ARM, but anyway
it happens only when root user intensively uses kprobes on FIQ handlers.

> If so, I propose that I put the patches in a branch for Russell to pull
> later (unless he pipes up with objections or says otherwise). Meantime
> I'll investigate the kprobes test failures I see (which actually looks
> like cache/TLB issues and not test code problems after all).

OK, btw, I couldn't reproduce the kprobes test failure with
CONFIG_DEBUG_RODATA=y on qemu...

> 
> BTW, I added the ARM kernel list to the CC. I spotted you didn't add it
> to you patch postings, which means people interested in ARM (other than
> Russell) wouldn't have seen them.

Ah, I forgot that, Thank you!

> 
> Thanks
> 
> -- 
> Tixy


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [BUGFIX PATCH V2 0/3] kprobes/arm: Improve kprobes implementation on arm

[Jon Medhurst (Tixy)]

On Sat, 2017-02-18 at 06:43 +0900, Masami Hiramatsu wrote:
> On Fri, 17 Feb 2017 11:01:10 +0000
> "Jon Medhurst (Tixy)" <tixy@linaro.org> wrote:
[...]
> > Meantime
> > I'll investigate the kprobes test failures I see (which actually looks
> > like cache/TLB issues and not test code problems after all).
> 
> OK, btw, I couldn't reproduce the kprobes test failure with
> CONFIG_DEBUG_RODATA=y on qemu...

That's because it's a problem with caches which qemu doesn't emulate.
I worked out the cause and posted a patch...
http://lists.infradead.org/pipermail/linux-arm-kernel/2017-February/489517.html

-- 
Tixy

Re: [BUGFIX PATCH V2 0/3] kprobes/arm: Improve kprobes implementation on arm

[Masami Hiramatsu]

On Wed, 22 Feb 2017 15:58:49 +0000
"Jon Medhurst (Tixy)" <tixy@linaro.org> wrote:

> On Sat, 2017-02-18 at 06:43 +0900, Masami Hiramatsu wrote:
> > On Fri, 17 Feb 2017 11:01:10 +0000
> > "Jon Medhurst (Tixy)" <tixy@linaro.org> wrote:
> [...]
> > > Meantime
> > > I'll investigate the kprobes test failures I see (which actually looks
> > > like cache/TLB issues and not test code problems after all).
> > 
> > OK, btw, I couldn't reproduce the kprobes test failure with
> > CONFIG_DEBUG_RODATA=y on qemu...
> 
> That's because it's a problem with caches which qemu doesn't emulate.
> I worked out the cause and posted a patch...
> http://lists.infradead.org/pipermail/linux-arm-kernel/2017-February/489517.html

OK, I see.

Thanks! 

-- 
Masami Hiramatsu <mhiramat@kernel.org>