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

[sashiko-bot@kernel.org]

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