[PATCH 0/2] Fix the race on &event->ftrace_ops between perf and ftrace

[PATCH 0/2] Fix the race on &event->ftrace_ops between perf and ftrace

[Li Huafei]

There is a race on &event->ftrace_ops between perf_alloc_event() and
__ftrace_ops_list_func(). When adding a perf event, if the event needs
to use the ftrace framework, it needs to register ftrace_ops with
ftrace, which is a structural member of perf event. If perf_alloc_event()
fails, it will free the allocated event directly, but if ftrace_ops has
been successfully registered, and the probe point is triggered, then
__ftrace_ops_list_func() may still reference the ftrace_ops that perf
just registered, but it has been freed with the event is freed,
so use-after-free happens.

This race condition was triggered when we did the fuzz test, see the commit
message of PATCH 2 for detailed logs. This patch set fixes this race.

Li Huafei (2):
  ftrace: Update ftrace_ops->next pointer with rcu_assign_pointer()
  perf, ftrace: Fix use-after-free in __ftrace_ops_list_func()

 include/linux/ftrace.h          | 15 +++++++++++++++
 kernel/trace/ftrace.c           |  4 ++--
 kernel/trace/trace_event_perf.c |  7 +++++++
 3 files changed, 24 insertions(+), 2 deletions(-)

-- 
2.17.1

[PATCH 1/2] ftrace: Update ftrace_ops->next pointer with rcu_assign_pointer()

[Li Huafei]

The unregistered ftrace_ops may be freed by the caller, so we should use
rcu_assign_pointer() in remove_ftrace_ops() to remove the ftrace_ops,
which ensures that no more users will reference the ftrace_ops after
synchronize_rcu() is called.

Signed-off-by: Li Huafei <lihuafei1@huawei.com>
---
 kernel/trace/ftrace.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 4d8e35575549..2e315a145d20 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -282,7 +282,7 @@ static int remove_ftrace_ops(struct ftrace_ops __rcu **list,
 			lockdep_is_held(&ftrace_lock)) == ops &&
 	    rcu_dereference_protected(ops->next,
 			lockdep_is_held(&ftrace_lock)) == &ftrace_list_end) {
-		*list = &ftrace_list_end;
+		rcu_assign_pointer(*list, &ftrace_list_end);
 		return 0;
 	}
 
@@ -293,7 +293,7 @@ static int remove_ftrace_ops(struct ftrace_ops __rcu **list,
 	if (*p != ops)
 		return -1;
 
-	*p = (*p)->next;
+	rcu_assign_pointer(*p, (*p)->next);
 	return 0;
 }
 
-- 
2.17.1

[PATCH 2/2] perf, ftrace: Fix use-after-free in __ftrace_ops_list_func()

[Li Huafei]

We got a use-after-free report when doing kernel fuzz tests with KSSAN
turned on:

