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

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

On 7/6/2026 2:45 PM, sashiko-bot@kernel.org wrote:
> 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?

This is not a bug. The software PMU has no such caps,
PERF_PMU_CAP_EXTENDED_REGS or PERF_PMU_CAP_SIMD_REGS, so if software events
want to sample these extended regs, the event creation would fail.


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

Ditto.


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

Yes, the perf_simd_reg_validate() would be enhanced to validate the
REGS_USER and REGS_INTR configuration separately.

Thanks.


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