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 <[email protected]> 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/[email protected]/ > 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/[email protected]?part=1
