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

Arnaldo Carvalho de Melo
2026-04-16T15:11:36Z

On Thu, Apr 16, 2026 at 01:54:55AM +0000, sashiko-bot@kernel.org wrote:
> 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?

Will fix

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

So this is valid but pre-existing, no? I'll cover this in an upcoming
patch series.

- Arnaldo

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