[PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem

[PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem

[Masami Hiramatsu]

Hello,

On recent talk with Andrea, I started more precise investigation on
the kernel panic with kretprobes on notrace functions, which Francis
had been reported last year ( https://lkml.org/lkml/2017/7/14/466 ).

At first, I tried to reproduce the issue. I picked up __fdget and
ftrace_ops_assist_func as probed functions.
With CONFIG_KPROBE_EVENTS_ON_NOTRACE=y, I could reproduce the kernel
panic as below.

=====
/sys/kernel/debug/tracing # echo "r:event_1 __fdget" >> kprobe_events 
/sys/kernel/debug/tracing # echo "r:event_2 ftrace_ops_assist_func" >> kprobe_events
/sys/kernel/debug/tracing # echo 1 > events/kprobes/enable 
[   70.491856] BUG: unable to handle kernel NULL pointer dereference at 0000000000000010
[   70.493203] PGD 800000001c62e067 P4D 800000001c62e067 PUD 1b5bf067 PMD 0 
[   70.494247] Oops: 0010 [#1] PREEMPT SMP PTI
[   70.494918] CPU: 6 PID: 1210 Comm: sh Not tainted 4.20.0-rc3+ #58
[   70.495931] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.10.1-0-g8891697-prebuilt.qemu-project.org 04/01/2014
[   70.497906] RIP: 0010:0x10
[   70.498465] Code: Bad RIP value.
[   70.499077] RSP: 0018:ffffb1d4c0347e78 EFLAGS: 00010246
[   70.499959] RAX: 00000000fffffff7 RBX: 0000000000000000 RCX: 0000000000000000
[   70.501383] RDX: ffff88f19f9c4f80 RSI: ffffffffb7d75e12 RDI: ffffffffb7d0ede7
[   70.502501] RBP: 00007ffc7061af20 R08: 0000000080000002 R09: ffff88f19f9c4f80
[   70.503698] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000005401
[   70.504810] R13: 0000000000000000 R14: ffffb1d4c0347f58 R15: 0000000000000000
[   70.506028] FS:  0000000000922880(0000) GS:ffff88f19d380000(0000) knlGS:0000000000000000
[   70.507354] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   70.508271] CR2: ffffffffffffffe6 CR3: 000000001f916000 CR4: 00000000000006e0
[   70.509419] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   70.510803] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[   70.511748] Call Trace:
[   70.512225]  ? ksys_ioctl+0x70/0x70
[   70.512884]  ? __fdget+0x5/0x10
[   70.513454]  ? __fdget+0x5/0x10
[   70.513980]  ? copy_oldmem_page_encrypted+0x20/0x20
[   70.514815]  ? __x64_sys_ioctl+0x16/0x20
[   70.515596]  ? do_syscall_64+0x50/0x100
[   70.516229]  ? entry_SYSCALL_64_after_hwframe+0x49/0xbe
[   70.517143] Modules linked in:
[   70.517806] CR2: 0000000000000010
[   70.518527] ---[ end trace ece844ac05189f10 ]---
[   70.519417] RIP: 0010:0x10
[   70.520026] Code: Bad RIP value.
[   70.520800] RSP: 0018:ffffb1d4c0347e78 EFLAGS: 00010246
[   70.521948] RAX: 00000000fffffff7 RBX: 0000000000000000 RCX: 0000000000000000
[   70.523315] RDX: ffff88f19f9c4f80 RSI: ffffffffb7d75e12 RDI: ffffffffb7d0ede7
[   70.524515] RBP: 00007ffc7061af20 R08: 0000000080000002 R09: ffff88f19f9c4f80
[   70.525702] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000005401
[   70.526715] R13: 0000000000000000 R14: ffffb1d4c0347f58 R15: 0000000000000000
[   70.527673] FS:  0000000000922880(0000) GS:ffff88f19d380000(0000) knlGS:0000000000000000
[   70.528896] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   70.529851] CR2: ffffffffffffffe6 CR3: 000000001f916000 CR4: 00000000000006e0
[   70.530922] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[   70.531907] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Killed
=====

This seems the kernel is trying to execute incorrect address.

Next, I focused on the combination of probes. From Francis's report,
this issue caused by the combination of kretprobes, not kprobes.
I ensured that was true.

r __fdget & r ftrace_ops_assist_func => NG
p __fdget & p ftrace_ops_assist_func => OK
p __fdget & r ftrace_ops_assist_func => OK
r __fdget & p ftrace_ops_assist_func => OK

r: kretprobe, p: kprobe

This gave me a hint of what happened. I can explain the cause of this
issue as below;
 
Correct processing of kretprobe on probed-function.

<caller>
 -><probed-function>
   ->fentry
    ->ftrace_ops_assist_func()
     ->kprobe_ftrace_handler()
      ...->pre_handler_kretprobe()
           push the return address (caller) of probed-function to top of the
           kretprobe list and replace it with kretprobe_trampoline.
    <-(ftrace_ops_assist_func())
   <-(fentry)
  <-(probed-function)
[kretprobe_trampoline]
 ->tampoline_handler()
   pop the return address (caller) from top of the kretprobe list
 <-(trampoline_handler())
<caller>

When we put a kretprobe on ftrace_ops_assist_func(), below happens

<caller>
 -><probed-function>
   ->fentry
    ->ftrace_ops_assist_func()
     ->int3
      ->kprobe_int3_handler()
      ...->pre_handler_kretprobe()
           push the return address (*fentry*) of ftrace_ops_assist_func() to
           top of the kretprobe list and replace it with kretprobe_trampoline.
      <-kprobe_int3_handler()
     <-(int3)
     ->kprobe_ftrace_handler()
      ...->pre_handler_kretprobe()
           push the return address (caller) of probed-function to top of the
           kretprobe list and replace it with kretprobe_trampoline.
     <-(kprobe_ftrace_handler())
    <-(ftrace_ops_assist_func())
    [kretprobe_trampoline]
     ->tampoline_handler()
       pop the return address (caller) from top of the kretprobe list
     <-(trampoline_handler())
    <caller>
    [run caller with incorrect stack information]
   <-(<caller>)
  !!KERNEL PANIC!!

Therefore, this kernel panic happens only when we put 2 k*ret*probes on
ftrace_ops_assist_func() and other functions. If we put kprobes, it
doesn't cause any issue, since it doesn't change the return address.

To fix (or just avoid) this issue, we can introduce a frame pointer
verification to skip wrong order entries. And I also would like to
blacklist those functions because those are part of ftrace-based 
kprobe handling routine.

BTW, this is not all of issues. To remove CONFIG_KPROBE_EVENTS_ON_NOTRACE
I'm trying to find out other notrace functions which can cause
kernel crash by probing. Mostly done on x86, so I'll post it
after this series.

Thank you,

---

Masami Hiramatsu (2):
      x86/kprobes: Verify stack frame on kretprobe
      kprobes: Mark ftrace mcount handler functions nokprobe


 arch/x86/kernel/kprobes/core.c |   26 ++++++++++++++++++++++++++
 include/linux/kprobes.h        |    1 +
 kernel/trace/ftrace.c          |    5 ++++-
 3 files changed, 31 insertions(+), 1 deletion(-)

--
Masami Hiramatsu (Linaro) <mhiramat@kernel.org>

[PATCH 1/2] x86/kprobes: Verify stack frame on kretprobe

[Masami Hiramatsu]

Verify the stack frame pointer on kretprobe trampoline handler,
If the stack frame pointer does not match, it skips the wrong
entry and tries to find correct one.

This can happen if user puts the kretprobe on the function
which can be used in the path of ftrace user-function call.
Such functions should not be probed, so this adds a warning
message that reports which function should be blacklisted.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/x86/kernel/kprobes/core.c |   26 ++++++++++++++++++++++++++
 include/linux/kprobes.h        |    1 +
 2 files changed, 27 insertions(+)

diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 4ba75afba527..69b6400d1ce2 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -569,6 +569,7 @@ void arch_prepare_kretprobe(struct kretprobe_instance *ri, struct pt_regs *regs)
 	unsigned long *sara = stack_addr(regs);
 
 	ri->ret_addr = (kprobe_opcode_t *) *sara;
+	ri->fp = sara;
 
 	/* Replace the return addr with trampoline addr */
 	*sara = (unsigned long) &kretprobe_trampoline;
@@ -759,15 +760,21 @@ static __used void *trampoline_handler(struct pt_regs *regs)
 	unsigned long flags, orig_ret_address = 0;
 	unsigned long trampoline_address = (unsigned long)&kretprobe_trampoline;
 	kprobe_opcode_t *correct_ret_addr = NULL;
+	void *frame_pointer;
+	bool skipped = false;
 
 	INIT_HLIST_HEAD(&empty_rp);
 	kretprobe_hash_lock(current, &head, &flags);
 	/* fixup registers */
 #ifdef CONFIG_X86_64
 	regs->cs = __KERNEL_CS;
+	/* On x86-64, we use pt_regs->sp for return address holder. */
+	frame_pointer = &regs->sp;
 #else
 	regs->cs = __KERNEL_CS | get_kernel_rpl();
 	regs->gs = 0;
+	/* On x86-32, we use pt_regs->flags for return address holder. */
+	frame_pointer = &regs->flags;
 #endif
 	regs->ip = trampoline_address;
 	regs->orig_ax = ~0UL;
@@ -789,8 +796,25 @@ static __used void *trampoline_handler(struct pt_regs *regs)
 		if (ri->task != current)
 			/* another task is sharing our hash bucket */
 			continue;
+		/*
+		 * Return probes must be pushed on this hash list correct
+		 * order (same as return order) so that it can be poped
+		 * correctly. However, if we find it is pushed it incorrect
+		 * order, this means we find a function which should not be
+		 * probed, because the wrong order entry is pushed on the
+		 * path of processing other kretprobe itself.
+		 */
+		if (ri->fp != frame_pointer) {
+			if (!skipped)
+				pr_warn("kretprobe is stacked incorrectly. Trying to fixup.\n");
+			skipped = true;
+			continue;
+		}
 
 		orig_ret_address = (unsigned long)ri->ret_addr;
+		if (skipped)
+			pr_warn("%ps must be blacklisted because of incorrect kretprobe order\n",
+				ri->rp->kp.addr);
 
 		if (orig_ret_address != trampoline_address)
 			/*
@@ -808,6 +832,8 @@ static __used void *trampoline_handler(struct pt_regs *regs)
 		if (ri->task != current)
 			/* another task is sharing our hash bucket */
 			continue;
+		if (ri->fp != frame_pointer)
+			continue;
 
 		orig_ret_address = (unsigned long)ri->ret_addr;
 		if (ri->rp && ri->rp->handler) {
diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index e07e91daaacc..72ff78c33033 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -173,6 +173,7 @@ struct kretprobe_instance {
 	struct kretprobe *rp;
 	kprobe_opcode_t *ret_addr;
 	struct task_struct *task;
+	void *fp;
 	char data[0];
 };
 

[PATCH 2/2] kprobes: Mark ftrace mcount handler functions nokprobe

[Masami Hiramatsu]

Mark ftrace mcount handler functions nokprobe since
probing on these functions with kretprobe pushes
return address incorrectly on kretprobe shadow stack.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Reported-by: Francis Deslauriers <francis.deslauriers@efficios.com>
---
 kernel/trace/ftrace.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index f0ff24173a0b..ad4babad4a03 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -6250,7 +6250,7 @@ void ftrace_reset_array_ops(struct trace_array *tr)
 	tr->ops->func = ftrace_stub;
 }
 
-static inline void
+static nokprobe_inline void
 __ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
 		       struct ftrace_ops *ignored, struct pt_regs *regs)
 {
@@ -6310,11 +6310,13 @@ static void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
 {
 	__ftrace_ops_list_func(ip, parent_ip, NULL, regs);
 }
+NOKPROBE_SYMBOL(ftrace_ops_list_func);
 #else
 static void ftrace_ops_no_ops(unsigned long ip, unsigned long parent_ip)
 {
 	__ftrace_ops_list_func(ip, parent_ip, NULL, NULL);
 }
+NOKPROBE_SYMBOL(ftrace_ops_no_ops);
 #endif
 
 /*
@@ -6341,6 +6343,7 @@ static void ftrace_ops_assist_func(unsigned long ip, unsigned long parent_ip,
 	preempt_enable_notrace();
 	trace_clear_recursion(bit);
 }
+NOKPROBE_SYMBOL(ftrace_ops_assist_func);
 
 /**
  * ftrace_ops_get_func - get the function a trampoline should call

Re: [PATCH 2/2] kprobes: Mark ftrace mcount handler functions nokprobe

[Andrea Righi]

On Mon, Jan 07, 2019 at 10:32:32PM +0900, Masami Hiramatsu wrote:
> Mark ftrace mcount handler functions nokprobe since
> probing on these functions with kretprobe pushes
> return address incorrectly on kretprobe shadow stack.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> Reported-by: Francis Deslauriers <francis.deslauriers@efficios.com>
> ---
>  kernel/trace/ftrace.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index f0ff24173a0b..ad4babad4a03 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -6250,7 +6250,7 @@ void ftrace_reset_array_ops(struct trace_array *tr)
>  	tr->ops->func = ftrace_stub;
>  }
>  
> -static inline void
> +static nokprobe_inline void

I think we need to #include <linux/kprobes.h>, otherwise:

  CC      kernel/trace/ftrace.o
kernel/trace/ftrace.c:6219:24: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘void’
 static nokprobe_inline void
                        ^~~~

 kernel/trace/ftrace.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 3a58ad280d83..0333241034d5 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -32,6 +32,7 @@
 #include <linux/sort.h>
 #include <linux/list.h>
 #include <linux/hash.h>
+#include <linux/kprobes.h>
 #include <linux/rcupdate.h>
 
 #include <trace/events/sched.h>

Thanks,
-Andrea

>  __ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
>  		       struct ftrace_ops *ignored, struct pt_regs *regs)
>  {
> @@ -6310,11 +6310,13 @@ static void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
>  {
>  	__ftrace_ops_list_func(ip, parent_ip, NULL, regs);
>  }
> +NOKPROBE_SYMBOL(ftrace_ops_list_func);
>  #else
>  static void ftrace_ops_no_ops(unsigned long ip, unsigned long parent_ip)
>  {
>  	__ftrace_ops_list_func(ip, parent_ip, NULL, NULL);
>  }
> +NOKPROBE_SYMBOL(ftrace_ops_no_ops);
>  #endif
>  
>  /*
> @@ -6341,6 +6343,7 @@ static void ftrace_ops_assist_func(unsigned long ip, unsigned long parent_ip,
>  	preempt_enable_notrace();
>  	trace_clear_recursion(bit);
>  }
> +NOKPROBE_SYMBOL(ftrace_ops_assist_func);
>  
>  /**
>   * ftrace_ops_get_func - get the function a trampoline should call

Re: [PATCH 2/2] kprobes: Mark ftrace mcount handler functions nokprobe

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 8336 bytes --]

Hi Masami,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.0-rc1 next-20190107]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Masami-Hiramatsu/kprobes-Fix-kretprobe-incorrect-stacking-order-problem/20190108-003448
config: i386-randconfig-x075-201901 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All error/warnings (new ones prefixed by >>):

>> kernel//trace/ftrace.c:6219:24: error: expected '=', ',', ';', 'asm' or '__attribute__' before 'void'
    static nokprobe_inline void
                           ^~~~
   kernel//trace/ftrace.c: In function 'ftrace_ops_list_func':
>> kernel//trace/ftrace.c:6277:2: error: implicit declaration of function '__ftrace_ops_list_func'; did you mean 'ftrace_ops_list_func'? [-Werror=implicit-function-declaration]
     __ftrace_ops_list_func(ip, parent_ip, NULL, regs);
     ^~~~~~~~~~~~~~~~~~~~~~
     ftrace_ops_list_func
   kernel//trace/ftrace.c: At top level:
>> kernel//trace/ftrace.c:6279:1: warning: data definition has no type or storage class
    NOKPROBE_SYMBOL(ftrace_ops_list_func);
    ^~~~~~~~~~~~~~~
>> kernel//trace/ftrace.c:6279:1: error: type defaults to 'int' in declaration of 'NOKPROBE_SYMBOL' [-Werror=implicit-int]
>> kernel//trace/ftrace.c:6279:1: warning: parameter names (without types) in function declaration
   kernel//trace/ftrace.c:6312:1: warning: data definition has no type or storage class
    NOKPROBE_SYMBOL(ftrace_ops_assist_func);
    ^~~~~~~~~~~~~~~
   kernel//trace/ftrace.c:6312:1: error: type defaults to 'int' in declaration of 'NOKPROBE_SYMBOL' [-Werror=implicit-int]
   kernel//trace/ftrace.c:6312:1: warning: parameter names (without types) in function declaration
   cc1: some warnings being treated as errors

vim +6277 kernel//trace/ftrace.c

4104d326 Steven Rostedt (Red Hat  2014-01-10  6218) 
0b5a8971 Masami Hiramatsu         2019-01-07 @6219  static nokprobe_inline void
2f5f6ad9 Steven Rostedt           2011-08-08  6220  __ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
a1e2e31d Steven Rostedt           2011-08-09  6221  		       struct ftrace_ops *ignored, struct pt_regs *regs)
b848914c Steven Rostedt           2011-05-04  6222  {
cdbe61bf Steven Rostedt           2011-05-05  6223  	struct ftrace_ops *op;
edc15caf Steven Rostedt           2012-11-02  6224  	int bit;
b848914c Steven Rostedt           2011-05-04  6225  
edc15caf Steven Rostedt           2012-11-02  6226  	bit = trace_test_and_set_recursion(TRACE_LIST_START, TRACE_LIST_MAX);
edc15caf Steven Rostedt           2012-11-02  6227  	if (bit < 0)
b1cff0ad Steven Rostedt           2011-05-25  6228  		return;
b1cff0ad Steven Rostedt           2011-05-25  6229  
cdbe61bf Steven Rostedt           2011-05-05  6230  	/*
cdbe61bf Steven Rostedt           2011-05-05  6231  	 * Some of the ops may be dynamically allocated,
74401729 Paul E. McKenney         2018-11-06  6232  	 * they must be freed after a synchronize_rcu().
cdbe61bf Steven Rostedt           2011-05-05  6233  	 */
cdbe61bf Steven Rostedt           2011-05-05  6234  	preempt_disable_notrace();
ba27f2bc Steven Rostedt (Red Hat  2015-11-30  6235) 
0a016409 Steven Rostedt           2012-11-02  6236  	do_for_each_ftrace_op(op, ftrace_ops_list) {
ba27f2bc Steven Rostedt (Red Hat  2015-11-30  6237) 		/*
ba27f2bc Steven Rostedt (Red Hat  2015-11-30  6238) 		 * Check the following for each ops before calling their func:
ba27f2bc Steven Rostedt (Red Hat  2015-11-30  6239) 		 *  if RCU flag is set, then rcu_is_watching() must be true
ba27f2bc Steven Rostedt (Red Hat  2015-11-30  6240) 		 *  if PER_CPU is set, then ftrace_function_local_disable()
ba27f2bc Steven Rostedt (Red Hat  2015-11-30  6241) 		 *                          must be false
ba27f2bc Steven Rostedt (Red Hat  2015-11-30  6242) 		 *  Otherwise test if the ip matches the ops filter
ba27f2bc Steven Rostedt (Red Hat  2015-11-30  6243) 		 *
ba27f2bc Steven Rostedt (Red Hat  2015-11-30  6244) 		 * If any of the above fails then the op->func() is not executed.
ba27f2bc Steven Rostedt (Red Hat  2015-11-30  6245) 		 */
ba27f2bc Steven Rostedt (Red Hat  2015-11-30  6246) 		if ((!(op->flags & FTRACE_OPS_FL_RCU) || rcu_is_watching()) &&
ba27f2bc Steven Rostedt (Red Hat  2015-11-30  6247) 		    ftrace_ops_test(op, ip, regs)) {
1d48d596 Steven Rostedt (Red Hat  2014-06-25  6248) 			if (FTRACE_WARN_ON(!op->func)) {
1d48d596 Steven Rostedt (Red Hat  2014-06-25  6249) 				pr_warn("op=%p %pS\n", op, op);
4104d326 Steven Rostedt (Red Hat  2014-01-10  6250) 				goto out;
4104d326 Steven Rostedt (Red Hat  2014-01-10  6251) 			}
a1e2e31d Steven Rostedt           2011-08-09  6252  			op->func(ip, parent_ip, op, regs);
4104d326 Steven Rostedt (Red Hat  2014-01-10  6253) 		}
0a016409 Steven Rostedt           2012-11-02  6254  	} while_for_each_ftrace_op(op);
4104d326 Steven Rostedt (Red Hat  2014-01-10  6255) out:
cdbe61bf Steven Rostedt           2011-05-05  6256  	preempt_enable_notrace();
edc15caf Steven Rostedt           2012-11-02  6257  	trace_clear_recursion(bit);
b848914c Steven Rostedt           2011-05-04  6258  }
b848914c Steven Rostedt           2011-05-04  6259  
2f5f6ad9 Steven Rostedt           2011-08-08  6260  /*
2f5f6ad9 Steven Rostedt           2011-08-08  6261   * Some archs only support passing ip and parent_ip. Even though
2f5f6ad9 Steven Rostedt           2011-08-08  6262   * the list function ignores the op parameter, we do not want any
2f5f6ad9 Steven Rostedt           2011-08-08  6263   * C side effects, where a function is called without the caller
2f5f6ad9 Steven Rostedt           2011-08-08  6264   * sending a third parameter.
a1e2e31d Steven Rostedt           2011-08-09  6265   * Archs are to support both the regs and ftrace_ops at the same time.
a1e2e31d Steven Rostedt           2011-08-09  6266   * If they support ftrace_ops, it is assumed they support regs.
a1e2e31d Steven Rostedt           2011-08-09  6267   * If call backs want to use regs, they must either check for regs
06aeaaea Masami Hiramatsu         2012-09-28  6268   * being NULL, or CONFIG_DYNAMIC_FTRACE_WITH_REGS.
06aeaaea Masami Hiramatsu         2012-09-28  6269   * Note, CONFIG_DYNAMIC_FTRACE_WITH_REGS expects a full regs to be saved.
a1e2e31d Steven Rostedt           2011-08-09  6270   * An architecture can pass partial regs with ftrace_ops and still
b8ec330a Li Bin                   2015-11-30  6271   * set the ARCH_SUPPORTS_FTRACE_OPS.
2f5f6ad9 Steven Rostedt           2011-08-08  6272   */
2f5f6ad9 Steven Rostedt           2011-08-08  6273  #if ARCH_SUPPORTS_FTRACE_OPS
2f5f6ad9 Steven Rostedt           2011-08-08  6274  static void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
a1e2e31d Steven Rostedt           2011-08-09  6275  				 struct ftrace_ops *op, struct pt_regs *regs)
2f5f6ad9 Steven Rostedt           2011-08-08  6276  {
a1e2e31d Steven Rostedt           2011-08-09 @6277  	__ftrace_ops_list_func(ip, parent_ip, NULL, regs);
2f5f6ad9 Steven Rostedt           2011-08-08  6278  }
0b5a8971 Masami Hiramatsu         2019-01-07 @6279  NOKPROBE_SYMBOL(ftrace_ops_list_func);
2f5f6ad9 Steven Rostedt           2011-08-08  6280  #else
2f5f6ad9 Steven Rostedt           2011-08-08  6281  static void ftrace_ops_no_ops(unsigned long ip, unsigned long parent_ip)
2f5f6ad9 Steven Rostedt           2011-08-08  6282  {
a1e2e31d Steven Rostedt           2011-08-09  6283  	__ftrace_ops_list_func(ip, parent_ip, NULL, NULL);
2f5f6ad9 Steven Rostedt           2011-08-08  6284  }
0b5a8971 Masami Hiramatsu         2019-01-07  6285  NOKPROBE_SYMBOL(ftrace_ops_no_ops);
2f5f6ad9 Steven Rostedt           2011-08-08  6286  #endif
2f5f6ad9 Steven Rostedt           2011-08-08  6287  

