Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [Critical] The added checks for `event->pmu` and `event->state` are insufficient to prevent Time-of-Check to Time-of-Use (TOCTOU) races during concurrent PMU unregistration, leading to potential NULL pointer dereferences or Use-After-Free. -- commit af8b37d090911ccca2049c1eddb3328a00155104 Author: Vinay Belgaumkar <[email protected]> perf: Add checks to prevent null ptr access This commit adds checks in __perf_event_read_cpu() and perf_event_read_local() to verify that the PMU pointer is valid and the event state is not revoked before proceeding. This aims to prevent null pointer access when reading PMU events. > diff --git a/kernel/events/core.c b/kernel/events/core.c > index 7777e82aad5e7..059f82f0cadd4 100644 > --- 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 caching the PMU pointer here leave us vulnerable to a Use-After-Free? If the PMU driver is concurrently unloaded (e.g., via module unload), perf_pmu_unregister() executes synchronize_rcu() before detaching events. A BPF program starting after the RCU sync might still find the event in its map and race with __pmu_detach_event(). Even though we pass the new !pmu check, if the concurrent unregistration completes and frees the driver's memory immediately after, accessing pmu->scope could result in a Use-After-Free. [ ... ] > @@ -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 is a pre-existing issue, but does this check completely prevent the NULL pointer dereference further down in the function? Later in perf_event_read_local(), there is this call: if (event_oncpu == smp_processor_id()) event->pmu->read(event); If a BPF program invokes perf_event_read_local() concurrently with PMU unregistration, the READ_ONCE(event->state) check might pass. However, __pmu_detach_event() forcefully sets event->pmu = NULL without waiting for perf_event_read_local() to finish. This could cause the subsequent event->pmu->read(event) to dereference a NULL pointer. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
