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

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

[Steven Rostedt]

From: Steven Rostedt <rostedt@goodmis.org>

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.

As the options are somewhat dynamic in their creation at boot up, each
file represents a flag in the trace_array. The trace_array has an array of
indexes to represent each of these flags that is stored in the
trace_flags_index array. The address of the index array element is used to
pass to the inode->i_private pointer. Then that element is read which
holds the index (which represents the flag) and then the index is used to
calculate the trace_array descriptor from its trace_flags_index array.

One issue is that the index element can not be referenced until the
trace_array's reference is taken. To handle this, create a new helper
function called: trace_array_options_get() that will iterate all the
existing trace_arrays in the ftrace_trace_arrays list (under the
trace_types_lock), and compare the passed in address of the index element
with the entire array of the trace_array's trace_flags_index array.
If it matches, then up the corresponding trace_array's reference and
return.

Cc: stable@vger.kernel.org
Fixes: 577b785f55168 ("tracing: add tracer dependent options to options directory")
Reported-by: sashiko-bot@kernel.org
Closes: https://lore.kernel.org/linux-trace-kernel/20260828135858.2AC501F000E9@smtp.kernel.org/
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
Changes since v2: ??
  Ignore it. I resent v1 because I didn't add the "-a" to my "git commit --amend"
  before sending.

Changes since v1: https://patch.msgid.link/20260901163620.6cbe0ada@gandalf.local.home

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

 kernel/trace/trace.c | 67 +++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 63 insertions(+), 4 deletions(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index a946e0183fd1..722d0ba2d233 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -7842,11 +7842,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])
+			return __trace_array_get(tr);
+	}
+	return -ENODEV;
+}
+
+static int trace_options_open(struct inode *inode, struct file *filp)
+{
+	void *tr_index = inode->i_private;
+
+	if (trace_array_options_get(tr_index) < 0)
+		return -ENODEV;
+
+	filp->private_data = tr_index;
+
+	return 0;
+}
+
+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,
 };
 
 struct dentry *trace_create_file(const char *name,
-- 
2.53.0

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

[sashiko-bot]

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