Re: [PATCH v2 perf-tools-next 4/6] perf trace beauty: Validate payload size in augmented timespec beautifier

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Validation logic relies on the host's native `struct timespec` layout instead of dynamically querying the trace environment, breaking cross-platform analysis.
- [Medium] Validation relies on an uninitialized/stale field from the BPF tracer, silently breaking timespec formatting.
--

commit 8e3c4b6ee60070e35799a1584520842d1e0f8b14
Author: Aaron Tomlin <atomlin@atomlin.com>

perf trace beauty: Validate payload size in augmented timespec beautifier

This commit adds bounds checking to the augmented timespec beautifier in perf 
trace to prevent reading past the end of the payload buffer for malformed or 
truncated perf.data records.

> diff --git a/tools/perf/trace/beauty/timespec.c b/tools/perf/trace/beauty/timespec.c
> index b14ab72a2738e..fad503bd953a6 100644
> --- a/tools/perf/trace/beauty/timespec.c
> +++ b/tools/perf/trace/beauty/timespec.c
> @@ -7,15 +7,28 @@
>  
>  static size_t syscall_arg__scnprintf_augmented_timespec(struct syscall_arg *arg, char *bf, size_t size)
>  {
> -	struct timespec *ts = (struct timespec *)arg->augmented.args->value;
> +	struct augmented_arg *augmented_arg = arg->augmented.args;
> +	struct timespec *ts;
>  
> +	if (arg->augmented.size < (int)sizeof(*augmented_arg))
> +		return 0;
> +
> +	if (augmented_arg->size < (int)sizeof(*ts) ||
> +	    augmented_arg->size > arg->augmented.size - (int)sizeof(*augmented_arg))
> +		return 0;

[Severity: Medium]
Does this validation break cross-architecture trace analysis? The check
relies strictly on the host's sizeof(*ts), which evaluates to 16 on a
64-bit host. If a perf.data file recorded on a 32-bit architecture is
analyzed here, wouldn't the tracee's valid, smaller payload be incorrectly
rejected by this bounds check?

[Severity: Medium]
Can this bounds check fail randomly due to uninitialized data from the
eBPF tracer? Looking at the eBPF side in sys_enter_clock_nanosleep:

tools/perf/util/bpf_skel/augmented_raw_syscalls.bpf.c:sys_enter_clock_nanosleep() {
    ...
    bpf_probe_read_user(&augmented_args->arg.value, size, rqtp_arg);
    return augmented__output(args, augmented_args, len + size);
}

The eBPF tracer does not appear to initialize augmented_args->arg.size.
Since the augmented_args_tmp per-CPU array is reused across tracepoints,
if a prior syscall (like open) set the size to a path length, wouldn't
the new check here read that stale size and reject valid payloads?

> +
> +	ts = (struct timespec *)augmented_arg->value;
>  	return scnprintf(bf, size, "{ .tv_sec: %" PRIu64 ", .tv_nsec: %" PRIu64 " }", ts->tv_sec, ts->tv_nsec);
>  }

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907015140.363076-1-atomlin@atomlin.com?part=4