Re: [PATCH 4/5] perf header: Validate bitmap size before allocating in do_read_bitmap()

Re: [PATCH 4/5] perf header: Validate bitmap size before allocating in do_read_bitmap()

[Arnaldo Carvalho de Melo]

On Thu, Apr 16, 2026 at 02:25:38AM +0000, sashiko-bot@kernel.org wrote:
> Sashiko AI review found 1 potential issue(s):
> - [High] Integer overflow in process_mem_topology() leading to heap buffer overflow
> --
> 
> commit cc7a509cea916b4f4ae9c056c5eb0517bac22dfa
> Author: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> perf header: Validate bitmap size before allocating in do_read_bitmap()
> 
> This patch updates do_read_bitmap() to validate the u64 bit count read
> from the file before allocating memory. It rejects sizes that exceed
> INT_MAX or exceed the remaining section bounds, preventing potential
> large allocations or truncation issues with bitmap_zalloc().
> 
> Link: https://lore.kernel.org/linux-perf-users/20260414224622.2AE69C19425@smtp.kernel.org/
> 
> > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> > index 696f19739074e..89697aec914c9 100644
> > --- a/tools/perf/util/header.c
> > +++ b/tools/perf/util/header.c
> > @@ -290,6 +290,11 @@ static int do_read_bitmap(struct feat_fd *ff, unsigned long **pset, u64 *psize)
> >  	if (ret)
> >  		return ret;
> >  
> > +	/* bitmap_zalloc() takes an int; reject u64 values that truncate. */
> > +	if (size > INT_MAX ||
> > +	    BITS_TO_U64(size) > (ff->size - ff->offset) / sizeof(u64))
> > +		return -1;
> > +
> >  	set = bitmap_zalloc(size);
> >  	if (!set)
> >  		return -ENOMEM;
> 
> This isn't a bug introduced by this patch, but since the commit message notes
> that do_read_bitmap() is used by process_mem_topology(), does the same missing
> validation exist when allocating the nodes array in process_mem_topology()?
> 
> tools/perf/util/header.c:process_mem_topology() {
>     ...
> 	if (do_read_u64(ff, &nr))
> 		return -1;
> 
> 	nodes = zalloc(sizeof(*nodes) * nr);

This code now already has a bounds check and is using calloc:

        if (do_read_u64(ff, &nr))
                return -1;

        if (nr > MAX_NUMA_NODES) {
                pr_err("Invalid HEADER_MEM_TOPOLOGY: nr_nodes (%llu) > %u\n",
                       (unsigned long long)nr, MAX_NUMA_NODES);
                return -1;
        }

        if (ff->size < 3 * sizeof(u64) + nr * 2 * sizeof(u64)) {
                pr_err("Invalid HEADER_MEM_TOPOLOGY: section too small (%zu) for %llu nodes\n",
                       ff->size, (unsigned long long)nr);
                return -1;
        }

        nodes = calloc(nr, sizeof(*nodes));
        if (!nodes)
                return -1;

Probably you're working with upstream and the fixes are in
perf-tools-next/perf-tools-next.

> 	if (!nodes)
> 		return -1;
> 
> 	for (i = 0; i < nr; i++) {
> 		struct memory_node n;
>     ...
> 		nodes[i] = n;
> 	}
>     ...
> }
> 
> Because nr is an unvalidated 64-bit integer read from perf.data, could a
> crafted value like 0x2000000000000000 cause the 64-bit multiplication
> (24 * nr) to wrap around to 0 during the zalloc() call?
> 
> If zalloc(0) succeeds and allocates a minimum-sized heap chunk, the loop
> could iterate and write beyond the allocated bounds when assigning
> nodes[i] = n on subsequent iterations.
> 
> Could this overflow the nodes[] buffer?
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260416001424.362797-1-acme@kernel.org?part=4