On 26.07.24 01:06, Steven Rostedt wrote: > On Thu, 25 Jul 2024 23:32:30 +0200 > Mathias Krause <mini...@grsecurity.net> 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.
Heureka, it did! Thanks, Steve! > > -- 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; > } > Will ack the patch you send. Thanks again, Mathias