[PATCH v4] tracing: Fix use-after-free freeing trigger private data

[PATCH v4] tracing: Fix use-after-free freeing trigger private data

[David Carlier]

Commit 61d445af0a7c ("tracing: Add bulk garbage collection of freeing
event_trigger_data") moved the kfree() of event_trigger_data to a kthread
that runs tracepoint_synchronize_unregister() before freeing. That removed
the synchronization the trigger .free callbacks used to get implicitly and
inline from trigger_data_free().

event_hist_trigger_free(), event_hist_trigger_named_free() and
event_enable_trigger_free() free their satellite data (hist_data, cmd_ops,
enable_data) right after trigger_data_free() returns. With the
synchronization now deferred to the kthread, a concurrent tracepoint
handler can still reach that data through the list_del_rcu()'d trigger,
causing a use-after-free.

The histogram teardown must stay synchronous: remove_hist_vars() and
unregister_field_var_hists() have to detach a synthetic event from the
histogram before the trigger-removal write returns, otherwise a following
command races in and the synthetic-event removal fails with -EBUSY, as the
trigger-synthetic-eprobe.tc selftest catches. Make those callbacks wait
with the correct barrier - tracepoint_synchronize_unregister(), matching
the free kthread - before freeing.

The enable trigger has no such synchronous requirement, and a blocking
synchronize there would re-serialize the path that commit deliberately
deferred. Give it an optional private_data_free() callback that the free
kthread runs after its grace period, and free enable_data from there.

Suggested-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Suggested-by: Steven Rostedt <rostedt@goodmis.org>
Fixes: 61d445af0a7c ("tracing: Add bulk garbage collection of freeing event_trigger_data")
Signed-off-by: David Carlier <devnexen@gmail.com>
---
 kernel/trace/trace.h                |  1 +
 kernel/trace/trace_events_hist.c    |  2 ++
 kernel/trace/trace_events_trigger.c | 18 +++++++++++++++---
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 80fe152af1dd..bf77331f56a4 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -1941,6 +1941,7 @@ struct event_trigger_data {
 	struct list_head		named_list;
 	struct event_trigger_data	*named_data;
 	struct llist_node		llist;
+	void				(*private_data_free)(struct event_trigger_data *data);
 };
 
 /* Avoid typos */
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index 82ce492ab268..58d28cd1afa3 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -6349,6 +6349,7 @@ static void event_hist_trigger_free(struct event_trigger_data *data)
 
 		trigger_data_free(data);
 
+		tracepoint_synchronize_unregister();
 		remove_hist_vars(hist_data);
 
 		unregister_field_var_hists(hist_data);
@@ -6388,6 +6389,7 @@ static void event_hist_trigger_named_free(struct event_trigger_data *data)
 
 		del_named_trigger(data);
 		trigger_data_free(data);
+		tracepoint_synchronize_unregister();
 		kfree(cmd_ops);
 	}
 }
diff --git a/kernel/trace/trace_events_trigger.c b/kernel/trace/trace_events_trigger.c
index 655db2e82513..46e60b70a4bb 100644
--- a/kernel/trace/trace_events_trigger.c
+++ b/kernel/trace/trace_events_trigger.c
@@ -38,6 +38,13 @@ static void trigger_create_kthread_locked(void)
 	}
 }
 
+static void trigger_data_free_one(struct event_trigger_data *data)
+{
+	if (data->private_data_free)
+		data->private_data_free(data);
+	kfree(data);
+}
+
 static void trigger_data_free_queued_locked(void)
 {
 	struct event_trigger_data *data, *tmp;
@@ -52,7 +59,7 @@ static void trigger_data_free_queued_locked(void)
 	tracepoint_synchronize_unregister();
 
 	llist_for_each_entry_safe(data, tmp, llnodes, llist)
-		kfree(data);
+		trigger_data_free_one(data);
 }
 
 /* Bulk garbage collection of event_trigger_data elements */
@@ -75,7 +82,7 @@ static int trigger_kthread_fn(void *ignore)
 		tracepoint_synchronize_unregister();
 
 		llist_for_each_entry_safe(data, tmp, llnodes, llist)
-			kfree(data);
+			trigger_data_free_one(data);
 	}
 
 	return 0;
@@ -1717,6 +1724,11 @@ int event_enable_trigger_print(struct seq_file *m,
 	return 0;
 }
 
