Re: tracing: user events UAF crash report

[Steven Rostedt]

On Thu, 25 Jul 2024 23:32:30 +0200
Mathias Krause <minipli@xxxxxxxxxxxxxx> 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.
> 
> > 
> > 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