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

[sashiko-bot@kernel.org]

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?

> +	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?

>  
>  	/* 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?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706015439.3040804-1-dapeng1.mi@linux.intel.com?part=22