Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Identifying trace arrays by raw memory address in `trace_array_options_get()` introduces an ABA/pointer reuse vulnerability, allowing cross-instance manipulation. - [Low] Call to undeclared function `__trace_array_get()` will break the build. Pre-existing issues: - [Critical] This problem wasn't introduced by this patch, but the patch addresses a Use-After-Free for core option files while overlooking tracer-specific option files and other per-instance tracefs files, leaving them vulnerable to the exact same Use-After-Free. -- commit a7557d52083622c7b02bffa097f88009b072e692 Author: Steven Rostedt <[email protected]> tracing: Take trace_array reference when opening options file This commit fixes a Use-After-Free vulnerability in trace options files by ensuring the trace_array reference is taken when the file is opened. It introduces trace_array_options_get() to safely acquire the reference. Link: https://lore.kernel.org/linux-trace-kernel/[email protected]/ > 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: High] Does this raw memory address comparison account for reallocation of the trace_array struct? If a trace instance is deleted, its trace_array is freed. If a new instance is subsequently created, it could be allocated at the exact same memory address. Could a user holding an O_PATH file descriptor to a core options file of the deleted instance reopen it and manipulate the tracer flags of the newly allocated instance? > + return __trace_array_get(tr); [Severity: Low] Will this compile successfully? It appears __trace_array_get() is an undeclared function, which might cause a build failure. > + } > + 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 does this same Use-After-Free vulnerability also exist for tracer-specific options and other per-instance files? While this patch addresses core option files by replacing tracing_open_generic with trace_options_open, files like tracing_max_latency and tracer-specific options still appear to use tracing_open_generic, which doesn't take a reference to the trace_array. For example, if a user opens a per-instance tracing_max_latency file, keeps the descriptor open, and the instance is removed, could a subsequent write dereference the freed memory? kernel/trace/trace.c:tracing_max_lat_write() { ... return tracing_nsecs_write(filp->private_data, ubuf, cnt, ppos); } Where tracing_nsecs_write would perform an unlocked write to the freed pointer? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