[ 1367.884099] BUG: KASAN: use-after-free in ftrace_ops_list_func+0xf7/0x220
[ 1367.885153] Read of size 8 at addr ffff8884f81a47d0 by tasksyz-executor/99086
[ 1367.886517] CPU: 2 PID: 99086 Comm: syz-executor Kdump: loaded Tainted: G                 --------- -t -4.18.0-147.5.1.2.h379.kasan.eulerosv2r9.x86_64 #1
[ 1367.886522] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS ?-20190727_073836-buildvm-ppc64le-16.ppc.fedoraproject.org-3.fc31 04/01/2014
[ 1367.886525] Call Trace:
[ 1367.886534]  dump_stack+0xc2/0x12e
[ 1367.886542]  ? orc_sort_cmp+0xb0/0xb0
[ 1367.886551]  print_address_description+0x70/0x360
[ 1367.886558]  ? orc_sort_cmp+0xb0/0xb0
[ 1367.886566]  ? perf_trace_buf_alloc+0x190/0x190
[ 1367.886571]  kasan_report+0x1b2/0x330
[ 1367.886578]  ? ftrace_ops_list_func+0xf7/0x220
[ 1367.886585]  ? orc_find+0x560/0x5a0
[ 1367.886597]  ? ftrace_ops_list_func+0xf7/0x220
[ 1367.886603]  ftrace_ops_list_func+0xf7/0x220
[ 1367.886609]  ? __save_stack_trace+0x92/0x100
[ 1367.886616]  ftrace_call+0x5/0x34
[ 1367.886623]  ? do_syscall_64+0x98/0x2c0
[ 1367.886629]  ? do_syscall_64+0x98/0x2c0
[ 1367.886635]  ? deref_stack_reg+0xd0/0xd0
[ 1367.886644]  ? unwind_get_return_address+0x5/0x50
[ 1367.886651]  unwind_get_return_address+0x5/0x50
[ 1367.886656]  __save_stack_trace+0x92/0x100
[ 1367.886665]  ? do_syscall_64+0x98/0x2c0
[ 1367.886673]  save_stack+0x47/0xd0
[ 1367.886680]  ? __kasan_slab_free+0x130/0x180
[ 1367.886685]  ? kfree+0xa5/0x1e0
[ 1367.886692]  ? cgroup_show_path+0x1fd/0x250
[ 1367.886699]  ? kernfs_sop_show_path+0xad/0xf0
[ 1367.886705]  ? show_mountinfo+0x169/0x4c0
[ 1367.886712]  ? seq_read+0x716/0x950
[ 1367.886718]  ? __vfs_read+0x55/0xb0
[ 1367.886723]  ? vfs_read+0xe7/0x210
[ 1367.886729]  ? ksys_pread64+0x95/0xd0
[ 1367.886734]  ? objects_show+0x10/0x10
[ 1367.886740]  ? ftrace_ops_test+0xba/0x120
[ 1367.886746]  ? ftrace_find_tramp_ops_next+0x90/0x90
[ 1367.886753]  ? ftrace_find_tramp_ops_next+0x90/0x90
[ 1367.886760]  ? ftrace_find_tramp_ops_next+0x90/0x90
[ 1367.886766]  ? objects_show+0x10/0x10
[ 1367.886772]  ? ftrace_ops_list_func+0x147/0x220
[ 1367.886778]  ? __kasan_slab_free+0xac/0x180
[ 1367.886784]  ? cgroup_show_path+0x1fd/0x250
[ 1367.886790]  ? ftrace_call+0x5/0x34
[ 1367.886796]  ? cgroup_show_path+0x1fd/0x250
[ 1367.886802]  ? cgroup_show_path+0x1fd/0x250
[ 1367.886811]  ? fixup_red_left+0x5/0x30
[ 1367.886817]  ? cgroup_show_path+0x1fd/0x250
[ 1367.886824]  __kasan_slab_free+0x130/0x180
[ 1367.886831]  ? cgroup_show_path+0x1fd/0x250
[ 1367.886835]  kfree+0xa5/0x1e0
[ 1367.886842]  cgroup_show_path+0x1fd/0x250
[ 1367.886850]  ? init_and_link_css+0x370/0x370
[ 1367.886856]  kernfs_sop_show_path+0xad/0xf0
[ 1367.886863]  show_mountinfo+0x169/0x4c0
[ 1367.886869]  ? kernfs_test_super+0x80/0x80
[ 1367.886875]  ? show_vfsmnt+0x270/0x270
[ 1367.886880]  ? m_next+0x32/0x80
[ 1367.886886]  ? show_vfsmnt+0x270/0x270
[ 1367.886891]  ? m_show+0x31/0x50
[ 1367.886900]  seq_read+0x716/0x950
[ 1367.886911]  ? seq_lseek+0x1e0/0x1e0
[ 1367.886916]  ? ftrace_call+0x5/0x34
[ 1367.886922]  ? ftrace_call+0x5/0x34
[ 1367.886931]  ? seq_lseek+0x1e0/0x1e0
[ 1367.886938]  __vfs_read+0x55/0xb0
[ 1367.886945]  vfs_read+0xe7/0x210
[ 1367.886954]  ksys_pread64+0x95/0xd0
[ 1367.886961]  do_syscall_64+0x98/0x2c0
[ 1367.886971]  entry_SYSCALL_64_after_hwframe+0x65/0xca
[ 1367.886976] RIP: 0033:0x46436d
[ 1367.886983] Code: 02 b8 ff ff ff ff c3 66 0f 1f 44 00 00 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b4 ff ff ff f7 d8 64 89 01 48
[ 1367.886987] RSP: 002b:00007f83ffff4c28 EFLAGS: 00000246 ORIG_RAX: 0000000000000011
[ 1367.886999] RAX: ffffffffffffffda RBX: 000000000057cfa0 RCX: 000000000046436d
[ 1367.887002] RDX: 0000000000001000 RSI: 0000000020000140 RDI: 0000000000000003
[ 1367.887006] RBP: 000000000057cfa0 R08: 0000000000000000 R09: 0000000000000000
[ 1367.887009] R10: 0000000000000000 R11: 0000000000000246 R12: 000000000057cfac
[ 1367.887013] R13: 00007f83ffff5700 R14: 00000000004d1e47 R15: 0000000000000fff

