Re: [Patch v10 05/10] perf regs: Support x86 eGPRs/SSP sampling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Hardcoded 32-byte SIMD header skip in `__evsel__parse_sample` corrupts parsing of future `perf.data` files containing actual SIMD data payloads.
- [High] `perf_event__synthesize_sample` fails to write the SIMD register header, leading to uninitialized memory leaks and corruption of synthesized events.
- [High] Silently substituting requested eGPRs with legacy XMM registers on non-hardware events.
--

--- Patch [5]: [Patch v10 05/10] perf regs: Support x86 eGPRs/SSP sampling ---
commit 8930556b90beb6ec5b6f4e43ddaf5145e6dda06d
Author: Dapeng Mi <dapeng1.mi@linux.intel.com>

perf regs: Support x86 eGPRs/SSP sampling

This commit adds support for sampling x86 extended GP registers (R16-R31)
and the shadow stack pointer (SSP) register. It reclaims the original XMM
registers space in the sampling masks to represent eGPRs and SSP when SIMD
sampling is supported, distinguishing them with a new abi flag.

> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index a5b0687a3a79f..da0ad06446091 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c

[ ... ]

> @@ -1661,12 +1664,16 @@ void evsel__config(struct evsel *evsel, const struct record_opts *opts,
>  	if (opts->sample_intr_regs && !evsel->no_aux_samples &&
>  	    !evsel__is_dummy_event(evsel)) {
>  		attr->sample_regs_intr = opts->sample_intr_regs;
> +		attr->sample_simd_regs_enabled = evsel__is_hardware_event(evsel) ?
> +				!!opts->sample_simd_regs_enabled : 0;

[Severity: High]
Does clearing the sample_simd_regs_enabled flag for non-hardware events
cause the kernel to misinterpret the bits as legacy XMM requests?

If a user requests sampling of eGPRs on a software event, this flag is
forcibly cleared, but the opts->sample_intr_regs bitmask is preserved.
Because eGPR definitions alias legacy XMM definitions, wouldn't the kernel
see the bits set alongside sample_simd_regs_enabled = 0 and dump legacy XMM
data instead?

The tool might then read it back without the SIMD ABI flag and print it as
XMM, silently losing the user's intent to capture eGPRs. Should the
aliasing eGPR bits be masked out from the sample_regs mask when clearing
the SIMD flag?

>  		evsel__set_sample_bit(evsel, REGS_INTR);
>  	}
>  
>  	if (opts->sample_user_regs && !evsel->no_aux_samples &&
>  	    !evsel__is_dummy_event(evsel)) {
>  		attr->sample_regs_user |= opts->sample_user_regs;
> +		attr->sample_simd_regs_enabled = evsel__is_hardware_event(evsel) ?
> +				!!opts->sample_simd_regs_enabled : 0;

[Severity: High]
This seems to have the same aliasing issue as above where eGPR requests on
non-hardware events will be silently interpreted as legacy XMM registers.

>  		evsel__set_sample_bit(evsel, REGS_USER);
>  	}

[ ... ]

> @@ -3679,6 +3686,13 @@ int __evsel__parse_sample(struct evsel *evsel, union perf_event *event,
>  			regs->mask = mask;
>  			regs->regs = (u64 *)array;
>  			array = (void *)array + sz;
> +
> +			if (regs->abi & PERF_SAMPLE_REGS_ABI_SIMD) {
> +				/* Skip SIMD-regs header. */
> +				sz = 4 * sizeof(u64);
> +				OVERFLOW_CHECK(array, sz, max_size);
> +				array = (void *)array + sz;
> +			}
>  		}
>  	}

[Severity: High]
Will this hardcoded 32-byte header skip corrupt parsing for files containing
actual SIMD data payloads?

The parser unconditionally skips exactly 4 u64s for the SIMD header, but it
doesn't dynamically compute and skip the variable-length SIMD payload that
might follow it in future files. This would misalign the array pointer for
subsequent fields like STACK_USER, causing garbage reads and triggering
OVERFLOW_CHECK aborts.

[ ... ]

> @@ -3736,6 +3750,13 @@ int __evsel__parse_sample(struct evsel *evsel, union perf_event *event,
>  			regs->mask = mask;
>  			regs->regs = (u64 *)array;
>  			array = (void *)array + sz;
> +
> +			if (regs->abi & PERF_SAMPLE_REGS_ABI_SIMD) {
> +				/* Skip SIMD-regs header. */
> +				sz = 4 * sizeof(u64);
> +				OVERFLOW_CHECK(array, sz, max_size);
> +				array = (void *)array + sz;
> +			}
>  		}
>  	}

[Severity: High]
This appears to have the same issue with skipping the SIMD header without
handling the variable-length payload.

> diff --git a/tools/perf/util/synthetic-events.c b/tools/perf/util/synthetic-events.c
> index b75f9dcf4dbfe..503a239b0b10f 100644
> --- a/tools/perf/util/synthetic-events.c
> +++ b/tools/perf/util/synthetic-events.c
> @@ -1527,6 +1527,8 @@ size_t perf_event__sample_event_size(const struct perf_sample *sample, u64 type,
>  		if (sample->user_regs && sample->user_regs->abi) {
>  			result += sizeof(u64);
>  			sz = hweight64(sample->user_regs->mask) * sizeof(u64);
> +			if (sample->user_regs->abi & PERF_SAMPLE_REGS_ABI_SIMD)
> +				sz += 4 * sizeof(u64);

[Severity: High]
Does perf_event__synthesize_sample() also need to be updated to write these
32 bytes?

Since perf_event__sample_event_size() allocates 32 extra bytes for the SIMD
header here, if perf_event__synthesize_sample() doesn't write them,
subsequent fields could be written 32 bytes early.

When the malformed event is parsed by __evsel__parse_sample(), the parser
will skip 32 bytes after the registers, completely missing the early-written
subsequent fields and corrupting the stream. The final 32 bytes of the event
buffer would remain uninitialized heap memory, leaking information when
emitted.

>  			result += sz;
>  		} else {
>  			result += sizeof(u64);
> @@ -1555,6 +1557,8 @@ size_t perf_event__sample_event_size(const struct perf_sample *sample, u64 type,
>  		if (sample->intr_regs && sample->intr_regs->abi) {
>  			result += sizeof(u64);
>  			sz = hweight64(sample->intr_regs->mask) * sizeof(u64);
> +			if (sample->intr_regs->abi & PERF_SAMPLE_REGS_ABI_SIMD)
> +				sz += 4 * sizeof(u64);

[Severity: High]
This appears to have the same issue as above, where the allocated SIMD
header bytes might not be written by perf_event__synthesize_sample().

>  			result += sz;
>  		} else {
>  			result += sizeof(u64);

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