:::::: The code at line 6277 was first introduced by commit
:::::: a1e2e31d175a1349274eba3465d17616c6725f8c ftrace: Return pt_regs to function trace callback

:::::: TO: Steven Rostedt <srostedt@redhat.com>
:::::: CC: Steven Rostedt <rostedt@goodmis.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 30841 bytes --]

Re: [PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem

[Andrea Righi]

On Mon, Jan 07, 2019 at 10:31:34PM +0900, Masami Hiramatsu wrote:
> Hello,
> 
> On recent talk with Andrea, I started more precise investigation on
> the kernel panic with kretprobes on notrace functions, which Francis
> had been reported last year ( https://lkml.org/lkml/2017/7/14/466 ).
> 
> At first, I tried to reproduce the issue. I picked up __fdget and
> ftrace_ops_assist_func as probed functions.
> With CONFIG_KPROBE_EVENTS_ON_NOTRACE=y, I could reproduce the kernel
> panic as below.
> 
> =====
> /sys/kernel/debug/tracing # echo "r:event_1 __fdget" >> kprobe_events 
> /sys/kernel/debug/tracing # echo "r:event_2 ftrace_ops_assist_func" >> kprobe_events
> /sys/kernel/debug/tracing # echo 1 > events/kprobes/enable 
> [   70.491856] BUG: unable to handle kernel NULL pointer dereference at 0000000000000010
> [   70.493203] PGD 800000001c62e067 P4D 800000001c62e067 PUD 1b5bf067 PMD 0 
> [   70.494247] Oops: 0010 [#1] PREEMPT SMP PTI
> [   70.494918] CPU: 6 PID: 1210 Comm: sh Not tainted 4.20.0-rc3+ #58
> [   70.495931] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.10.1-0-g8891697-prebuilt.qemu-project.org 04/01/2014
> [   70.497906] RIP: 0010:0x10
> [   70.498465] Code: Bad RIP value.
> [   70.499077] RSP: 0018:ffffb1d4c0347e78 EFLAGS: 00010246
> [   70.499959] RAX: 00000000fffffff7 RBX: 0000000000000000 RCX: 0000000000000000
> [   70.501383] RDX: ffff88f19f9c4f80 RSI: ffffffffb7d75e12 RDI: ffffffffb7d0ede7
> [   70.502501] RBP: 00007ffc7061af20 R08: 0000000080000002 R09: ffff88f19f9c4f80
> [   70.503698] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000005401
> [   70.504810] R13: 0000000000000000 R14: ffffb1d4c0347f58 R15: 0000000000000000
> [   70.506028] FS:  0000000000922880(0000) GS:ffff88f19d380000(0000) knlGS:0000000000000000
> [   70.507354] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   70.508271] CR2: ffffffffffffffe6 CR3: 000000001f916000 CR4: 00000000000006e0
> [   70.509419] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [   70.510803] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [   70.511748] Call Trace:
> [   70.512225]  ? ksys_ioctl+0x70/0x70
> [   70.512884]  ? __fdget+0x5/0x10
> [   70.513454]  ? __fdget+0x5/0x10
> [   70.513980]  ? copy_oldmem_page_encrypted+0x20/0x20
> [   70.514815]  ? __x64_sys_ioctl+0x16/0x20
> [   70.515596]  ? do_syscall_64+0x50/0x100
> [   70.516229]  ? entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [   70.517143] Modules linked in:
> [   70.517806] CR2: 0000000000000010
> [   70.518527] ---[ end trace ece844ac05189f10 ]---
> [   70.519417] RIP: 0010:0x10
> [   70.520026] Code: Bad RIP value.
> [   70.520800] RSP: 0018:ffffb1d4c0347e78 EFLAGS: 00010246
> [   70.521948] RAX: 00000000fffffff7 RBX: 0000000000000000 RCX: 0000000000000000
> [   70.523315] RDX: ffff88f19f9c4f80 RSI: ffffffffb7d75e12 RDI: ffffffffb7d0ede7
> [   70.524515] RBP: 00007ffc7061af20 R08: 0000000080000002 R09: ffff88f19f9c4f80
> [   70.525702] R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000005401
> [   70.526715] R13: 0000000000000000 R14: ffffb1d4c0347f58 R15: 0000000000000000
> [   70.527673] FS:  0000000000922880(0000) GS:ffff88f19d380000(0000) knlGS:0000000000000000
> [   70.528896] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   70.529851] CR2: ffffffffffffffe6 CR3: 000000001f916000 CR4: 00000000000006e0
> [   70.530922] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [   70.531907] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Killed
> =====
> 
> This seems the kernel is trying to execute incorrect address.
> 
> Next, I focused on the combination of probes. From Francis's report,
> this issue caused by the combination of kretprobes, not kprobes.
> I ensured that was true.
> 
> r __fdget & r ftrace_ops_assist_func => NG
> p __fdget & p ftrace_ops_assist_func => OK
> p __fdget & r ftrace_ops_assist_func => OK
> r __fdget & p ftrace_ops_assist_func => OK
> 
> r: kretprobe, p: kprobe
> 
> This gave me a hint of what happened. I can explain the cause of this
> issue as below;
>  
> Correct processing of kretprobe on probed-function.
> 
> <caller>
>  -><probed-function>
>    ->fentry
>     ->ftrace_ops_assist_func()
>      ->kprobe_ftrace_handler()
>       ...->pre_handler_kretprobe()
>            push the return address (caller) of probed-function to top of the
>            kretprobe list and replace it with kretprobe_trampoline.
>     <-(ftrace_ops_assist_func())
>    <-(fentry)
>   <-(probed-function)
> [kretprobe_trampoline]
>  ->tampoline_handler()
>    pop the return address (caller) from top of the kretprobe list
>  <-(trampoline_handler())
> <caller>
> 
> When we put a kretprobe on ftrace_ops_assist_func(), below happens
> 
> <caller>
>  -><probed-function>
>    ->fentry
>     ->ftrace_ops_assist_func()
>      ->int3
>       ->kprobe_int3_handler()
>       ...->pre_handler_kretprobe()
>            push the return address (*fentry*) of ftrace_ops_assist_func() to
>            top of the kretprobe list and replace it with kretprobe_trampoline.
>       <-kprobe_int3_handler()
>      <-(int3)
>      ->kprobe_ftrace_handler()
>       ...->pre_handler_kretprobe()
>            push the return address (caller) of probed-function to top of the
>            kretprobe list and replace it with kretprobe_trampoline.
>      <-(kprobe_ftrace_handler())
>     <-(ftrace_ops_assist_func())
>     [kretprobe_trampoline]
>      ->tampoline_handler()
>        pop the return address (caller) from top of the kretprobe list
>      <-(trampoline_handler())
>     <caller>
>     [run caller with incorrect stack information]
>    <-(<caller>)
>   !!KERNEL PANIC!!
> 
> Therefore, this kernel panic happens only when we put 2 k*ret*probes on
> ftrace_ops_assist_func() and other functions. If we put kprobes, it
> doesn't cause any issue, since it doesn't change the return address.
> 
> To fix (or just avoid) this issue, we can introduce a frame pointer
> verification to skip wrong order entries. And I also would like to
> blacklist those functions because those are part of ftrace-based 
> kprobe handling routine.
> 
> BTW, this is not all of issues. To remove CONFIG_KPROBE_EVENTS_ON_NOTRACE
> I'm trying to find out other notrace functions which can cause
> kernel crash by probing. Mostly done on x86, so I'll post it
> after this series.
> 
> Thank you,

Apart than the missing include <linux/kprobes.h> in PATCH 2/2
everything else looks good to me.

Tested-by: Andrea Righi <righi.andrea@gmail.com>

Thanks!
-Andrea

Re: [PATCH 2/2] kprobes: Mark ftrace mcount handler functions nokprobe

[Steven Rostedt]

On Mon, 7 Jan 2019 15:55:36 +0100
Andrea Righi <righi.andrea@gmail.com> wrote:

> On Mon, Jan 07, 2019 at 10:32:32PM +0900, Masami Hiramatsu wrote:
> > Mark ftrace mcount handler functions nokprobe since
> > probing on these functions with kretprobe pushes
> > return address incorrectly on kretprobe shadow stack.
> > 
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > Reported-by: Francis Deslauriers <francis.deslauriers@efficios.com>
> > ---
> >  kernel/trace/ftrace.c |    5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index f0ff24173a0b..ad4babad4a03 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -6250,7 +6250,7 @@ void ftrace_reset_array_ops(struct trace_array *tr)
> >  	tr->ops->func = ftrace_stub;
> >  }
> >  
> > -static inline void
> > +static nokprobe_inline void  
> 
> I think we need to #include <linux/kprobes.h>, otherwise:
> 
>   CC      kernel/trace/ftrace.o
> kernel/trace/ftrace.c:6219:24: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘void’
>  static nokprobe_inline void
>                         ^~~~
> 
>  kernel/trace/ftrace.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 3a58ad280d83..0333241034d5 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -32,6 +32,7 @@
>  #include <linux/sort.h>
>  #include <linux/list.h>
>  #include <linux/hash.h>
> +#include <linux/kprobes.h>
>  #include <linux/rcupdate.h>
>  
>  #include <trace/events/sched.h>

