Re: [PATCH v6 1/6] perf mem: Fix size tracking for mem_lvl's in perf_script__meminfo_scnprintf()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Medium] Unbounded string concatenations (`strcat`) and integer underflows of remaining buffer size limits (`sz - l`) cause severe stack buffer overflows when processing memory events.
- [Medium] Integer underflow of the initial buffer size decrement (`sz -= 1`) leads to an out-of-bounds null byte write.
- [Medium] Out-of-bounds array access on the `mem_hops` string array.
- [Medium] Memory leak of `cpu_map` on the error path in `perf_mem_events__record_args`.
--

commit 6817197b03ba87e0b80a68302df13245c2c2a0e9
Author: Thomas Falcon <thomas.falcon@intel.com>

perf mem: Fix size tracking for mem_lvl's in perf_script__meminfo_scnprintf()

When printing memory info in perf script, the entire size of the
buffer is passed to perf_mem__lvl_scnprintf() instead of the remaining
size. Pass the remaining buffer size instead.

> diff --git a/tools/perf/util/mem-events.c b/tools/perf/util/mem-events.c
> index 0b49fce251fcc..4e490f9cd3488 100644
> --- a/tools/perf/util/mem-events.c
> +++ b/tools/perf/util/mem-events.c
> @@ -610,7 +610,7 @@ int perf_script__meminfo_scnprintf(char *out, size_t sz, const struct mem_info *
>  	i += scnprintf(out, sz, "|OP ");
>  	i += perf_mem__op_scnprintf(out + i, sz - i, mem_info);
>  	i += scnprintf(out + i, sz - i, "|LVL ");
> -	i += perf_mem__lvl_scnprintf(out + i, sz, mem_info);
> +	i += perf_mem__lvl_scnprintf(out + i, sz - i, mem_info);

[Severity: Medium]
This is a pre-existing issue, but passing sz - i can now propagate a zero or 
small size to perf_mem__lvl_scnprintf(). If sz == 0 (or sz - i == 0), the 
initial size decrement in the helper underflows:

tools/perf/util/mem-events.c:perf_mem__lvl_scnprintf() {
	...
	sz -= 1; /* -1 for null termination */
	out[0] = '\0';
	...
}

Will this cause an out-of-bounds null byte write and result in sz becoming 
SIZE_MAX for subsequent operations?

[Severity: Medium]
This isn't a bug introduced by this patch, but with the caller passing sz - i,
is there a risk of unbounded strcat overflows?

tools/perf/util/mem-events.c:perf_mem__lvl_scnprintf() {
	...
	for (i = 0; lvl && i < ARRAY_SIZE(mem_lvl); i++, lvl >>= 1) {
		...
		if (printed++) {
			strcat(out, " or ");
			l += 4;
		}
		l += scnprintf(out + l, sz - l, mem_lvl[i]);
	}
	...
}

If the appended length l exceeds sz, sz - l underflows to SIZE_MAX, 
disabling bounds checks for subsequent scnprintf calls. The identical 
logic exists in perf_mem__snp_scnprintf(). Could this allow stack 
corruption when processing untrusted perf.data?


While reviewing this file, I noticed a couple of other pre-existing issues:

[Severity: Medium]
This is a pre-existing issue, but does this code read out-of-bounds?

tools/perf/util/mem-events.c:perf_mem__lvl_scnprintf() {
    ...
		if (data_src.mem_hops)
			l += scnprintf(out + l, sz - l, "%s ", mem_hops[data_src.mem_hops]);
    ...
}

The mem_hops bitfield in the perf_mem_data_src structure is 3 bits wide, 
so it can hold values up to 7, but the mem_hops string array only has 
5 elements. Could a crafted perf.data file providing a mem_hops value of 
5, 6, or 7 cause an out-of-bounds read?

[Severity: Medium]
This isn't a bug introduced by this patch, but does this code leak cpu_map?

tools/perf/util/mem-events.c:perf_mem_events__record_args() {
    ...
			ret = perf_cpu_map__merge(&cpu_map, pmu->cpus);
			if (ret < 0) {
				free(event_name_storage);
				return ret;
			}
    ...
}

If perf_cpu_map__merge() returns a negative error code, the caller returns 
immediately and fails to call perf_cpu_map__put(cpu_map), leaking the 
previously accumulated CPU map reference. Is this cpu_map leak expected?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831204613.2793526-1-thomas.falcon@intel.com?part=1