Re: [PATCH v4] perf: Fix null pointer access in is_include_guest_event()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] A TOCTOU race condition in `__perf_event_read_cpu()` can cause a NULL pointer dereference because the result of `READ_ONCE(event->pmu)` is not cached locally.

Pre-existing issues:
- [High] Incomplete fix: TOCTOU races in `perf_event_read_local()` leading to NULL pointer dereference or Use-After-Free of `event->pmu`.
- [High] Blind mutation of `pmu->exclusive_cnt` on PMU unregister for non-exclusive events.
--

commit 5b5202c48251a852d95e17a012ae0db3e210700b
Author: Vinay Belgaumkar <vinay.belgaumkar@intel.com>

perf: Fix null pointer access in is_include_guest_event()

This commit addresses a null pointer dereference in is_include_guest_event()
when perf_pmu_unregister() is called during an active session. It also adds
null and state checks in __perf_event_read_cpu() and perf_event_read_local()
to prevent similar crashes.

> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index a6c8e38a31104..f105cd7ac1524 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -4794,6 +4794,9 @@ static int __perf_event_read_cpu(struct perf_event *event, int event_cpu)
>  	if ((unsigned)event_cpu >= nr_cpu_ids)
>  		return event_cpu;
>  
> +	if (!READ_ONCE(event->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);

[Severity: High]
Does this code leave a time-of-check to time-of-use race condition open?

Because the result of READ_ONCE(event->pmu) is not cached locally, a
concurrent detachment could set event->pmu to NULL immediately after the
check.

If that happens, the next line in __perf_event_read_cpu() would dereference a
NULL pointer when accessing event->pmu->scope.

Could we store the PMU pointer in a local variable instead of re-reading
event->pmu?

[ ... ]

> @@ -4917,6 +4920,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;
> +	}
> +
>  	/*
>  	 * Get the event CPU numbers, and adjust them to local if the event is
>  	 * a per-package event that can be read locally

[Severity: High]
This is a pre-existing issue, but does this state check fully prevent
concurrent PMU unregistration issues in perf_event_read_local()?

Even if the state check passes, a concurrent detachment could still set
event->pmu to NULL before the function completes.

Later in perf_event_read_local(), the code directly dereferences the PMU
pointer:

kernel/events/core.c:perf_event_read_local() {
	...
	if (event_oncpu == smp_processor_id())
		event->pmu->read(event);
	...
}

Would it be possible for event->pmu to become NULL prior to this final
execution, causing a panic?

[ ... ]

> @@ -13002,6 +13013,7 @@ static void __pmu_detach_event(struct pmu *pmu, struct perf_event *event,
>  	exclusive_event_destroy(event);
>  	module_put(pmu->module);
>  
> +	mediated_pmu_unaccount_event(event);

[Severity: High]
This is a pre-existing issue, but does calling exclusive_event_destroy()
here cause problems for non-exclusive events?

In __pmu_detach_event(), exclusive_event_destroy() is invoked without
verifying if the event has the PERF_ATTACH_EXCLUSIVE flag set.

Looking at exclusive_event_destroy(), it directly modifies the PMU's atomic
exclusive counter:

kernel/events/core.c:exclusive_event_destroy() {
	...
	if (event->attach_state & PERF_ATTACH_TASK)
		atomic_dec(&pmu->exclusive_cnt);
	else
		atomic_inc(&pmu->exclusive_cnt);
	...
}

Could this blindly corrupt the PMU's reference limits during unregistration
for normal, non-exclusive events?

>  	event->pmu = NULL; /* force fault instead of UAF */
>  }
>

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