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

Pre-existing issues:
- [High] Stack buffer overflow in `perf_pmu__for_each_event` due to unchecked `snprintf` return values underflowing the remaining buffer size.
--

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

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

hists__scnprintf_title() accumulates formatted output into a buffer
using scnprintf() for all filter clauses except the UID filter, which
uses snprintf().  If the buffer fills up and snprintf() returns more
than the remaining space, printed exceeds size and the next 'size -
printed' underflows, causing later scnprintf() calls to write past
the buffer. Switch the UID filter clause to scnprintf() to match 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);
> +	}

[Severity: High]
This isn't a bug introduced by this patch, but while you are fixing
this snprintf() underflow pattern, the exact same issue exists in
tools/perf/util/pmu.c:perf_pmu__for_each_event() where it can cause a
stack buffer overflow.

In that function, sequential snprintf() calls format event strings into a
1024-byte stack buffer, accumulating returned lengths into buf_used:

tools/perf/util/pmu.c:perf_pmu__for_each_event() {
...
		if (strlen(event->unit) || event->scale != 1.0) {
			info.scale_unit = buf + buf_used;
			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;
...
}

Because snprintf() returns the length it would have written, if a long
event alias or term string (e.g., from a parsed JSON definition) fills
the buffer, buf_used will exceed sizeof(buf). On the next snprintf() call,
the limit calculation sizeof(buf) - buf_used undergoes unsigned underflow
to a massive positive size_t value.

Could this unsigned underflow disable bounds checking entirely and allow
snprintf() to write string data out of bounds on the stack? Should we switch
these calls to scnprintf() as well?

>  	if (thread) {
>  		if (hists__has(hists, thread)) {
>  			printed += scnprintf(bf + printed, size - printed,

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