Re: [PATCH] perf/x86/amd/uncore: Extend num_umc_pmc to fix UMC discovery

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] perf/x86/amd: OOB write on active_mask via unvalidated CPUID
--

commit 00612d17a5d2f57d4e77167b7e9d6303f0e776ac
Author: Sandipan Das <sandipan.das@amd.com>

perf/x86/amd/uncore: Extend num_umc_pmc to fix UMC discovery

The commit extends the width of the num_umc_pmc bitfield from six bits
to eight bits to match Zen 5 documentation. This allows discovery of
Unified Memory Controller (UMC) counters up to 64 in fully-populated
Zen 6 configurations.

> diff --git a/arch/x86/include/asm/perf_event.h b/arch/x86/include/asm/perf_event.h
> index 1eb13673e889f..d419b26086b22 100644
> --- a/arch/x86/include/asm/perf_event.h
> +++ b/arch/x86/include/asm/perf_event.h
> @@ -293,7 +293,7 @@ union cpuid_0x80000022_ebx {
>  		/* Number of Data Fabric Counters */
>  		unsigned int	num_df_pmc:6;
>  		/* Number of Unified Memory Controller Counters */
> -		unsigned int	num_umc_pmc:6;
> +		unsigned int	num_umc_pmc:8;

[Severity: High]
Does this expansion allow values up to 255 from CPUID to propagate unchecked?

When the field was 6 bits, the natural boundary limited it to 63, which fit
safely inside NUM_COUNTERS_MAX (64). With an 8-bit width, a hypervisor could
supply a value greater than 64 in EBX.NumPerfCtrUmc.

This value gets propagated down during initialization without validation:

arch/x86/events/amd/uncore.c:amd_uncore_umc_ctx_init() {
    ...
    pmu->num_counters = group_num_pmcs[gid] / group_num_pmus[gid];
    ...
}

If pmu->num_counters exceeds NUM_COUNTERS_MAX, can it cause an
out-of-bounds write when starting the event?

arch/x86/events/amd/uncore.c:amd_uncore_umc_start() {
    ...
    __set_bit(hwc->idx, ctx->active_mask);
    ...
}

Since ctx->active_mask is sized by BITS_TO_LONGS(NUM_COUNTERS_MAX), writing
a bit greater than 63 might overflow into the adjacent nr_active and
hrtimer fields in struct amd_uncore_ctx.

Should we cap pmu->num_counters against NUM_COUNTERS_MAX during
initialization to prevent this regression?

>  	} split;
>  	unsigned int		full;
>  };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007152746.3658877-1-sandipan.das@amd.com?part=1