Re: [PATCH 10/11] perf tools: Use scnprintf() in build_id__snprintf() and hwmon read_events()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Off-by-one stack buffer overflow when reading sysfs files without reserving space for the null terminator.
--

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

perf tools: Use scnprintf() in build_id__snprintf() and hwmon read_events()

build_id__snprintf() and hwmon_pmu__read_events() accumulate formatted
output via snprintf(), which returns the would-have-been-written count
on truncation.

> diff --git a/tools/perf/util/hwmon_pmu.c b/tools/perf/util/hwmon_pmu.c
> index fb3ffa8d32ad2..dbf6a71af47f9 100644
> --- a/tools/perf/util/hwmon_pmu.c
> +++ b/tools/perf/util/hwmon_pmu.c
> @@ -442,12 +442,12 @@ static size_t hwmon_pmu__describe_items(struct hwmon_pmu *hwm, char *out_buf, si
>  
>  				buf[read_len] = '\0';

[Severity: High]
This isn't a bug introduced by this patch, but could this lead to an
off-by-one stack buffer overflow?

Looking at hwmon_pmu__describe_items() in tools/perf/util/hwmon_pmu.c, we
read up to sizeof(buf) bytes:

    ssize_t read_len = read(fd, buf, sizeof(buf));

If the sysfs file contains exactly the capacity of the buffer (64 bytes) and
does not end with a newline, read_len will be 64.

When terminating the string here:

    buf[read_len] = '\0';

Does this write a null byte exactly one byte past the end of the buf array?
It appears similar patterns exist in hwmon_pmu__read_events() and
evsel__hwmon_pmu_read() as well, where reading the full buffer size
leads to an out-of-bounds null termination.

Should we reserve space for the null terminator by reading sizeof(buf) - 1
instead?

>  				val = strtoll(buf, /*endptr=*/NULL, 10);
> -				len += snprintf(out_buf + len, out_buf_len - len, "%s%s%s=%g%s",
> -						len == 0 ? " " : ", ",
> -						hwmon_item_strs[bit],
> -						is_alarm ? "_alarm" : "",
> -						(double)val / 1000.0,
> -						hwmon_units[key.type]);
> +				len += scnprintf(out_buf + len, out_buf_len - len, "%s%s%s=%g%s",
> +						 len == 0 ? " " : ", ",
> +						 hwmon_item_strs[bit],
> +						 is_alarm ? "_alarm" : "",
> +						 (double)val / 1000.0,
> +						 hwmon_units[key.type]);
>  			}
>  			close(fd);
>  		}

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