[ 1367.887275] Allocated by task 99101:
[ 1367.887848]  kasan_kmalloc+0xa0/0xd0
[ 1367.887853]  kmem_cache_alloc_trace+0xfc/0x220
[ 1367.887860]  perf_event_alloc.part.19+0x50/0x14d0
[ 1367.887865]  perf_event_alloc+0x67/0x90
[ 1367.887871]  __do_sys_perf_event_open+0x20e/0x14c0
[ 1367.887876]  do_syscall_64+0x98/0x2c0
[ 1367.887882]  entry_SYSCALL_64_after_hwframe+0x65/0xca

[ 1367.888133] Freed by task 99101:
[ 1367.888651]  __kasan_slab_free+0x130/0x180
[ 1367.888655]  kfree+0xa5/0x1e0
[ 1367.888661]  perf_event_alloc.part.19+0xca4/0x14d0
[ 1367.888666]  perf_event_alloc+0x67/0x90
[ 1367.888672]  __do_sys_perf_event_open+0x20e/0x14c0
[ 1367.888677]  do_syscall_64+0x98/0x2c0
[ 1367.888683]  entry_SYSCALL_64_after_hwframe+0x65/0xca

[ 1367.888935] The buggy address belongs to the object at ffff8884f81a4400
		which belongs to the cache kmalloc-2k of size 2048
[ 1367.890854] The buggy address is located 976 bytes inside of
		2048-byte region [ffff8884f81a4400, ffff8884f81a4c00)
[ 1367.892661] The buggy address belongs to the page:
[ 1367.893404] page:ffffea0013e06800 count:1 mapcount:0 mapping:ffff888107c0cf00 index:0x0 compound_mapcount: 0
[ 1367.894915] flags: 0x17ffffc0008100(slab|head)
[ 1367.895613] raw: 0017ffffc0008100 ffffea0014bda208 ffffea00140c4208 ffff888107c0cf00
[ 1367.896808] raw: 0000000000000000 00000000000f000f 00000001ffffffff 0000000000000000
[ 1367.898000] page dumped because: kasan: bad access detected

[ 1367.899107] Memory state around the buggy address:
[ 1367.899880]  ffff8884f81a4680: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[ 1367.900995]  ffff8884f81a4700: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[ 1367.902106] >ffff8884f81a4780: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[ 1367.903218]

[ 1367.904122]  ffff8884f81a4800: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[ 1367.905234]  ffff8884f81a4880: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb

There is a race on 'event' between perf_alloc_event() and
__ftrace_ops_list_func(). When adding a perf event, if the event needs
to use the ftrace framework, it needs to register ftrace_ops with
ftrace, which is a structural member of perf event. If perf_alloc_event()
fails, it will free the allocated event directly, but if ftrace_ops has
been successfully registered, and the probe point is triggered, then
__ftrace_ops_list_func() may still reference the ftrace_ops that perf
just registered, but it has been freed with the event is freed,
so use-after-free happens.

