Message-ID: <7396e7b2079644a6aafd9670a111232b@trendmicro.com>
Date:   Mon, 24 Aug 2020 16:41:58 +0000
From:   "Eddy_Wu@...ndmicro.com" <Eddy_Wu@...ndmicro.com>
To:     Masami Hiramatsu <mhiramat@...nel.org>
CC:     Peter Zijlstra <peterz@...radead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "x86@...nel.org" <x86@...nel.org>,
        "David S. Miller" <davem@...emloft.net>
Subject: RE: x86/kprobes: kretprobe fails to triggered if kprobe at function
 entry is not optimized (trigger by int3 breakpoint)

> -----Original Message-----
> From: Masami Hiramatsu <mhiramat@...nel.org>
> Sent: Monday, August 24, 2020 11:54 PM
> To: Eddy Wu (RD-TW) <Eddy_Wu@...ndmicro.com>
> Cc: Peter Zijlstra <peterz@...radead.org>; linux-kernel@...r.kernel.org; x86@...nel.org; David S. Miller <davem@...emloft.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@...ndmicro.com" <Eddy_Wu@...ndmicro.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@...nel.org>

Kretprobe might still trigger from NMI with nmi counter == 1 (if entry kprobe is jump-optimized).
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

Thanks

---
 arch/x86/kernel/kprobes/core.c |  6 ++++++
 include/linux/kprobes.h        |  1 +
 kernel/kprobes.c               | 13 +------------
 3 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index fdadc37d72af..1b785aef85ef 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -699,6 +699,12 @@ int kprobe_int3_handler(struct pt_regs *regs)
 set_current_kprobe(p, regs, kcb);
 kcb->kprobe_status = KPROBE_HIT_ACTIVE;

+if (p->pre_handler == pre_handler_kretprobe && in_nmi() != (1 << NMI_SHIFT)) {
+struct kretprobe *rp = container_of(p, struct kretprobe, kp);
+rp->nmissed++;
+setup_singlestep(p, regs, kcb, 0);
+return 1;
+}
 /*
  * If we have no pre-handler or it returned 0, we
  * continue with normal processing.  If we have a
diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index 9be1bff4f586..3ded8e46ada5 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -494,5 +494,6 @@ static nokprobe_inline bool kprobe_page_fault(struct pt_regs *regs,
 return false;
 return kprobe_fault_handler(regs, trap);
 }
+int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs);

 #endif /* _LINUX_KPROBES_H */
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 287b263c9cb9..0f4d61613ded 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1931,23 +1931,12 @@ unsigned long __weak arch_deref_entry_point(void *entry)
  * This kprobe pre_handler is registered with every kretprobe. When probe
  * hits it will set up the return probe.
  */
-static int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
+int pre_handler_kretprobe(struct kprobe *p, struct pt_regs *regs)
 {
 struct kretprobe *rp = container_of(p, struct kretprobe, kp);
 unsigned long hash, flags = 0;
 struct kretprobe_instance *ri;

-/*
- * To avoid deadlocks, prohibit return probing in NMI contexts,
- * just skip the probe and increase the (inexact) 'nmissed'
- * statistical counter, so that the user is informed that
- * something happened:
- */
-if (unlikely(in_nmi())) {
-rp->nmissed++;
-return 0;
-}
-
 /* TODO: consider to only swap the RA after the last pre_handler fired */
 hash = hash_ptr(current, KPROBE_HASH_BITS);
 raw_spin_lock_irqsave(&rp->lock, flags);
--
2.17.1

TREND MICRO EMAIL NOTICE

The information contained in this email and any attachments is confidential and may be subject to copyright or other intellectual property protection. If you are not the intended recipient, you are not authorized to use or disclose this information, and we request that you notify us by reply mail or telephone and delete the original message from your mail system.

For details about what personal information we collect and why, please see our Privacy Notice on our website at: Read privacy policy<http://www.trendmicro.com/privacy>

Powered by blists - more mailing lists