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 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing size adjustment in read() and pread() size arguments leads to out-of-bounds null termination on stack buffers.
- [High] If the `read()` system call returns `<= 0`, uninitialized stack memory is evaluated and potentially duplicated, leading to an information leak or crash.
- [Medium] A flawed check `if (fd > 0)` fails to process or close file descriptor 0, causing a resource leak if openat returns 0.
--

commit 63dc76c4229ffb5cbb25da7f0bdc34b17ef66fae
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 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.

[ ... ]

> 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 doesn't this result in an
out-of-bounds write?

Since read() can return up to sizeof(buf), if the sysfs file contents
completely fill the buffer (i.e. read_len == sizeof(buf)), writing the null
terminator here will overwrite one byte past the end of the buf array on the
stack.

Could we pass sizeof(buf) - 1 to read() to avoid this?

This same pre-existing issue seems to be present in hwmon_pmu__read_events()
and evsel__hwmon_pmu_read() as well, where read() and pread() are called with
the full buffer size and similarly null-terminated.


[Severity: Medium]
Also a pre-existing issue in hwmon_pmu__describe_items() just above this hunk,
can this leak a file descriptor?

hwmon_pmu__describe_items() opens a sysfs file using openat(), and then checks:

    fd = openat(dir, buf, O_RDONLY);
    if (fd > 0) {

If the tool runs in an environment where standard input has been closed,
openat() can successfully return 0. The current logic treats 0 as an error,
skips file processing, and fails to call close(fd), causing a resource leak.

Should this check be if (fd >= 0) instead?


[Severity: High]
Another pre-existing issue in hwmon_pmu__read_events(), can this leak
uninitialized stack memory?

If read() returns 0 or less (for example, on an error or empty file), read_len
will be 0 or less. The code skips null-terminating the buffer and proceeds
to check:

    if (buf[0] == '\0') {
        ...
    value->label = strdup(buf);

Since char buf[128] is uninitialized, wouldn't evaluating buf[0] and passing
buf to strdup() result in reading and copying garbage stack memory until it
hits a random null byte?

>  				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/20260608013057.1942953-1-acme@kernel.org?part=11