We see the comment of ftrace_ops in include/linux/ftrace.h, which
actually mentions that for dynamically allocated ftrace_ops, after
unregistering it should be guaranteed that no user will reference that
ftrace_ops again, but the current interface unregister_ftrace_function()
does not seem to guarantee this.

We add the ftrace_function_unregister_sync() interface to give users
like perf event a chance to synchronize. The reason we don't do
synchronization directly in unregister_ftrace_function() is that for
some users whose registered ftrace_ops is static, synchronization is not
necessary for them and is performance intensive, so we add a separate
interface.

Signed-off-by: Li Huafei <lihuafei1@huawei.com>
---
 include/linux/ftrace.h          | 15 +++++++++++++++
 kernel/trace/trace_event_perf.c |  7 +++++++
 2 files changed, 22 insertions(+)

diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 86e5028bfa20..c87de01f792e 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -282,6 +282,20 @@ extern enum ftrace_tracing_type_t ftrace_tracing_type;
 int register_ftrace_function(struct ftrace_ops *ops);
 int unregister_ftrace_function(struct ftrace_ops *ops);
 
+/*
+ * If the ftrace_ops is dynamically allocated, the unregistering of it
+ * should call ftrace_function_unregister_sync() to make sure that there
+ * are no more users references it. Then free it should be safe for ftrace.
+ *
+ * We don't call synchronize_rcu() in unregister_ftrace_function() because
+ * some of the registered ftrace_ops are static and doing synchronize_rcu()
+ * for them is unnecessary and is a performance penalty.
+ */
+static inline void ftrace_function_unregister_sync(void)
+{
+	synchronize_rcu();
+}
+
 extern void ftrace_stub(unsigned long a0, unsigned long a1,
 			struct ftrace_ops *op, struct ftrace_regs *fregs);
 
