Re: [PATCH v6 04/26] perf trace: Bounds check augmented arguments before reading them

[Namhyung Kim <namhyung@kernel.org>]

On Mon, Sep 28, 2026 at 11:25:43AM -0700, Ian Rogers wrote:
> syscall_arg__scnprintf_buf() and syscall_arg__scnprintf_augmented_string()
> trust the augmented arg's size, so a bad one reads out of bounds:
> 
>   #3 0x4c0fa0 in syscall_arg__scnprintf_buf builtin-trace.c:1955
>   #4 0x4c2f3d in syscall_arg_fmt__scnprintf_val builtin-trace.c:2632
>   #5 0x4c33ae in syscall__scnprintf_args builtin-trace.c:2722
>   #6 0x4c43d3 in trace__sys_enter builtin-trace.c:3094
>   #7 0x4c7865 in trace__handle_event builtin-trace.c:4013
> 
> Move the check in btf_struct_scnprintf() to a helper,
> syscall_arg__augmented_args_valid(), and use it in both. When the check
> fails, syscall_arg__scnprintf_filename() now falls back to vfs_getname or
> the pointer.
> 
> Reported-by: Arnaldo Carvalho de Melo <acme@kernel.org>
> Closes: https://lore.kernel.org/linux-perf-users/arJ-gpzqOHk-gF8T@x2/
> Assisted-by: Antigravity:gemini-3.1-pro
> Signed-off-by: Ian Rogers <irogers@google.com>

Reviewed-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung

> ---
>  tools/perf/builtin-trace.c       | 23 ++++++++++++++---------
>  tools/perf/trace/beauty/beauty.h | 17 +++++++++++++++++
>  2 files changed, 31 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index c39de91140a0..85db74965280 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -1139,14 +1139,11 @@ 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 < (int)sizeof(*augmented_arg) ||
> +	if (!syscall_arg__augmented_args_valid(arg, type->size) ||
>  	    arg->fmt == NULL || !arg->fmt->from_user)
>  		return 0;
>  
>  	augmented_arg = arg->augmented.args;
> -	if (augmented_arg->size <= 0 || augmented_arg->size > arg->augmented.size - (int)sizeof(*augmented_arg) ||
> -	    (size_t)augmented_arg->size < type->size)
> -		return 0;
>  
>  	dump_data_opts.compact	  = true;
>  	dump_data_opts.skip_names = !arg->trace->show_arg_names;
> @@ -1904,12 +1901,18 @@ 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 (!syscall_arg__augmented_args_valid(arg, 0))
> +		return 0;
> +
> +	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;
> @@ -1922,7 +1925,7 @@ static size_t syscall_arg__scnprintf_filename(char *bf, size_t size,
>  {
>  	unsigned long ptr = arg->val;
>  
> -	if (arg->augmented.args)
> +	if (syscall_arg__augmented_args_valid(arg, 0))
>  		return syscall_arg__scnprintf_augmented_string(arg, bf, size);
>  
>  	if (!arg->trace->vfs_getname)
> @@ -1938,13 +1941,15 @@ static size_t syscall_arg__scnprintf_filename(char *bf, size_t size,
>  static size_t syscall_arg__scnprintf_buf(char *bf, size_t size, struct syscall_arg *arg)
>  {
>  	struct augmented_arg *augmented_arg = arg->augmented.args;
> -	unsigned char *orig = (unsigned char *)augmented_arg->value;
>  	size_t printed = 0;
> +	unsigned char *orig;
>  	int consumed;
>  
> -	if (augmented_arg == NULL)
> +	if (!syscall_arg__augmented_args_valid(arg, 0))
>  		return 0;
>  
> +	orig = (unsigned char *)augmented_arg->value;
> +
>  	for (int j = 0; j < augmented_arg->size; ++j) {
>  		bool control_char = orig[j] <= MAX_CONTROL_CHAR || orig[j] >= MAX_ASCII;
>  		/* print control characters (0~31 and 127), and non-ascii characters in \(digits) */
> diff --git a/tools/perf/trace/beauty/beauty.h b/tools/perf/trace/beauty/beauty.h
> index 0f4801c61a5b..1cd307328bb2 100644
> --- a/tools/perf/trace/beauty/beauty.h
> +++ b/tools/perf/trace/beauty/beauty.h
> @@ -124,6 +124,23 @@ struct syscall_arg {
>  
>  unsigned long syscall_arg__val(struct syscall_arg *arg, u8 idx);
>  
> +/* Is there a whole augmented arg left with at least @need bytes of payload? */
> +static inline bool syscall_arg__augmented_args_valid(struct syscall_arg *arg, size_t need)
> +{
> +	const struct augmented_arg *augmented_arg;
> +
> +	if (arg == NULL || arg->augmented.args == NULL ||
> +	    arg->augmented.size < (int)sizeof(*augmented_arg))
> +		return false;
> +
> +	augmented_arg = arg->augmented.args;
> +	if (augmented_arg->size < 0 ||
> +	    augmented_arg->size > arg->augmented.size - (int)sizeof(*augmented_arg))
> +		return false;
> +
> +	return (size_t)augmented_arg->size >= need;
> +}
> +
>  size_t syscall_arg__scnprintf_strarray_flags(char *bf, size_t size, struct syscall_arg *arg);
>  #define SCA_STRARRAY_FLAGS syscall_arg__scnprintf_strarray_flags
>  
> -- 
> 2.56.0.rc1.315.gc6ed9934b7-goog
>