Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [Critical] The added NULL and state checks do not fully resolve the Time-of-Check to Time-of-Use (TOCTOU) race condition against PMU unregistration, leaving NULL pointer dereference and Use-After-Free (UAF) vulnerabilities. -- commit 63da6f63d2cf2058c6cbd31229c510f17f749641 Author: Vinay Belgaumkar <[email protected]> perf: Add checks to prevent null ptr access This patch attempts to prevent null pointer accesses by adding revoked state checks in perf_event_read_local() and null checks in __perf_event_read_cpu(). > diff --git a/kernel/events/core.c b/kernel/events/core.c > --- a/kernel/events/core.c > +++ b/kernel/events/core.c [ ... ] > @@ -4788,14 +4788,19 @@ static inline const struct cpumask > *perf_scope_cpu_topology_cpumask(unsigned int > > static int __perf_event_read_cpu(struct perf_event *event, int event_cpu) > { > + struct pmu *pmu = READ_ONCE(event->pmu); > int local_cpu = smp_processor_id(); > u16 local_pkg, event_pkg; > > if ((unsigned)event_cpu >= nr_cpu_ids) > return event_cpu; > > + if (!pmu) > + return -ENODEV; > + > if (event->group_caps & PERF_EV_CAP_READ_SCOPE) { > - const struct cpumask *cpumask = > perf_scope_cpu_topology_cpumask(event->pmu->scope, event_cpu); > + const struct cpumask *cpumask = > perf_scope_cpu_topology_cpumask(pmu->scope, > + > event_cpu); [Severity: Critical] This is a pre-existing issue, but does reading pmu->scope here leave us vulnerable to a use-after-free if the PMU object is concurrently freed? Since we only read the pointer with READ_ONCE() and do not appear to be holding RCU or a lock protecting the PMU's lifetime against unregistration, could the PMU be detached and freed right before we access it? [ ... ] > @@ -4917,6 +4922,11 @@ int perf_event_read_local(struct perf_event *event, > u64 *value, > goto out; > } > > + if (READ_ONCE(event->state) <= PERF_EVENT_STATE_REVOKED) { > + ret = -ENODEV; > + goto out; > + } > + [Severity: Critical] This isn't a bug introduced by this patch, but does this state check fully resolve the time-of-check to time-of-use race condition against PMU unregistration? If a userspace program triggers a BPF map update on a perf event array, it calls perf_event_read_local() without holding rcu_read_lock(). If a concurrent hardware hot-unplug calls perf_pmu_unregister(), it might issue an IPI to a remote CPU to detach the event while this local CPU passes the revoked state check. If that race occurs, the PMU can be freed immediately after this check, but further down in perf_event_read_local() we still directly dereference event->pmu: if (event_oncpu == smp_processor_id()) event->pmu->read(event); Could this lead to a NULL pointer dereference or use-after-free since the local IRQ disable doesn't serialize against the remote IPI during PMU unregistration? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
