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 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Repeated reads of `event->header.size` directly from a potentially shared memory-mapped buffer (`MAP_SHARED`) allow an attacker to bypass bounds validation by concurrently shrinking the size after checks. This leads to severe integer underflows and subsequent heap buffer overflows or massive memory allocations.
--

commit 28785713230e6589c3240262dcee366a34bae636
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf session: Validate HEADER_ATTR attr.size before swapping

Harden PERF_RECORD_HEADER_ATTR handling against crafted perf.data:

- Validate attr.size: must be >= PERF_ATTR_SIZE_VER0, a multiple
  of sizeof(u64), and fit within the event payload.
- Copy only min(attr.size, sizeof(struct perf_event_attr)) bytes
  into a local attr, zeroing the rest so legacy files don't leak
  adjacent event data into new fields.
- Keep the original attr.size so perf_event__synthesize_attr()
  uses it for both allocation and ID-array placement.

> diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> index 41a3721a194dc..d8cb1f562f690 100644
> --- a/tools/perf/builtin-inject.c
> +++ b/tools/perf/builtin-inject.c
[ ... ]
> @@ -244,24 +245,34 @@ static int perf_event__repipe_attr(const struct perf_tool *tool,
>  	if (!inject->itrace_synth_opts.set)
>  		return perf_event__repipe_synth(tool, event);
>  
> -	if (event->header.size < sizeof(struct perf_event_header) + sizeof(u64)) {
> +	if (event->header.size < sizeof(struct perf_event_header) + PERF_ATTR_SIZE_VER0) {
>  		pr_err("Attribute event size %u is too small\n", event->header.size);
>  		return -EINVAL;
>  	}
>  
> -	if (event->header.size - sizeof(event->header) < event->attr.attr.size) {
> +	/*
> +	 * ABI0 pipe/inject events have attr.size == 0; default to
> +	 * PERF_ATTR_SIZE_VER0 (the ABI0 footprint) for the bounded
> +	 * copy and ID array position.  Same pattern as
> +	 * perf_event__process_attr() in header.c.
> +	 */
> +	raw_attr_size = event->attr.attr.size;
> +	attr_size = raw_attr_size ?: PERF_ATTR_SIZE_VER0;
> +
> +	if (raw_attr_size && (raw_attr_size < PERF_ATTR_SIZE_VER0 ||
> +			      raw_attr_size > event->header.size - sizeof(event->header))) {
>  		pr_err("Attribute event size %u is too small for attr.size %u\n",
> -		       event->header.size, event->attr.attr.size);
> +		       event->header.size, raw_attr_size);
>  		return -EINVAL;
>  	}
>  
>  	memset(&attr, 0, sizeof(attr));
>  	memcpy(&attr, &event->attr.attr,
> -	       min_t(size_t, sizeof(attr), (size_t)event->attr.attr.size));
> +	       min_t(size_t, sizeof(attr), attr_size));
>  
> -	n_ids = event->header.size - sizeof(event->header) - event->attr.attr.size;
> +	n_ids = event->header.size - sizeof(event->header) - attr_size;

[Severity: High]
This is a pre-existing issue, but does reading event->header.size here directly
from a potentially shared memory-mapped buffer (MAP_SHARED) allow a
time-of-check to time-of-use (TOCTOU) race?

If an attacker concurrently shrinks event->header.size after the bounds
validation checks above, this calculation can severely underflow.

The underflowed n_ids is then passed to perf_event__synthesize_attr() which,
on 32-bit architectures, wraps around during the size calculation.
This results in a small memory allocation followed by a massive memcpy(),
causing a heap buffer overflow.

Should we cache event->header.size in a local variable like we did for
raw_attr_size?

>  	n_ids /= sizeof(u64);
> -	ids = perf_record_header_attr_id(event);
> +	ids = (void *)&event->attr.attr + attr_size;
>  
>  	attr.size = sizeof(struct perf_event_attr);
>  	attr.sample_type &= ~PERF_SAMPLE_AUX;

[ ... ]
> 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,
>  			     union perf_event *event,
>  			     struct evlist **pevlist)
>  {
> -	u32 i, n_ids;
> +	struct perf_event_attr attr;
> +	u32 i, n_ids, raw_attr_size;
>  	u64 *ids;
> +	size_t attr_size, copy_size;
>  	struct evsel *evsel;
>  	struct evlist *evlist = *pevlist;
>  
> +	/*
> +	 * HEADER_ATTR event layout (pipe/inject mode):
> +	 *
> +	 *   [header (8 bytes)] [attr (attr_size bytes)] [id0 id1 ... idN]
> +	 *   |<------------------ header.size --------------------------->|
> +	 *
> +	 * attr_size varies across perf versions: VER0 = 64 bytes,
> +	 * current sizeof(struct perf_event_attr) = larger.  A newer
> +	 * producer may emit a larger attr than we understand.
> +	 *
> +	 * attr.size == 0 (ABI0) means the producer didn't set it
> +	 * (e.g., bench/inject-buildid, older perf).  Treat as VER0.
> +	 *
> +	 * Require 8-byte alignment so the u64 ID array is aligned
> +	 * and attr.size fits cleanly within the payload.
> +	 *
> +	 * Read attr.size once — the event may be on a shared mmap
> +	 * and re-reading could yield a different value.
> +	 */
> +	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;
> +	}
> +
>  	if (dump_trace)
>  		perf_event__fprintf_attr(event, stdout);
>  
> @@ -5108,13 +5155,46 @@ int perf_event__process_attr(const struct perf_tool *tool __maybe_unused,
>  			return -ENOMEM;
>  	}
>  
> -	evsel = evsel__new(&event->attr.attr);
> +	/*
> +	 * attr_size = footprint of the attr in the event — determines
> +	 * where the ID array starts.  For ABI0, assume VER0 (64 bytes).
> +	 *
> +	 * copy_size = how much we copy into our local struct, capped at
> +	 * sizeof(attr) so a newer producer's larger attr doesn't
> +	 * overflow.  Fields beyond copy_size are zeroed.
> +	 *
> +	 * Do NOT write attr_size back to the event — native-endian
> +	 * files use MAP_SHARED (read-only), writing would SIGSEGV.
> +	 * The swap path handles ABI0 in perf_event__attr_swap()
> +	 * which writes to the writable MAP_PRIVATE copy instead.
> +	 */
> +	attr_size = raw_attr_size ?: PERF_ATTR_SIZE_VER0;
> +	copy_size = min(attr_size, sizeof(attr));
> +	memcpy(&attr, &event->attr.attr, copy_size);
> +	if (copy_size < sizeof(attr))
> +		memset((void *)&attr + copy_size, 0, sizeof(attr) - copy_size);
> +
> +	/*
> +	 * Normalize ABI0: the swap path sets attr.size = VER0 on the
> +	 * event, but the native path leaves it as 0.  Set it on the
> +	 * local copy so perf inject re-synthesizes with consistent
> +	 * layout regardless of endianness.
> +	 */
> +	attr.size = attr_size;
> +
> +	evsel = evsel__new(&attr);
>  	if (evsel == NULL)
>  		return -ENOMEM;
>  
>  	evlist__add(evlist, evsel);
>  
> -	n_ids = event->header.size - sizeof(event->header) - event->attr.attr.size;
> +	/*
> +	 * IDs occupy the remainder after header + attr.  Use attr_size
> +	 * (not copy_size) — even if the producer's attr is larger than
> +	 * our struct, the IDs start after attr_size bytes in the event.
> +	 * Validation above guarantees attr_size <= payload size.
> +	 */
> +	n_ids = event->header.size - sizeof(event->header) - attr_size;

[Severity: High]
This is a pre-existing issue, but can an attacker concurrently modify
event->header.size here?

Because event can point to a MAP_SHARED buffer, reading event->header.size
multiple times allows a TOCTOU race where the size is shrunk after the bounds
checks.

This would cause n_ids to severely underflow, leading to a massive
memory allocation in perf_evsel__alloc_id() and out-of-bounds reads.

Could we fix this by caching event->header.size into a local variable before
performing the initial validation?

>  	n_ids = n_ids / sizeof(u64);
>  	/*
>  	 * We don't have the cpu and thread maps on the header, so

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