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
