Re: [PATCH bpf-next v1 11/17] bpf: Report Call Type Safety argument errors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] bpf_diag_report_call_type incorrectly uses a register number (regno) instead of an argument ordinal, breaking diagnostic ordinals and dropping causal history tracking for stack arguments.
- [Medium] bpf_diag_reg_type_plain completely omits the 'nullable' qualifier for PTR_TO_BTF_ID types, leading to highly confusing type-mismatch errors.
--

commit d36f4b9fea5180f51f3ba1c10068790e858fb646
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>

bpf: Report Call Type Safety argument errors

This commit augments selected helper and kfunc argument-contract failures
with Call Type Safety reports. It adds reason, source context, causal register
history, and targeted suggestions to the verifier messages. The changes cover
helper register-type mismatch, nullability requirements, release kfunc
requirements, and memory arguments.

> diff --git a/kernel/bpf/diagnostics.c b/kernel/bpf/diagnostics.c
> index 369b7813b5dcf..169c1a2425c8c 100644
> --- a/kernel/bpf/diagnostics.c
> +++ b/kernel/bpf/diagnostics.c
[ ... ]
> @@ -714,6 +714,43 @@ static const char *bpf_diag_arg_ordinal(int argno)
>  	}
>  }
>  
> +void bpf_diag_report_call_type(struct bpf_verifier_env *env, u32 insn_idx,
> +			       int regno, const char *call_name,
> +			       const char *arg_name, const char *reason,
> +			       const char *suggestion)
> +{
> +	struct bpf_diag_history_opts opts = {
> +		.scope = BPF_DIAG_HISTORY_SCOPE_REG,
> +		.frameno = bpf_diag_current_frameno(env),
> +		.regno = regno,
> +	};
> +	const char *ordinal = bpf_diag_arg_ordinal(regno);

[Severity: Medium]
Will this return NULL and drop the ordinal string for stack arguments?

For arguments > 5, reg_from_argno() returns -1, which is passed here as
regno. Calling bpf_diag_arg_ordinal(-1) returns NULL, even though it appears
designed to handle up to 12 arguments.

Should we be passing argno instead of regno to bpf_diag_report_call_type(),
or at least passing argno directly to bpf_diag_arg_ordinal()?

> +	char arg_desc[48];
> +
> +	if (ordinal && arg_name)
> +		scnprintf(arg_desc, sizeof(arg_desc), "%s argument (%s)",
> +			  ordinal, arg_name);
> +	else if (arg_name)
> +		scnprintf(arg_desc, sizeof(arg_desc), "argument %s", arg_name);
> +	else
> +		scnprintf(arg_desc, sizeof(arg_desc), "argument");
> +
> +	bpf_diag_report_header(env, BPF_DIAG_CATEGORY_CALL_TYPE_SAFETY,
> +			       "invalid call argument");
> +	bpf_diag_report_reason(env,
> +			       "The %s to %s does not satisfy the verifier contract: %s.",
> +			       arg_desc, call_name, reason);
> +
> +	bpf_diag_report_section(env, "At");
> +	bpf_diag_report_source(env, insn_idx, '!',
> +			       "invalid %s for %s", arg_desc, call_name);
> +
> +	if (regno >= 0)
> +		bpf_diag_print_history(env, &opts);

[Severity: Medium]
Does this silently bypass causal history tracking for stack arguments?

Because regno evaluates to -1 for arguments > 5, this check skips printing
the history. Is it intentional to drop history tracking for stack arguments,
or can we add support to track their history?

> +
> +	bpf_diag_report_suggestion(env, "%s", suggestion);
> +}
> +
>  void bpf_diag_report_invalid_deref(struct bpf_verifier_env *env, u32 insn_idx,
>  				   int regno, const char *reg_name,
>  				   const char *type_name,
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 1c26614bec2b7..d2a776d2059cc 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
[ ... ]
> @@ -8039,6 +8039,39 @@ static const struct bpf_reg_types *compatible_reg_types[__BPF_ARG_TYPE_MAX] = {
>  	[ARG_PTR_TO_DYNPTR]		= &dynptr_types,
>  };
>  
> +static const char *bpf_diag_reg_type_plain(struct bpf_verifier_env *env,
> +					   enum bpf_reg_type type)
> +{
> +	switch (base_type(type)) {
> +	case NOT_INIT:
> +		return "an uninitialized value";
> +	case SCALAR_VALUE:
> +		return "an integer scalar";
> +	case PTR_TO_CTX:
> +		return "a context pointer";
> +	case PTR_TO_STACK:
> +		return "a stack pointer";
> +	case PTR_TO_MAP_VALUE:
> +		if (type_may_be_null(type))
> +			return "a nullable map value pointer";
> +		return "a map value pointer";
> +	case PTR_TO_MEM:
> +		if (type_may_be_null(type))
> +			return "a nullable memory pointer";
> +		return "a memory pointer";
> +	case PTR_TO_BTF_ID:
> +		if (type_is_non_owning_ref(type))
> +			return "a borrowed allocated object pointer";

[Severity: Medium]
Should we be checking type_may_be_null(type) for PTR_TO_BTF_ID types here?

PTR_TO_MAP_VALUE and PTR_TO_MEM correctly identify nullable pointers above,
but PTR_TO_BTF_ID omits the check.

When a nullable PTR_TO_BTF_ID is passed to a helper expecting a non-nullable
pointer, it fails the type check in check_reg_type(), but the error message
will drop the "nullable" qualifier. This causes the verifier to report "it
has type a kernel object pointer, but this helper argument accepts...", making
the actual cause of the rejection (nullability) very confusing for users.

> +		if (type_is_ptr_alloc_obj(type))
> +			return "an owned allocated object pointer";
> +		if (type_flag(type) & PTR_UNTRUSTED)
> +			return "an untrusted kernel object pointer";
> +		return "a kernel object pointer";
> +	default:
> +		return reg_type_str(env, type);
> +	}
> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605063412.974640-1-memxor@gmail.com?part=11