Re: [PATCH 10/29] perf session: Validate HEADER_ATTR attr.size before swapping

[sashiko-bot@kernel.org]

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

New issues:
- [High] Double-fetch of `event->header.size` from a shared memory mapping allows an attacker to bypass bounds validation and trigger an out-of-bounds read.

Pre-existing issues:
- [High] The patch fails to apply the ABI0 (attr.size = 0) fix to `perf_event__repipe_attr()` in `tools/perf/builtin-inject.c`, causing `perf inject` to synthesize corrupted events.
--

commit 3c40045df7139a78501fe156fe285fc86c33951e
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf session: Validate HEADER_ATTR attr.size before swapping

This commit hardens PERF_RECORD_HEADER_ATTR handling against crafted perf.data
by validating attr.size. It copies only bounded sizes into a local attr and
handles ABI0 pipe/inject events where attr.size is 0.

>     Handle ABI0 pipe/inject events where attr.size is 0: use a local
>     attr_size variable set to PERF_ATTR_SIZE_VER0 for both the bounded
>     copy and ID array position, instead of writing back to the event.

[Severity: High]
This is a pre-existing issue, but does the patch miss applying the ABI0 fix
to perf_event__repipe_attr() in tools/perf/builtin-inject.c?

In tools/perf/builtin-inject.c, perf_event__repipe_attr() still directly
reads event->attr.attr.size. For native-endian ABI0 events, this size remains
0, causing memcpy to copy 0 bytes (zeroing the local attr) and n_ids to be
heavily overcalculated.

perf_record_header_attr_id(event) then incorrectly points the ID array to
the start of the attribute payload. This causes perf inject to synthesize
a corrupted event, breaking functionality for legacy perf.data files.

> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index f30e48eb3fc32..967c3d8ff12c8 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
[ ... ]
> @@ -5094,11 +5110,42 @@ int perf_event__process_attr(const struct perf_tool *tool __maybe_unused,
[ ... ]
> +	raw_attr_size = event->attr.attr.size;
> +	if (event->header.size < sizeof(event->header) + PERF_ATTR_SIZE_VER0 ||
> +	    (raw_attr_size && (raw_attr_size < PERF_ATTR_SIZE_VER0 ||
> +			      raw_attr_size % sizeof(u64) ||
> +			      raw_attr_size > event->header.size - sizeof(event->header)))) {
> +		pr_err("PERF_RECORD_HEADER_ATTR: invalid attr.size %u (event size %u, min %d)\n",
> +		       raw_attr_size, event->header.size, PERF_ATTR_SIZE_VER0);
> +		return -EINVAL;
> +	}
[ ... ]
> -	n_ids = event->header.size - sizeof(event->header) - event->attr.attr.size;
> +	n_ids = event->header.size - sizeof(event->header) - attr_size;

[Severity: High]
Does this code introduce a double-fetch of event->header.size from a shared
memory mapping?

For native-endian files mapped via MAP_SHARED, could a concurrent change
provide a valid event->header.size to pass the bounds check in the if
condition, and then overwrite it with a large value before it is fetched
again for the n_ids calculation?

This might inflate n_ids, causing the subsequent ID array loop to perform
out-of-bounds reads past the event, which could lead to a segmentation fault
or leak adjacent memory contents. Could we cache event->header.size to
prevent this?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260524032709.1080771-1-acme@kernel.org?part=10