Re: [PATCH v3 perf-tools-next 2/7] perf trace: Validate payload bounds in augmented string beautifier

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf: parser desynchronization on empty augmented string
--

commit c657a00bfdd12e1a3584bb0cd84d1c32e885c3a9
Author: Aaron Tomlin <atomlin@atomlin.com>

perf trace: Validate payload bounds in augmented string beautifier

This commit adds bounds checking for augmented string sizes in perf trace
to prevent out-of-bounds reads and integer overflows when parsing malformed
perf.data records. If validation fails, it falls back to raw pointer printing.

> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index 20fffc24507b5..91461ab927b63 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -1898,12 +1898,21 @@ static void thread__set_filename_pos(struct thread *thread, const char *bf,
>  static size_t syscall_arg__scnprintf_augmented_string(struct syscall_arg *arg, char *bf, size_t size)
>  {
>  	struct augmented_arg *augmented_arg = arg->augmented.args;
> -	size_t printed = scnprintf(bf, size, "\"%.*s\"", augmented_arg->size, augmented_arg->value);
> +	size_t printed;
> +	int consumed;
> +
> +	if (arg->augmented.size < (int)sizeof(*augmented_arg))
> +		return 0;
> +
> +	if (augmented_arg->size <= 0 || augmented_arg->size > arg->augmented.size - (int)sizeof(*augmented_arg))
> +		return 0;

[Severity: Medium]
When a BPF program pushes an empty augmented string record (e.g., size <= 0
due to a read failure like -EFAULT), does returning early here cause the
payload parser to desynchronize?

By returning 0 without consuming the header (sizeof(*augmented_arg)) and
advancing the parsing pointer arg->augmented.args, it seems this might corrupt
the formatting of subsequent augmented arguments in multi-argument syscalls
(like link), displaying them as raw pointers because the pointer is left at
the wrong offset.

Should the header be consumed even when the size is empty or invalid to
maintain parser synchronization?

> +
> +	printed = scnprintf(bf, size, "\"%.*s\"", augmented_arg->size, augmented_arg->value);
>  	/*
>  	 * So that the next arg with a payload can consume its augmented arg, i.e. for rename* syscalls
>  	 * we would have two strings, each prefixed by its size.
>  	 */
> -	int consumed = sizeof(*augmented_arg) + augmented_arg->size;
> +	consumed = sizeof(*augmented_arg) + augmented_arg->size;
>  
>  	arg->augmented.args = ((void *)arg->augmented.args) + consumed;
>  	arg->augmented.size -= consumed;
>  
>  	return printed;
>  }

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