Re: [PATCH 10/11] perf hists: Fix snprintf() in hists__scnprintf_title() UID filter path

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Buffer overflow in config_show() via test_driver/test_fs sysfs attributes due to snprintf underflow.
- [High] Buffer overflow in pm80xx_get_non_fatal_dump() when reading sysfs attributes due to unbounded snprintf usage.
- [Medium] Stack buffer overflow in hwmon_pmu__for_each_event() due to snprintf underflow.
- [Medium] Stack buffer overflow in perf_pmu__for_each_event() when parsing PMU aliases due to snprintf accumulation.
--

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

perf hists: Fix snprintf() in hists__scnprintf_title() UID filter path

This commit replaces snprintf() with scnprintf() in the UID filter path
of hists__scnprintf_title() to prevent buffer size underflow. It ensures
consistent buffer length calculation, matching the rest of the function.

> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index 811d68fa6770c..df978c996b6c2 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -2963,9 +2963,10 @@ int __hists__scnprintf_title(struct hists *hists, char *bf, size_t size, bool sh
>  			   ev_name, sample_freq_str, enable_ref ? ref : " ", nr_events);
>  
>  
> -	if (hists->uid_filter_str)
> -		printed += snprintf(bf + printed, size - printed,
> -				    ", UID: %s", hists->uid_filter_str);
> +	if (hists->uid_filter_str) {
> +		printed += scnprintf(bf + printed, size - printed,
> +				     ", UID: %s", hists->uid_filter_str);
> +	}
>  	if (thread) {
>  		if (hists__has(hists, thread)) {
>  			printed += scnprintf(bf + printed, size - printed,

While reviewing this fix, I noticed a few similar pre-existing issues in
other areas of the codebase where snprintf() return values might cause buffer
overflows.

lib/test_kmod.c:config_show() {
    ...
	if (config->test_driver)
		len += snprintf(buf+len, PAGE_SIZE - len,
				"driver:\t%s\n",
				config->test_driver);
    ...
}

[Severity: High]
This is a pre-existing issue, but could this calculation underflow
PAGE_SIZE - len?

Since test_driver can be up to PAGE_SIZE bytes long, the accumulated length
can exceed PAGE_SIZE. If snprintf() returns the would-have-written length,
does the subtraction result in a large positive value that allows subsequent
snprintf() calls to write out of bounds of the sysfs buffer?


drivers/scsi/pm8001/pm80xx_hwi.c:pm80xx_get_non_fatal_dump() {
    ...
	for (index = 0; index < output_length/4; index++)
		buf_copy += snprintf(buf_copy, PAGE_SIZE,
				"%08x ", *(temp+index));
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but might this loop write beyond
the bounds of the sysfs buffer?

The pointer buf_copy advances with each write, but the size parameter passed
to snprintf() is constantly PAGE_SIZE instead of the remaining buffer space.
If the hardware provides a large output_length, would this continuously write
up to PAGE_SIZE bytes and cause a heap buffer overflow?


tools/perf/util/hwmon_pmu.c:hwmon_pmu__for_each_event() {
    ...
		char desc_buf[256];
    ...
		len = snprintf(desc_buf, sizeof(desc_buf), "%s in unit %s named %s.",
			hwmon_desc[key.type],
			pmu->name + 6,
			value->label ?: info.name);

		len += hwmon_pmu__describe_items(hwm, desc_buf + len, sizeof(desc_buf) - len,
						key, value->items, /*is_alarm=*/false);
    ...
}

[Severity: Medium]
This is a pre-existing issue, but is there a risk of a stack buffer overflow
here?

Since value->label can be up to 255 bytes, snprintf() might return a length
greater than sizeof(desc_buf). Would passing sizeof(desc_buf) - len to
hwmon_pmu__describe_items() then underflow to a massive size_t value,
allowing scnprintf() to write out-of-bounds on the stack?


tools/perf/util/pmu.c:perf_pmu__for_each_event() {
    ...
	char buf[1024];
    ...
			buf_used += snprintf(buf + buf_used, sizeof(buf) - buf_used,
					"%G%s", event->scale, event->unit) + 1;
		}
		info.desc = event->desc;
		info.long_desc = event->long_desc;
		info.encoding_desc = buf + buf_used;
		buf_used += snprintf(buf + buf_used, sizeof(buf) - buf_used,
				"%.*s/%s/", (int)pmu_name_len, info.pmu_name, event->terms) + 1;
    ...
}

[Severity: Medium]
This isn't a bug introduced by this patch, but does accumulating the snprintf()
return values into buf_used risk underflowing sizeof(buf) - buf_used?

If an earlier snprintf() truncates, it returns the number of bytes it would
have written. Could this cause buf_used to exceed sizeof(buf), making the
size limit on the next snprintf() underflow and write beyond the end of the
stack buffer?

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