Re: [PATCH perf-tools-next] perf trace: Add upper bound checks for augmented BTF struct printing

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

On Sat, Sep 05, 2026 at 09:32:27PM -0400, Aaron Tomlin wrote:
> On Sun, Sep 06, 2026 at 01:23:58AM +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] The patch adds bounds checking to `btf_struct_scnprintf` to prevent buffer over-reads and integer overflows from untrusted `perf.data` files, but identical vulnerabilities remain unfixed in several other augmented argument formatters.
> > --
> > 
> > commit 292e31352e85e8bac1a8cec2bc218077f4783d42
> > Author: Aaron Tomlin <atomlin@atomlin.com>
> > 
> > perf trace: Add upper bound checks for augmented BTF struct printing
> > 
> > This commit adds bounds checking to btf_struct_scnprintf() when
> > pretty-printing augmented struct payloads. It prevents buffer over-reads and
> > integer overflow when parsing untrusted perf.data files.
> > 
> > > diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> > > index be19d70eba092..20fffc24507b5 100644
> > > --- a/tools/perf/builtin-trace.c
> > > +++ b/tools/perf/builtin-trace.c
> > > @@ -1133,12 +1133,13 @@ static size_t btf_struct_scnprintf(const struct btf_type *type, struct btf *btf,
> > >  	LIBBPF_OPTS(btf_dump_opts, dump_opts);
> > >  	LIBBPF_OPTS(btf_dump_type_data_opts, dump_data_opts);
> > >  
> > > -	if (arg == NULL || arg->augmented.args == NULL || arg->augmented.size <= 0 ||
> > > +	if (arg == NULL || arg->augmented.args == NULL || arg->augmented.size < (int)sizeof(*augmented_arg) ||
> > >  	    arg->fmt == NULL || !arg->fmt->from_user)
> > >  		return 0;
> > >  
> > >  	augmented_arg = arg->augmented.args;
> > > -	if (augmented_arg->size <= 0)
> > > +	if (augmented_arg->size <= 0 || augmented_arg->size > arg->augmented.size - (int)sizeof(*augmented_arg) ||
> > > +	    (size_t)augmented_arg->size < type->size)
> > >  		return 0;
> > 
> > [Severity: High]
> > This is a pre-existing issue, but while this change correctly adds bounds
> > checking to prevent buffer over-reads and integer overflows in
> > btf_struct_scnprintf(), identical vulnerabilities remain unfixed in several
> > other augmented argument formatters.
> > 
> > When a crafted perf.data file provides an artificially large
> > augmented_arg->size or a truncated payload buffer, could the following
> > formatters still be vulnerable to out-of-bounds reads or state corruption?
> > 
> > In tools/perf/builtin-trace.c:
> > syscall_arg__scnprintf_augmented_string():
> >     size_t printed = scnprintf(bf, size, "\"%.*s\"", augmented_arg->size,
> >                                augmented_arg->value);
> >     int consumed = sizeof(*augmented_arg) + augmented_arg->size;
> > 
> > This lacks validation that arg->augmented.size is >= sizeof(*augmented_arg),
> > fails to bound augmented_arg->size against the remaining payload buffer, and
> > performs a pointer advance calculation that can overflow and corrupt parsing
> > state.
> > 
> > In tools/perf/builtin-trace.c:
> > syscall_arg__scnprintf_buf():
> >     for (int j = 0; j < augmented_arg->size; ++j) {
> > 
> > This unconditionally iterates up to augmented_arg->size without bounds
> > checking against the remaining payload buffer arg->augmented.size, leading to
> > buffer over-reads.
> > 
> > In tools/perf/trace/beauty/perf_event_open.c:
> > syscall_arg__scnprintf_augmented_perf_event_attr():
> >     struct perf_event_attr *attr = (void *)arg->augmented.args->value;
> > 
> > This fails to ensure arg->augmented.size is large enough to contain the
> > perf_event_attr struct before casting and reading from it.
> > 
> > In tools/perf/trace/beauty/sockaddr.c:
> > syscall_arg__scnprintf_augmented_sockaddr():
> >     struct sockaddr *sa = (struct sockaddr *)&arg->augmented.args->value;
> > 
> > This reads fields like sa->sa_family without any payload size validation,
> > leading to potential out-of-bounds reads.
> > 
> > In tools/perf/trace/beauty/timespec.c:
> > syscall_arg__scnprintf_augmented_timespec():
> >     struct timespec *ts = (struct timespec *)arg->augmented.args->value;
> > 
> > This dereferences struct fields without verifying that the payload contains at
> > least sizeof(struct timespec) bytes.
> > 
> > Would it make sense to update these other formatters to include the same
> > bounds checking being added here?
> > 
> > -- 
> > Sashiko AI review · https://sashiko.dev/#/patchset/20260906011132.279321-1-atomlin@atomlin.com?part=1
> 
> Acknowledged.

Thanks, applied to perf-tools-next, for v7.4.

- Arnaldo