+static void enable_trigger_private_data_free(struct event_trigger_data *data)
+{
+	kfree(data->private_data);
+}
+
 void event_enable_trigger_free(struct event_trigger_data *data)
 {
 	struct enable_trigger_data *enable_data = data->private_data;
@@ -1728,9 +1740,9 @@ void event_enable_trigger_free(struct event_trigger_data *data)
 	if (!data->ref) {
 		/* Remove the SOFT_MODE flag */
 		trace_event_enable_disable(enable_data->file, 0, 1);
+		data->private_data_free = enable_trigger_private_data_free;
 		trace_event_put_ref(enable_data->file->event_call);
 		trigger_data_free(data);
-		kfree(enable_data);
 	}
 }
 
-- 
2.53.0

Re: [PATCH v4] tracing: Fix use-after-free freeing trigger private data

[Steven Rostedt]

[ Replying to Sashiko ]

> --- Patch [1]: [PATCH v4] tracing: Fix use-after-free freeing trigger private data ---
> commit cd29772ccd2634c1b696094501e9c7fd0bcd1c25
> Author: David Carlier <devnexen@gmail.com>
> 
> > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> > index 82ce492ab2681..58d28cd1afa3b 100644
> > --- a/kernel/trace/trace_events_hist.c
> > +++ b/kernel/trace/trace_events_hist.c
> > @@ -6349,6 +6349,7 @@ static void event_hist_trigger_free(struct event_trigger_data *data)
> >  
> >  		trigger_data_free(data);
> >  
> > +		tracepoint_synchronize_unregister();
> >  		remove_hist_vars(hist_data);
> >  
> >  		unregister_field_var_hists(hist_data);
> 
> This isn't a bug introduced by this patch, but since we are fixing the teardown
> paths here, do the error paths in event_hist_trigger_parse() and
> event_enable_trigger_parse() need similar synchronization?
> 
> For example, in event_hist_trigger_parse():
> 
> out_free:
> 	remove_hist_vars(hist_data);
> 	trigger_data_free(trigger_data);
> 	destroy_hist_data(hist_data);
> 
> If trigger registration fails after the trigger was briefly published to
> file->triggers, could a concurrent tracepoint that observed the trigger
> dereference the synchronously freed hist_data because we didn't wait for an RCU
> grace period?

Where was it published to file->triggers?

I see it happening as the last operation that can fail. If that fails, then
we hit the error path where it is not registered. Is there someplace it can
become visible and then fail that I'm not seeing?

From what I see, the last command before "success" is:

  ret = hist_trigger_enable(trigger_data, file);

And that does:

static int hist_trigger_enable(struct event_trigger_data *data,
			       struct trace_event_file *file)
{
	int ret = 0;

	list_add_tail_rcu(&data->list, &file->triggers);

	update_cond_flag(file);

	if (trace_event_trigger_enable_disable(file, 1) < 0) {
		list_del_rcu(&data->list);
		update_cond_flag(file);
		ret--;
	}

	return ret;
}

Now it adds to the file trigger and then tries to enable that event. If it
fails to enable it, it removes it from the list. But if the event is not
enabled, it can't be racing with this.

> 
> [ ... ]
> 
> > diff --git a/kernel/trace/trace_events_trigger.c b/kernel/trace/trace_events_trigger.c
> > index 655db2e825134..46e60b70a4bbc 100644
> > --- a/kernel/trace/trace_events_trigger.c
> > +++ b/kernel/trace/trace_events_trigger.c
> > @@ -1728,9 +1740,9 @@ void event_enable_trigger_free(struct event_trigger_data *data)
> >  	if (!data->ref) {
> >  		/* Remove the SOFT_MODE flag */
> >  		trace_event_enable_disable(enable_data->file, 0, 1);
> > +		data->private_data_free = enable_trigger_private_data_free;
> >  		trace_event_put_ref(enable_data->file->event_call);
> >  		trigger_data_free(data);
> > -		kfree(enable_data);
> >  	}
> >  }
> 
> This is a pre-existing issue, but by deferring the kfree of enable_data while
> keeping trace_event_put_ref() synchronous, could we still face a Use-After-Free
> issue during module unload?
> 
> If trace_event_put_ref(enable_data->file->event_call) allows the target module
> to be unloaded and its trace event files to be freed immediately, could a
> concurrent tracepoint RCU reader dereference the freed enable_data->file in
> event_enable_trigger() when attempting to clear
> EVENT_FILE_FL_SOFT_DISABLED_BIT?

Hmm, this does look like it can be an issue. I'll write up a patch on top
of this one.

-- Steve