Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   "Eddy_Wu@trendmicro.com" <Eddy_Wu@trendmicro.com>
To:     Masami Hiramatsu <mhiramat@kernel.org>
CC:     Peter Zijlstra <peterz@infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "x86@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)
Thread-Topic: x86/kprobes: kretprobe fails to triggered if kprobe at function
 entry is not optimized (trigger by int3 breakpoint)
Thread-Index: AdZ5/y9ucVi7Kf0NSwa8vnc6Q/ol0wAL6hEAAAENaQA=
Date:   Mon, 24 Aug 2020 16:41:58 +0000
Message-ID: <7396e7b2079644a6aafd9670a111232b@trendmicro.com>
References: <8816bdbbc55c4d2397e0b02aad2825d3@trendmicro.com>
 <20200825005426.f592075d13be740cb3c9aa77@kernel.org>
In-Reply-To: <20200825005426.f592075d13be740cb3c9aa77@kernel.org>
Accept-Language: zh-TW, en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

> -----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 funct=
ion entry is not optimized (trigger by int3 breakpoint)
>
>
> This message was sent from outside of Trend Micro. Please do not click li=
nks 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 no=
ticed that.
>
> > Step to reproduce this:
> > 1) Build the kretprobe example module (CONFIG_SAMPLE_KRETPROBES=3Dm)
> > 2) Disable jump optimization (`sysctl debug.kprobes-optimization=3D0` o=
r 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=3D0
> > debug.kprobes-optimization =3D 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 commi=
t: (5.8-rc1)
> > 0d00449c7a28a1514595630735df383dec606812 x86: Replace ist_enter() with =
nmi_enter()
> > This make kprobe_int3_handler() effectively running as NMI context, whi=
ch 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 repla=
ce the in_nmi() check with in_nmi() =3D=3D (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 r=
eturns !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 chan=
ge 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 =3D=3D 1 (if entry =
kprobe is jump-optimized).
The arch- dependent weak function looks cleaner than doing this in kprobe_i=
nt3_handler() under x86/, but I don't know if there is a way to check if ca=
lled 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 =3D KPROBE_HIT_ACTIVE;

+if (p->pre_handler =3D=3D pre_handler_kretprobe && in_nmi() !=3D (1 << NMI=
_SHIFT)) {
+struct kretprobe *rp =3D 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 *e=
ntry)
  * 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 =3D container_of(p, struct kretprobe, kp);
 unsigned long hash, flags =3D 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 =3D 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 di=
sclose 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.trendm=
icro.com/privacy>