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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Cross-instance privilege escalation via SLAB reuse in `trace_array_options_get`.
- [Low] Missing definition of `__trace_array_get()`, causing a compilation failure.

Pre-existing issues:
- [Critical] The patch only fixes the use-after-free for core options files (`trace_options_core_fops`), leaving other instance files vulnerable to the exact same UAF.
--

commit 692e0bd47e6544a583d3d3e1c5faabd7a045c813
Author: Steven Rostedt <rostedt@goodmis.org>

tracing: Take trace_array reference when opening options file

The options files do not take the trace_array reference for the options
they represent. This could cause a use-after-free kernel crash if one of
these files is opened by one task and another task removes the instance
that the option is for. Because it doesn't take a reference upon opening,
it will not stop the removal which will free the options descriptor that
is being used.

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

> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index d3005279165d9..f172946edcb23 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
[ ... ]
> @@ -8809,11 +8809,70 @@ trace_options_core_write(struct file *filp, const char __user *ubuf, size_t cnt,
>  	return cnt;
>  }
>  
> +/*
> + * The tr_index is the address of a trace_array->trace_flags_index[]
> + * element that holds the index of the trace flag. But since the
> + * trace_array reference has not been taken yet, it cannot be referenced
> + * as it could have been freed by a rmdir of the instance the trace_array
> + * represents.
> + *
> + * Search the list of trace_arrays and compare the tr_index to the
> + * address of the entire trace_array trace_flags_index array for each
> + * trace_array in the list. If one is matched, then take the reference
> + * and return it. If not, the trace_array no longer exits.
> + */
> +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 >= (void *)&tr->trace_flags_index[0] &&
> +		    tr_index < (void *)&tr->trace_flags_index[TRACE_FLAGS_MAX_SIZE])

[Severity: High]
Can this raw memory address validation in trace_array_options_get() be spoofed
if a trace instance is removed and its memory is reused by the SLAB allocator
for a new, potentially more privileged trace instance?

If an unprivileged user starts opening an option file for instance A, but
instance A is removed and its memory is reallocated to instance B before this
check runs, could the stale tr_index match the new instance B's memory bounds
and grant unauthorized access to its options?

> +			return __trace_array_get(tr);

[Severity: Low]
Is __trace_array_get() defined anywhere?

Will this cause a compilation failure because of an implicit function
declaration or undefined reference when compiling the kernel with tracing
enabled?

> +	}
> +	return -ENODEV;
> +}
> +
[ ... ]
> +static int trace_options_release(struct inode *inode, struct file *filp)
> +{
> +	void *tr_index = filp->private_data;
> +	struct trace_array *tr;
> +	unsigned int index;
> +
> +	get_tr_index(tr_index, &tr, &index);
> +
> +	trace_array_put(tr);
> +
> +	return 0;
> +}
> +
>  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 since we are fixing the use-after-free for
core option files by changing .open to trace_options_open here, do we also
need to fix other instance files like trace_options_fops, set_tracer_fops,
and tracing_max_lat_fops?

These structures still use tracing_open_generic, which does not take a
reference on the trace array.

Could they be vulnerable to the same use-after-free if another task removes
the trace instance while their files are held open?

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