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

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [Critical] Heap Buffer Overflow via Integer Truncation in do_read_bitmap()
- [Low] Inaccurate bounds check under-approximates minimum section size
--

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

perf header: Sanity check HEADER_MEM_TOPOLOGY

This commit adds validation to process_mem_topology() to protect against
malformed perf.data files. It introduces an upper bound check on nr_nodes
using MAX_NUMA_NODES and verifies the section size is large enough before
allocating memory.

> 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
[ ... ]
> @@ -3308,6 +3308,18 @@ static int process_mem_topology(struct feat_fd *ff,
>  	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)) {

Does the per-node size calculation introduce a regression by
under-approximating the minimum section size?

Each memory node actually consumes at least 3 u64 fields because
do_read_bitmap() also reads a u64 size field before its payload:

process_mem_topology() {
    ...
        _R(node)
        _R(size)
        ...
        if (do_read_bitmap(ff, &n.set, &n.size))
    ...
}

And do_read_bitmap() reads the size:

do_read_bitmap() {
    ...
    ret = do_read_u64(ff, &size);
    ...
}

Should this check instead be nr * 3 * sizeof(u64) to account for the bitmap
size field?

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

While this patch hardens topology parsing, is there a regression further down
in the parsing logic that could cause an out-of-bounds write?

When process_mem_topology() calls do_read_bitmap(), a 64-bit size is read and
passed to bitmap_zalloc():

do_read_bitmap() {
    u64 size, *p;
    ...
    ret = do_read_u64(ff, &size);
    if (ret)
        return ret;

    set = bitmap_zalloc(size);
    ...
}

If bitmap_zalloc() takes a 32-bit integer for the number of bits, a size
exceeding 32 bits would be truncated, resulting in a small allocation.

The subsequent loop then calculates its bounds using the full 64-bit value:

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

Can this loop write past the end of the truncated allocation if a large size is
supplied?

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