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

[sashiko-bot@kernel.org]

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