Re: [Patch v8 13/23] perf/x86: Support XMM sampling using sample_simd_vec_reg_* fields

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

On 5/29/2026 4:49 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Critical] The validation logic in `perf_simd_reg_validate()` strictly checks unused, flag-gated syscall parameters outside of their respective flag scopes, breaking ABI compatibility.
> --
>
> commit aeb4cf15401ade404bfd333c1c769dd23251abf8
> Author: Dapeng Mi <dapeng1.mi@linux.intel.com>
>
> perf/x86: Support XMM sampling using sample_simd_vec_reg_* fields
>
> This patch adds support for sampling XMM registers using the new
> sample_simd_vec_reg_* fields. It introduces ABI tracking for perf_regs
> on x86 and implements validation logic for SIMD registers.
>
>> diff --git a/arch/x86/kernel/perf_regs.c b/arch/x86/kernel/perf_regs.c
>> index 81204cb7f7239..7b9b38c189de2 100644
>> --- a/arch/x86/kernel/perf_regs.c
>> +++ b/arch/x86/kernel/perf_regs.c
> [ ... ]
>> @@ -74,6 +77,71 @@ u64 perf_reg_value(struct pt_regs *regs, int idx)
> [ ... ]
>> +int perf_simd_reg_validate(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;
>> +
>> +	if (!simd_enabled) {
>> +		if (vec_qwords || vec_mask_intr || vec_mask_user ||
>> +		    pred_qwords || pred_mask_intr || pred_mask_user)
>> +			return -EINVAL;
> [Severity: Critical]
> Are we unconditionally validating syscall parameters outside their flag scopes
> here?
>
> If userspace sets PERF_SAMPLE_REGS_USER but does not set
> PERF_SAMPLE_REGS_INTR, the vec_mask_intr and pred_mask_intr parameters
> could contain uninitialized stack garbage. Because userspace is not required
> to zero-fill unused arguments, returning -EINVAL for garbage in these unused
> masks could break ABI compatibility for programs recompiled against newer
> kernel headers.

Hmm, it makes sense partly. Would add sample_type argument into this
function and check more accurately.

Thanks.


>
>> +		return 0;
>> +	}
>> +
>> +	if (!vec_qwords) {
>> +		if (vec_mask_intr || vec_mask_user)
>> +			return -EINVAL;
>> +	} else {
>> +		if (vec_qwords != PERF_X86_XMM_QWORDS)
>> +			return -EINVAL;
>> +		if ((!vec_mask_intr && !vec_mask_user) ||
>> +		    (vec_mask_intr & ~PERF_X86_SIMD_VEC_MASK) ||
>> +		    (vec_mask_user & ~PERF_X86_SIMD_VEC_MASK))
>> +			return -EINVAL;
> [Severity: Critical]
> Similarly, could checking these masks cause false -EINVAL errors
> if only one of the sampling flags (INTR or USER) is enabled?
>
> If PERF_SAMPLE_REGS_USER is enabled but PERF_SAMPLE_REGS_INTR is disabled,
> vec_mask_intr might contain random bits that fail the PERF_X86_SIMD_VEC_MASK
> check. Should these validation checks be gated behind the presence of their
> respective flags in the sample_type to prevent regressions?
>
>> +	}
>> +
>> +	if (pred_qwords || pred_mask_intr || pred_mask_user)
>> +		return -EINVAL;
>> +
>> +	size = ((vec_qwords * hweight64(vec_mask_intr)) +
>> +		(vec_qwords * hweight64(vec_mask_user)) +
>> +		(pred_qwords * hweight32(pred_mask_intr)) +
>> +		(pred_qwords * hweight32(pred_mask_user))) * sizeof(u64);
>> +	if (size > U16_MAX)
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}