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

[Arnaldo Carvalho de Melo <acme@kernel.org>]

On Wed, Jun 10, 2026 at 05:17:26PM +0000, sashiko-bot@kernel.org wrote:
> 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';

This is fixed in the following series, acting on your comments.

- Arnaldo
 
> 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