Re: [PATCH 3/5] perf header: Sanity check HEADER_EVENT_DESC

[sashiko-bot@kernel.org]

Sashiko AI review found 3 potential issue(s):
- [Critical] An integer overflow in `nre + 1` causes a 0-byte allocation, leading to a massive heap out-of-bounds write.
- [Critical] `perf_event__attr_swap()` reads and writes out-of-bounds on the heap due to an unvalidated internal `attr->size` field.
- [High] The expression `sz + sizeof(u32)` can overflow to zero on 32-bit architectures, causing a division by zero crash.
--

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

perf header: Sanity check HEADER_EVENT_DESC

This commit adds bounds checks to read_event_desc() to prevent large
allocations or excessive loop iterations by validating sizes against the
section size.

> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index f8acd06ae9235..696f19739074e 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
[ ... ]
> @@ -2101,6 +2101,13 @@ static struct evsel *read_event_desc(struct feat_fd *ff)
>  	if (do_read_u32(ff, &sz))
>  		goto error;
>  
> +	/*
> +	 * The minimum section footprint per event is sz bytes for the attr
> +	 * plus a u32 for the id count, check that nre events fit.
> +	 */
> +	if (sz == 0 || sz > ff->size || nre > (ff->size - ff->offset) / (sz + sizeof(u32)))
> +		goto error;
> +

Can the expression sz + sizeof(u32) overflow to zero on 32-bit architectures?

Since sz is a u32 read directly from the file, if an attacker crafts a file
where sz is UINT32_MAX - 3 (0xFFFFFFFC), the addition wraps around to exactly 0
in 32-bit arithmetic. This would cause the bounds check to divide by zero.

The preceding sz > ff->size check might not prevent this because ff->size is
read from the file header and could be arbitrarily large.

>  	/* buffer to hold on file attr struct */
>  	buf = malloc(sz);
>  	if (!buf)
>  		goto error;
>  
>  	/* the last event terminates with evsel->core.attr.size == 0: */
>  	events = calloc(nre + 1, sizeof(*events));

Could nre + 1 wrap around to 0 if nre is UINT32_MAX?

The new bounds check restricts nre based on ff->size, but if ff->size is
controlled by the file and set to UINT64_MAX, nre can still be 0xFFFFFFFF.

If nre + 1 wraps to 0, calloc returns a minimal valid 0-byte allocation. The
loop below would then write out of bounds on its first iteration:

    for (i = 0, evsel = events; i < nre; evsel++, i++) {
        evsel->core.idx = i;
        ...

>  	if (!events)
>  		goto error;
>  
>  	msz = sizeof(evsel->core.attr);
>  	if (sz < msz)
>  		msz = sz;
>  
>  	for (i = 0, evsel = events; i < nre; evsel++, i++) {
>  		evsel->core.idx = i;
>  
>  		/*
>  		 * must read entire on-file attr struct to
>  		 * sync up with layout.
>  		 */
>  		if (__do_read(ff, buf, sz))
>  			goto error;
>  
>  		if (ff->ph->needs_swap)
>  			perf_event__attr_swap(buf);

Is it possible for perf_event__attr_swap() to read and write beyond the buf
allocation here?

The buf allocation is sized using the user-provided sz. When
perf_event__attr_swap(buf) executes, it byte-swaps the attr->size field inside
the buffer, and then uses the newly swapped attr->size to determine how many
subsequent fields to byte-swap.

If sz is small, but the embedded attr->size in the payload is large,
perf_event__attr_swap() will swap fields based on the larger internal size
instead of the allocated sz bytes, causing an out-of-bounds access.

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