Re: tracing: user events UAF crash report

On Thu, 25 Jul 2024 23:32:30 +0200
Mathias Krause [off-list ref] wrote:

That was for a single run of
tools/testing/selftests/user_events/ftrace_test with the read loop of
/sys/kernel/tracing/events/user_events/__test_event/format in a
different shell.

quoted

destroy_user_event() which is under event_mutex calls
user_event_set_call_visible() with false, that will then call:

trace_remove_event_call() -> probe_remove_event_call() ->
 __trace_remove_event_call() -> event_remove() ->
 remove_event_from_tracers()

Where remove_event_from_tracers() loops over all the instances and will set
each of the file pointers flags associated to the event: EVENT_FILE_FL_FREED

Then it returns back to destroy_user_event() that would free the event.

The f_start() that was in your crash, with the new patch, should take the
event_mutex before referencing the event that was freed. And with that flag
being set, it should exit out.  

Looking at the very first report:

[   76.306946] BUG: KASAN: slab-use-after-free in f_start+0x36e/0x3d0

That's what faddr2line gives me:

f_start+0x36e/0x3d0:
f_start at kernel/trace/trace_events.c:1637 (discriminator 1)

Which is:
1635     mutex_lock(&event_mutex);
1636     file = event_file_data(m->private);
1637     if (!file || (file->flags & EVENT_FILE_FL_FREED))
1638         return ERR_PTR(-ENODEV);

BAH! I finally figured it out.

I was able to reproduce it and this does stop the UAF from happening.

The issue was, as a short cut, I had the "format" file's i_private point to
the "call" entry directly, and not go via the "file". This is because the
all format files are the same for the same "call", so no reason to
differentiate them.  The other files maintain state (like the "enable",
"trigger", etc). But this means if the file were to disappear, the "format"
file would be unaware of it.

This should fix it for you. It fixed it for me.

-- Steve

diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 6ef29eba90ce..852643d957de 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c

@@ -1540,7 +1540,8 @@ enum {
 
 static void *f_next(struct seq_file *m, void *v, loff_t *pos)
 {
-	struct trace_event_call *call = event_file_data(m->private);
+	struct trace_event_file *file = event_file_data(m->private);
+	struct trace_event_call *call = file->event_call;
 	struct list_head *common_head = &ftrace_common_fields;
 	struct list_head *head = trace_get_fields(call);
 	struct list_head *node = v;

@@ -1572,7 +1573,8 @@ static void *f_next(struct seq_file *m, void *v, loff_t *pos)
 
 static int f_show(struct seq_file *m, void *v)
 {
-	struct trace_event_call *call = event_file_data(m->private);
+	struct trace_event_file *file = event_file_data(m->private);
+	struct trace_event_call *call = file->event_call;
 	struct ftrace_event_field *field;
 	const char *array_descriptor;
 

@@ -1627,12 +1629,14 @@ static int f_show(struct seq_file *m, void *v)
 
 static void *f_start(struct seq_file *m, loff_t *pos)
 {
+	struct trace_event_file *file;
 	void *p = (void *)FORMAT_HEADER;
 	loff_t l = 0;
 
 	/* ->stop() is called even if ->start() fails */
 	mutex_lock(&event_mutex);
-	if (!event_file_data(m->private))
+	file = event_file_data(m->private);
+	if (!file || (file->flags & EVENT_FILE_FL_FREED))
 		return ERR_PTR(-ENODEV);
 
 	while (l < *pos && p)

@@ -2485,7 +2489,6 @@ static int event_callback(const char *name, umode_t *mode, void **data,
 	if (strcmp(name, "format") == 0) {
 		*mode = TRACE_MODE_READ;
 		*fops = &ftrace_event_format_fops;
-		*data = call;
 		return 1;
 	}
 

-- 
2.43.0