Re: [PATCH v5 1/6] perf mem: Fix size tracking for mem_lvl's in perf_script__meminfo_scnprintf()

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

Pre-existing issues:
- [High] Unsafe use of strcat and integer underflows in memory event formatting helpers cause stack buffer overflows.
--

--- Patch [1]: [PATCH v5 1/6] perf mem: Fix size tracking for mem_lvl's in perf_script__meminfo_scnprintf() ---
commit 658933771fa5b1a578c37be7178d9227ed479ba1
Author: Thomas Falcon <[email protected]>
perf mem: Fix size tracking for mem_lvl's in perf_script__meminfo_scnprintf()

This commit fixes the size parameter passed to perf_mem__lvl_scnprintf()
by providing the remaining buffer size instead of the entire buffer size.

> --- a/tools/perf/util/mem-events.c
> +++ b/tools/perf/util/mem-events.c
> @@ -610,7 +610,7 @@ int perf_script__meminfo_scnprintf(char *out, size_t sz, const struct mem_info *
>  	i += scnprintf(out, sz, "|OP ");
>  	i += perf_mem__op_scnprintf(out + i, sz - i, mem_info);
>  	i += scnprintf(out + i, sz - i, "|LVL ");
> -	i += perf_mem__lvl_scnprintf(out + i, sz, mem_info);
> +	i += perf_mem__lvl_scnprintf(out + i, sz - i, mem_info);

[Severity: High]
This is a pre-existing issue, but does passing the remaining buffer size
here expose a stack buffer overflow in the formatting helpers?

When processing a maliciously crafted perf.data file with multiple flag bits
set, the string formatting can exhaust the 200-byte stack buffer decode[]
allocated in data_src__fprintf(). As the remaining size (sz - i) approaches
zero, perf_mem__lvl_scnprintf() receives a small or zero size.

Looking at perf_mem__lvl_scnprintf() and perf_mem__snp_scnprintf(), they
unconditionally decrement the size parameter:

perf_mem__lvl_scnprintf() {
        ...
        sz -= 1; /* -1 for null termination */
        out[0] = '\0';
        ...
}

If sz is 0, this causes an integer underflow to SIZE_MAX. Furthermore, the
helpers use unbounded strcat() when multiple bits are set:

perf_mem__lvl_scnprintf() {
        ...
        if (printed++) {
                strcat(out, " or ");
                l += 4;
        }
        l += scnprintf(out + l, sz - l, ...);
        ...
}

The strcat() can write past the end of the buffer. Because l then exceeds
sz, sz - l underflows, which disables bounds checking for subsequent
scnprintf() calls.

Should these helpers be updated to return early if sz is 0, and use bounds
checking instead of strcat() to avoid corrupting the stack?

>  	i += scnprintf(out + i, sz - i, "|SNP ");
>  	i += perf_mem__snp_scnprintf(out + i, sz - i, mem_info);
>  	i += scnprintf(out + i, sz - i, "|TLB ");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1