Message-ID: <20240426073410.17154-1-Tze-nan.Wu@mediatek.com>
Date: Fri, 26 Apr 2024 15:34:08 +0800
From: Tze-nan wu <Tze-nan.Wu@...iatek.com>
To: <rostedt@...dmis.org>, <mhiramat@...nel.org>
CC: <bobule.chang@...iatek.com>, <wsd_upstream@...iatek.com>, Tze-nan wu
	<Tze-nan.Wu@...iatek.com>, Cheng-Jui Wang <cheng-jui.wang@...iatek.com>,
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
	<linux-kernel@...r.kernel.org>, <linux-trace-kernel@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>, <linux-mediatek@...ts.infradead.org>
Subject: [PATCH] tracing: Fix uaf issue in tracing_open_file_tr

"tracing_event_file" is at the risk of use-after-free due to the race of
two functions "tracing_open_file_tr" and "synth_event_release".
Specifically, it could be freed by synth_event_release before
tracing_open_file_tr has the opportunity to access its members.

It's easy to reproduced by first executing script 'A' and then script 'B'
in my environment.

  Script 'A':
    echo "hello int aaa" > /sys/kernel/tracing/synthetic_events
    while :
    do
      echo 0 > /sys/kernel/tracing/events/synthetic/hello/enable
    done

  Script 'B':
    echo > /sys/kernel/tracing/synthetic/events

  My environment:
    arm64 + generic and swtag based KASAN both enabled + kernel-6.6.18

KASAN report
==================================================================
BUG: KASAN: slab-use-after-free in tracing_open_file_tr
Read of size 8 at addr 9*ffff********** by task sh/3485
Pointer tag: [9*], memory tag: [fe]

CPU: * PID: 3485 Comm: sh Tainted: ****************
Call trace:
 __hwasan_tag_mismatch
 tracing_open_file_tr
 do_dentry_open
 vfs_open
 path_openat

Freed by task 3490:
 slab_free_freelist_hook
 kmem_cache_free
 event_file_put
 remove_event_file_dir
 __trace_remove_event_call
 trace_remove_event_call
 synth_event_release
 dyn_events_release_all
 synth_events_open

page last allocated via order 0, migratetype Unmovable:
 __slab_alloc
 kmem_cache_alloc
 trace_create_new_event
 trace_add_event_call
 register_synth_event
 __create_synth_event
 create_or_delete_synth_event
 trace_parse_run_command
 synth_events_write
 vfs_write

Based on the assumption that eventfs_inode will persist throughout the
execution of the tracing_open_file_tr function,
we can fix this issue by incrementing the reference count of
trace_event_file once it is assigned to eventfs_inode->data.
The reference count will then be decremented during the release phase of
eventfs_inode.

This ensures that trace_event_file is not prematurely freed while the
tracing_open_file_tr function is being called.

Fixes: bb32500fb9b7 ("tracing: Have trace_event_file have ref counters")
Co-developed-by: Cheng-Jui Wang <cheng-jui.wang@...iatek.com>
Signed-off-by: Cheng-Jui Wang <cheng-jui.wang@...iatek.com>
Signed-off-by: Tze-nan wu <Tze-nan.Wu@...iatek.com>
---
BTW, I've also attempted to reproduce the same issue in another
environment (described below).
It's also reproducible but in a lower rate.

another environment:
  x86 + ubuntu + generic kasan enabled + kernel-6.9-rc4

After applying the patch, KASAN no longer reports any issues when
following the same reproduction steps in my arm64 environment. 
However, I am concerned about potential side effects that the patch might introduce.
Additionally, the newly added function "is_entry_event_callback" may not
be reliable in my opinion,
as the callback function used in the comparison could change in future. 
Nonetheless, this is the best solution I can come up with now.

Looking for any suggestion or solution, appreciate.

 fs/tracefs/event_inode.c     | 3 +++
 include/linux/trace_events.h | 4 ++++
 kernel/trace/trace_events.c  | 6 ++++++
 3 files changed, 13 insertions(+)

diff --git a/fs/tracefs/event_inode.c b/fs/tracefs/event_inode.c
index 894c6ca1e500..fd49c0f88500 100644
--- a/fs/tracefs/event_inode.c
+++ b/fs/tracefs/event_inode.c
@@ -20,6 +20,7 @@
 #include <linux/workqueue.h>
 #include <linux/security.h>
 #include <linux/tracefs.h>
+#include <linux/trace_events.h>
 #include <linux/kref.h>
 #include <linux/delay.h>
 #include "internal.h"
@@ -90,6 +91,8 @@ static void release_ei(struct kref *ref)
 
 	kfree(ei->entry_attrs);
 	kfree_const(ei->name);
+	if (ei->nr_entries && is_entry_event_callback(ei->entries[0]))
+		event_file_put(ei->data);
 	if (ei->is_events) {
 		rei = get_root_inode(ei);
 		kfree_rcu(rei, ei.rcu);
diff --git a/include/linux/trace_events.h b/include/linux/trace_events.h
index 6f9bdfb09d1d..602e87682ee2 100644
--- a/include/linux/trace_events.h
+++ b/include/linux/trace_events.h
@@ -9,6 +9,7 @@
 #include <linux/hardirq.h>
 #include <linux/perf_event.h>
 #include <linux/tracepoint.h>
+#include <linux/tracefs.h>
 
 struct trace_array;
 struct array_buffer;
@@ -505,6 +506,7 @@ extern struct trace_event_file *trace_get_event_file(const char *instance,
 						     const char *system,
 						     const char *event);
 extern void trace_put_event_file(struct trace_event_file *file);
+bool is_entry_event_callback(struct eventfs_entry entry);
 
 #define MAX_DYNEVENT_CMD_LEN	(2048)
 
@@ -731,6 +733,8 @@ extern void
 event_triggers_post_call(struct trace_event_file *file,
 			 enum event_trigger_type tt);
 
+extern void event_file_put(struct trace_event_file *file);
+
 bool trace_event_ignore_this_pid(struct trace_event_file *trace_file);
 
 bool __trace_trigger_soft_disabled(struct trace_event_file *file);
diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 52f75c36bbca..de01676b59ff 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -2626,6 +2626,7 @@ event_create_dir(struct eventfs_inode *parent, struct trace_event_file *file)
 		return -1;
 	}
 
+	event_file_get(file);
 	file->ei = ei;
 
 	ret = event_define_fields(call);
@@ -3420,6 +3421,11 @@ void trace_put_event_file(struct trace_event_file *file)
 }
 EXPORT_SYMBOL_GPL(trace_put_event_file);
 
+bool is_entry_event_callback(struct eventfs_entry entry)
+{
+	return entry.callback == event_callback;
+}
+
 #ifdef CONFIG_DYNAMIC_FTRACE
 
 /* Avoid typos */
-- 
2.18.0

Powered by blists - more mailing lists