Thread (4 messages) flat view 4 messages, 2 authors, 2d ago

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

From: Steven Rostedt <rostedt@goodmis.org>
Date: 2026-09-02 16:14:35
Also in: sashiko-reviews

On Wed, 02 Sep 2026 16:02:07 +0000
sashiko-bot@kernel.org wrote:
[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.
Bah! I forgot to hit git commit -a --amend before sending the patch!

quoted
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  
[ ... ]
quoted
+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.
As said before, we don't care.
quoted
+			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.
quoted
+	}
+	return -ENODEV;
+}  
[ ... ]
quoted
 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.
The trace_options_fops does indeed have the issue as it uses topts->tr
where it can not trust the topts. But set_tracer_fops and
tracing_max_lat_fops use tracing_open_generic_tr().

Are you using the master branch of the repo? That's from 2022 and very old.

I'll set the default branch to be the for-next branch so hopefully you
don't report old bugs that have been fixed a long time ago anymore.

-- Steve

Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help