Re: [PATCH 06/13] perf header: Sanity check HEADER_MEM_TOPOLOGY

sashiko-bot
2026-04-10T01:04:13Z

Sashiko AI review found 3 potential issue(s):
- [Critical] Integer truncation in `do_read_bitmap` leads to a severe heap buffer overflow when reading malformed `perf.data` files.
- [Medium] The newly introduced minimum section size check mathematically undercalculates the minimum required bytes per node.
- [Low] `MAX_NUMA_NODES` macro used for validation is structurally brittle as it is defined inside the body of another function.
--

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

perf header: Sanity check HEADER_MEM_TOPOLOGY

This patch adds validation to process_mem_topology() to harden against
malformed perf.data files. It introduces an upper bound check on nr_nodes
using MAX_NUMA_NODES and a minimum section size check before allocating
memory.

> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index 2c6f7c96e2dcc..45f533fbb8fdc 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -3306,6 +3306,18 @@ static int process_mem_topology(struct feat_fd *ff,
> if (do_read_u64(ff, &nr))
> return -1;
>
> + if (nr > MAX_NUMA_NODES) {

Is it safe to reuse MAX_NUMA_NODES here?

It appears to be defined inside the body of process_numa_topology() earlier
in the file. If the file is ever refactored or process_numa_topology() is
conditionally compiled out, could this break the compilation of
process_mem_topology()?

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

Does this check mathematically undercalculate the minimum required bytes per
node?

The formula assumes that each memory node consumes exactly 2 u64 fields
(node and size). However, do_read_bitmap() unconditionally reads an
additional u64 size field from the file before any bitmap data is processed:

tools/perf/util/header.c:do_read_bitmap() {
...
ret = do_read_u64(ff, &size);
...
}

Should the absolute minimum size required per node actually be 3 u64s
(24 bytes) to prevent structurally undersized sections from passing the
initial size validation?

> + pr_err("Invalid HEADER_MEM_TOPOLOGY: section too small (%zu) for %llu nodes\n",
> + ff->size, (unsigned long long)nr);
> + return -1;
> + }

Since this patch hardens against malformed files, could the truncation in
do_read_bitmap() overflow the set allocation?

In do_read_bitmap(), a 64-bit size is read from the file and passed to
bitmap_zalloc():

tools/perf/util/header.c:do_read_bitmap() {
...
ret = do_read_u64(ff, &size);
if (ret)
return ret;

set = bitmap_zalloc(size);
...
}

Because bitmap_zalloc() takes a 32-bit signed integer, an
attacker-controlled size exceeding INT_MAX (e.g., 0x100000040ULL) would be
implicitly truncated, resulting in a tiny allocation.

However, the subsequent loop uses the original, untruncated 64-bit value to
bound its execution:

for (i = 0; (u64) i < BITS_TO_U64(size); i++) {
ret = do_read_u64(ff, p + i);
...
}

Could this read millions of 64-bit words directly from the untrusted file
into the undersized set allocation?

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