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

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

[Masami Hiramatsu]

Hi,

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 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(), acquire a spinlock on
the hash table in kretprobe_hash_lock() and disable 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 fix 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 2/3] kprobes/arm64: Fix a possible deadlock case in kretprobe

[Masami Hiramatsu]

Similar to x86 kretprobe deadlock issue, arm64 also implements
kretprobe-booster (trampoline code directly call handler.)
So it has same deadlock issue if there are 2 kretprobes on
normal function and the function called from FIQ (or anywhere
which can be invoked when local_irq_disabled).

This fixes the issue as same as we did on x86.

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 3/3] kprobes/arm: Fix a possible deadlock case in kretprobe

[Masami Hiramatsu]

Similar to x86 kretprobe deadlock issue, arm also implements
kretprobe-booster (trampoline code directly call handler.)
So it has same deadlock issue if there are 2 kretprobes on
normal function and the function called from FIQ (or anywhere
which can be invoked when local_irq_disabled).

This fixes the issue as same as we did on x86.

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 2/3] kprobes/arm64: Fix a possible deadlock case in kretprobe

[Will Deacon]

[adding linux-arm-kernel]

On Wed, Feb 08, 2017 at 12:13:14AM +0900, Masami Hiramatsu wrote:
> Similar to x86 kretprobe deadlock issue, arm64 also implements
> kretprobe-booster (trampoline code directly call handler.)
> So it has same deadlock issue if there are 2 kretprobes on
> normal function and the function called from FIQ (or anywhere
> which can be invoked when local_irq_disabled).

We don't support FIQ on arm64, so I'm not worried about that particular
case. What are the other cases? I can think of debug exceptions, but those
shouldn't be generally kprobe-able, and taking data aborts in things like
get_user/put_user. Are those affected by this bug?

Either way, could you please expand the commit message like you have
for x86? It makes it much easier to understand the change when looking
back at the log in future.

Thanks,

Will

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

[Masami Hiramatsu]

On Wed, 8 Feb 2017 15:06:55 +0000
Will Deacon <will.deacon@arm.com> wrote:

> [adding linux-arm-kernel]
> 
> On Wed, Feb 08, 2017 at 12:13:14AM +0900, Masami Hiramatsu wrote:
> > Similar to x86 kretprobe deadlock issue, arm64 also implements
> > kretprobe-booster (trampoline code directly call handler.)
> > So it has same deadlock issue if there are 2 kretprobes on
> > normal function and the function called from FIQ (or anywhere
> > which can be invoked when local_irq_disabled).
> 
> We don't support FIQ on arm64, so I'm not worried about that particular
> case. What are the other cases? I can think of debug exceptions, but those
> shouldn't be generally kprobe-able, and taking data aborts in things like
> get_user/put_user. Are those affected by this bug?

Hmm, in that case, this may not needed at this point. Would you have
any plan to support FIQ like as NMI in x86?
If something can interrupt while the critical region between 
spin_lock_irqsave() and spin_unlock_irqrestore(), and it can be
kprobe'd, it is safer to apply this patch.

> Either way, could you please expand the commit message like you have
> for x86? It makes it much easier to understand the change when looking
> back at the log in future.

Ah, sorry. I will update the comment.

Thank you,

> 
> Thanks,
> 
> Will


-- 
Masami Hiramatsu <mhiramat@kernel.org>