Re: tracing: user events UAF crash report

[Steven Rostedt <rostedt@goodmis.org>]

On Thu, 25 Jul 2024 20:12:33 +0200
Mathias Krause <minipli@grsecurity.net> wrote:

> 
> >   
> >> +
> >>  	if (WARN_ON_ONCE(!schedule_work(&user->put_work))) {
> >>  		/*
> >>  		 * If we fail we must wait for an admin to attempt delete or
> >> @@ -973,6 +975,11 @@ size_t copy_nofault(void *addr, size_t bytes, struct iov_iter *i)
> >>  static struct list_head *user_event_get_fields(struct trace_event_call *call)
> >>  {
> >>  	struct user_event *user = (struct user_event *)call->data;  
> 
> Dereferencing a potentially free'd object, so 'user' is now "random" data.

This is the callback function of user->call.get_fields.

That is, we have:

	user->call.get_fields = user_event_get_fields;

And the f_start() code eventually calls trace_get_fields() that has (from a
previous email in this thread).

    trace_get_fields(struct trace_event_call *event_call)
    {
	if (!event_call->class->get_fields)
        	return &event_call->class->fields;
        return event_call->class->get_fields(event_call);
    }

Where it calls the ->class->get_fields(event_call);

that calls this function. By setting:

	user->call.get_fields = NULL;

this will never get called and no random data will be accessed.

That said, I was talking with Beau, we concluded that this shouldn't be the
responsibility of the user of event call, and should be cleaned up by the
event system.

> 
> >> +	static LIST_HEAD(head);
> >> +
> >> +	/* If the user event is about to be deleted, return no fields */
> >> +	if (!user)
> >> +		return &head;
> >>  
> >>  	return &user->fields;
> >>  }  

Here's the proper fix:

diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 6ef29eba90ce..3a2d2ff1625b 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c
@@ -3140,8 +3140,10 @@ EXPORT_SYMBOL_GPL(trace_add_event_call);
  */
 static void __trace_remove_event_call(struct trace_event_call *call)
 {
+	lockdep_assert_held(&event_mutex);
 	event_remove(call);
 	trace_destroy_fields(call);
+	call->get_fields = NULL;
 	free_event_filter(call->filter);
 	call->filter = NULL;
 }

Can you try it out?

-- Steve