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

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [Critical] Integer truncation in `bitmap_zalloc()` leads to a massive heap buffer overflow. While the added bounds check ensures `BITS_TO_U64(size)` elements fit in the declared section size, `bitmap_zalloc(int nbits)` explicitly takes a 32-bit signed integer. A crafted `size` (e.g., `0x100000040`) passes the 64-bit bounds check but truncates to 64 when passed to `bitmap_zalloc()`, allocating only 8 bytes. The un-truncated 64-bit size is then used as the upper bound for the read loop, causing `do_read_u64()` to write millions of elements sequentially past the 8-byte allocation until the physical end of the file is reached.
- [High] Architecture-dependent heap buffer overflow on 32-bit systems due to mismatched allocation and read sizes. On 32-bit architectures, `bitmap_zalloc(size)` allocates memory based on `sizeof(unsigned long)`, which is 4 bytes. However, `do_read_bitmap()` casts the allocated buffer to a `u64 *` and reads 8-byte chunks using `do_read_u64()`. For small bit counts like `size = 1`, `bitmap_zalloc()` allocates exactly 4 bytes, but the read loop executes once and writes 8 bytes, causing a 4-byte out-of-bounds heap write.
--

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

perf header: Validate bitmap size before allocating in do_read_bitmap()

This commit adds a bounds check in do_read_bitmap() to prevent large
allocations that exceed the remaining section size. It validates that
the requested bit count fits within the file data before calling
bitmap_zalloc().

> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -290,6 +290,9 @@ static int do_read_bitmap(struct feat_fd *ff, unsigned long **pset, u64 *psize)
>  	if (ret)
>  		return ret;
>  
> +	if (BITS_TO_U64(size) > (ff->size - ff->offset) / sizeof(u64))
> +		return -1;
> +
>  	set = bitmap_zalloc(size);

Does this code truncate the allocation size?

The size variable is a u64, but bitmap_zalloc() takes a 32-bit signed integer
for its nbits parameter. If a crafted file provides a large size value like
0x100000040, the 64-bit bounds check above would pass if the section is
large enough. 

However, the value would be truncated to 64 when passed to bitmap_zalloc(),
allocating only an 8 byte buffer.

[ ... ]

>  	p = (u64 *) set;
>  
>  	for (i = 0; (u64) i < BITS_TO_U64(size); i++) {
>  		ret = do_read_u64(ff, p + i);

Could this loop write out of bounds on 32-bit architectures?

The bitmap_zalloc() function allocates memory based on sizeof(unsigned long),
which is 4 bytes on 32-bit architectures. For a small bit count like size = 1,
the allocation would be exactly 4 bytes.

Since the code casts the buffer to a u64 pointer and reads 8-byte chunks using
do_read_u64(), the first read would write 8 bytes, overflowing the 4-byte
allocation.

Could this loop also cause a heap buffer overflow due to the truncation
mentioned earlier?

If size is truncated by bitmap_zalloc() to a very small allocation, this
loop still uses the un-truncated 64-bit size as its upper bound. It would
sequentially write millions of elements past the small allocation until it
reaches the physical end of the file.

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