Re: [PATCH 01/11] perf tools: Fix get_max_num() size_t underflow on empty sysfs file

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Heap buffer underflow due to unsafe length manipulation after `filename__read_str` on empty PMU identifier file.
- [High] Heap buffer underflow due to unsafe length manipulation after `procfs__read_str` on empty procfs file.
- [High] Reading an empty PMU scale file leads to a stack buffer underflow.
- [High] Reading an empty PMU unit file leads to an out-of-bounds array access inside the `perf_pmu_alias` struct.
--

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

perf tools: Fix get_max_num() size_t underflow on empty sysfs file

This commit fixes a heap buffer underflow in get_max_num() by adding an
early return when an empty sysfs file is read. This prevents the size_t
variable from underflowing during a backward scan of the buffer.

> diff --git a/tools/perf/util/cpumap.c b/tools/perf/util/cpumap.c
> index 21fa781b03cc7..1fab00ec4a59a 100644
> --- a/tools/perf/util/cpumap.c
> +++ b/tools/perf/util/cpumap.c
> @@ -448,6 +448,12 @@ static int get_max_num(char *path, int *max)
>  
>  	buf[num] = '\0';
>  
> +	/* empty file — nothing to parse */
> +	if (num == 0) {
> +		err = -1;
> +		goto out;
> +	}
> +

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a similar pattern in
tools/perf/util/pmu.c:pmu_id(). Since filename__read_str() returns success (0)
and sets len to 0 when reading an empty file, does the following code
underflow the len variable?

tools/perf/util/pmu.c:pmu_id() {
...
	if (filename__read_str(path, &str, &len) < 0)
		return NULL;

	str[len - 1] = 0; /* remove line feed */
...
}

[Severity: High]
This is a pre-existing issue, but another instance seems to exist in
tools/perf/util/thread.c:thread__set_comm_from_proc(). If the procfs file is
empty (e.g., due to a race), procfs__read_str() returns 0 and sets sz to 0.
Could this lead to a heap buffer underflow when setting the NUL byte?

tools/perf/util/thread.c:thread__set_comm_from_proc() {
...
	if (!(snprintf(path, sizeof(path), "%d/task/%d/comm",
		       thread__pid(thread), thread__tid(thread)) >= (int)sizeof(path)) &&
	    procfs__read_str(path, &comm, &sz) == 0) {
		comm[sz - 1] = '\0';
...
}

[Severity: High]
This isn't a bug introduced by this patch, but reading an empty PMU scale
file might cause a stack buffer underflow in
tools/perf/util/pmu.c:perf_pmu__parse_scale(). 

If the file is empty, read() returns 0, setting sret to 0. The error check
passes, and we evaluate scale[-1]. Could this read uninitialized stack
memory and potentially write a NUL byte out-of-bounds?

tools/perf/util/pmu.c:perf_pmu__parse_scale() {
...
	sret = read(fd, scale, sizeof(scale)-1);
	if (sret < 0)
		goto error;

	if (scale[sret - 1] == '\n')
		scale[sret - 1] = '\0';
...
}

[Severity: High]
This is a pre-existing issue, but there appears to be a similar out-of-bounds
access in tools/perf/util/pmu.c:perf_pmu__parse_unit(). 

If read() returns 0 for an empty file, sret is set to 0. The code then
evaluates alias->unit[-1]. Since unit is part of the heap-allocated struct
perf_pmu_alias, could this overwrite the adjacent pmu_name pointer field if
the preceding byte happens to be a newline?

tools/perf/util/pmu.c:perf_pmu__parse_unit() {
...
	sret = read(fd, alias->unit, UNIT_MAX_LEN);
	if (sret < 0)
		goto error;

	close(fd);

	if (alias->unit[sret - 1] == '\n')
		alias->unit[sret - 1] = '\0';
...
}

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

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