Re: [Patch v9 22/24] perf/x86/intel: Enable PERF_PMU_CAP_SIMD_REGS capability

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

On 7/6/2026 10:57 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>
> New issues:
> - [Critical] The `XSAVES` instruction writes PMU extended register state out-of-bounds due to incorrect buffer sizing in `reserve_ext_regs_buffers()`.
> - [Low] Fragile bitmask comparison using the strictly greater-than operator (`>`).
>
> Pre-existing issues:
> - [High] Pre-existing type confusion in `perf_reg_value` allowing out-of-bounds stack memory read/dereference via software PMUs.
> --
>
> commit 9750372940712500ad29f87f143032cba6ffbe36
> Author: Dapeng Mi <dapeng1.mi@linux.intel.com>
>
> perf/x86/intel: Enable PERF_PMU_CAP_SIMD_REGS capability
>
> This commit enables the PERF_PMU_CAP_SIMD_REGS capability when XSAVES support
> is available for extended registers. It simplifies validation logic by tying
> the capability to XSAVES and architectural PEBS support.
>
>> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
>> index 15962a3457eeb..56997731dc83a 100644
>> --- a/arch/x86/events/intel/core.c
>> +++ b/arch/x86/events/intel/core.c
> [ ... ]
>
>> @@ -6303,14 +6303,37 @@ static inline void __intel_update_pmu_xregs_caps(struct pmu *pmu)
>>  	 */
>>  	x86_pmu.ext_regs_mask |= XFEATURE_MASK_SSE;
>>  
>> +	if (boot_cpu_has(X86_FEATURE_AVX) &&
>> +	    cpu_has_xfeatures(XFEATURE_MASK_YMM, NULL))
>> +		x86_pmu.ext_regs_mask |= XFEATURE_MASK_YMM;
>> +	if (boot_cpu_has(X86_FEATURE_APX) &&
>> +	    cpu_has_xfeatures(XFEATURE_MASK_APX, NULL))
>> +		x86_pmu.ext_regs_mask |= XFEATURE_MASK_APX;
> [Severity: Critical]
> Can modifying this mask cause an out-of-bounds write during an NMI?
>
> When extended registers like APX are requested, the PMU allocates ext_regs_buf
> using xstate_calculate_size(x86_pmu.ext_regs_mask, compacted) in
> arch/x86/events/core.c:reserve_ext_regs_buffers(). This calculation omits the
> sizes of non-PMU active states like AMX.
>
> However, when xsaves_nmi() executes the XSAVES instruction in
> x86_pmu_sample_xregs(), the hardware determines compacted offsets based on ALL
> active CPU features (XCR0 | IA32_XSS). For a CPU with AMX enabled, XSAVES will
> write the APX state at an offset exceeding the allocated buffer size.
>
> Does this mismatch lead to a heap buffer overflow when XSAVES executes?

This is not true. xsaves_nmi() would take an argument. If the argument
doesn't contain AMX flag, AMX registers won't be really read. Since
currently perf doesn't support AMX sampling, so there would be on
out-of-bounds write.


>
>> +	if (boot_cpu_has(X86_FEATURE_AVX512F)) {
>> +		if (cpu_has_xfeatures(XFEATURE_MASK_OPMASK, NULL))
>> +			x86_pmu.ext_regs_mask |= XFEATURE_MASK_OPMASK;
>> +		if (cpu_has_xfeatures(XFEATURE_MASK_ZMM_Hi256, NULL))
>> +			x86_pmu.ext_regs_mask |= XFEATURE_MASK_ZMM_Hi256;
>> +		if (cpu_has_xfeatures(XFEATURE_MASK_Hi16_ZMM, NULL))
>> +			x86_pmu.ext_regs_mask |= XFEATURE_MASK_Hi16_ZMM;
>> +	}
>> +	if (cpu_feature_enabled(X86_FEATURE_USER_SHSTK))
>> +		x86_pmu.ext_regs_mask |= XFEATURE_MASK_CET_USER;
>> +
>>  	/* PEBS supported case */
>> -	if ((x86_pmu.arch_pebs && (caps & ARCH_PEBS_VECR_XMM)) ||
>> -	    (x86_pmu.intel_cap.pebs_format >= 4 && x86_pmu.intel_cap.pebs_baseline))
>> +	if (x86_pmu.intel_cap.pebs_format >= 4 && x86_pmu.intel_cap.pebs_baseline)
>>  		dest_pmu->capabilities |= PERF_PMU_CAP_EXTENDED_REGS;
>> +	if (x86_pmu.arch_pebs && (caps & ARCH_PEBS_VECR_EXT)) {
>> +		dest_pmu->capabilities |= PERF_PMU_CAP_EXTENDED_REGS |
>> +					  PERF_PMU_CAP_SIMD_REGS;
>> +	}
> [Severity: High]
> This is a pre-existing issue, but does enabling these capabilities expose a
> type confusion bug in software PMUs?
>
> In arch/x86/kernel/perf_regs.c:perf_reg_validate(), the PERF_REG_X86_RESERVED
> mask only covers bits 16-31. When simd_enabled is false, this allows a user
> requesting extended register bits (>= 32) to bypass validation.
>
> When a tracepoint fires, perf_output_sample_regs() passes a raw
> stack-allocated pt_regs pointer to perf_reg_value(). The perf_reg_value()
> function incorrectly casts it to a larger struct x86_perf_regs using
> container_of(), which reads the abi and xmm_regs fields from uninitialized
> adjacent stack memory and dereferences them.
>
> Can a user trigger this out-of-bounds stack read by creating a tracepoint or
> software PMU event with PERF_SAMPLE_REGS_INTR and requesting extended bits?

This would not really happen. The below code would check if
PERF_PMU_CAP_EXTENDED_REGS is set if  extended register bits are set.
Obviously, software PMU has no such cap PERF_PMU_CAP_EXTENDED_REG.

    if (!(pmu->capabilities & PERF_PMU_CAP_EXTENDED_REGS) &&
        event_has_extended_regs(event)) {
        ret = -EOPNOTSUPP;
        goto err_destroy;
    }



>
>>  
>>  	/* PEBS unsupported case (e.g., guest) */
>> -	if (!x86_pmu.intel_cap.pebs_format)
>> +	if (!x86_pmu.intel_cap.pebs_format) {
>>  		dest_pmu->capabilities |= PERF_PMU_CAP_EXTENDED_REGS;
>> +		if (x86_pmu.ext_regs_mask > XFEATURE_MASK_SSE)
> [Severity: Low]
> Is it safe to use a strictly greater-than operator here?
>
> If a new feature using bit 0 is ever added to ext_regs_mask, the mask
> value would become 3. The condition 3 > 2 evaluates to true, incorrectly
> enabling PERF_PMU_CAP_SIMD_REGS even if no SIMD extended features are
> supported.
>
> Could this use a bitwise comparison like (mask & ~XFEATURE_MASK_SSE) != 0
> instead?

Sure.


>
>> +			dest_pmu->capabilities |= PERF_PMU_CAP_SIMD_REGS;
>> +	}
>>  }