Re: [PATCH v6 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:
- [High] The lockless checks for revoked state and NULL PMU pointers are insufficient to prevent a Use-After-Free of the PMU structure or a NULL pointer dereference during concurrent unregistration.
--

commit 4a985bf8ccfd6853c89a279d4b7181e83c33bcef
Author: Vinay Belgaumkar <vinay.belgaumkar@intel.com>

perf: Add checks to prevent null ptr access

This commit adds a check for revoked states in perf_event_read_local() and
a null pointer check for the PMU in __perf_event_read_cpu().

> 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;
> +

[Severity: High]
This is a pre-existing issue, but does this lockless check provide a false
sense of security against concurrent PMU unregistration? A concurrent
perf_pmu_unregister() can set event->pmu = NULL and free the PMU memory. Since
this check relies on a cached pointer without holding locks that serialize
against unregistration (e.g., ctx->lock), the PMU can be freed immediately
after this check passes.

>  	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: High]
This is a pre-existing issue, but can this lead to a Use-After-Free when
accessing pmu->scope? If the PMU is unregistered and freed by another CPU
after we cache it but before we dereference it here, pmu->scope will read from
freed memory.

[ ... ]

> @@ -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: High]
This is a pre-existing issue, but is this check vulnerable to a TOCTOU race?
The event state can change immediately after this check if the event context is
inactive or belongs to a different CPU, allowing a concurrent unregistration to
detach the event and free the PMU while perf_event_read_local() continues
executing. This leaves the subsequent call to event->pmu->read(event)
unprotected, potentially resulting in a NULL pointer dereference or
Use-After-Free.

>  	/*
>  	 * Get the event CPU numbers, and adjust them to local if the event is
>  	 * a per-package event that can be read locally

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902141824.733112-1-vinay.belgaumkar@intel.com?part=2