@@ -292,6 +306,7 @@ extern void ftrace_stub(unsigned long a0, unsigned long a1,
  */
 #define register_ftrace_function(ops) ({ 0; })
 #define unregister_ftrace_function(ops) ({ 0; })
+static inline void ftrace_function_unregister_sync(void) { }
 static inline void ftrace_kill(void) { }
 static inline void ftrace_free_init_mem(void) { }
 static inline void ftrace_free_mem(struct module *mod, void *start, void *end) { }
diff --git a/kernel/trace/trace_event_perf.c b/kernel/trace/trace_event_perf.c
index 288ad2c274fb..4749f6f6adf8 100644
--- a/kernel/trace/trace_event_perf.c
+++ b/kernel/trace/trace_event_perf.c
@@ -494,6 +494,13 @@ static int perf_ftrace_function_unregister(struct perf_event *event)
 {
 	struct ftrace_ops *ops = &event->ftrace_ops;
 	int ret = unregister_ftrace_function(ops);
+
+	/*
+	 * We may free the event later, before we free the event, make sure
+	 * no user references &event->ftrace_ops.
+	 */
+	ftrace_function_unregister_sync();
+
 	ftrace_free_filter(ops);
 	return ret;
 }
-- 
2.17.1

Re: [PATCH 2/2] perf, ftrace: Fix use-after-free in __ftrace_ops_list_func()

[Steven Rostedt]

On Wed, 17 Mar 2021 18:25:29 +0800
Li Huafei <lihuafei1@huawei.com> wrote:

> We see the comment of ftrace_ops in include/linux/ftrace.h, which
> actually mentions that for dynamically allocated ftrace_ops, after
> unregistering it should be guaranteed that no user will reference that
> ftrace_ops again, but the current interface unregister_ftrace_function()
> does not seem to guarantee this.
> 
> We add the ftrace_function_unregister_sync() interface to give users
> like perf event a chance to synchronize. The reason we don't do
> synchronization directly in unregister_ftrace_function() is that for
> some users whose registered ftrace_ops is static, synchronization is not
> necessary for them and is performance intensive, so we add a separate
> interface.

You are papering over a real bug then.

Also, synchronize_rcu() is not strong enough. Callbacks can happen that
reference ftrace_ops that are not protected by standard RCU. You need
full synchronize_rcu_tasks_rude() which is much more disruptive that
normal synchronize_rcu().

The code has:

	if (ops->flags & FTRACE_OPS_FL_DYNAMIC) {
		/*
		 * We need to do a hard force of sched synchronization.
		 * This is because we use preempt_disable() to do RCU, but
		 * the function tracers can be called where RCU is not watching
		 * (like before user_exit()). We can not rely on the RCU
		 * infrastructure to do the synchronization, thus we must do it
		 * ourselves.
		 */
		synchronize_rcu_tasks_rude();

		/*
		 * When the kernel is preemptive, tasks can be preempted
		 * while on a ftrace trampoline. Just scheduling a task on
		 * a CPU is not good enough to flush them. Calling
		 * synchronize_rcu_tasks() will wait for those tasks to
		 * execute and either schedule voluntarily or enter user space.
		 */
		if (IS_ENABLED(CONFIG_PREEMPTION))
			synchronize_rcu_tasks();

 free_ops:
		ftrace_trampoline_free(ops);
	}

And what you are saying is that we are not getting there, where the
dynamically allocated perf ops is not set to be DYNAMIC?

That should be set as DYNAMIC if the ops was allocated, and can later
be freed. This code was written specifically to handle perf.

Thus, NACK on the patch. I want to know exactly what went wrong instead
of just saying "but the current interface unregister_ftrace_function()
does not seem to guarantee this", let's actually fix the bug and not just paper over it!

Re: [PATCH 2/2] perf, ftrace: Fix use-after-free in __ftrace_ops_list_func()

[Steven Rostedt]

On Wed, 24 Mar 2021 21:11:23 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:


> And what you are saying is that we are not getting there, where the
> dynamically allocated perf ops is not set to be DYNAMIC?
> 
> That should be set as DYNAMIC if the ops was allocated, and can later
> be freed. This code was written specifically to handle perf.
> 
> Thus, NACK on the patch. I want to know exactly what went wrong instead
> of just saying "but the current interface unregister_ftrace_function()
> does not seem to guarantee this", let's actually fix the bug and not just paper over it!

Bah, I forgot to edit the claws-mail shortcut to disable "Ctrl-Enter"
to send the email, and went to hit Ctrl-L (to do the line wrapping) but
accidentally hit Ctrl-Enter instead, and sent this prematurely. And now
I forgot what else I was going to say. Sucks getting old.

-- Steve
 

Re: [PATCH 1/2] ftrace: Update ftrace_ops->next pointer with rcu_assign_pointer()

[Steven Rostedt]

On Wed, 17 Mar 2021 18:25:28 +0800
Li Huafei <lihuafei1@huawei.com> wrote:

> The unregistered ftrace_ops may be freed by the caller, so we should use
> rcu_assign_pointer() in remove_ftrace_ops() to remove the ftrace_ops,
> which ensures that no more users will reference the ftrace_ops after
> synchronize_rcu() is called.

The patch is fine, but we don't use synchronize_rcu() to protect the
list. We use synchronize_rcu_tasks().

-- Steve


> 
> Signed-off-by: Li Huafei <lihuafei1@huawei.com>
> ---
>  kernel/trace/ftrace.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 4d8e35575549..2e315a145d20 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -282,7 +282,7 @@ static int remove_ftrace_ops(struct ftrace_ops __rcu **list,
>  			lockdep_is_held(&ftrace_lock)) == ops &&
>  	    rcu_dereference_protected(ops->next,
>  			lockdep_is_held(&ftrace_lock)) == &ftrace_list_end) {
> -		*list = &ftrace_list_end;
> +		rcu_assign_pointer(*list, &ftrace_list_end);
>  		return 0;
>  	}
>  
> @@ -293,7 +293,7 @@ static int remove_ftrace_ops(struct ftrace_ops __rcu **list,
>  	if (*p != ops)
>  		return -1;
>  
> -	*p = (*p)->next;
> +	rcu_assign_pointer(*p, (*p)->next);
>  	return 0;
>  }
>