Re: tracing: user events UAF crash report

On Thu, 25 Jul 2024 21:42:41 +0200
Mathias Krause [off-list ref] wrote:

Right. But the point is, that 'event_call' is really some '&user->call'.
With 'user' being free'd memory, what gives? Dereferencing 'event_call'
is UB, so this function is doomed to fail because it cannot know if its
only argument points to still valid memory or not. And that's the core
issue -- calling that function for an object that's long gone -- the
missing refcounting I hinted at in my first Email.

Ah, I missed that the call was part of the user structure. But I think I
found the real fix.

quoted

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.  

As 'user' is free'd or soon-to-be-free'd memory, that's a non-starter.

quoted

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.

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;

Actually, this is wrong as it would be: call->class->get_fields, and this
is not the right place to clear it, as class can be used by multiple calls.

quoted

 	free_event_filter(call->filter);
 	call->filter = NULL;
 }

Can you try it out?  

I can try but I don't think that's the proper fix for above reasons, I'm
sorry.

I believe the issue is that f_start() needs to check if the event file has
been freed.

New patch:

diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
index 6ef29eba90ce..5fbfa1c885de 100644
--- a/kernel/trace/trace_events.c
+++ b/kernel/trace/trace_events.c

@@ -1627,12 +1627,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)


-- Steve