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

[Steven Rostedt <rostedt@goodmis.org>]

[ 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