[BUGFIX PATCH tip/master V2 0/3] kprobes: Fix a possible deadlock in kretprobe

[BUGFIX PATCH tip/master V2 0/3] kprobes: Fix a possible deadlock in kretprobe

[Masami Hiramatsu]

Hi,

Here is 2nd version of the series. I just updated the
patch description to make it easier to understand
on arm and arm64, no code change.

V1 is here:
http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1327856.html

----
This series will fix a possible deadlock case in kretprobe
on x86, arm, arm64. Since kretprobe has been optimized on
those arch, they have similar possible deadlock issue.

Problem
=====

The deadlock senario is when a user puts 2 kretprobes, 
one on normal function and one on a function which can be
called from NMI or FIQ where normal interrupt disabled.
(we don't recommend it, but possible.) In this case, if
the kernel hits the 1st kretprobe on a normal function
return which calls trampoline_handler(), acquire a 
spinlock on the hash table in kretprobe_hash_lock() and
disable irqs.
After that, if NMI(or FIQ on arm/arm64) is occurred and
the 2nd kretprobe is kicked, it also calls
trampoline_handler() and tries to acquire the same 
spinlock (since the hash is based on current task, same
as the 1st kretprobe), it causes a deadlock on the
spinlock.
Note that this is very rare case, but theoretically happens.

Reason and Affected Arch
=====

Actually, this bug has been introduced by kretprobe-booster,
which removes a kprobe from return trampoline code, but also
resets current kprobe, which can be a stopper for the nested
k(ret)probes. So, currently only x86, arm, and arm64 are
affected, because other arch have not implemented the
kretprobe-booster.

Solution
=====

To fix this issue, I introduced a dummy kprobe which is set
as a current kprobe while holding the kretprobe-hash lock.
With that, if an NMI/FIQ occurred and 2nd kretprobe's kprobe
is kicked (to modify the return address, a kprobe is kicked
when the target function is called), the kprobe (and the 2nd
kretprobe also) is skipped because it detects there is
another kprobe is running.
    
This reentrance detection and nested kprobe blocker had
existed when the original kretprobe was implemented by
using kprobe on trampoline code. This fix just revived it.

Thank you,

---

Masami Hiramatsu (3):
      kprobes/x86: Fix a possible deadlock case in kretprobe
      kprobes/arm64: Fix a possible deadlock case in kretprobe
      kprobes/arm: Fix a possible deadlock case in kretprobe


 arch/arm/probes/kprobes/core.c     |   12 ++++++++++--
 arch/arm64/kernel/probes/kprobes.c |   12 ++++++++++--
 arch/x86/kernel/kprobes/core.c     |   13 ++++++++++---
 3 files changed, 30 insertions(+), 7 deletions(-)

--
Masami Hiramatsu

[BUGFIX PATCH tip/master V2 1/3] kprobes/x86: Fix a possible deadlock case in kretprobe

[Masami Hiramatsu]

Fix a possibility of deadlock case in kretprobe on x86
implementation. There is a small chance that the kretprobe
hash table lock can cause a dead lock.

The senario is that a user puts 2 kretprobes, one on normal
function and one on a function which can be called from NMI
(we don't recommend it, but possible). In this case, if the
kernel hits the 1st kretprobe on a normal function return
which calls trampoline_handler(), acquires a spinlock on
the hash table in kretprobe_hash_lock() and disables irqs.
After that, if NMI is occurred and the 2nd kretprobe is kicked,
it also calls trampoline_handler() and tries to acquire the
same spinlock (since the hash is based on current task,
same as the 1st kretprobe), it causes a deadlock.
Note that this is very rare case, but theoretically happens.

Actually, this bug has been introduced by kretprobe-booster,
which removes a kprobe from return trampoline code, but also
resets current kprobe, which can be a stopper for the nested
k(ret)probes.

To fix this issue, I introduced a dummy kprobe which is set
as a current kprobe while holding the kretprobe-hash lock.
With that, if an NMI occurred and 2nd kretprobe's kprobe
is kicked (to modify the return address, a kprobe is kicked
when the target function is called), the kprobe (and the 2nd
kretprobe also) is skipped because it detects there is
another kprobe is running.

This reentrance detection and nested kprobe blocker had
existed when the original kretprobe was implemented by
using kprobe on trampoline code. This fixes just revived it.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/x86/kernel/kprobes/core.c |   13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 6384eb7..6aaabe1 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -709,6 +709,8 @@ asm(
 NOKPROBE_SYMBOL(kretprobe_trampoline);
 STACK_FRAME_NON_STANDARD(kretprobe_trampoline);
 
+static struct kprobe dummy_retprobe = {.addr = (void *)&kretprobe_trampoline};
+
 /*
  * Called from kretprobe_trampoline
  */
@@ -722,7 +724,6 @@ __visible __used void *trampoline_handler(struct pt_regs *regs)
 	kprobe_opcode_t *correct_ret_addr = NULL;
 
 	INIT_HLIST_HEAD(&empty_rp);
-	kretprobe_hash_lock(current, &head, &flags);
 	/* fixup registers */
 #ifdef CONFIG_X86_64
 	regs->cs = __KERNEL_CS;
@@ -733,6 +734,11 @@ __visible __used void *trampoline_handler(struct pt_regs *regs)
 	regs->ip = trampoline_address;
 	regs->orig_ax = ~0UL;
 
+	/* This prevents kernel to change running cpu while processing */
+	preempt_disable();
+	get_kprobe_ctlblk()->kprobe_status = KPROBE_HIT_ACTIVE;
+	__this_cpu_write(current_kprobe, &dummy_retprobe);
+	kretprobe_hash_lock(current, &head, &flags);
 	/*
 	 * It is possible to have multiple instances associated with a given
 	 * task either because multiple functions in the call path have
@@ -773,10 +779,9 @@ __visible __used void *trampoline_handler(struct pt_regs *regs)
 		orig_ret_address = (unsigned long)ri->ret_addr;
 		if (ri->rp && ri->rp->handler) {
 			__this_cpu_write(current_kprobe, &ri->rp->kp);
-			get_kprobe_ctlblk()->kprobe_status = KPROBE_HIT_ACTIVE;
 			ri->ret_addr = correct_ret_addr;
 			ri->rp->handler(ri, regs);
-			__this_cpu_write(current_kprobe, NULL);
+			__this_cpu_write(current_kprobe, &dummy_retprobe);
 		}
 
 		recycle_rp_inst(ri, &empty_rp);
@@ -791,6 +796,8 @@ __visible __used void *trampoline_handler(struct pt_regs *regs)
 	}
 
 	kretprobe_hash_unlock(current, &flags);
+	__this_cpu_write(current_kprobe, NULL);
+	preempt_enable_no_resched();
 
 	hlist_for_each_entry_safe(ri, tmp, &empty_rp, hlist) {
 		hlist_del(&ri->hlist);

[BUGFIX PATCH tip/master V2 2/3] kprobes/arm64: Fix a possible deadlock case in kretprobe

[Masami Hiramatsu]

Fix a possibility of deadlock case in kretprobe on arm64
implementation. There may be a chance that the kretprobe
hash table lock can cause a dead lock.

The senario is that a user puts 2 kretprobes, one on normal
function and one on a function which can be called from
somewhare which can interrupt in irq disabled critical
section like hw-breakpoint, FIQ in the future, etc.
(At this point those code should be protected from kprobes.)

In this case, if the kernel hits the 1st kretprobe on a
normal function return which calls trampoline_handler(),
acquire a spinlock on the hash table in kretprobe_hash_lock()
and disable irqs. After that, if the 2nd kretprobe is kicked,
it also calls trampoline_handler() and tries to acquire the
same spinlock (since the hash is based on current task,
same as the 1st kretprobe), it causes a deadlock.
Note that again, this must not happen at this moment, but
if we support FIQ, it can happen.

Actually, this bug has been introduced by kretprobe-booster
which removes a kprobe from return trampoline code, but also
resets current kprobe, which can be a stopper for the nested
k(ret)probes. On arm64, kretprobe-booster has been ported
from x86.

To fix this issue, I introduced a dummy kprobe which is set
as a current kprobe while holding the kretprobe-hash lock.
With that, if the 2nd kretprobe's kprobe is kicked
(to modify the return address, a kprobe is kicked when
the target function is called), that kprobe (and the 2nd
kretprobe also) is skipped because it detects there is
another kprobe is running.

This reentrance detection and nested kprobe blocker had
existed when the original kretprobe on x86 was implemented by
using a kprobe on trampoline code. This fixes just revived it.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/arm64/kernel/probes/kprobes.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
index 2a07aae..401f7c9 100644
--- a/arch/arm64/kernel/probes/kprobes.c
+++ b/arch/arm64/kernel/probes/kprobes.c
@@ -561,6 +561,8 @@ bool arch_within_kprobe_blacklist(unsigned long addr)
 	return false;
 }
 
+static struct kprobe dummy_retprobe = {.addr = (void *)&kretprobe_trampoline};
+
 void __kprobes __used *trampoline_probe_handler(struct pt_regs *regs)
 {
 	struct kretprobe_instance *ri = NULL;
@@ -572,6 +574,11 @@ void __kprobes __used *trampoline_probe_handler(struct pt_regs *regs)
 	kprobe_opcode_t *correct_ret_addr = NULL;
 
 	INIT_HLIST_HEAD(&empty_rp);
+
+	/* This prevents kernel to change running cpu while processing */
+	preempt_disable();
+	get_kprobe_ctlblk()->kprobe_status = KPROBE_HIT_ACTIVE;
+	__this_cpu_write(current_kprobe, &dummy_retprobe);
 	kretprobe_hash_lock(current, &head, &flags);
 
 	/*
@@ -614,10 +621,9 @@ void __kprobes __used *trampoline_probe_handler(struct pt_regs *regs)
 		orig_ret_address = (unsigned long)ri->ret_addr;
 		if (ri->rp && ri->rp->handler) {
 			__this_cpu_write(current_kprobe, &ri->rp->kp);
-			get_kprobe_ctlblk()->kprobe_status = KPROBE_HIT_ACTIVE;
 			ri->ret_addr = correct_ret_addr;
 			ri->rp->handler(ri, regs);
-			__this_cpu_write(current_kprobe, NULL);
+			__this_cpu_write(current_kprobe, &dummy_retprobe);
 		}
 
 		recycle_rp_inst(ri, &empty_rp);
@@ -632,6 +638,8 @@ void __kprobes __used *trampoline_probe_handler(struct pt_regs *regs)
 	}
 
 	kretprobe_hash_unlock(current, &flags);
+	__this_cpu_write(current_kprobe, NULL);
+	preempt_enable_no_resched();
 
 	hlist_for_each_entry_safe(ri, tmp, &empty_rp, hlist) {
 		hlist_del(&ri->hlist);

[BUGFIX PATCH tip/master V2 3/3] kprobes/arm: Fix a possible deadlock case in kretprobe

[Masami Hiramatsu]

Fix a possibility of deadlock case in kretprobe on arm
implementation. There may be a chance that the kretprobe
hash table lock can cause a dead lock.

The senario is that a user puts 2 kretprobes, one on normal
function and one on a function which can be called from
somewhare which can interrupt in irq disabled critical
section like FIQ.

In this case, if the kernel hits the 1st kretprobe on a
normal function return which calls trampoline_handler(),
acquire a spinlock on the hash table in kretprobe_hash_lock()
and disable irqs. After that, if the 2nd kretprobe is kicked
from FIQ, it also calls trampoline_handler() and tries to
acquire the same spinlock (since the hash is based on
current task, same as the 1st kretprobe), it causes
a deadlock.

Actually, this bug has been introduced by kretprobe-booster
which removes a kprobe from return trampoline code, but also
resets current kprobe, which can be a stopper for the nested
k(ret)probes.

To fix this issue, I introduced a dummy kprobe which is set
as a current kprobe while holding the kretprobe-hash lock.
With that, if the 2nd kretprobe's kprobe is kicked
(to modify the return address, a kprobe is kicked when
the target function is called), that kprobe (and the 2nd
kretprobe also) is skipped because it detects there is
another kprobe is running.

This reentrance detection and nested kprobe blocker had
existed when the original kretprobe was implemented by
using a kprobe on trampoline code. This fixes just revived it.


Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/arm/probes/kprobes/core.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/arm/probes/kprobes/core.c b/arch/arm/probes/kprobes/core.c
index b6dc9d8..3e5aab7 100644
--- a/arch/arm/probes/kprobes/core.c
+++ b/arch/arm/probes/kprobes/core.c
@@ -426,6 +426,8 @@ void __naked __kprobes kretprobe_trampoline(void)
 		: : : "memory");
 }
 
+static struct kprobe dummy_retprobe = {.addr = (void *)&kretprobe_trampoline};
+
 /* Called from kretprobe_trampoline */
 static __used __kprobes void *trampoline_handler(struct pt_regs *regs)
 {
@@ -436,6 +438,11 @@ static __used __kprobes void *trampoline_handler(struct pt_regs *regs)
 	unsigned long trampoline_address = (unsigned long)&kretprobe_trampoline;
 
 	INIT_HLIST_HEAD(&empty_rp);
+
+	/* This prevents kernel to change running cpu while processing */
+	preempt_disable();
+	get_kprobe_ctlblk()->kprobe_status = KPROBE_HIT_ACTIVE;
+	__this_cpu_write(current_kprobe, &dummy_retprobe);
 	kretprobe_hash_lock(current, &head, &flags);
 
 	/*
@@ -458,9 +465,8 @@ static __used __kprobes void *trampoline_handler(struct pt_regs *regs)
 
 		if (ri->rp && ri->rp->handler) {
 			__this_cpu_write(current_kprobe, &ri->rp->kp);
-			get_kprobe_ctlblk()->kprobe_status = KPROBE_HIT_ACTIVE;
 			ri->rp->handler(ri, regs);
-			__this_cpu_write(current_kprobe, NULL);
+			__this_cpu_write(current_kprobe, &dummy_retprobe);
 		}
 
 		orig_ret_address = (unsigned long)ri->ret_addr;
@@ -477,6 +483,8 @@ static __used __kprobes void *trampoline_handler(struct pt_regs *regs)
 
 	kretprobe_assert(ri, orig_ret_address, trampoline_address);
 	kretprobe_hash_unlock(current, &flags);
+	__this_cpu_write(current_kprobe, NULL);
+	preempt_enable_no_resched();
 
 	hlist_for_each_entry_safe(ri, tmp, &empty_rp, hlist) {
 		hlist_del(&ri->hlist);

Re: [BUGFIX PATCH tip/master V2 3/3] kprobes/arm: Fix a possible deadlock case in kretprobe

[Russell King - ARM Linux]

On Fri, Feb 10, 2017 at 01:32:22AM +0900, Masami Hiramatsu wrote:
> Fix a possibility of deadlock case in kretprobe on arm
> implementation. There may be a chance that the kretprobe
> hash table lock can cause a dead lock.
> 
> The senario is that a user puts 2 kretprobes, one on normal
> function and one on a function which can be called from
> somewhare which can interrupt in irq disabled critical
> section like FIQ.

If we:
- hit a kernel tracing feature from FIQ context
- the tracing feature takes a lock
- the lock is also taken elsewhere on the same CPU with IRQs disabled

we will quite simply deadlock.

In this case, kretprobe_hash_lock() takes the hlist_lock using
raw_spin_lock_irqsave().

Now, from what I can see in the kprobes code, this lock is taken in
other contexts (eg, kprobe_flush_task()), which means even with this
fix, it's still risky if a kprobe is placed on a FIQ-called function.

> In this case, if the kernel hits the 1st kretprobe on a
> normal function return which calls trampoline_handler(),
> acquire a spinlock on the hash table in kretprobe_hash_lock()
> and disable irqs. After that, if the 2nd kretprobe is kicked
> from FIQ, it also calls trampoline_handler() and tries to
> acquire the same spinlock (since the hash is based on
> current task, same as the 1st kretprobe), it causes
> a deadlock.

So my deadlock scenario is:

- we're in the middle of kprobe_flush_task()
- FIQ happens, calls trampoline_handler()
- deadlock in kretprobe_hash_lock()

>From what I can see, kretprobes in FIQ are just unsafe.

I suspect that avoiding these deadlocks means that we have to deny
kprobes from FIQ context - making trampoline_handler() return
immediately if in_nmi() is true.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [BUGFIX PATCH tip/master V2 3/3] kprobes/arm: Fix a possible deadlock case in kretprobe

[Masami Hiramatsu]

On Thu, 9 Feb 2017 16:49:00 +0000
Russell King - ARM Linux <linux@armlinux.org.uk> wrote:

> On Fri, Feb 10, 2017 at 01:32:22AM +0900, Masami Hiramatsu wrote:
> > Fix a possibility of deadlock case in kretprobe on arm
> > implementation. There may be a chance that the kretprobe
> > hash table lock can cause a dead lock.
> > 
> > The senario is that a user puts 2 kretprobes, one on normal
> > function and one on a function which can be called from
> > somewhare which can interrupt in irq disabled critical
> > section like FIQ.
> 
> If we:
> - hit a kernel tracing feature from FIQ context
> - the tracing feature takes a lock
> - the lock is also taken elsewhere on the same CPU with IRQs disabled
> 
> we will quite simply deadlock.

Correct.

> In this case, kretprobe_hash_lock() takes the hlist_lock using
> raw_spin_lock_irqsave().
> 
> Now, from what I can see in the kprobes code, this lock is taken in
> other contexts (eg, kprobe_flush_task()), which means even with this
> fix, it's still risky if a kprobe is placed on a FIQ-called function.

Oops, right! I'll fix that too. Thanks for pointed out.

> 
> > In this case, if the kernel hits the 1st kretprobe on a
> > normal function return which calls trampoline_handler(),
> > acquire a spinlock on the hash table in kretprobe_hash_lock()
> > and disable irqs. After that, if the 2nd kretprobe is kicked
> > from FIQ, it also calls trampoline_handler() and tries to
> > acquire the same spinlock (since the hash is based on
> > current task, same as the 1st kretprobe), it causes
> > a deadlock.
> 
> So my deadlock scenario is:
> 
> - we're in the middle of kprobe_flush_task()
> - FIQ happens, calls trampoline_handler()
> - deadlock in kretprobe_hash_lock()
> 
> From what I can see, kretprobes in FIQ are just unsafe.

Yes, NMI on x86 too.

> I suspect that avoiding these deadlocks means that we have to deny
> kprobes from FIQ context - making trampoline_handler() return
> immediately if in_nmi() is true.

Ah, in_nmi() means FIQ on arm :)
OK, but actually it is too late to check it in the enter of
trampoline_handler() since we don't know where is the real
return address at that point. So I'll check that in setup site
- kretprobe_pre_handler().

Thank you!


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [BUGFIX PATCH tip/master V2 3/3] kprobes/arm: Fix a possible deadlock case in kretprobe

[Masami Hiramatsu]

On Fri, 10 Feb 2017 11:34:45 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> On Thu, 9 Feb 2017 16:49:00 +0000
> Russell King - ARM Linux <linux@armlinux.org.uk> wrote:
> 
> > On Fri, Feb 10, 2017 at 01:32:22AM +0900, Masami Hiramatsu wrote:
> > > Fix a possibility of deadlock case in kretprobe on arm
> > > implementation. There may be a chance that the kretprobe
> > > hash table lock can cause a dead lock.
> > > 
> > > The senario is that a user puts 2 kretprobes, one on normal
> > > function and one on a function which can be called from
> > > somewhare which can interrupt in irq disabled critical
> > > section like FIQ.
> > 
> > If we:
> > - hit a kernel tracing feature from FIQ context
> > - the tracing feature takes a lock
> > - the lock is also taken elsewhere on the same CPU with IRQs disabled
> > 
> > we will quite simply deadlock.
> 
> Correct.
> 
> > In this case, kretprobe_hash_lock() takes the hlist_lock using
> > raw_spin_lock_irqsave().
> > 
> > Now, from what I can see in the kprobes code, this lock is taken in
> > other contexts (eg, kprobe_flush_task()), which means even with this
> > fix, it's still risky if a kprobe is placed on a FIQ-called function.
> 
> Oops, right! I'll fix that too. Thanks for pointed out.
> 
> > 
> > > In this case, if the kernel hits the 1st kretprobe on a
> > > normal function return which calls trampoline_handler(),
> > > acquire a spinlock on the hash table in kretprobe_hash_lock()
> > > and disable irqs. After that, if the 2nd kretprobe is kicked
> > > from FIQ, it also calls trampoline_handler() and tries to
> > > acquire the same spinlock (since the hash is based on
> > > current task, same as the 1st kretprobe), it causes
> > > a deadlock.
> > 
> > So my deadlock scenario is:
> > 
> > - we're in the middle of kprobe_flush_task()
> > - FIQ happens, calls trampoline_handler()
> > - deadlock in kretprobe_hash_lock()
> > 
> > From what I can see, kretprobes in FIQ are just unsafe.
> 
> Yes, NMI on x86 too.
> 
> > I suspect that avoiding these deadlocks means that we have to deny
> > kprobes from FIQ context - making trampoline_handler() return
> > immediately if in_nmi() is true.
> 
> Ah, in_nmi() means FIQ on arm :)
> OK, but actually it is too late to check it in the enter of
> trampoline_handler() since we don't know where is the real
> return address at that point. So I'll check that in setup site
> - kretprobe_pre_handler().

Hmm, pre_handler_kretprobe() already checked in_nmi().
So, I think this will no problem on FIQ too.

Thanks,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [BUGFIX PATCH tip/master V2 0/3] kprobes: Fix a possible deadlock in kretprobe

[Masami Hiramatsu]

On Fri, 10 Feb 2017 01:28:47 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Hi,
> 
> Here is 2nd version of the series. I just updated the
> patch description to make it easier to understand
> on arm and arm64, no code change.
> 
> V1 is here:
> http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1327856.html
> 
> ----
> This series will fix a possible deadlock case in kretprobe
> on x86, arm, arm64. Since kretprobe has been optimized on
> those arch, they have similar possible deadlock issue.
> 
> Problem
> =====
> 
> The deadlock senario is when a user puts 2 kretprobes, 
> one on normal function and one on a function which can be
> called from NMI or FIQ where normal interrupt disabled.
> (we don't recommend it, but possible.) In this case, if
> the kernel hits the 1st kretprobe on a normal function
> return which calls trampoline_handler(), acquire a 
> spinlock on the hash table in kretprobe_hash_lock() and
> disable irqs.
> After that, if NMI(or FIQ on arm/arm64) is occurred and
> the 2nd kretprobe is kicked, it also calls
> trampoline_handler() and tries to acquire the same 
> spinlock (since the hash is based on current task, same
> as the 1st kretprobe), it causes a deadlock on the
> spinlock.
> Note that this is very rare case, but theoretically happens.

Please ignore this series. I found that the kretprobe
already rejected probes in NMI (and FIQ) so this senario
never be true (as far as in_nmi() works in nested interrupt
context.)
Sorry for bother you.

And thank you!

> 
> Reason and Affected Arch
> =====
> 
> Actually, this bug has been introduced by kretprobe-booster,
> which removes a kprobe from return trampoline code, but also
> resets current kprobe, which can be a stopper for the nested
> k(ret)probes. So, currently only x86, arm, and arm64 are
> affected, because other arch have not implemented the
> kretprobe-booster.
> 
> Solution
> =====
> 
> To fix this issue, I introduced a dummy kprobe which is set
> as a current kprobe while holding the kretprobe-hash lock.
> With that, if an NMI/FIQ occurred and 2nd kretprobe's kprobe
> is kicked (to modify the return address, a kprobe is kicked
> when the target function is called), the kprobe (and the 2nd
> kretprobe also) is skipped because it detects there is
> another kprobe is running.
>     
> This reentrance detection and nested kprobe blocker had
> existed when the original kretprobe was implemented by
> using kprobe on trampoline code. This fix just revived it.
> 
> Thank you,
> 
> ---
> 
> Masami Hiramatsu (3):
>       kprobes/x86: Fix a possible deadlock case in kretprobe
>       kprobes/arm64: Fix a possible deadlock case in kretprobe
>       kprobes/arm: Fix a possible deadlock case in kretprobe
> 
> 
>  arch/arm/probes/kprobes/core.c     |   12 ++++++++++--
>  arch/arm64/kernel/probes/kprobes.c |   12 ++++++++++--
>  arch/x86/kernel/kprobes/core.c     |   13 ++++++++++---
>  3 files changed, 30 insertions(+), 7 deletions(-)
> 
> --
> Masami Hiramatsu


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [BUGFIX PATCH tip/master V2 3/3] kprobes/arm: Fix a possible deadlock case in kretprobe

[Russell King - ARM Linux]

On Sat, Feb 11, 2017 at 07:33:16AM +0900, Masami Hiramatsu wrote:
> On Fri, 10 Feb 2017 11:34:45 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > Ah, in_nmi() means FIQ on arm :)
> > OK, but actually it is too late to check it in the enter of
> > trampoline_handler() since we don't know where is the real
> > return address at that point. So I'll check that in setup site
> > - kretprobe_pre_handler().
> 
> Hmm, pre_handler_kretprobe() already checked in_nmi().
> So, I think this will no problem on FIQ too.

I don't blame you for missing that - the tracing and probes code is (at
least to me) quite a maze of code.

>From what I can tell, you're right - pre_handler_kretprobe() checks
in_nmi() early on, which prevents arch_prepare_kretprobe() (which
replaces regs->ARM_lr with the trampoline address) being run.  Hence,
the trampoline should not be run if we were entered in FIQ mode.

However, looking at kprobe_handler(), I'm much less convinced.  This is
called as a result of hitting a probe instruction via
kprobe_trap_handler().

Now, if we have two kprobes, one in non-FIQ context and one in FIQ
context, and the non-FIQ context one is hit, we set the current kprobe:

                } else if (p->ainsn.insn_check_cc(regs->ARM_cpsr)) {
                        /* Probe hit and conditional execution check ok. */
                        set_current_kprobe(p);
                        kcb->kprobe_status = KPROBE_HIT_ACTIVE;

and call the pre-handler (which succeeds.)  If we then take a FIQ and
hit a kprobe in a function called from FIQ, we will re-enter this
function.

In this case, "cur" will be the non-FIQ kprobe, and "p" will be the FIQ
kprobe.  It looks to me like we will single-step over the kprobe, and
resume.  However, it will modify the kprobe_status to KPROBE_REENTER,
which may not be desirable.

However, there does seem to be a hole.  Let's say that we have a similar
scenario, except that the FIQ is well-timed to happen:

                        if (!p->pre_handler || !p->pre_handler(p, regs)) {
                                kcb->kprobe_status = KPROBE_HIT_SS;
/* HERE */
                                singlestep(p, regs, kcb);
                                if (p->post_handler) {
                                        kcb->kprobe_status = KPROBE_HIT_SSDONE;

In that case:

                        /* Kprobe is pending, so we're recursing. */
                        switch (kcb->kprobe_status) {
                        case KPROBE_HIT_ACTIVE:
                        case KPROBE_HIT_SSDONE:
...
                        default:
                                /* impossible cases */
                                BUG();

becomes not such an "impossible case", so the kernel is likely to
explode.

This doesn't look good to me, and the pre-handler does nothing to
prevent this, so I still think we need some higher level protection in
kprobe_handler() against being entered in FIQ context - not only to
prevent that BUG() but also to prevent the kprobe status being changed
to "re-enter".

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [BUGFIX PATCH tip/master V2 3/3] kprobes/arm: Fix a possible deadlock case in kretprobe

[Masami Hiramatsu]

On Fri, 10 Feb 2017 23:21:13 +0000
Russell King - ARM Linux <linux@armlinux.org.uk> wrote:

> On Sat, Feb 11, 2017 at 07:33:16AM +0900, Masami Hiramatsu wrote:
> > On Fri, 10 Feb 2017 11:34:45 +0900
> > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > > Ah, in_nmi() means FIQ on arm :)
> > > OK, but actually it is too late to check it in the enter of
> > > trampoline_handler() since we don't know where is the real
> > > return address at that point. So I'll check that in setup site
> > > - kretprobe_pre_handler().
> > 
> > Hmm, pre_handler_kretprobe() already checked in_nmi().
> > So, I think this will no problem on FIQ too.
> 
> I don't blame you for missing that - the tracing and probes code is (at
> least to me) quite a maze of code.
> 
> From what I can tell, you're right - pre_handler_kretprobe() checks
> in_nmi() early on, which prevents arch_prepare_kretprobe() (which
> replaces regs->ARM_lr with the trampoline address) being run.  Hence,
> the trampoline should not be run if we were entered in FIQ mode.

Right.

> However, looking at kprobe_handler(), I'm much less convinced.  This is
> called as a result of hitting a probe instruction via
> kprobe_trap_handler().
> 
> Now, if we have two kprobes, one in non-FIQ context and one in FIQ
> context, and the non-FIQ context one is hit, we set the current kprobe:
> 
>                 } else if (p->ainsn.insn_check_cc(regs->ARM_cpsr)) {
>                         /* Probe hit and conditional execution check ok. */
>                         set_current_kprobe(p);
>                         kcb->kprobe_status = KPROBE_HIT_ACTIVE;
> 
> and call the pre-handler (which succeeds.)  If we then take a FIQ and
> hit a kprobe in a function called from FIQ, we will re-enter this
> function.
> 
> In this case, "cur" will be the non-FIQ kprobe, and "p" will be the FIQ
> kprobe.  It looks to me like we will single-step over the kprobe, and
> resume.  However, it will modify the kprobe_status to KPROBE_REENTER,
> which may not be desirable.
> 
> However, there does seem to be a hole.  Let's say that we have a similar
> scenario, except that the FIQ is well-timed to happen:
> 
>                         if (!p->pre_handler || !p->pre_handler(p, regs)) {
>                                 kcb->kprobe_status = KPROBE_HIT_SS;
> /* HERE */
>                                 singlestep(p, regs, kcb);
>                                 if (p->post_handler) {
>                                         kcb->kprobe_status = KPROBE_HIT_SSDONE;
> 
> In that case:
> 
>                         /* Kprobe is pending, so we're recursing. */
>                         switch (kcb->kprobe_status) {
>                         case KPROBE_HIT_ACTIVE:
>                         case KPROBE_HIT_SSDONE:
> ...
>                         default:
>                                 /* impossible cases */
>                                 BUG();
> 
> becomes not such an "impossible case", so the kernel is likely to
> explode.

OK, this one should be a bug on arm implementation.
On x86, we also check status == KPROBE_HIT_SS too, see reenter_kprobe()
at arch/x86/kernel/kprobes/core.c. (see commit 6a5022a56)
It seems same issue on arm64. I'll fix that.

> This doesn't look good to me, and the pre-handler does nothing to
> prevent this, so I still think we need some higher level protection in
> kprobe_handler() against being entered in FIQ context - not only to
> prevent that BUG() but also to prevent the kprobe status being changed
> to "re-enter".

What would you mean higher level?

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>