Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [Critical] Race condition in rv_edge_setup error path causes Use-After-Free and NULL pointer dereference by concurrent trace events. - [High] Race condition and non-atomic update of state_ns leads to corrupted per-edge dwell time statistics. Pre-existing issues: - [Critical] Use-After-Free and Control-Flow Hijack when reading tracefs files after the monitor module is unloaded. -- commit 087c3d3fb8881f044270530a0540c2d8d606c74c Author: Tobias Schaffner <[email protected]> rv: collect per-edge dwell time for per-cpu DA/HA monitors With the core facility in place, hook it into the DA/HA layer so that any per-cpu automaton monitor also reports how long it lingers in each state. Add a selftest that enables a per-cpu monitor, checks the stats file appears and is populated under load, and skips cleanly otherwise. > diff --git a/include/rv/da_monitor.h b/include/rv/da_monitor.h > index 34b8fba9ecd44..59ca9a286c106 100644 > --- a/include/rv/da_monitor.h > +++ b/include/rv/da_monitor.h [ ... ] > +static void > +rv_this_edge_name(unsigned int edge, char *buf, size_t len) > +{ > + snprintf(buf, len, "%s:%s", model_get_state_name(edge / EVENT_MAX), > + model_get_event_name(edge % EVENT_MAX)); > +} > + > +static const struct rv_edge_cfg rv_this_edge_cfg = { > + .n_edges = STATE_MAX * EVENT_MAX, > + .edge_name = rv_this_edge_name, > +}; [Severity: Critical] This is a pre-existing issue, but is it possible to trigger a use-after-free and control-flow hijack when reading tracefs files after the monitor module is unloaded? The RV tracefs files (like stats, enable, desc) are created by the core RV module, so their fops.owner does not pin the individual monitor modules. If a user keeps monitors/<mon>/stats open and unloads the monitor module, the module's memory is freed. A subsequent read() on the open file descriptor invokes rv_edge_stats_show() which dereferences seq->private pointing to the freed struct rv_monitor and calls cfg->edge_name(), jumping to unmapped or attacker-controlled memory. [ ... ] > +/* Account the dwell in @curr, then stamp entry into the next state. */ > +static __always_inline void > +rv_da_edge_account(struct da_monitor *da_mon, enum states curr, enum events > ev) > +{ > + u64 now = local_clock(); > + u64 prev = da_mon->state_ns; > + > + da_mon->state_ns = now; [Severity: High] Is this non-atomic update of state_ns detached from the atomic cmpxchg of curr_state, allowing nested interrupts to read a stale timestamp or torn value? In da_event(), try_cmpxchg is used to safely advance curr_state under concurrent execution on the same CPU. If an interrupt fires immediately after try_cmpxchg succeeds but before this rv_da_edge_account() runs, the nested interrupt will execute its own state transition and read the stale state_ns. It will attribute the outer state's dwell time to its own transition, and write a new state_ns. When the interrupt returns, the outer event resumes and overwrites state_ns again, swapping timestamps and causing massive artificial spikes or underflows in the statistics. > + /* > + * local_clock() is not guaranteed monotonic; drop the sample if it did > + * not advance so a backward step cannot underflow into a bogus dwell. > + */ > + if (rv_this.edge_pcpu && prev && now > prev) [Severity: Critical] Does reading rv_this.edge_pcpu here without READ_ONCE() or synchronization make it vulnerable to a race condition with the error path in rv_edge_setup()? In rv_enable_single() from kernel/trace/rv/rv.c, mon->enable() is called to activate trace events globally before rv_edge_setup() allocates edge_pcpu. If rv_create_file() fails during rv_edge_setup(), the error path calls free_percpu(mon->edge_pcpu) and sets the pointer to NULL while tracepoints are already active. If a concurrent trace event firing on another CPU evaluates rv_this.edge_pcpu before it is NULLed but uses it after it is freed, a use-after-free occurs. Additionally, if the compiler reloads the NULL pointer, this_cpu_ptr(NULL) resolves to the base address of the CPU's per-cpu region. This causes trace events to overwrite critical per-cpu data (such as current_task or irq_stack_ptr) and lead to an immediate kernel crash. > + rv_edge_account(&rv_this, curr * EVENT_MAX + ev, now - prev); > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