And zero day just caught it too.

Masami, want to fold this into your patch and send out a v2?

-- Steve

Re: [PATCH 2/2] kprobes: Mark ftrace mcount handler functions nokprobe

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 8285 bytes --]

Hi Masami,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.0-rc1 next-20190107]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Masami-Hiramatsu/kprobes-Fix-kretprobe-incorrect-stacking-order-problem/20190108-003448
config: i386-randconfig-s3-201901 (attached as .config)
compiler: gcc-6 (Debian 6.4.0-9) 6.4.0 20171026
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   kernel/trace/ftrace.c:6219:24: error: expected '=', ',', ';', 'asm' or '__attribute__' before 'void'
    static nokprobe_inline void
                           ^~~~
   kernel/trace/ftrace.c: In function 'ftrace_ops_list_func':
>> kernel/trace/ftrace.c:6277:2: error: implicit declaration of function '__ftrace_ops_list_func' [-Werror=implicit-function-declaration]
     __ftrace_ops_list_func(ip, parent_ip, NULL, regs);
     ^~~~~~~~~~~~~~~~~~~~~~
   kernel/trace/ftrace.c: At top level:
   kernel/trace/ftrace.c:6279:1: warning: data definition has no type or storage class
    NOKPROBE_SYMBOL(ftrace_ops_list_func);
    ^~~~~~~~~~~~~~~
   kernel/trace/ftrace.c:6279:1: error: type defaults to 'int' in declaration of 'NOKPROBE_SYMBOL' [-Werror=implicit-int]
   kernel/trace/ftrace.c:6279:1: warning: parameter names (without types) in function declaration
   kernel/trace/ftrace.c:6312:1: warning: data definition has no type or storage class
    NOKPROBE_SYMBOL(ftrace_ops_assist_func);
    ^~~~~~~~~~~~~~~
   kernel/trace/ftrace.c:6312:1: error: type defaults to 'int' in declaration of 'NOKPROBE_SYMBOL' [-Werror=implicit-int]
   kernel/trace/ftrace.c:6312:1: warning: parameter names (without types) in function declaration
   cc1: some warnings being treated as errors

