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

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

New issues:
- [Medium] Pointers to function pointers (and deeper pointer indirections) will be incorrectly symbolized as kernel functions due to greedy unwrapping in `btf_is_func_ptr()` and missing pointer checks in `trace__btf_scnprintf()`.

Pre-existing issues:
- [Medium] Incorrect error handling in `btf_struct_scnprintf` causes `arg->augmented.args` to be inappropriately advanced on failure, leading to an out-of-bounds read in fallback formatters.
--

--- Patch [3]: [PATCH perf-tools-next v5 3/4] perf trace: Enhance BTF type formatting to symbolise kernel function pointers ---
commit 544922c709767641e98cb8ac1eabf4bff305afea
Author: Aaron Tomlin <[email protected]>

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

When BTF (BPF Type Format) metadata is loaded from vmlinux, 'perf trace'
can inspect the precise C types of tracepoint and system call parameters.
This patch introduces btf_is_func_ptr() to inspect BTF type hierarchies
to determine whether a parameter resolves to a function prototype.

> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index 10fbea7ed4dbf..4f8b4f2cf677e 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c

[ ... ]

> @@ -979,21 +980,61 @@ static size_t syscall_arg__scnprintf_getrandom_flags(char *bf, size_t size,
>  #define SCA_GETRANDOM_FLAGS syscall_arg__scnprintf_getrandom_flags
>  
>  #ifdef HAVE_LIBBPF_SUPPORT
> -static void syscall_arg_fmt__cache_btf_enum(struct syscall_arg_fmt *arg_fmt, struct btf *btf, char *type)
> +static bool btf_is_func_ptr(const struct btf *btf, const struct btf_type *type)
>  {
> +	while (type) {
> +		if (btf_is_ptr(type) || btf_is_typedef(type) || btf_is_mod(type))
> +			type = btf__type_by_id(btf, type->type);
> +		else
> +			break;
> +	}
> +	return type && btf_is_func_proto(type);
> +}

[Severity: Medium]
Will this loop eagerly unwrap all pointer modifiers, failing to enforce a
strict single-pointer limit for function pointers?

If perf trace encounters a tracepoint or syscall argument whose type
string contains an asterisk but resolves to a function pointer typedef
(e.g. bpf_callback_t *), wouldn't this greedily unwrap all pointer
indirections, causing pointers to function pointers to be incorrectly
symbolized as kernel functions?

[ ... ]

> @@ -1095,8 +1142,10 @@ static size_t btf_struct_scnprintf(const struct btf_type *type, struct btf *btf,
>  		return 0;
>  
>  	/* 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)
> +	if (btf_dump__dump_type_data(btf_dump, type_id, augmented_arg->value, 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;

[Severity: Medium]
This is a pre-existing issue, but does this error handling correctly check
for negative error codes from btf_dump__dump_type_data()?

If btf_dump__dump_type_data() returns a negative error code (e.g., -EINVAL),
the equality check against 0 evaluates to false. The function incorrectly
proceeds to advance the arg->augmented.args pointer by the consumed struct
size and returns 0.

Then, in syscall__scnprintf_args():

tools/perf/builtin-trace.c:syscall__scnprintf_args() {
    ...
			if (trace->force_btf || default_scnprintf == NULL ||
			    default_scnprintf == SCA_PTR || default_scnprintf == SCA_KSYM) {
				btf_printed = trace__btf_scnprintf(trace, &arg, bf + printed,
								   size - printed, val, field->type);
				if (btf_printed) {
					printed += btf_printed;
					continue;
				}
			}

			printed += syscall_arg_fmt__scnprintf_val(&sc->arg_fmt[arg.idx],
								  bf + printed, size - printed, &arg, val);
    ...
}

Because btf_printed is 0, it falls back to the default formatter
(syscall_arg_fmt__scnprintf_val). Does this fallback formatter then read from
the erroneously advanced arg->augmented.args pointer (now pointing to the
next argument or past the buffer end), leading to an out-of-bounds memory
access?

[ ... ]

> @@ -1108,33 +1157,37 @@ static size_t btf_struct_scnprintf(const struct btf_type *type, struct btf *btf,
>  }
>  
>  static size_t trace__btf_scnprintf(struct trace *trace, struct syscall_arg *arg, char *bf,
> -				   size_t size, int val, char *type)
> +				   size_t size, unsigned long val, char *type)
>  {
>  	struct syscall_arg_fmt *arg_fmt = arg->fmt;
>  
>  	if (trace->btf == NULL)
>  		return 0;
>  
> -	if (arg_fmt->type == NULL) {
> -		// Check if this is an enum and if we have the BTF type for it.
> -		syscall_arg_fmt__cache_btf_enum(arg_fmt, trace->btf, type);
> -	}
> +	if (!arg_fmt->btf_type_cached)
> +		syscall_arg_fmt__cache_btf_type(arg_fmt, trace->btf, type);
>  
>  	// Did we manage to find a BTF type for the syscall/tracepoint argument?
>  	if (arg_fmt->type == NULL)
>  		return 0;
>  
> -	if (btf_is_enum(arg_fmt->type))
> +	if (btf_is_enum(arg_fmt->type)) {
> +		if (type && strchr(type, '*'))
> +			return 0;
>  		return btf_enum_scnprintf(arg_fmt->type, trace->btf, bf, size, val);
> -	else if (btf_is_struct(arg_fmt->type) || btf_is_union(arg_fmt->type))
> +	} else if (btf_is_struct(arg_fmt->type) || btf_is_union(arg_fmt->type))
>  		return btf_struct_scnprintf(arg_fmt->type, trace->btf, bf, size, arg);
> +	else if (btf_is_func_ptr(trace->btf, arg_fmt->type)) {
> +		arg->val = val;
> +		return syscall_arg__scnprintf_ksym(bf, size, arg);
> +	}
>  
>  	return 0;
>  }

[Severity: Medium]
Does this branch lack a safeguard for function pointers similar to the enum
check above?

Unlike the strchr(type, '*') check for enums, there is no verification that
the original C type string lacks an extra pointer asterisk. Since
btf_is_func_ptr() greedily unwraps pointer indirections, could a pointer to a
function pointer be mistakenly routed to syscall_arg__scnprintf_ksym(),
resulting in mis-symbolization of the memory address instead of the kernel
symbol?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3