Re: [PATCH] tracing/histograms: Fix use-after-free with same-name named triggers

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Missing rollback of data->ops->init() when tracing_set_clock() fails, leading to a use-after-free in the global named_triggers list.
- [Critical] Double-free and use-after-free of `hist_data` and global list corruption of `trigger_data` when a reused named trigger successfully registers but fails to enable.
--

--- Patch [1]: [PATCH] tracing/histograms: Fix use-after-free with same-name named triggers ---
commit 5aedb1f49e82add364d36e7e38374f9abcb9b2b9
Author: Hui Su <[email protected]>

tracing/histograms: Fix use-after-free with same-name named triggers

When two hist triggers on different events are registered with the same
name=, the second one reuses the first as named_data. Both are added to
tr->hist_vars by save_hist_vars() during event_hist_trigger_parse(),
but the named reuse is only detected later. This commit fixes a dangling
pointer issue by properly removing the hist_data from tr->hist_vars before
freeing it.

> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index fdf784620c283..50e86f81ea32a 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -6044,8 +6044,10 @@ static int hist_register_trigger(char *glob,
>  		tracing_set_filter_buffering(file->tr, true);
>  	}
>  

[Severity: Critical]
This is a pre-existing issue, but I noticed a potential problem in the error
handling right above this block when tracing_set_clock() fails.

If tracing_set_clock() fails, the code executes goto out, returning without
rolling back the data->ops->init() call that added the trigger to the global
list.

Would this result in event_hist_trigger_parse() executing kfree(trigger_data)
via its out_free path while trigger_data->named_list is still linked in the
global named_triggers list? Could this lead to a slab-use-after-free during
global list traversal?

> -	if (named_data)
> +	if (named_data) {
> +		remove_hist_vars(hist_data);
>  		destroy_hist_data(hist_data);

[Severity: Critical]
This is another pre-existing issue, but might freeing hist_data here cause a
double-free if registration ultimately fails?

When reusing a named trigger, hist_data is freed here. However, it appears
event_hist_trigger_parse() does not update its local hist_data pointer. If
a subsequent operation like hist_trigger_enable() fails, will the out_free
path in event_hist_trigger_parse() execute remove_hist_vars() and
destroy_hist_data() on the already-freed hist_data pointer?

Additionally, it seems kfree(trigger_data) would be called in that same
error path without unlinking it from the global named_triggers list. Can this
corrupt the global list?

> +	}
>   out:
>  	return ret;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1