vim +/__ftrace_ops_list_func +6277 kernel/trace/ftrace.c

4104d326 Steven Rostedt (Red Hat  2014-01-10  6218) 
0b5a8971 Masami Hiramatsu         2019-01-07 @6219  static nokprobe_inline void
2f5f6ad9 Steven Rostedt           2011-08-08  6220  __ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
a1e2e31d Steven Rostedt           2011-08-09  6221  		       struct ftrace_ops *ignored, struct pt_regs *regs)
b848914c Steven Rostedt           2011-05-04  6222  {
cdbe61bf Steven Rostedt           2011-05-05  6223  	struct ftrace_ops *op;
edc15caf Steven Rostedt           2012-11-02  6224  	int bit;
b848914c Steven Rostedt           2011-05-04  6225  
edc15caf Steven Rostedt           2012-11-02  6226  	bit = trace_test_and_set_recursion(TRACE_LIST_START, TRACE_LIST_MAX);
edc15caf Steven Rostedt           2012-11-02  6227  	if (bit < 0)
b1cff0ad Steven Rostedt           2011-05-25  6228  		return;
b1cff0ad Steven Rostedt           2011-05-25  6229  
cdbe61bf Steven Rostedt           2011-05-05  6230  	/*
cdbe61bf Steven Rostedt           2011-05-05  6231  	 * Some of the ops may be dynamically allocated,
74401729 Paul E. McKenney         2018-11-06  6232  	 * they must be freed after a synchronize_rcu().
cdbe61bf Steven Rostedt           2011-05-05  6233  	 */
cdbe61bf Steven Rostedt           2011-05-05  6234  	preempt_disable_notrace();
ba27f2bc Steven Rostedt (Red Hat  2015-11-30  6235) 
0a016409 Steven Rostedt           2012-11-02  6236  	do_for_each_ftrace_op(op, ftrace_ops_list) {
ba27f2bc Steven Rostedt (Red Hat  2015-11-30  6237) 		/*
ba27f2bc Steven Rostedt (Red Hat  2015-11-30  6238) 		 * Check the following for each ops before calling their func:
ba27f2bc Steven Rostedt (Red Hat  2015-11-30  6239) 		 *  if RCU flag is set, then rcu_is_watching() must be true
ba27f2bc Steven Rostedt (Red Hat  2015-11-30  6240) 		 *  if PER_CPU is set, then ftrace_function_local_disable()
ba27f2bc Steven Rostedt (Red Hat  2015-11-30  6241) 		 *                          must be false
ba27f2bc Steven Rostedt (Red Hat  2015-11-30  6242) 		 *  Otherwise test if the ip matches the ops filter
ba27f2bc Steven Rostedt (Red Hat  2015-11-30  6243) 		 *
ba27f2bc Steven Rostedt (Red Hat  2015-11-30  6244) 		 * If any of the above fails then the op->func() is not executed.
ba27f2bc Steven Rostedt (Red Hat  2015-11-30  6245) 		 */
ba27f2bc Steven Rostedt (Red Hat  2015-11-30  6246) 		if ((!(op->flags & FTRACE_OPS_FL_RCU) || rcu_is_watching()) &&
ba27f2bc Steven Rostedt (Red Hat  2015-11-30  6247) 		    ftrace_ops_test(op, ip, regs)) {
1d48d596 Steven Rostedt (Red Hat  2014-06-25  6248) 			if (FTRACE_WARN_ON(!op->func)) {
1d48d596 Steven Rostedt (Red Hat  2014-06-25  6249) 				pr_warn("op=%p %pS\n", op, op);
4104d326 Steven Rostedt (Red Hat  2014-01-10  6250) 				goto out;
4104d326 Steven Rostedt (Red Hat  2014-01-10  6251) 			}
a1e2e31d Steven Rostedt           2011-08-09  6252  			op->func(ip, parent_ip, op, regs);
4104d326 Steven Rostedt (Red Hat  2014-01-10  6253) 		}
0a016409 Steven Rostedt           2012-11-02  6254  	} while_for_each_ftrace_op(op);
4104d326 Steven Rostedt (Red Hat  2014-01-10  6255) out:
cdbe61bf Steven Rostedt           2011-05-05  6256  	preempt_enable_notrace();
edc15caf Steven Rostedt           2012-11-02  6257  	trace_clear_recursion(bit);
b848914c Steven Rostedt           2011-05-04  6258  }
b848914c Steven Rostedt           2011-05-04  6259  
2f5f6ad9 Steven Rostedt           2011-08-08  6260  /*
2f5f6ad9 Steven Rostedt           2011-08-08  6261   * Some archs only support passing ip and parent_ip. Even though
2f5f6ad9 Steven Rostedt           2011-08-08  6262   * the list function ignores the op parameter, we do not want any
2f5f6ad9 Steven Rostedt           2011-08-08  6263   * C side effects, where a function is called without the caller
2f5f6ad9 Steven Rostedt           2011-08-08  6264   * sending a third parameter.
a1e2e31d Steven Rostedt           2011-08-09  6265   * Archs are to support both the regs and ftrace_ops at the same time.
a1e2e31d Steven Rostedt           2011-08-09  6266   * If they support ftrace_ops, it is assumed they support regs.
a1e2e31d Steven Rostedt           2011-08-09  6267   * If call backs want to use regs, they must either check for regs
06aeaaea Masami Hiramatsu         2012-09-28  6268   * being NULL, or CONFIG_DYNAMIC_FTRACE_WITH_REGS.
06aeaaea Masami Hiramatsu         2012-09-28  6269   * Note, CONFIG_DYNAMIC_FTRACE_WITH_REGS expects a full regs to be saved.
a1e2e31d Steven Rostedt           2011-08-09  6270   * An architecture can pass partial regs with ftrace_ops and still
b8ec330a Li Bin                   2015-11-30  6271   * set the ARCH_SUPPORTS_FTRACE_OPS.
2f5f6ad9 Steven Rostedt           2011-08-08  6272   */
2f5f6ad9 Steven Rostedt           2011-08-08  6273  #if ARCH_SUPPORTS_FTRACE_OPS
2f5f6ad9 Steven Rostedt           2011-08-08  6274  static void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
a1e2e31d Steven Rostedt           2011-08-09  6275  				 struct ftrace_ops *op, struct pt_regs *regs)
2f5f6ad9 Steven Rostedt           2011-08-08  6276  {
a1e2e31d Steven Rostedt           2011-08-09 @6277  	__ftrace_ops_list_func(ip, parent_ip, NULL, regs);
2f5f6ad9 Steven Rostedt           2011-08-08  6278  }
0b5a8971 Masami Hiramatsu         2019-01-07  6279  NOKPROBE_SYMBOL(ftrace_ops_list_func);
2f5f6ad9 Steven Rostedt           2011-08-08  6280  #else
2f5f6ad9 Steven Rostedt           2011-08-08  6281  static void ftrace_ops_no_ops(unsigned long ip, unsigned long parent_ip)
2f5f6ad9 Steven Rostedt           2011-08-08  6282  {
a1e2e31d Steven Rostedt           2011-08-09  6283  	__ftrace_ops_list_func(ip, parent_ip, NULL, NULL);
2f5f6ad9 Steven Rostedt           2011-08-08  6284  }
0b5a8971 Masami Hiramatsu         2019-01-07  6285  NOKPROBE_SYMBOL(ftrace_ops_no_ops);
2f5f6ad9 Steven Rostedt           2011-08-08  6286  #endif
2f5f6ad9 Steven Rostedt           2011-08-08  6287  

