Re: [PATCH V3 2/7] perf/x86/intel/uncore: Guard against invalid box control address

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing memory barriers when publishing and reading `pmu->boxes[die]`, which can cause `uncore_pmu_event_init()` to use a completely uninitialized PMU box.
--

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

perf/x86/intel/uncore: Guard against invalid box control address

This commit adds null pointer checks against the return value of
intel_generic_uncore_box_ctl() to prevent invalid hardware accesses
when a CPU die is offline during uncore enumeration.

> diff --git a/arch/x86/events/intel/uncore_discovery.c b/arch/x86/events/intel/uncore_discovery.c
> index 60e1200c4691d..af7e80fee81fb 100644
> --- a/arch/x86/events/intel/uncore_discovery.c
> +++ b/arch/x86/events/intel/uncore_discovery.c

[ ... ]

[Severity: High]
This is a pre-existing issue, but does the initialization of PMU boxes lack
necessary memory barriers?

In arch/x86/events/intel/uncore.c, a new PMU box is allocated, initialized,
and published to the array without a write memory barrier:

arch/x86/events/intel/uncore.c:allocate_boxes() {
    ...
    box->pmu->boxes[die] = box;
    ...
}

Because MSR/MMIO PMUs are registered to the perf core before allocate_boxes()
is invoked via CPU hotplug callbacks, could userspace concurrently invoke
perf_event_open()?

This would trigger uncore_pmu_event_init(), which locklessly reads the box
without a read memory barrier:

arch/x86/events/intel/uncore.c:uncore_pmu_to_box() {
    ...
    return dieid < uncore_max_dies() ? pmu->boxes[dieid] : NULL;
}

This data race might allow the CPU to observe the box pointer before its
contents (such as box->cpu and box->pmu) are fully initialized or zeroed.
If the uninitialized memory happens to have a value >= 0 for box->cpu,
would it bypass the validation check?

arch/x86/events/intel/uncore.c:uncore_pmu_event_init() {
    ...
    if (!box || box->cpu < 0)
        return -EINVAL;
    ...
}

Consequently, a partially initialized box might be associated with the event,
and when the event is later added or scheduled, uninitialized fields like
box->pmu could be dereferenced.

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