On Thu, 25 Jul 2024 20:12:33 +0200
Mathias Krause <mini...@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