:::::: The code at line 6277 was first introduced by commit
:::::: a1e2e31d175a1349274eba3465d17616c6725f8c ftrace: Return pt_regs to function trace callback

:::::: TO: Steven Rostedt <srostedt@redhat.com>
:::::: CC: Steven Rostedt <rostedt@goodmis.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32939 bytes --]

Re: [PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem

[Andrea Righi]

On Mon, Jan 07, 2019 at 10:31:34PM +0900, Masami Hiramatsu wrote:
...
> BTW, this is not all of issues. To remove CONFIG_KPROBE_EVENTS_ON_NOTRACE
> I'm trying to find out other notrace functions which can cause
> kernel crash by probing. Mostly done on x86, so I'll post it
> after this series.

Not sure if you found it already, but it looks like some of the
_raw_spin_lock/unlock* functions (when they're not inlined) are causing
the same problem (or something similar), I can deadlock the system by
doing this for example:

 echo "r:event_1 __fdget" >> kprobe_events
 echo "r:event_2 _raw_spin_lock_irqsave" >> kprobe_events
 echo 1 > events/kprobes/enable
 [DEADLOCK]

Sending the following just in case...

Thanks,

 kernel/locking/spinlock.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/kernel/locking/spinlock.c b/kernel/locking/spinlock.c
index 936f3d14dd6b..d93e88019239 100644
--- a/kernel/locking/spinlock.c
+++ b/kernel/locking/spinlock.c
@@ -19,6 +19,7 @@
 #include <linux/preempt.h>
 #include <linux/spinlock.h>
 #include <linux/interrupt.h>
+#include <linux/kprobes.h>
 #include <linux/debug_locks.h>
 #include <linux/export.h>
 
@@ -128,6 +129,7 @@ int __lockfunc _raw_spin_trylock(raw_spinlock_t *lock)
 	return __raw_spin_trylock(lock);
 }
 EXPORT_SYMBOL(_raw_spin_trylock);
+NOKPROBE_SYMBOL(_raw_spin_trylock);
 #endif
 
 #ifndef CONFIG_INLINE_SPIN_TRYLOCK_BH
@@ -136,6 +138,7 @@ int __lockfunc _raw_spin_trylock_bh(raw_spinlock_t *lock)
 	return __raw_spin_trylock_bh(lock);
 }
 EXPORT_SYMBOL(_raw_spin_trylock_bh);
