Hi Eddy,
On Mon, 24 Aug 2020 16:41:58 +0000
"eddy...@trendmicro.com" <eddy...@trendmicro.com> wrote:
> > -----Original Message-----
> > From: Masami Hiramatsu <mhira...@kernel.org>
> > Sent: Monday, August 24, 2020 11:54 PM
> > To: Eddy Wu (RD-TW) <eddy...@trendmicro.com>
> > Cc: Peter Zijlstra <pet...@infradead.org>; linux-kernel@vger.kernel.org;
> > x...@kernel.org; David S. Miller <da...@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...@trendmicro.com" <eddy...@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 <mhira...@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 <mhira...@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...@trendmicro.com>
Signed-off-by: Masami Hiramatsu <mhira...@kernel.org>
Cc: sta...@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 <mhira...@kernel.org>
