Re: [PATCH 11/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 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Reading sysfs file contents into fixed-size stack buffers causes a 1-byte out-of-bounds write when appending a null terminator if the read fills the buffer.
- [High] User-controlled stack buffer overflow via misused `strlcpy` in `parse_hwmon_filename()`.
--

commit 48628bf245e27619381dcbb4cf7723991e4f5a11
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.  In build_id__snprintf(), this inflates the return
value beyond the buffer size.  In hwmon_pmu__read_events(), len
overshoots out_buf_len and the next 'out_buf_len - len' underflows.

Switch both to scnprintf() which returns actual bytes written.

Fixes: fccaaf6fbbc59910 ("perf build-id: Change sprintf functions to snprintf")
Fixes: 53cc0b351ec99278 ("perf hwmon_pmu: Add a tool PMU exposing events from hwmon in sysfs")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Ian Rogers <irogers@google.com>
Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

[ ... ]

> 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 does this write out of bounds?

In hwmon_pmu__describe_items(), the code reads from sysfs directly into a
stack buffer (buf) using sizeof(buf) as the maximum length. If the read
fills the buffer exactly and lacks a trailing newline, read_len will
equal sizeof(buf).

Writing to buf[read_len] would then write one byte past the end of the
array, corrupting an adjacent byte on the stack with a null byte. Should
the read size be sizeof(buf) - 1 to safely null-terminate?

This same pattern appears to exist in evsel__hwmon_pmu_read() and
hwmon_pmu__read_events().

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

[Severity: High]
This is a pre-existing issue, but I also noticed a potential stack buffer
overflow in parse_hwmon_filename() in this same file.

When copying an event prefix into fn_type:

tools/perf/util/hwmon_pmu.c:parse_hwmon_filename() {
    ...
	char fn_type[24];
    ...
	*alarm = false;
	fn_item_len = strlen(fn_item);
	if (fn_item_len > 6 && !strcmp(&fn_item[fn_item_len - 6], "_alarm")) {
		assert(strlen(LONGEST_HWMON_ITEM_STR) < sizeof(fn_type));
		strlcpy(fn_type, fn_item, fn_item_len - 5);
		fn_item = fn_type;
		*alarm = true;
	}
    ...
}

The third argument to strlcpy() is passed as fn_item_len - 5 instead of the
destination buffer capacity (sizeof(fn_type)). Since fn_item originates from
user-controlled event input, an arbitrarily long name ending in "_alarm"
could cause strlcpy() to copy more than 24 characters into fn_type,
overflowing the stack buffer.

Could this be exploited for memory corruption if perf is run with elevated
capabilities?

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