Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Critical] Tracefs lockdown and tracing_disabled checks are bypassed for the global ftrace instance and stack tracer. - [High] Module and probe filters for the global ftrace instance cannot be cleared or read. --
commit 42c008f904dcd516dc1681a548fae6a4bba9a162 Author: Steven Rostedt <[email protected]> ftrace: Take trace_array reference before accessing its ftrace_ops The trace instance files set_ftrace_filter and set_ftrace_notrace are updated to work with specific trace instances (trace_arrays). This patch passes the trace_array as the file private data to allow incrementing its reference before touching the ftrace_ops, preventing a use-after-free. Closes: https://lore.kernel.org/all/[email protected]/ > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > index f9d80c7bd9f16..4babd86c7be00 100644 > --- a/kernel/trace/ftrace.c > +++ b/kernel/trace/ftrace.c [ ... ] > int > -ftrace_regex_open(struct ftrace_ops *ops, int flag, > +ftrace_regex_open(struct trace_array *tr, struct ftrace_ops *ops, int flag, > struct inode *inode, struct file *file) > { > - struct ftrace_iterator *iter; > + struct ftrace_iterator *iter = NULL; > struct ftrace_hash *hash; > struct list_head *mod_head; > - struct trace_array *tr = ops->private; > - int ret = -ENOMEM; > - > - ftrace_ops_init(ops); > + int ret = -ENODEV; > > if (unlikely(ftrace_disabled)) > return -ENODEV; > > - if (tracing_check_open_get_tr(tr)) > + if (tr && tracing_check_open_get_tr(tr)) > return -ENODEV; [Severity: Critical] By adding the `tr &&` short-circuit here, does this cause a regression where tracefs lockdown and tracing_disabled checks are bypassed for the global ftrace instance and the stack tracer? Since `tr` is explicitly passed as `NULL` for these instances, `tracing_check_open_get_tr(NULL)` is never called. [ ... ] > @@ -4788,21 +4801,19 @@ ftrace_regex_open(struct ftrace_ops *ops, int flag, > static int > ftrace_filter_open(struct inode *inode, struct file *file) > { > - struct ftrace_ops *ops = inode->i_private; > + struct trace_array *tr = inode->i_private; > > - /* Checks for tracefs lockdown */ > - return ftrace_regex_open(ops, > - FTRACE_ITER_FILTER | FTRACE_ITER_DO_PROBES, > - inode, file); > + return ftrace_regex_open(tr, NULL, > + FTRACE_ITER_FILTER | FTRACE_ITER_DO_PROBES, > + inode, file); > } [Severity: High] For the global ftrace instance, `tr` is `NULL`. Does passing `NULL` for `tr` break the module and probe filters? In `ftrace_regex_open()`, the `mod_head` pointer is initialized like this: mod_head = tr ? &tr->mod_trace : NULL; Because `tr` is `NULL`, `mod_head` will also be `NULL`. When opening the file with `O_TRUNC`, `clear_ftrace_mod_list(mod_head)` will do nothing, failing to clear the module filters. Additionally, `iter->tr` is set to `NULL`. When reading the file, `t_mod_start()` relies on `iter->tr` and will return early: /* probes are only available if tr is set */ if (!iter->tr) return NULL; Could this skip the enumeration of active module and probe filters for the global instance entirely? [ ... ] > diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c > index 0aa2514a6593b..e7f4e523587d0 100644 > --- a/kernel/trace/trace_stack.c > +++ b/kernel/trace/trace_stack.c > @@ -499,7 +499,7 @@ stack_trace_filter_open(struct inode *inode, struct file > *file) > struct ftrace_ops *ops = inode->i_private; > > /* Checks for tracefs lockdown */ > - return ftrace_regex_open(ops, FTRACE_ITER_FILTER, > + return ftrace_regex_open(NULL, ops, FTRACE_ITER_FILTER, > inode, file); > } [Severity: Critical] The stack tracer explicitly passes `NULL` for the trace array. Due to the short-circuit mentioned earlier, doesn't this also bypass the lockdown check documented in the comment right above this call? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
