Re: tracing: user events UAF crash report

[Mathias Krause]

On 26.07.24 01:06, Steven Rostedt wrote:
> 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.

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