+NOKPROBE_SYMBOL(_raw_spin_trylock_bh);
 #endif
 
 #ifndef CONFIG_INLINE_SPIN_LOCK
@@ -144,6 +147,7 @@ void __lockfunc _raw_spin_lock(raw_spinlock_t *lock)
 	__raw_spin_lock(lock);
 }
 EXPORT_SYMBOL(_raw_spin_lock);
+NOKPROBE_SYMBOL(_raw_spin_lock);
 #endif
 
 #ifndef CONFIG_INLINE_SPIN_LOCK_IRQSAVE
@@ -152,6 +156,7 @@ unsigned long __lockfunc _raw_spin_lock_irqsave(raw_spinlock_t *lock)
 	return __raw_spin_lock_irqsave(lock);
 }
 EXPORT_SYMBOL(_raw_spin_lock_irqsave);
+NOKPROBE_SYMBOL(_raw_spin_lock_irqsave);
 #endif
 
 #ifndef CONFIG_INLINE_SPIN_LOCK_IRQ
@@ -160,6 +165,7 @@ void __lockfunc _raw_spin_lock_irq(raw_spinlock_t *lock)
 	__raw_spin_lock_irq(lock);
 }
 EXPORT_SYMBOL(_raw_spin_lock_irq);
+NOKPROBE_SYMBOL(_raw_spin_lock_irq);
 #endif
 
 #ifndef CONFIG_INLINE_SPIN_LOCK_BH
@@ -168,6 +174,7 @@ void __lockfunc _raw_spin_lock_bh(raw_spinlock_t *lock)
 	__raw_spin_lock_bh(lock);
 }
 EXPORT_SYMBOL(_raw_spin_lock_bh);
+NOKPROBE_SYMBOL(_raw_spin_lock_bh);
 #endif
 
 #ifdef CONFIG_UNINLINE_SPIN_UNLOCK
@@ -176,6 +183,7 @@ void __lockfunc _raw_spin_unlock(raw_spinlock_t *lock)
 	__raw_spin_unlock(lock);
 }
 EXPORT_SYMBOL(_raw_spin_unlock);
+NOKPROBE_SYMBOL(_raw_spin_unlock);
 #endif
 
 #ifndef CONFIG_INLINE_SPIN_UNLOCK_IRQRESTORE
@@ -184,6 +192,7 @@ void __lockfunc _raw_spin_unlock_irqrestore(raw_spinlock_t *lock, unsigned long
 	__raw_spin_unlock_irqrestore(lock, flags);
 }
 EXPORT_SYMBOL(_raw_spin_unlock_irqrestore);
+NOKPROBE_SYMBOL(_raw_spin_unlock_irqrestore);
 #endif
 
 #ifndef CONFIG_INLINE_SPIN_UNLOCK_IRQ
@@ -192,6 +201,7 @@ void __lockfunc _raw_spin_unlock_irq(raw_spinlock_t *lock)
 	__raw_spin_unlock_irq(lock);
 }
 EXPORT_SYMBOL(_raw_spin_unlock_irq);
+NOKPROBE_SYMBOL(_raw_spin_unlock_irq);
 #endif
 
 #ifndef CONFIG_INLINE_SPIN_UNLOCK_BH
@@ -200,6 +210,7 @@ void __lockfunc _raw_spin_unlock_bh(raw_spinlock_t *lock)
 	__raw_spin_unlock_bh(lock);
 }
 EXPORT_SYMBOL(_raw_spin_unlock_bh);
+NOKPROBE_SYMBOL(_raw_spin_unlock_bh);
 #endif
 
 #ifndef CONFIG_INLINE_READ_TRYLOCK

Signed-off-by: Andrea Righi <righi.andrea@gmail.com>

