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