Re: [PATCH 5/7] perf tools: Fix int16_t truncation of max_cpu_num in set_max_cpu_num()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Stack buffer overflow in `cpu_map__snprint` due to unsafe accumulation of `snprintf` return values.
- [Medium] Integer underflow in `get_max_num()` leads to out-of-bounds heap read when parsing an empty sysfs file.
--

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

perf tools: Fix int16_t truncation of max_cpu_num in set_max_cpu_num()

set_max_cpu_num() assigns the sysfs "possible" CPU count to
max_cpu_num.cpu which is int16_t (struct perf_cpu).  On systems
with >32767 possible CPUs the value silently truncates, potentially
wrapping negative.  This causes cpunode_map to be underallocated
and subsequent cpu__get_node() calls to read out of bounds.

> diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
> index d3432622b2adc..21fa781b03cc7 100644
> --- a/tools/perf/util/cpumap.c
> +++ b/tools/perf/util/cpumap.c
> @@ -494,6 +494,16 @@ static void set_max_cpu_num(void)
>  	if (ret)
>  		goto out;
>  

[Severity: Medium]
This isn't a bug introduced by this patch, but while reviewing
get_max_num() which is called just before this line, I noticed a
pre-existing issue where an integer underflow can occur.

If filename__read_str() reads an empty sysfs file (for example from a
mocked sysfs directory or malformed kernel state), it returns 0 and sets
num to 0.

tools/perf/util/cpumap.c:get_max_num() {
...
    buf[num] = '\0';

    /* start on the right, to find highest node num */
    while (--num) {
        if ((buf[num] == ',') || (buf[num] == '-')) {
...
}

When num is 0, the pre-decrement --num on a size_t underflows to SIZE_MAX.
This causes an immediate out-of-bounds memory read on the dynamically
allocated buf array. Can we add a check to ensure num is greater than 0
before entering the loop?

> +	/*
> +	 * struct perf_cpu.cpu is int16_t (libperf ABI) — clamp to avoid
> +	 * truncation to negative.  See tools/lib/perf/TODO for the ABI
> +	 * widening plan.
> +	 */

[ ... ]

While reviewing cpumap.c, I also noticed another pre-existing issue in
cpu_map__snprint():

tools/perf/util/cpumap.c:cpu_map__snprint() {
...
    if (start == end) {
        ret += snprintf(buf + ret, size - ret,
                        "%s%d", COMMA,
                        perf_cpu_map__cpu(map, start).cpu);
    } else {
...
}

[Severity: Critical]
This is a pre-existing issue, but does this loop unsafely accumulate
snprintf return values?

The standard snprintf function returns the number of characters that would
have been written if the buffer was large enough, not the actual bytes
written. If the constructed string length exceeds the provided buffer size
(such as the 1024-byte stack buffer passed by cpu_map__fprintf()), ret
will become strictly greater than size.

On the next iteration, the unsigned arithmetic size - ret will underflow
to a huge value, and buf + ret will point past the end of the stack buffer.

Subsequent snprintf calls could write entirely out of bounds on the stack,
leading to a stack buffer overflow.

Could this happen when processing an untrusted perf.data file with a
highly fragmented or large CPU map?

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