Re: [Patch v8 1/5] perf headers: Sync perf_event.h/perf_regs.h with the kernel headers

[sashiko-bot@kernel.org]

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

New issues:
- [High] The newly introduced variable-length SIMD payload in `PERF_SAMPLE_REGS_USER` and `PERF_SAMPLE_REGS_INTR` breaks sample parsing, leading to severe data corruption for subsequent fields.
- [High] The newly added SIMD configuration and reserved fields in `struct perf_event_attr` are not byte-swapped in `perf_event__attr_swap()`.

Pre-existing issues:
- [Critical] An integer underflow in `read_attr` when processing the `size` field of a `perf_event_attr` structure leads to a massive stack-based buffer overflow.
- [Critical] In `read_event_desc`, a lack of minimum size validation on the `sz` variable leads to out-of-bounds heap memory corruption during byte-swapping.
--

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

perf headers: Sync perf_event.h/perf_regs.h with the kernel headers

This patch syncs the UAPI header changes to support SIMD, eGPRs, and SSP
sampling into the corresponding perf tool UAPI headers. It also adds a
sanity check for the newly introduced __reserved_4 field in
perf_attr_check().

> diff --git a/tools/include/uapi/linux/perf_event.h b/tools/include/uapi/linux/perf_event.h
> index fd10aa8d697f2..c49fc76292f79 100644
> --- a/tools/include/uapi/linux/perf_event.h
> +++ b/tools/include/uapi/linux/perf_event.h
[ ... ]
> @@ -547,6 +549,29 @@ struct perf_event_attr {
>  
>  	__u64	config3; /* extension of config2 */
>  	__u64	config4; /* extension of config3 */
> +
> +	/*
> +	 * Defines the sampling SIMD/PRED(predicate) registers bitmap and
> +	 * qwords (8 bytes) length.
> +	 *
> +	 * sample_simd_regs_enabled != 0 indicates there are SIMD/PRED
> +	 * registers to be sampled, the SIMD/PRED registers bitmap and
> +	 * qwords length are represented in
> +	 * sample_simd_{vec|pred}_reg_{intr|user} and
> +	 * sample_simd_{vec|pred}_reg_qwords fields separately.
> +	 *
> +	 * sample_simd_regs_enabled == 0 indicates no SIMD/PRED registers
> +	 * are sampled.
> +	 */
> +	__u16	sample_simd_regs_enabled;
> +	__u16	sample_simd_pred_reg_qwords;
> +	__u16	sample_simd_vec_reg_qwords;
> +	__u16	__reserved_4;
> +
> +	__u32	sample_simd_pred_reg_intr;
> +	__u32	sample_simd_pred_reg_user;
> +	__u64	sample_simd_vec_reg_intr;
> +	__u64	sample_simd_vec_reg_user;
>  };

[Severity: High]
Are these newly added fields missing byte-swapping logic in
perf_event__attr_swap()?

When perf reads a cross-endian perf.data file, if these fields are not added
to perf_event__attr_swap() in tools/perf/util/session.c, they will remain in
their original byte order. 

Later, when evsel__parse_sample() relies on fields like attr_nr_vectors and
attr_vec_qwords to validate SIMD data bounds, could incorrect values bypass
validation or cause out-of-bounds read faults?

[ ... ]
> @@ -1020,7 +1045,15 @@ enum perf_event_type {
>  	 *      } && PERF_SAMPLE_BRANCH_STACK
>  	 *
>  	 *	{ u64			abi; # enum perf_sample_regs_abi
> -	 *	  u64			regs[weight(mask)]; } && PERF_SAMPLE_REGS_USER
> +	 *	  u64			regs[weight(mask)];
> +	 *	  struct {
> +	 *		u64 nr_vectors;		# 0 ... weight(sample_simd_vec_reg_user)
> +	 *		u64 vector_qwords;	# 0 ... sample_simd_vec_reg_qwords
> +	 *		u64 nr_pred;		# 0 ... weight(sample_simd_pred_reg_user)
> +	 *		u64 pred_qwords;	# 0 ... sample_simd_pred_reg_qwords
> +	 *		u64 data[nr_vectors * vector_qwords + nr_pred * pred_qwords];
> +	 *	  } && (abi & PERF_SAMPLE_REGS_ABI_SIMD)
> +	 *	} && PERF_SAMPLE_REGS_USER

[Severity: High]
Does appending this dynamic-length SIMD structure to PERF_SAMPLE_REGS_USER
break sample parsing for older perf tools?

Older parsers do not know about the PERF_SAMPLE_REGS_ABI_SIMD bit. When
parsing the sample payload, they will only advance the array pointer by
hweight64(attr.sample_regs_user) * sizeof(u64) bytes, failing to skip the new
SIMD header and data. 

Could this cause subsequent fields like PERF_SAMPLE_STACK_USER to read the
SIMD data as their own, resulting in parsing desynchronization and data
corruption?

Should a new bit be added to enum perf_event_sample_format instead?

[ ... ]
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index f30e48eb3fc32..e8e4e00d6b4d0 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c

[Severity: Critical]
This isn't a bug introduced by this patch, but is there an integer underflow
in read_attr() when processing the size field of the perf_event_attr structure?

The function reads the first 64 bytes and extracts attr->size into sz. It
checks if sz == 0 or sz > our_sz, but fails to verify if sz < PERF_ATTR_SIZE_VER0.
If a perf.data file supplies a size between 1 and 63, could the calculation
left = sz - PERF_ATTR_SIZE_VER0 underflow the unsigned size_t variable left,
producing a huge positive value?

The subsequent readn(fd, ptr, left) attempts to read this massive amount of
data into f_attr, which is allocated on the stack in
perf_session__read_header(), potentially resulting in a stack buffer overflow.

[Severity: Critical]
This is a pre-existing issue, but does read_event_desc() lack minimum size
validation on the sz variable before byte-swapping?

The function reads a 32-bit sz from the file, allocates a heap buffer, and
calls perf_event__attr_swap(buf). Inside this function, the code
unconditionally reads and byte-swaps the first 8 bytes.

If sz is between 1 and 7, attr->size reads out-of-bounds heap memory. Because
bswap_safe() uses this uncontrolled attr->size to determine which subsequent
fields to swap, could it proceed to byte-swap up to 176 bytes of heap memory
past the small allocation, leading to heap memory corruption?

> @@ -2091,7 +2091,8 @@ static void free_event_desc(struct evsel *events)
>  
>  static bool perf_attr_check(struct perf_event_attr *attr)
>  {
> -	if (attr->__reserved_1 || attr->__reserved_2 || attr->__reserved_3) {
> +	if (attr->__reserved_1 || attr->__reserved_2 ||
> +	    attr->__reserved_3 || attr->__reserved_4) {
>  		pr_warning("Reserved bits are set unexpectedly. "
>  			   "Please update perf tool.\n");
>  		return false;

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