Re: [PATCH perf-tools-next v6 4/5] perf trace: Enhance BTF type formatting to symbolise kernel function pointers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The `btf_struct_scnprintf` function fails to properly validate `augmented_arg->size` against buffer limits and is vulnerable to an integer overflow, leading to out-of-bounds memory access.
--

--- Patch [4]: [PATCH perf-tools-next v6 4/5] perf trace: Enhance BTF type formatting to symbolise kernel function pointers ---
commit c939026eded5d8aaf079b56970d0829b2a8ad559
Author: Aaron Tomlin <atomlin@atomlin.com>

perf trace: Enhance BTF type formatting to symbolise kernel function pointers

This commit introduces btf_is_func_ptr() to identify function pointers in BTF
types. It caches the BTF type in syscall_arg_fmt and routes function pointers
to syscall_arg__scnprintf_ksym() during BTF pretty-printing.

> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index f6ab7dabfa4c3..d0089230dc5d4 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c

[ ... ]

> @@ -1077,14 +1126,19 @@ static size_t btf_struct_scnprintf(const struct btf_type *type, struct btf *btf,
>  		.bf   = bf,
>  		.size = size,
>  	};
> -	struct augmented_arg *augmented_arg = arg->augmented.args;
> +	struct augmented_arg *augmented_arg;
>  	int type_id = arg->fmt->type_id, consumed;
>  	struct btf_dump *btf_dump;
>  
>  	LIBBPF_OPTS(btf_dump_opts, dump_opts);
>  	LIBBPF_OPTS(btf_dump_type_data_opts, dump_data_opts);
>  
> -	if (arg == NULL || arg->augmented.args == NULL)
> +	if (arg == NULL || arg->augmented.args == NULL || arg->augmented.size <= 0 ||
> +	    arg->fmt == NULL || !arg->fmt->from_user)
> +		return 0;
> +
> +	augmented_arg = arg->augmented.args;
> +	if (augmented_arg->size <= 0)
>  		return 0;
>  
>  	dump_data_opts.compact	  = true;
> @@ -1097,8 +1151,10 @@ static size_t btf_struct_scnprintf(const struct btf_type *type, struct btf *btf,
>  	/* pretty print the struct data here */
>  	if (btf_dump__dump_type_data(btf_dump, type_id,
>  				     arg->augmented.args->value,
> -				     type->size, &dump_data_opts) <= 0)
> +				     type->size, &dump_data_opts) <= 0) {
> +		btf_dump__free(btf_dump);
>  		return 0;
> +	}
>  
>  	consumed = sizeof(*augmented_arg) + augmented_arg->size;
>  	arg->augmented.args = ((void *)arg->augmented.args) + consumed;
>  	arg->augmented.size -= consumed;

[Severity: High]
This is a pre-existing issue, but does btf_struct_scnprintf() need upper bound
checks for augmented_arg->size and type->size?

When parsing ring buffer data from a potentially untrusted perf.data file,
augmented_arg->size is read directly from the buffer. While this patch adds
a check for augmented_arg->size <= 0, it doesn't enforce an upper bound
against the total available buffer (arg->augmented.size >=
sizeof(*augmented_arg) + augmented_arg->size).

Additionally, type->size from BTF is passed to btf_dump__dump_type_data()
without verifying that the available payload is large enough
(arg->augmented.size - sizeof(*augmented_arg) >= type->size). Could this lead
to out-of-bounds reads if the BTF struct is large but the captured payload
is truncated?

Finally, if a crafted file has an extremely large augmented_arg->size (e.g.,
INT_MAX), could the calculation for consumed overflow the signed int variable
and corrupt the augmented parsing state?

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