Re: [PATCH] tracing: Hold trace_types_lock when updating modules in persistent instances

[sashiko-bot@kernel.org]

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 <devnexen@gmail.com>

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/20261016053835.204353-1-devnexen@gmail.com?part=1