Re: [Patch v9 02/24] perf/x86: Move hybrid PMU initialization before x86_pmu_starting_cpu()

["Mi, Dapeng" <dapeng1.mi@linux.intel.com>]

On 7/6/2026 10:18 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] Preexisting out-of-bounds read in `check_hw_exists()` on hybrid platforms due to an invalid `container_of()` cast on the global static `pmu` struct via the `hybrid()` macro.
> --
>
> commit 8216e613cee4854e16daaa11279146d17bc7f313
> Author: Dapeng Mi <dapeng1.mi@linux.intel.com>
>
> perf/x86: Move hybrid PMU initialization before x86_pmu_starting_cpu()
>
> The current approach initializes hybrid PMU structures immediately before
> registering them. This is risky as it can lead to key fields being
> inadvertently overwritten. This patch mitigates this by moving the
> default hybrid structure initialization before calling
> x86_pmu_starting_cpu().
>
>> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
>> index 872d07a5fa80d..0888d3b0923e1 100644
>> --- a/arch/x86/events/core.c
>> +++ b/arch/x86/events/core.c
>> @@ -2195,8 +2195,20 @@ static int __init init_hw_perf_events(void)
>>  
>>  	pmu.attr_update = x86_pmu.attr_update;
>>  
>> -	if (!is_hybrid())
>> +	if (!is_hybrid()) {
>>  		x86_pmu_show_pmu_cap(NULL);
>> +	} else {
>> +		int i;
>> +
>> +		/*
>> +		 * Init default ops.
>> +		 * Must be called before registering x86_pmu_starting_cpu(),
>> +		 * otherwise some key PMU fields, e.g., capabilities
>> +		 * initialized in x86_pmu_starting_cpu(), would be overwritten.
>> +		 */
>> +		for (i = 0; i < x86_pmu.num_hybrid_pmus; i++)
>> +			x86_pmu.hybrid_pmu[i].pmu = pmu;
>> +	}
> [Severity: High]
> This is a pre-existing issue, but does passing the global pmu struct to
> check_hw_exists() earlier in this function cause an out-of-bounds read on
> hybrid platforms?
>
> In init_hw_perf_events(), check_hw_exists(&pmu, ...) is called after
> perf_is_hybrid is enabled. Inside check_hw_exists(), it iterates over
> fixed counters and calls fixed_counter_disabled():
>
> arch/x86/events/perf_event.h:fixed_counter_disabled() {
>     ...
>     u64 intel_ctrl = hybrid(pmu, intel_ctrl);
>     ...
> }
>
> The hybrid() macro expands to a container_of() cast:
>
>     container_of(_pmu, struct x86_hybrid_pmu, pmu)->_field
>
> Because the pmu passed in is the standalone global struct pmu (not embedded
> within a struct x86_hybrid_pmu), this cast yields an invalid pointer.
>
> Could this read garbage data past the end of the global pmu struct when
> accessing intel_ctrl, potentially causing the BIOS hardware check to act on
> garbage data or trigger a kernel panic if it crosses an unmapped page
> boundary?

Yes, a real issue. It looks unnecessary to run fixed_counter_disabled()
check since intel_ctrl always comes from the the bitwised-or between
cntr_mask64 and fixed_cntr_mask64. Would drop fixed_counter_disabled() and
all calling in a separate patch.

Thanks.


>