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

["Chen, Zide" <zide.chen@intel.com>]

On 5/13/2026 5:58 PM, sashiko-bot@kernel.org wrote:
> 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:

Yes, you are right, uncore_pci_pmu_unregister() should validate the box:

diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
index 208901ea280c..edf608ba8e9a 100644
--- a/arch/x86/events/intel/uncore.c
+++ b/arch/x86/events/intel/uncore.c
@@ -1285,8 +1285,11 @@ static void uncore_pci_pmu_unregister(struct
intel_uncore_pmu *pmu, int die)
        pmu->boxes[die] = NULL;
        if (atomic_dec_return(&pmu->die_refcnt) == 0)
                uncore_pmu_unregister(pmu);
-       uncore_box_exit(box);
-       kfree(box);
+
+       if (box) {
+               uncore_box_exit(box);
+               kfree(box);
+       }
 }

> 
> 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.

Yes, this series is mostly focusing on the MSR/MMIO uncore paths, not
PCI hot plug/unplug handling yet.  I plan to address them in follow-up
patches.


> 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);
>>  }
>