Re: [Patch v8 18/23] perf/x86: Support eGPRs sampling using sample_regs_* fields

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] The patch unconditionally rejects a mask of 0 when simd_enabled is false, breaking userspace ABI.
- [High] The patch evaluates sample_regs_user and sample_regs_intr without verifying their respective flag gates are set.
- [High] The 32-bit implementation of perf_reg_validate() is not updated to handle extended register masks, allowing a DoS via WARN_ON_ONCE.

Pre-existing issues:
- [Critical] perf_reg_value() blindly casts the raw pt_regs pointer to struct x86_perf_regs, leading to an out-of-bounds read for software events.
- [Critical] perf_reg_validate() fails to mask out bits >= 32 when simd_enabled is false, allowing out-of-bounds register indices.
--

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

perf/x86: Support eGPRs sampling using sample_regs_* fields

This patch adds support for sampling APX eGPRs (R16 ~ R31) by reclaiming
spare space in the sample_regs_* fields when sample_simd_regs_enabled is set.
It updates perf_reg_value() and perf_reg_validate() to handle the new eGPRs
bitmap alongside legacy XMM registers.

> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
[ ... ]
> @@ -708,26 +708,24 @@ int x86_pmu_hw_config(struct perf_event *event)
[ ... ]
>  		if (event_has_simd_regs(event)) {
> +			u64 reserved = ~GENMASK_ULL(PERF_REG_MISC_MAX - 1, 0);
> +
>  			if (!(event->pmu->capabilities & PERF_PMU_CAP_SIMD_REGS))
>  				return -EINVAL;
>  			if (is_sampling_event(event) && !event->attr.precise_ip &&
>  			    !this_cpu_has(X86_FEATURE_XSAVES))
>  				return -EINVAL;
> +			/*
> +			 * The XMM space in the perf_event_x86_regs is reclaimed
> +			 * for eGPRs and other general registers.
> +			 */
> +			if ((event->attr.sample_regs_user & reserved) ||
> +			    (event->attr.sample_regs_intr & reserved))
> +				return -EINVAL;

[Severity: High]
Is it safe to evaluate both sample_regs_user and sample_regs_intr here without
first verifying their respective flag gates are set?

If a user only sets one of the flags (e.g., PERF_SAMPLE_REGS_USER), does the
other field contain uninitialized garbage from userspace? If that garbage
happens to overlap with the reserved mask, it seems this could trigger
validation failures and cause valid syscalls to randomly fail.

[ ... ]
> diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
> --- a/arch/x86/events/perf_event.h
> +++ b/arch/x86/events/perf_event.h
> @@ -197,6 +197,16 @@ static inline bool event_needs_opmask(struct perf_event *event)
>  	return false;
>  }
>  
> +static inline bool event_needs_egprs(struct perf_event *event)
> +{
> +	if (event->attr.sample_simd_regs_enabled &&
> +	    (event->attr.sample_regs_user & PERF_X86_EGPRS_MASK ||
> +	     event->attr.sample_regs_intr & PERF_X86_EGPRS_MASK))
> +		return true;
> +
> +	return false;
> +}

[Severity: High]
Similarly to x86_pmu_hw_config(), does this evaluate sample_regs_user and
sample_regs_intr without verifying their respective flag gates are set?

Could uninitialized data from userspace falsely trigger this condition?

[ ... ]
> diff --git a/arch/x86/kernel/perf_regs.c b/arch/x86/kernel/perf_regs.c
> --- a/arch/x86/kernel/perf_regs.c
> +++ b/arch/x86/kernel/perf_regs.c
> @@ -61,14 +61,24 @@ u64 perf_reg_value(struct pt_regs *regs, int idx)
>  {
>  	struct x86_perf_regs *perf_regs;
>  
> -	if (idx >= PERF_REG_X86_XMM0 && idx < PERF_REG_X86_XMM_MAX) {
> +	if (idx > PERF_REG_X86_R15) {
>  		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)
>  			return 0;
> -		if (!perf_regs->xmm_regs)
> -			return 0;
> -		return perf_regs->xmm_regs[idx - PERF_REG_X86_XMM0];
> +
> +		if (perf_regs->abi & PERF_SAMPLE_REGS_ABI_SIMD) {

[Severity: Critical]
This is a pre-existing issue, but does casting the raw pt_regs pointer to
struct x86_perf_regs lead to an out-of-bounds read for software events?

For software events, the generic perf code passes a standard pt_regs pointer.
Because the x86_perf_regs wrapper is not allocated for software events, using
container_of() and reading perf_regs->abi appears to access memory past the
end of the pt_regs struct.

[ ... ]
>  #ifdef CONFIG_X86_32
> -#define REG_NOSUPPORT ((1ULL << PERF_REG_X86_R8) | \
> -		       (1ULL << PERF_REG_X86_R9) | \
> -		       (1ULL << PERF_REG_X86_R10) | \
> -		       (1ULL << PERF_REG_X86_R11) | \
> -		       (1ULL << PERF_REG_X86_R12) | \
> -		       (1ULL << PERF_REG_X86_R13) | \
> -		       (1ULL << PERF_REG_X86_R14) | \
> -		       (1ULL << PERF_REG_X86_R15))
> +#define REG_NOSUPPORT GENMASK_ULL(PERF_REG_X86_R15, PERF_REG_X86_R8)
>  
>  int perf_reg_validate(u64 mask, bool simd_enabled)

[Severity: High]
Does the 32-bit implementation of perf_reg_validate() need to be updated to
handle extended register masks?

It looks like the 64-bit version was updated to reject bits 32-63 when
simd_enabled is true, but the CONFIG_X86_32 block was missed. If a 32-bit
user sets sample_simd_regs_enabled and requests bit 32, could this fall
through to the WARN_ON_ONCE() in perf_reg_value() and allow a denial of
service?

[ ... ]
> @@ -214,8 +218,13 @@ u64 perf_reg_abi(struct task_struct *task)
>  
>  int perf_reg_validate(u64 mask, bool simd_enabled)
>  {
> +	if (!simd_enabled &&
> +	    (!mask || (mask & (REG_NOSUPPORT | PERF_REG_X86_RESERVED))))
> +		return -EINVAL;

[Severity: Critical]
Does rejecting a mask of 0 when simd_enabled is false break the userspace ABI?

Profiling tools sometimes set sample_regs_user = 0 to obtain the abi indicator
field in the perf ring buffer without dumping any specific registers.

[Severity: Critical]
This is a pre-existing issue, but does this check fail to mask out bits >= 32
when simd_enabled is false?

PERF_REG_X86_RESERVED only covers bits 16-31. It appears to ignore bits 32-63.
Could an unprivileged user request bit 63 for a software event, bypassing
validation, and eventually hitting the WARN_ON_ONCE() and reading out of
bounds in perf_reg_value()?

>  	/* The mask could be 0 if only the SIMD registers are interested */
> -	if (mask & (REG_NOSUPPORT | PERF_REG_X86_RESERVED))
> +	if (simd_enabled &&
> +	    (mask & (REG_NOSUPPORT | PERF_REG_X86_EXT_RESERVED)))
>  		return -EINVAL;
>  
>  	return 0;

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