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

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

[Arnaldo Carvalho de Melo]

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