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:
- [Medium] Missing TOCTOU protection for `event->header.size` during validation on shared mmaps.
--

commit e4ad3606afbf2146d8e039237ce12985c68071fa
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
Subject: perf session: Validate HEADER_ATTR attr.size before swapping

This patch hardens PERF_RECORD_HEADER_ATTR handling against crafted perf.data
files. It validates attr.size, bounds the copy size to prevent leaking
adjacent event data, and retains the original attr.size for correct event
re-synthesis.

> 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: Medium]
Is it possible for event->header.size to change between the validation above
and this calculation?

Since raw_attr_size was explicitly snapshotted to prevent race conditions on
shared memory mappings, could a concurrently modified event->header.size
bypass the bounds check?

If event->header.size is shrunk concurrently after the validation, could this
cause n_ids to underflow?

>  	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;
> +	}
> +
[ ... ]
> -	n_ids = event->header.size - sizeof(event->header) - event->attr.attr.size;
> +	n_ids = event->header.size - sizeof(event->header) - attr_size;

[Severity: Medium]
Could this calculation also suffer from the same race condition if
event->header.size is modified after the validation above?

If an attacker shrinks event->header.size concurrently, could n_ids underflow
and lead to an out-of-bounds memory read when allocating and populating the
IDs array?

>  	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/20260526211806.1193848-1-acme@kernel.org?part=10