Re: [Patch v7 22/24] perf/x86/intel: Enable arch-PEBS based SIMD/eGPRs/SSP sampling

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [Critical] Leaking stale per-CPU FPU/extended register state to user space when a PEBS event triggers in kernel mode and requests PERF_SAMPLE_REGS_USER.
- [High] Incorrect bitwise AND with a raw u64 bitmask in `intel_pebs_support_regs` permanently disables large PEBS for XMM registers on arch-PEBS platforms.
--

commit 7ad07fe08e2e0b394952e6f0ccf5f040f99870e3
Author: Dapeng Mi <dapeng1.mi@linux.intel.com>

perf/x86/intel: Enable arch-PEBS based SIMD/eGPRs/SSP sampling

This patch enables arch-PEBS based SIMD/eGPRs/SSP registers sampling
using the XSAVE-Enabled Registers (XER) group.

> diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> index 3d64f28b7e9ac..b280a39f68bca 100644
> --- a/arch/x86/events/intel/core.c
> +++ b/arch/x86/events/intel/core.c

[ ... ]

> @@ -4416,6 +4431,34 @@ static void intel_pebs_aliases_skl(struct perf_event *event)
>  	return intel_pebs_aliases_precdist(event);
>  }
>  
> +static inline bool intel_pebs_support_regs(struct perf_event *event, u64 regs)
> +{
> +	struct arch_pebs_cap cap = hybrid(event->pmu, arch_pebs_cap);
> +	int pebs_format = x86_pmu.intel_cap.pebs_format;
> +	bool supported = true;
> +
> +	/* SSP */
> +	if (regs & PEBS_DATACFG_GP)
> +		supported &= x86_pmu.arch_pebs && (ARCH_PEBS_GPR & cap.caps);
> +	if (regs & PEBS_DATACFG_XMMS) {
> +		supported &= x86_pmu.arch_pebs ?
> +			     ARCH_PEBS_VECR_XMM & cap.caps :
> +			     pebs_format > 3 && x86_pmu.intel_cap.pebs_baseline;
> +	}

Will this permanently disable large PEBS for XMM registers?

Because supported is a boolean (initially 1), ARCH_PEBS_VECR_XMM & cap.caps
evaluates to BIT_ULL(49). The bitwise AND operation 1 &= 0x2000000000000ULL
strictly yields 0.

Should this bitmask be normalized to a boolean value, perhaps using
!!(ARCH_PEBS_VECR_XMM & cap.caps) inside the ternary operator?

[ ... ]

> diff --git a/arch/x86/events/intel/ds.c b/arch/x86/events/intel/ds.c
> index 560f178d7ac92..61d72c58f9a25 100644
> --- a/arch/x86/events/intel/ds.c
> +++ b/arch/x86/events/intel/ds.c

[ ... ]

> @@ -2708,15 +2719,69 @@ static void setup_arch_pebs_sample_data(struct perf_event *event,
>  					   meminfo->tsx_tuning, ax);
>  	}
>  
> -	if (header->xmm) {
> +	if (header->xmm || header->ymmh || header->egpr ||
> +	    header->opmask || header->zmmh || header->h16zmm) {
> +		struct arch_pebs_xer_header *xer_header = next_record;
>  		struct pebs_xmm *xmm;
> +		struct ymmh_struct *ymmh;
> +		struct avx_512_zmm_uppers_state *zmmh;
> +		struct avx_512_hi16_state *h16zmm;
> +		struct avx_512_opmask_state *opmask;
> +		struct apx_state *egpr;
>  
>  		next_record += sizeof(struct arch_pebs_xer_header);
>  
> -		ignore_mask |= XFEATURE_MASK_SSE;
> -		xmm = next_record;
> -		perf_regs->xmm_regs = xmm->xmm;
> -		next_record = xmm + 1;
> +		if (header->xmm) {
> +			ignore_mask |= XFEATURE_MASK_SSE;
> +			xmm = next_record;
> +			/*
> +			 * Only output XMM regs to user space when arch-PEBS
> +			 * really writes data into xstate area.
> +			 */
> +			if (xer_header->xstate & XFEATURE_MASK_SSE)
> +				perf_regs->xmm_regs = xmm->xmm;
> +			next_record = xmm + 1;
> +		}

Is it possible for this code to leak stale per-CPU FPU/extended register
state to user space?

When a PEBS event triggers in kernel mode and requests PERF_SAMPLE_REGS_USER,
this code processes the PEBS data and sets the corresponding bits
(e.g., XFEATURE_MASK_SSE) in ignore_mask. It then calls
x86_pmu_setup_regs_data(), which assigns data->regs_user.regs to the
static per-CPU x86_user_regs.

If x86_user_regs is never fully cleared, its extended register pointers
(like xmm_regs) still point to the per-CPU ext_regs_buf from prior samples.
Because XFEATURE_MASK_SSE is excluded via ignore_mask, xsaves_nmi() and
x86_pmu_update_xregs() skip updating x86_user_regs.xmm_regs.

Consequently, the xmm_regs pointer remains pointing to the stale data in
ext_regs_buf. When the perf core outputs the user registers, could it
read the stale FPU state from ext_regs_buf and leak it across processes?

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