Re: [PATCH v7 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 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 <vinay.belgaumkar@intel.com>

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