Re: [PATCH v2] tracing: Take trace_array reference when opening options file

[sashiko-bot@kernel.org]

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

New issues:
- [Low] The patch introduces a call to `__trace_array_get()`, which is neither declared nor defined anywhere in the kernel, causing a build failure.
- [Low] The commit message claims to have added a `(void *)` cast to fix a kernel test robot warning, but the cast is missing from the submitted patch.

Pre-existing issues:
- [Critical] The patch fixes the use-after-free for core options, but leaves tracer-specific options and other per-instance files vulnerable to the exact same use-after-free.
- [Critical] The validation logic for the dangling trace_array pointer is vulnerable to an ABA (Slab Reuse) problem, allowing cross-instance privilege escalation.
--

commit 16f2151bdbc0994e69e88bb0c5631ea2027da7fa
Author: Steven Rostedt <rostedt@goodmis.org>

tracing: Take trace_array reference when opening options file

This patch addresses a use-after-free when an options file is opened by
one task and the underlying trace instance is removed by another. It adds
a new helper to validate the index element address and safely acquire a
reference to the trace_array.

Closes: https://lore.kernel.org/linux-trace-kernel/20260828135858.2AC501F000E9@smtp.kernel.org/

> - Added (void *) typecast to comparison. (kernel test robot)

[Severity: Low]
Does the pointer comparison in the code still trigger a compiler warning?
The patch notes mention adding a (void *) typecast to fix a kernel test
robot warning, but the cast appears to be missing from the submitted code.

> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index d3005279165d9..3e001ed8cf376 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c

[ ... ]

> +static int trace_array_options_get(void *tr_index)
> +{
> +	struct trace_array *tr;
> +	int ret;
> +
> +	ret = security_locked_down(LOCKDOWN_TRACEFS);
> +	if (ret)
> +		return ret;
> +
> +	if (tracing_disabled)
> +		return -ENODEV;
> +
> +	guard(mutex)(&trace_types_lock);
> +	list_for_each_entry(tr, &ftrace_trace_arrays, list) {
> +		if (tr_index >= &tr->trace_flags_index[0] &&
> +		    tr_index < &tr->trace_flags_index[TRACE_FLAGS_MAX_SIZE])

[Severity: Critical]
This is a pre-existing issue, but does this validation logic remain
vulnerable to an ABA (slab reuse) problem? If an unprivileged user holds
an O_PATH file descriptor to a freed instance's options file, the inode
is kept alive with a dangling i_private pointer.

If a new instance is created and allocated at the exact same memory
address, wouldn't this bounds check falsely succeed on the dangling
pointer, granting read and write access to the new (potentially root-created)
trace_array options? This same ABA flaw seems to exist in the general
trace_array_get() function as well.

> +			return __trace_array_get(tr);

[Severity: Low]
Is __trace_array_get() defined anywhere in the kernel? It looks like
this might cause a build failure because the function is neither declared
nor defined.

> +	}
> +	return -ENODEV;
> +}

[ ... ]

>  static const struct file_operations trace_options_core_fops = {
> -	.open = tracing_open_generic,
> -	.read = trace_options_core_read,
> -	.write = trace_options_core_write,
> -	.llseek = generic_file_llseek,
> +	.open		= trace_options_open,
> +	.read		= trace_options_core_read,
> +	.write		= trace_options_core_write,
> +	.llseek		= generic_file_llseek,
> +	.release	= trace_options_release,
>  };

[Severity: Critical]
This is a pre-existing issue, but by only updating trace_options_core_fops,
do tracer-specific options and other per-instance files remain vulnerable
to the exact same use-after-free? 

Files like trace_options_fops, set_tracer_fops, and tracing_max_lat_fops
still use tracing_open_generic for their open callbacks, which fails to
take a reference to the instance's trace_array.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902114502.723d8243@gandalf.local.home?part=1