Re: [PATCH v2] tracing/user_events: Don't destroy fields when event removal fails

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Data race leading to out-of-bounds read and BPF context size check bypass when event removal races with BPF attach.
--

commit 67af0e963857d01d3ef2cc1bd224b3d373fe0483
Author: Henry Martin <bsdhenrymartin@gmail.com>

tracing/user_events: Don't destroy fields when event removal fails

destroy_user_event() previously destroyed fields before attempting to
remove the trace event call. This patch moves the field destruction after 
the call removal and temporarily moves the fields to a local list to prevent
them from being irreversibly destroyed if the removal fails.

> diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
> index 2bbc89d4a2666..dbd8e9f95a4a7 100644
> --- a/kernel/trace/trace_events_user.c
> +++ b/kernel/trace/trace_events_user.c
[ ... ]
> @@ -1499,17 +1498,32 @@ static int user_event_set_call_visible(struct user_event *user, bool visible)
>  
>  static int destroy_user_event(struct user_event *user)
>  {
> +	LIST_HEAD(fields);
>  	int ret = 0;
>  
>  	lockdep_assert_held(&event_mutex);
>  
> -	/* Must destroy fields before call removal */
> -	user_event_destroy_fields(user);
> +	/*
> +	 * Detach the fields before removing the call. Removing the event
> +	 * frees the field list memory (trace_destroy_fields() is run on
> +	 * successful removal and kmem_cache_free()s the fields), but the
> +	 * fields here are allocated and owned by user_events. Destroy
> +	 * them separately once removal has succeeded.
> +	 */
> +	list_splice_init(&user->fields, &fields);

[Severity: High]
This is a pre-existing issue, but both the old behavior and this new 
list_splice_init() leave the published field list temporarily empty while 
the event is still active. Does this create a data race with concurrent 
BPF attaches?

A concurrent PERF_EVENT_IOC_SET_BPF can call trace_event_get_offsets(), 
which locklessly accesses this list without holding event_mutex or 
trace_event_sem:

    tail = list_first_entry(head, struct ftrace_event_field, link);
    return tail->offset + tail->size;

Because the list is empty at this point, list_first_entry() resolves to the
list head itself (&user->fields). This causes tail->offset and tail->size
to read garbage values from adjacent struct members (such as
user->put_work).

Could this out-of-bounds read result in the BPF verifier bypassing context
size checks, potentially allowing a kernel memory information leak?

>  
>  	ret = user_event_set_call_visible(user, false);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904115223.2976446-1-bsdhenrymartin@gmail.com?part=1