lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 25 Aug 2020 15:15:38 +0900
From:   Masami Hiramatsu <mhiramat@...nel.org>
To:     "Eddy_Wu@...ndmicro.com" <Eddy_Wu@...ndmicro.com>
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)

Hi Eddy,

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

> > -----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).

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@...nel.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@...ndmicro.com>
Signed-off-by: Masami Hiramatsu <mhiramat@...nel.org>
Cc: stable@...r.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@...nel.org>

Powered by blists - more mailing lists