Re: [PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem

[Steven Rostedt]

On Mon, 7 Jan 2019 19:34:44 +0100
Andrea Righi <righi.andrea@gmail.com> wrote:

> On Mon, Jan 07, 2019 at 10:31:34PM +0900, Masami Hiramatsu wrote:
> ...
> > BTW, this is not all of issues. To remove CONFIG_KPROBE_EVENTS_ON_NOTRACE
> > I'm trying to find out other notrace functions which can cause
> > kernel crash by probing. Mostly done on x86, so I'll post it
> > after this series.  
> 
> Not sure if you found it already, but it looks like some of the
> _raw_spin_lock/unlock* functions (when they're not inlined) are causing
> the same problem (or something similar), I can deadlock the system by
> doing this for example:
> 
>  echo "r:event_1 __fdget" >> kprobe_events
>  echo "r:event_2 _raw_spin_lock_irqsave" >> kprobe_events
>  echo 1 > events/kprobes/enable
>  [DEADLOCK]
> 
> Sending the following just in case...
>

Ug, kretprobe calls spinlocks in the callback? I wonder if we can
remove them.

I'm guessing this is a different issue than the one that this patch
fixes. This sounds like we are calling kretprobe from kretprobe?

-- Steve

Re: [PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem

[Andrea Righi]

On Mon, Jan 07, 2019 at 02:27:49PM -0500, Steven Rostedt wrote:
> On Mon, 7 Jan 2019 19:34:44 +0100
> Andrea Righi <righi.andrea@gmail.com> wrote:
> 
> > On Mon, Jan 07, 2019 at 10:31:34PM +0900, Masami Hiramatsu wrote:
> > ...
> > > BTW, this is not all of issues. To remove CONFIG_KPROBE_EVENTS_ON_NOTRACE
> > > I'm trying to find out other notrace functions which can cause
> > > kernel crash by probing. Mostly done on x86, so I'll post it
> > > after this series.  
> > 
> > Not sure if you found it already, but it looks like some of the
> > _raw_spin_lock/unlock* functions (when they're not inlined) are causing
> > the same problem (or something similar), I can deadlock the system by
> > doing this for example:
> > 
> >  echo "r:event_1 __fdget" >> kprobe_events
> >  echo "r:event_2 _raw_spin_lock_irqsave" >> kprobe_events
> >  echo 1 > events/kprobes/enable
> >  [DEADLOCK]
> > 
> > Sending the following just in case...
> >
> 
> Ug, kretprobe calls spinlocks in the callback? I wonder if we can
> remove them.
> 
> I'm guessing this is a different issue than the one that this patch
> fixes. This sounds like we are calling kretprobe from kretprobe?
> 
> -- Steve

kretprobe_trampoline()
  -> trampoline_handler()
    -> kretprobe_hash_lock()
      -> raw_spin_lock_irqsave()

If we put a kretprobe to raw_spin_lock_irqsave() it looks like
kretprobe is going to call kretprobe...

-Andrea

Re: [PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem

[Steven Rostedt]

On Mon, 7 Jan 2019 20:52:09 +0100
Andrea Righi <righi.andrea@gmail.com> wrote:

> > Ug, kretprobe calls spinlocks in the callback? I wonder if we can
> > remove them.
> > 
> > I'm guessing this is a different issue than the one that this patch
> > fixes. This sounds like we are calling kretprobe from kretprobe?
> > 
> > -- Steve  
> 
> kretprobe_trampoline()
>   -> trampoline_handler()
>     -> kretprobe_hash_lock()
>       -> raw_spin_lock_irqsave()  
> 
> If we put a kretprobe to raw_spin_lock_irqsave() it looks like
> kretprobe is going to call kretprobe...

Right, but we should be able to add some recursion protection to stop
that. I have similar protection in the ftrace code.

-- Steve

Re: [PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem

[Andrea Righi]

On Mon, Jan 07, 2019 at 02:59:18PM -0500, Steven Rostedt wrote:
> On Mon, 7 Jan 2019 20:52:09 +0100
> Andrea Righi <righi.andrea@gmail.com> wrote:
> 
> > > Ug, kretprobe calls spinlocks in the callback? I wonder if we can
> > > remove them.
> > > 
> > > I'm guessing this is a different issue than the one that this patch
> > > fixes. This sounds like we are calling kretprobe from kretprobe?
> > > 
> > > -- Steve  
> > 
> > kretprobe_trampoline()
> >   -> trampoline_handler()
> >     -> kretprobe_hash_lock()
> >       -> raw_spin_lock_irqsave()  
> > 
> > If we put a kretprobe to raw_spin_lock_irqsave() it looks like
> > kretprobe is going to call kretprobe...
> 
> Right, but we should be able to add some recursion protection to stop
> that. I have similar protection in the ftrace code.

If we assume that __raw_spin_lock/unlock*() are always inlined a
possible way to prevent this recursion could be to use directly those
functions to do locking from the kretprobe trampoline.

But I'm not sure if that's a safe assumption... if not I'll see if I can
find a better solution.

Thanks,

From: Andrea Righi <righi.andrea@gmail.com>
Subject: [PATCH] kprobes: prevent recursion deadlock with kretprobe and
 spinlocks

kretprobe_trampoline() is using a spinlock to protect the hash of
kretprobes. Adding a kretprobe to the spinlock functions may cause
a recursion deadlock where kretprobe is calling itself:

 kretprobe_trampoline()
   -> trampoline_handler()
     -> kretprobe_hash_lock()
       -> raw_spin_lock_irqsave()
         -> _raw_spin_lock_irqsave()
 kretprobe_trampoline from _raw_spin_lock_irqsave => DEADLOCK

 kretprobe_trampoline()
   -> trampoline_handler()
     -> recycle_rp_inst()
       -> raw_spin_lock()
         -> _raw_spin_lock()
 kretprobe_trampoline from _raw_spin_lock => DEADLOCK

Use the corresponding inlined spinlock functions to prevent this
recursion.

Signed-off-by: Andrea Righi <righi.andrea@gmail.com>
---
 kernel/kprobes.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index f4ddfdd2d07e..b89bef5e3d80 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1154,9 +1154,9 @@ void recycle_rp_inst(struct kretprobe_instance *ri,
 	hlist_del(&ri->hlist);
 	INIT_HLIST_NODE(&ri->hlist);
 	if (likely(rp)) {
-		raw_spin_lock(&rp->lock);
+		__raw_spin_lock(&rp->lock);
 		hlist_add_head(&ri->hlist, &rp->free_instances);
-		raw_spin_unlock(&rp->lock);
+		__raw_spin_unlock(&rp->lock);
 	} else
 		/* Unregistering */
 		hlist_add_head(&ri->hlist, head);
@@ -1172,7 +1172,7 @@ __acquires(hlist_lock)
 
 	*head = &kretprobe_inst_table[hash];
 	hlist_lock = kretprobe_table_lock_ptr(hash);
-	raw_spin_lock_irqsave(hlist_lock, *flags);
+	*flags = __raw_spin_lock_irqsave(hlist_lock);
 }
 NOKPROBE_SYMBOL(kretprobe_hash_lock);
 
@@ -1193,7 +1193,7 @@ __releases(hlist_lock)
 	raw_spinlock_t *hlist_lock;
 
 	hlist_lock = kretprobe_table_lock_ptr(hash);
-	raw_spin_unlock_irqrestore(hlist_lock, *flags);
+	__raw_spin_unlock_irqrestore(hlist_lock, *flags);
 }
 NOKPROBE_SYMBOL(kretprobe_hash_unlock);
 
-- 
2.17.1

Re: [PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem

[Steven Rostedt]

On Mon, 7 Jan 2019 22:19:04 +0100
Andrea Righi <righi.andrea@gmail.com> wrote:

> > > If we put a kretprobe to raw_spin_lock_irqsave() it looks like
> > > kretprobe is going to call kretprobe...  
> > 
> > Right, but we should be able to add some recursion protection to stop
> > that. I have similar protection in the ftrace code.  
> 
> If we assume that __raw_spin_lock/unlock*() are always inlined a

I wouldn't assume that.

> possible way to prevent this recursion could be to use directly those
> functions to do locking from the kretprobe trampoline.
> 
> But I'm not sure if that's a safe assumption... if not I'll see if I can
> find a better solution.

All you need to do is have a per_cpu variable, where you just do:

	preempt_disable_notrace();
	if (this_cpu_read(kprobe_recursion))
		goto out;
	this_cpu_inc(kprobe_recursion);
	[...]
	this_cpu_dec(kprobe_recursion);
out:
	preempt_enable_notrace();

And then just ignore any kprobes that trigger while you are processing
the current kprobe.

Something like that. If you want (or if it already happens) replace
preempt_disable() with local_irq_save().

-- Steve

> 
> Thanks,
> 
> From: Andrea Righi <righi.andrea@gmail.com>
> Subject: [PATCH] kprobes: prevent recursion deadlock with kretprobe and
>  spinlocks
> 
> kretprobe_trampoline() is using a spinlock to protect the hash of
> kretprobes. Adding a kretprobe to the spinlock functions may cause
> a recursion deadlock where kretprobe is calling itself:
> 
>  kretprobe_trampoline()
>    -> trampoline_handler()
>      -> kretprobe_hash_lock()
>        -> raw_spin_lock_irqsave()
>          -> _raw_spin_lock_irqsave()  
>  kretprobe_trampoline from _raw_spin_lock_irqsave => DEADLOCK
> 
>  kretprobe_trampoline()
>    -> trampoline_handler()
>      -> recycle_rp_inst()
>        -> raw_spin_lock()
>          -> _raw_spin_lock()  
>  kretprobe_trampoline from _raw_spin_lock => DEADLOCK
> 
> Use the corresponding inlined spinlock functions to prevent this
> recursion.
> 
> Signed-off-by: Andrea Righi <righi.andrea@gmail.com>
> ---
>  kernel/kprobes.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index f4ddfdd2d07e..b89bef5e3d80 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -1154,9 +1154,9 @@ void recycle_rp_inst(struct kretprobe_instance *ri,
>  	hlist_del(&ri->hlist);
>  	INIT_HLIST_NODE(&ri->hlist);
>  	if (likely(rp)) {
> -		raw_spin_lock(&rp->lock);
> +		__raw_spin_lock(&rp->lock);
>  		hlist_add_head(&ri->hlist, &rp->free_instances);
> -		raw_spin_unlock(&rp->lock);
> +		__raw_spin_unlock(&rp->lock);
>  	} else
>  		/* Unregistering */
>  		hlist_add_head(&ri->hlist, head);
> @@ -1172,7 +1172,7 @@ __acquires(hlist_lock)
>  
>  	*head = &kretprobe_inst_table[hash];
>  	hlist_lock = kretprobe_table_lock_ptr(hash);
> -	raw_spin_lock_irqsave(hlist_lock, *flags);
> +	*flags = __raw_spin_lock_irqsave(hlist_lock);
>  }
>  NOKPROBE_SYMBOL(kretprobe_hash_lock);
>  
> @@ -1193,7 +1193,7 @@ __releases(hlist_lock)
>  	raw_spinlock_t *hlist_lock;
>  
>  	hlist_lock = kretprobe_table_lock_ptr(hash);
> -	raw_spin_unlock_irqrestore(hlist_lock, *flags);
> +	__raw_spin_unlock_irqrestore(hlist_lock, *flags);
>  }
>  NOKPROBE_SYMBOL(kretprobe_hash_unlock);
>  

Re: [PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem

[Andrea Righi]

On Mon, Jan 07, 2019 at 04:28:33PM -0500, Steven Rostedt wrote:
> On Mon, 7 Jan 2019 22:19:04 +0100
> Andrea Righi <righi.andrea@gmail.com> wrote:
> 
> > > > If we put a kretprobe to raw_spin_lock_irqsave() it looks like
> > > > kretprobe is going to call kretprobe...  
> > > 
> > > Right, but we should be able to add some recursion protection to stop
> > > that. I have similar protection in the ftrace code.  
> > 
> > If we assume that __raw_spin_lock/unlock*() are always inlined a
> 
> I wouldn't assume that.
> 
> > possible way to prevent this recursion could be to use directly those
> > functions to do locking from the kretprobe trampoline.
> > 
> > But I'm not sure if that's a safe assumption... if not I'll see if I can
> > find a better solution.
> 
> All you need to do is have a per_cpu variable, where you just do:
> 
> 	preempt_disable_notrace();
> 	if (this_cpu_read(kprobe_recursion))
> 		goto out;
> 	this_cpu_inc(kprobe_recursion);
> 	[...]
> 	this_cpu_dec(kprobe_recursion);
> out:
> 	preempt_enable_notrace();
> 
> And then just ignore any kprobes that trigger while you are processing
> the current kprobe.
> 
> Something like that. If you want (or if it already happens) replace
> preempt_disable() with local_irq_save().

Oh.. definitely much better. I'll work on that and send a new patch.
Thanks for the suggestion!

-Andrea

Re: [PATCH 2/2] kprobes: Mark ftrace mcount handler functions nokprobe

[Masami Hiramatsu]

On Mon, 7 Jan 2019 15:55:36 +0100
Andrea Righi <righi.andrea@gmail.com> wrote:

> On Mon, Jan 07, 2019 at 10:32:32PM +0900, Masami Hiramatsu wrote:
> > Mark ftrace mcount handler functions nokprobe since
> > probing on these functions with kretprobe pushes
> > return address incorrectly on kretprobe shadow stack.
> > 
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > Reported-by: Francis Deslauriers <francis.deslauriers@efficios.com>
> > ---
> >  kernel/trace/ftrace.c |    5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index f0ff24173a0b..ad4babad4a03 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -6250,7 +6250,7 @@ void ftrace_reset_array_ops(struct trace_array *tr)
> >  	tr->ops->func = ftrace_stub;
> >  }
> >  
> > -static inline void
> > +static nokprobe_inline void
> 
> I think we need to #include <linux/kprobes.h>, otherwise:
> 
>   CC      kernel/trace/ftrace.o
> kernel/trace/ftrace.c:6219:24: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘void’
>  static nokprobe_inline void
>                         ^~~~
> 
>  kernel/trace/ftrace.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 3a58ad280d83..0333241034d5 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -32,6 +32,7 @@
>  #include <linux/sort.h>
>  #include <linux/list.h>
>  #include <linux/hash.h>
> +#include <linux/kprobes.h>
>  #include <linux/rcupdate.h>
>  
>  #include <trace/events/sched.h>


Oops, I missed it while reordering other patches...

Thank you,

> 
> Thanks,
> -Andrea
> 
> >  __ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
> >  		       struct ftrace_ops *ignored, struct pt_regs *regs)
> >  {
> > @@ -6310,11 +6310,13 @@ static void ftrace_ops_list_func(unsigned long ip, unsigned long parent_ip,
> >  {
> >  	__ftrace_ops_list_func(ip, parent_ip, NULL, regs);
> >  }
> > +NOKPROBE_SYMBOL(ftrace_ops_list_func);
> >  #else
> >  static void ftrace_ops_no_ops(unsigned long ip, unsigned long parent_ip)
> >  {
> >  	__ftrace_ops_list_func(ip, parent_ip, NULL, NULL);
> >  }
> > +NOKPROBE_SYMBOL(ftrace_ops_no_ops);
> >  #endif
> >  
> >  /*
> > @@ -6341,6 +6343,7 @@ static void ftrace_ops_assist_func(unsigned long ip, unsigned long parent_ip,
> >  	preempt_enable_notrace();
> >  	trace_clear_recursion(bit);
> >  }
> > +NOKPROBE_SYMBOL(ftrace_ops_assist_func);
> >  
> >  /**
> >   * ftrace_ops_get_func - get the function a trampoline should call


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 2/2] kprobes: Mark ftrace mcount handler functions nokprobe

[Masami Hiramatsu]

On Mon, 7 Jan 2019 12:29:51 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Mon, 7 Jan 2019 15:55:36 +0100
> Andrea Righi <righi.andrea@gmail.com> wrote:
> 
> > On Mon, Jan 07, 2019 at 10:32:32PM +0900, Masami Hiramatsu wrote:
> > > Mark ftrace mcount handler functions nokprobe since
> > > probing on these functions with kretprobe pushes
> > > return address incorrectly on kretprobe shadow stack.
> > > 
> > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > > Reported-by: Francis Deslauriers <francis.deslauriers@efficios.com>
> > > ---
> > >  kernel/trace/ftrace.c |    5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > > index f0ff24173a0b..ad4babad4a03 100644
> > > --- a/kernel/trace/ftrace.c
> > > +++ b/kernel/trace/ftrace.c
> > > @@ -6250,7 +6250,7 @@ void ftrace_reset_array_ops(struct trace_array *tr)
> > >  	tr->ops->func = ftrace_stub;
> > >  }
> > >  
> > > -static inline void
> > > +static nokprobe_inline void  
> > 
> > I think we need to #include <linux/kprobes.h>, otherwise:
> > 
> >   CC      kernel/trace/ftrace.o
> > kernel/trace/ftrace.c:6219:24: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘void’
> >  static nokprobe_inline void
> >                         ^~~~
> > 
> >  kernel/trace/ftrace.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index 3a58ad280d83..0333241034d5 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -32,6 +32,7 @@
> >  #include <linux/sort.h>
> >  #include <linux/list.h>
> >  #include <linux/hash.h>
> > +#include <linux/kprobes.h>
> >  #include <linux/rcupdate.h>
> >  
> >  #include <trace/events/sched.h>
> 
> And zero day just caught it too.
> 
> Masami, want to fold this into your patch and send out a v2?

Yes, I'll do that.

Thank you,

> 
> -- Steve


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 0/2] kprobes: Fix kretprobe incorrect stacking order problem

[Masami Hiramatsu]

Hi Andrea and Steve,

On Mon, 7 Jan 2019 22:34:39 +0100
Andrea Righi <righi.andrea@gmail.com> wrote:

> On Mon, Jan 07, 2019 at 04:28:33PM -0500, Steven Rostedt wrote:
> > On Mon, 7 Jan 2019 22:19:04 +0100
> > Andrea Righi <righi.andrea@gmail.com> wrote:
> > 
> > > > > If we put a kretprobe to raw_spin_lock_irqsave() it looks like
> > > > > kretprobe is going to call kretprobe...  
> > > > 
> > > > Right, but we should be able to add some recursion protection to stop
> > > > that. I have similar protection in the ftrace code.  
> > > 
> > > If we assume that __raw_spin_lock/unlock*() are always inlined a
> > 
> > I wouldn't assume that.
> > 
> > > possible way to prevent this recursion could be to use directly those
> > > functions to do locking from the kretprobe trampoline.
> > > 
> > > But I'm not sure if that's a safe assumption... if not I'll see if I can
> > > find a better solution.
> > 
> > All you need to do is have a per_cpu variable, where you just do:
> > 
> > 	preempt_disable_notrace();
> > 	if (this_cpu_read(kprobe_recursion))
> > 		goto out;
> > 	this_cpu_inc(kprobe_recursion);
> > 	[...]
> > 	this_cpu_dec(kprobe_recursion);
> > out:
> > 	preempt_enable_notrace();
> > 
> > And then just ignore any kprobes that trigger while you are processing
> > the current kprobe.
> > 
> > Something like that. If you want (or if it already happens) replace
> > preempt_disable() with local_irq_save().
> 
> Oh.. definitely much better. I'll work on that and send a new patch.
> Thanks for the suggestion!

Thank you for pointing it out, 

Since we already have current_kprobe per_cpu, it can be done by setting up
a dummy kprobe on it. I'll add that in v2 series.

Actually, this bug has been introduced a long time ago by me... when I
introduced asm-coded kretprobe-trampoline. Before that, kretprobe trampoline
handler uses a kprobe to hook it, so the 2nd kretprobe must be skipped
automatically.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>