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

[sashiko-bot@kernel.org]

Sashiko AI review found 3 potential issue(s):
- [Critical] The patch rejects `sz == 0` but fails to enforce a minimum safe length for `sz`, leading to a critical heap buffer overflow during attribute byte-swapping. This problem wasn't introduced by this patch, but the patch's attempt to sanitize `sz` is incomplete.
- [Critical] The newly added bounds check for `nre` can be trivially bypassed, allowing a 32-bit integer overflow when allocating the `events` array that results in a massive heap out-of-bounds write. This problem wasn't introduced by this patch, but the patch completely fails to prevent it.
- [High] The newly added bounds check introduces a division-by-zero vulnerability on 32-bit architectures.
--

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

perf header: Sanity check HEADER_EVENT_DESC

read_event_desc() reads nre, sz, and nr from the file and uses them to
control allocations and loops. This commit adds bounds checks to reject
sz == 0, sz exceeding section size, and checks that nre and nr fit in
the remaining section.

> 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)))

Could this regression cause a division by zero on 32-bit architectures?

Since sz is a 32-bit unsigned integer read directly from the file, if it is
set to 0xFFFFFFFC, sz + sizeof(u32) will overflow to 0 on 32-bit systems where
sizeof() evaluates to a 32-bit size_t. This would evaluate to:
(ff->size - ff->offset) / 0

Also, does this code allow a small sz value to cause an out-of-bounds access
later during byte-swapping?

If sz is set to a small value between 1 and 7 (e.g., 4), buf = malloc(sz) will
allocate a tiny chunk. If the file requires byte-swapping, the subsequent call
to perf_event__attr_swap() will unconditionally read attr->size (bytes 4-7).
This would read out of bounds and potentially use heap garbage to byte-swap
adjacent memory.

Finally, is ff->size validated against the actual physical file size before
this check?

If ff->size is an unvalidated value from the file header, this bounds check
can be bypassed with a huge value. If a file then sets nre to 0xFFFFFFFF,
it would cause the subsequent allocation to overflow to 0:

events = calloc(nre + 1, sizeof(*events));

This would return a valid minimal chunk, but the following loop:

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

would iterate 0xFFFFFFFF times, overwriting the heap array.

> +		goto error;
> +
>  	/* buffer to hold on file attr struct */
>  	buf = malloc(sz);
>  	if (!buf)

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