Re: x86/kprobes: kretprobe fails to triggered if kprobe at function entry is not optimized (trigger by int3 breakpoint)

[Masami Hiramatsu <mhiramat@kernel.org>]

Hi Eddy,

On Mon, 24 Aug 2020 16:41:58 +0000
"Eddy_Wu@trendmicro.com" <Eddy_Wu@trendmicro.com> wrote:

> > -----Original Message-----
> > From: Masami Hiramatsu <mhiramat@kernel.org>
> > Sent: Monday, August 24, 2020 11:54 PM
> > To: Eddy Wu (RD-TW) <Eddy_Wu@trendmicro.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>; linux-kernel@vger.kernel.org; x86@kernel.org; David S. Miller <davem@davemloft.net>
> > Subject: Re: x86/kprobes: kretprobe fails to triggered if kprobe at function entry is not optimized (trigger by int3 breakpoint)
> >
> >
> > This message was sent from outside of Trend Micro. Please do not click links or open attachments unless you recognise the source of this
> > email and know the content is safe.
> >
> >
> > On Mon, 24 Aug 2020 12:02:58 +0000
> > "Eddy_Wu@trendmicro.com" <Eddy_Wu@trendmicro.com> wrote:
> >
> > > Greetings!
> > >
> > > Starting from kernel 5.8 (x86_64), kretprobe handler will always missed if corresponding kprobe on function entry is not optimized
> > (using break point instead).
> >
> > Oops, good catch. I always enabled ftrace hook for kretprobe, I didn't noticed that.
> >
> > > Step to reproduce this:
> > > 1) Build the kretprobe example module (CONFIG_SAMPLE_KRETPROBES=m)
> > > 2) Disable jump optimization (`sysctl debug.kprobes-optimization=0` or register any kprobe.post_handler at same location)
> > > 3) Insert the kretprobe_example module
> > > 4) Launch some process to trigger _do_fork
> > > 5) Remove kretprobe_example module
> > > 6) dmesg shows that all probing instances are missed
> > >
> > > Example output:
> > > # sysctl debug.kprobes-optimization=0
> > > debug.kprobes-optimization = 0
> > > # insmod samples/kprobes/kretprobe_example.ko
> > > # ls > /dev/null
> > > # rmmod kretprobe_example
> > > # dmesg
> > > [48555.067295] Planted return probe at _do_fork: 0000000038ae0211
> > > [48560.229459] kretprobe at 0000000038ae0211 unregistered
> > > [48560.229460] Missed probing 3 instances of _do_fork
> > >
> > > After bisecting, I found this behavior seems to introduce by this commit: (5.8-rc1)
> > > 0d00449c7a28a1514595630735df383dec606812 x86: Replace ist_enter() with nmi_enter()
> > > This make kprobe_int3_handler() effectively running as NMI context, which pre_handler_kretprobe() explicitly checked to prevent
> > recursion.
> >
> > Thanks for the bisecting!
> >
> > >
> > > (in_nmi() check appears from v3.17)
> > > f96f56780ca584930bb3a2769d73fd9a101bcbbe kprobes: Skip kretprobe hit in NMI context to avoid deadlock
> > >
> > > To make kretprobe work again with int3 breakpoint, I think we can replace the in_nmi() check with in_nmi() == (1 << NMI_SHIFT) at
> > kprobe_int3_handler() and skip kretprobe if nested NMI.
> >
> > Ah, I see. Now int3 is a kind of NMI, so in the handler in_nmi() always returns !0.
> >
> > > Did a quick test on 5.9-rc2 and it seems to be working.
> > > I'm not sure if it is the best way to do since it may also require change to other architecture as well, any thought?
> >
> > Hmm, this behavior is arch-dependent. So I think we need an weak function like this.
> >
> > @kernel/kprobes.c
> >
> > bool __weak arch_kprobe_in_nmi(void)
> > {
> >         return in_nmi()
> > }
> >
> > @arch/x86/kernel/kprobes/core.c
> >
> > bool arch_kprobe_in_nmi(void)
> > {
> >        /*
> >         * Since the int3 is one of NMI, we have to check in_nmi() is
> >         * bigger than 1 << NMI_SHIFT instead of !0.
> >         */
> >        return in_nmi() > (1 << NMI_SHIFT);
> > }
> >
> > And use arch_kprobe_in_nmi() instead of in_nmi() in kprobes.c.
> >
> > Thanks,
> >
> > --
> > Masami Hiramatsu <mhiramat@kernel.org>
> 
> Kretprobe might still trigger from NMI with nmi counter == 1 (if entry kprobe is jump-optimized).

Ah, right. Hmm, in that case, we can store the int3 status in 
the kprobe_ctlblk and refer it in the handler.


> The arch- dependent weak function looks cleaner than doing this in kprobe_int3_handler() under x86/, but I don't know if there is a way to check if called by specific int3 handler or not.
> 
> My original patch below, need to change all architecture support kretprobe though

