Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] tracing: lockless user-space reads race with update_last_data() causing UAF/NULL-deref --
commit b27de9515407670f957a6af5954413aab48a1320 Author: David Carlier <[email protected]> tracing: Hold trace_types_lock when updating modules in persistent instances trace_module_record() walks ftrace_trace_arrays from the module notifier without trace_types_lock. A concurrent rmdir of an instance removes it from the list and frees it under that lock, so a module load or unload can walk into a freed trace_array. The notifier also reads and updates tr->scratch and tr->module_delta while update_last_data() may be resetting them. > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c > index e4a490d3d08c8..d3ffbed9c44b3 100644 > --- a/kernel/trace/trace.c > +++ b/kernel/trace/trace.c > @@ -9247,6 +9247,8 @@ static void trace_module_record(struct module *mod, > bool add) > struct trace_array *tr; > unsigned long flags; > > + guard(mutex)(&trace_types_lock); > + [Severity: High] Does acquiring trace_types_lock here fully protect against concurrent modifications to tr->module_delta? The lock is taken here to serialize trace_module_record() and update_last_data(), but it appears user-space read paths can invoke update_last_data() without acquiring trace_types_lock. For example, when a user reads from the trace pipe, tracing_read_pipe() calls update_last_data_if_empty(iter->tr) while only holding iter->mutex: tracing_read_pipe() ... waitagain: if (update_last_data_if_empty(iter->tr)) return 0; update_last_data_if_empty() then calls update_last_data(), which proceeds to free module_delta: update_last_data() ... module_delta = READ_ONCE(tr->module_delta); WRITE_ONCE(tr->module_delta, NULL); kfree_rcu(module_delta, rcu); If a module load/unload runs concurrently with this unprotected read path, make_mod_delta() could race with update_last_data() and access the freed or NULL module_delta: make_mod_delta() ... module_delta = READ_ONCE(tr->module_delta); for (...) { ... if (mod->state == MODULE_STATE_GOING) module_delta->delta[i] = 0; Could this concurrent execution result in a NULL pointer dereference or use-after-free in make_mod_delta()? Additionally, if multiple readers execute concurrently, could they race inside update_last_data() and trigger a double-free of module_delta? > list_for_each_entry(tr, &ftrace_trace_arrays, list) { > flags = tr->flags & (TRACE_ARRAY_FL_BOOT | > TRACE_ARRAY_FL_LAST_BOOT); > /* Update any persistent trace array that has already been > started */ -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
