Re: [PATCH v5 2/2] perf: Add checks to prevent null ptr access

[sashiko-bot@kernel.org]

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 <vinay.belgaumkar@intel.com>

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/20260901211915.127704-1-vinay.belgaumkar@intel.com?part=2