OK, here is my fix. This will not change the other arches. please try it.


From 24390dffe6eb9a3e95f7d46a528a1dcfd716dc81 Mon Sep 17 00:00:00 2001
From: Masami Hiramatsu <mhiramat@kernel.org>
Date: Tue, 25 Aug 2020 01:37:00 +0900
Subject: [PATCH] kprobes/x86: Fixes NMI context check on x86

Since commit 0d00449c7a28 ("x86: Replace ist_enter() with nmi_enter()")
made int3 as one of NMI, in_nmi() in kprobe handlers always returns !0.
Thus the kretprobe handlers always skipped the execution on x86 if it
is using int3. (CONFIG_KPROBES_ON_FTRACE=n and
echo 0 > /proc/sys/debug/kprobe_optimization)

To avoid this issue, introduce arch_kprobe_in_nmi() and check the
in_nmi() count is bigger than 1 << NMI_SHIFT on x86 if the handler
has been invoked from kprobe_int3_handler. By default, the
arch_kprobe_in_nmi() will be same as in_nmi().

Fixes: 0d00449c7a28 ("x86: Replace ist_enter() with nmi_enter()")
Reported-by: Eddy Wu <Eddy_Wu@trendmicro.com>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Cc: stable@vger.kernel.org
---
 arch/x86/include/asm/kprobes.h |  1 +
 arch/x86/kernel/kprobes/core.c | 18 ++++++++++++++++++
 kernel/kprobes.c               |  8 +++++++-
 3 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/kprobes.h b/arch/x86/include/asm/kprobes.h
index 143bc9abe99c..ddb24feb95ad 100644
--- a/arch/x86/include/asm/kprobes.h
+++ b/arch/x86/include/asm/kprobes.h
@@ -98,6 +98,7 @@ struct kprobe_ctlblk {
 	unsigned long kprobe_old_flags;
 	unsigned long kprobe_saved_flags;
 	struct prev_kprobe prev_kprobe;
+	bool 	in_int3;
 };
 
 extern int kprobe_fault_handler(struct pt_regs *regs, int trapnr);
diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 2ca10b770cff..649d467c8231 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -583,6 +583,20 @@ static nokprobe_inline void restore_btf(void)
 	}
 }
 
+bool arch_kprobe_in_nmi(void)
+{
+	struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
+
+	if (kcb->in_int3) {
+		/*
+		 * Since the int3 is one of NMI, we have to check in_nmi() is
+		 * bigger than 1 << NMI_SHIFT instead of !0.
+		 */
+		return in_nmi() > (1 << NMI_SHIFT);
+	} else
+		return in_nmi();
+}
+
 void arch_prepare_kretprobe(struct kretprobe_instance *ri, struct pt_regs *regs)
 {
 	unsigned long *sara = stack_addr(regs);
@@ -697,6 +711,7 @@ int kprobe_int3_handler(struct pt_regs *regs)
 				return 1;
 		} else {
 			set_current_kprobe(p, regs, kcb);
+			kcb->in_int3 = true;
 			kcb->kprobe_status = KPROBE_HIT_ACTIVE;
 
 			/*
@@ -710,6 +725,7 @@ int kprobe_int3_handler(struct pt_regs *regs)
 				setup_singlestep(p, regs, kcb, 0);
 			else
 				reset_current_kprobe();
+			kcb->in_int3 = false;
 			return 1;
 		}
 	} else if (*addr != INT3_INSN_OPCODE) {
@@ -994,7 +1010,9 @@ int kprobe_debug_handler(struct pt_regs *regs)
 
 	if ((kcb->kprobe_status != KPROBE_REENTER) && cur->post_handler) {
 		kcb->kprobe_status = KPROBE_HIT_SSDONE;
+		kcb->in_int3 = true;
 		cur->post_handler(cur, regs, 0);
+		kcb->in_int3 = false;
 	}
 
 	/* Restore back the original saved kprobes variables and continue. */
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 287b263c9cb9..9564928fb882 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1927,6 +1927,12 @@ unsigned long __weak arch_deref_entry_point(void *entry)
 }
 
 #ifdef CONFIG_KRETPROBES
+
+bool __weak arch_kprobe_in_nmi(void)
+{
+	return in_nmi();
+}
+
 /*
  * This kprobe pre_handler is registered with every kretprobe. When probe
  * hits it will set up the return probe.
@@ -1943,7 +1949,7 @@ static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
 	 * statistical counter, so that the user is informed that
 	 * something happened:
 	 */
-	if (unlikely(in_nmi())) {
+	if (unlikely(arch_kprobe_in_nmi())) {
 		rp->nmissed++;
 		return 0;
 	}
-- 
2.25.1


-- 
Masami Hiramatsu <mhiramat@kernel.org>