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

On Tue, 2026-07-14 at 00:51 +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Severe pre-existing stack buffer overflows and integer
> underflows in `perf_mem__*_scnprintf` functions due to unsafe
> `strcat` usage and missing bounds checks.

Since these are pre-existing issues, can they be handled in a separate
patch series?

Thanks,
Tom

> --
> 
> --- Patch [1]: [PATCH v2 1/6] perf mem: Fix size tracking for
> mem_lvl's in perf_script__meminfo_scnprintf() ---
> commit cf8f02c307060f141740bb3567a69f77761e0f5f
> Author: Thomas Falcon <[email protected]>
> 
> perf mem: Fix size tracking for mem_lvl's in
> perf_script__meminfo_scnprintf()
> 
> When printing memory info in perf script, the entire size of the
> buffer is passed to perf_mem__lvl_scnprintf() instead of the
> remaining
> size. Pass the remaining buffer size instead.
> 
> > diff --git a/tools/perf/util/mem-events.c b/tools/perf/util/mem-
> > events.c
> > index 0b49fce251fcc..4e490f9cd3488 100644
> > --- 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 sz - i here risk an
> integer
> underflow and stack buffer overflow in the callee if the buffer is
> already
> full?
> 
> If previous fields exhaust the buffer, sz - i can be 0 (or wrap
> around if
> i > sz). If 0 is passed to perf_mem__lvl_scnprintf(), the size
> decrement
> at the start of the function will underflow:
> 
> tools/perf/util/mem-events.c:perf_mem__lvl_scnprintf() {
>         sz -= 1; /* -1 for null termination */
>         out[0] = '\0';
>         ...
>         strcat(out, "Remote ");
> }
> 
> When sz underflows to SIZE_MAX, out[0] = '\0' writes out of bounds,
> and
> the unbounded strcat operations can corrupt the stack if a malformed
> or
> untrusted perf.data file sets multiple bits in data_src.
> 
> Does this same unsafe pattern also exist in perf_mem__snp_scnprintf,
> perf_mem__tlb_scnprintf, and perf_mem__blk_scnprintf, which all start
> with the same sz -= 1 decrement?
> 
> >  	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 ");
>