Re: [PATCH 1/7] perf/x86/intel/uncore: Rename refcount fields and other cleanups

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] NULL Pointer Dereference in `uncore_pci_pmu_unregister()` when device registration fails.
- [High] NULL Pointer Dereference (previously reported as UAF) during perf event operations if a PCI device is removed while events are active.
--

commit 247a617f910d49e9f19af35d0216a5b938d9d034
Author: Zide Chen <zide.chen@intel.com>

perf/x86/intel/uncore: Rename refcount fields and other cleanups

This commit renames several fields and parameters in the intel uncore PMU
driver to clarify their purpose, such as activeboxes to die_refcnt and
box->refcnt to box->cpu_refcnt. It also fixes a typo and removes an
incorrect refcount increment.

> diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
> index e9cc1ba921c5e..e200f4744eb9b 100644
> --- a/arch/x86/events/intel/uncore.c
> +++ b/arch/x86/events/intel/uncore.c

[ ... ]

> @@ -1230,7 +1229,7 @@ static void uncore_pci_pmu_unregister(struct intel_uncore_pmu *pmu, int die)
>  	struct intel_uncore_box *box = pmu->boxes[die];
>  
>  	pmu->boxes[die] = NULL;
> -	if (atomic_dec_return(&pmu->activeboxes) == 0)
> +	if (atomic_dec_return(&pmu->die_refcnt) == 0)
>  		uncore_pmu_unregister(pmu);
>  	uncore_box_exit(box);

While reviewing these unregistration changes, I noticed a pre-existing issue
here.

If PMU registration fails during PCI device probe, uncore_pci_pmu_register()
frees the box and sets pmu->boxes[die] = NULL.

Later, when the device is removed, the bus notifier calls
uncore_pci_pmu_unregister() which fetches the NULL box:

    struct intel_uncore_box *box = pmu->boxes[die];

and passes it to uncore_box_exit(box).

Since uncore_box_exit() does not check for NULL before testing box->flags:

static inline void uncore_box_exit(struct intel_uncore_box *box)
{
        if (test_and_clear_bit(UNCORE_BOX_FLAG_INITIALIZED, &box->flags)) {

Does this sequence result in a NULL pointer dereference?

Additionally, if a PCI device is removed while perf events are still active
on its CPU, this function sets pmu->boxes[die] = NULL and frees the box.

If other dies still have boxes present, die_refcnt will be greater than zero
and uncore_pmu_unregister(pmu) is bypassed.

Could this leave active events scheduled in the perf subsystem?

If the perf subsystem later attempts to read or stop the event,
uncore_event_to_box() would return NULL. Would subsequent calls like
uncore_perf_event_update() or accesses to box->n_active then trigger a NULL
pointer dereference when operating on the missing box?

>  	kfree(box);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512233048.9577-1-zide.chen@intel.com?part=1