Re: [Patch v9 14/24] perf/x86: Support XMM sampling using sample_simd_vec_reg_* fields

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Validation logic unconditionally evaluates flag-gated syscall parameters (`vec_mask_user`, `pred_mask_user`) without checking their respective flag gates, leading to validation bypasses due to uninitialized garbage.

Pre-existing issues:
- [Critical] The functions `perf_reg_value` and `perf_simd_reg_value` unsafely assume the provided `pt_regs *` is always embedded within an `x86_perf_regs` structure, leading to out-of-bounds reads and arbitrary pointer dereferences for software events and `PERF_SAMPLE_REGS_USER`.
--

commit 9c74aceec4401218b750679a5b08ba8009aa85a3
Author: Dapeng Mi <dapeng1.mi@linux.intel.com>

perf/x86: Support XMM sampling using sample_simd_vec_reg_* fields

This commit adds support for sampling XMM registers by introducing a new ABI
field and extracting XMM values through the sample_simd_vec_reg_* fields. It
adds a new perf_simd_reg_value function to retrieve the values and updates
the SIMD registration validation logic.

> diff --git a/arch/x86/kernel/perf_regs.c b/arch/x86/kernel/perf_regs.c
> index 81204cb7f7239..8514baefb400e 100644
> --- a/arch/x86/kernel/perf_regs.c
> +++ b/arch/x86/kernel/perf_regs.c

[ ... ]

> @@ -63,6 +63,9 @@ u64 perf_reg_value(struct pt_regs *regs, int idx)
>  
>  	if (idx >= PERF_REG_X86_XMM0 && idx < PERF_REG_X86_XMM_MAX) {
>  		perf_regs = container_of(regs, struct x86_perf_regs, regs);
> +		/* SIMD registers are moved to dedicated sample_simd_vec_reg */
> +		if (perf_regs->abi & PERF_SAMPLE_REGS_ABI_SIMD)

[Severity: Critical]
This isn't a bug introduced by this patch, but evaluating perf_regs->abi
here relies on an unsafe cast.

For tracepoints and PERF_SAMPLE_REGS_USER, regs points to a bare pt_regs
instance rather than an embedded x86_perf_regs struct. The container_of()
cast accesses memory outside the bounds of the pt_regs struct.

Could this allow an attacker to control the out-of-bounds pointers (like
xmm_regs) and read arbitrary kernel memory into the perf ring buffer?

> +			return 0;
>  		if (!perf_regs->xmm_regs)
>  			return 0;
>  		return perf_regs->xmm_regs[idx - PERF_REG_X86_XMM0];

[ ... ]

> @@ -74,6 +77,77 @@ u64 perf_reg_value(struct pt_regs *regs, int idx)
>  	return regs_get_register(regs, pt_regs_offset[idx]);
>  }
>  
> +u64 perf_simd_reg_value(struct pt_regs *regs, int idx,
> +			u16 qwords_idx, bool pred)
> +{
> +	struct x86_perf_regs *perf_regs =
> +			container_of(regs, struct x86_perf_regs, regs);
> +
> +	if (!(perf_regs->abi & PERF_SAMPLE_REGS_ABI_SIMD))

[Severity: Critical]
This is a pre-existing issue, but this new code copies the same unsafe
container_of() cast pattern.

Since regs is not actually embedded in an x86_perf_regs struct when
called for PERF_SAMPLE_REGS_USER, reading perf_regs->abi reads
out-of-bounds memory.

> +		return 0;
> +
> +	if (pred)
> +		return 0;

[ ... ]

> +int perf_simd_reg_validate(u64 sample_type, u16 simd_enabled,
> +			   u16 vec_qwords, u64 vec_mask_intr,
> +			   u64 vec_mask_user, u16 pred_qwords,
> +			   u32 pred_mask_intr, u32 pred_mask_user)
> +{
> +	u64 size = 0;
> +
> +	if (!(sample_type &
> +	      (PERF_SAMPLE_REGS_INTR | PERF_SAMPLE_REGS_USER)))
> +		return 0;
> +
> +	if (!simd_enabled)
> +		return 0;
> +
> +	if (vec_qwords) {
> +		if (vec_qwords != PERF_X86_XMM_QWORDS)
> +			return -EINVAL;
> +		if (!vec_mask_intr && !vec_mask_user)
> +			return -EINVAL;

[Severity: Medium]
Does this logic evaluate flag-gated parameters without checking their
respective flag gates?

PERF_SAMPLE_REGS_INTR and PERF_SAMPLE_REGS_USER are independent flags. If a
user sets INTR but not USER, vec_mask_user is outside its flag gate and
can contain arbitrary garbage from userspace.

If vec_mask_intr is 0 (an invalid configuration), but the garbage in
vec_mask_user is non-zero, this check passes and the validation is bypassed.

Should these masks be validated against their respective sample_type flags
before checking if they are both empty?

> +		if ((sample_type & PERF_SAMPLE_REGS_INTR) &&
> +		    (vec_mask_intr & ~PERF_X86_SIMD_VEC_MASK))
> +			return -EINVAL;

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