Re: [RFC bpf-next 2/3] bpf: add support for module helpers in verifier

[Kumar Kartikeya Dwivedi <memxor@gmail.com>]

On Sat, Jan 22, 2022 at 01:09:55AM IST, Usama Arif wrote:
> After the kernel module registers the helper, its BTF id
> and func_proto are available during verification. During
> verification, it is checked to see if insn->imm is available
> in the list of module helper btf ids. If it is,
> check_helper_call is called, otherwise check_kfunc_call.
> The module helper function proto is obtained in check_helper_call
> via get_mod_helper_proto function.
>
> Signed-off-by: Usama Arif <usama.arif@bytedance.com>
> ---
>  kernel/bpf/verifier.c | 50 +++++++++++++++++++++++++++++++++----------
>  1 file changed, 39 insertions(+), 11 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 8c5a46d41f28..bf7605664b95 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -6532,19 +6532,39 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
>  	int insn_idx = *insn_idx_p;
>  	bool changes_data;
>  	int i, err, func_id;
> +	const struct btf_type *func;
> +	const char *func_name;
> +	struct btf *desc_btf;
>
>  	/* find function prototype */
>  	func_id = insn->imm;
> -	if (func_id < 0 || func_id >= __BPF_FUNC_MAX_ID) {
> -		verbose(env, "invalid func %s#%d\n", func_id_name(func_id),
> -			func_id);
> -		return -EINVAL;
> -	}
>
>  	if (env->ops->get_func_proto)
>  		fn = env->ops->get_func_proto(func_id, env->prog);
> -	if (!fn) {
> -		verbose(env, "unknown func %s#%d\n", func_id_name(func_id),
> +
> +	if (func_id >= __BPF_FUNC_MAX_ID) {
> +		desc_btf = find_kfunc_desc_btf(env, insn->imm, insn->off);

I am not sure this is right, even if we reached this point. add_kfunc_call would
not be called for a helper call, which means the kfunc_btf_tab will not be
populated. I think this code is not reachable from your test, which is why you
didn't see this. More below.

> +		if (IS_ERR(desc_btf))
> +			return PTR_ERR(desc_btf);
> +
> +		fn = get_mod_helper_proto(desc_btf, func_id);
> +		if (!fn) {
> +			func = btf_type_by_id(desc_btf, func_id);
> +			func_name = btf_name_by_offset(desc_btf, func->name_off);
> +			verbose(env, "unknown module helper func %s#%d\n", func_name,
> +				func_id);
> +			return -EACCES;
> +		}
> +	} else if (func_id >= 0) {
> +		if (env->ops->get_func_proto)
> +			fn = env->ops->get_func_proto(func_id, env->prog);
> +		if (!fn) {
> +			verbose(env, "unknown in-kernel helper func %s#%d\n", func_id_name(func_id),
> +				func_id);
> +			return -EINVAL;
> +		}
> +	} else {
> +		verbose(env, "invalid func %s#%d\n", func_id_name(func_id),
>  			func_id);
>  		return -EINVAL;
>  	}
> @@ -11351,6 +11371,7 @@ static int do_check(struct bpf_verifier_env *env)
>  	int insn_cnt = env->prog->len;
>  	bool do_print_state = false;
>  	int prev_insn_idx = -1;
> +	struct btf *desc_btf;
>
>  	for (;;) {
>  		struct bpf_insn *insn;
> @@ -11579,10 +11600,17 @@ static int do_check(struct bpf_verifier_env *env)
>  				}
>  				if (insn->src_reg == BPF_PSEUDO_CALL)
>  					err = check_func_call(env, insn, &env->insn_idx);
> -				else if (insn->src_reg == BPF_PSEUDO_KFUNC_CALL)
> -					err = check_kfunc_call(env, insn, &env->insn_idx);
> -				else
> -					err = check_helper_call(env, insn, &env->insn_idx);
> +				else {
> +					desc_btf = find_kfunc_desc_btf(env, insn->imm, insn->off);
> +					if (IS_ERR(desc_btf))
> +						return PTR_ERR(desc_btf);
> +

I didn't get this part at all.

At this point src_reg can be BPF_PSEUDO_KFUNC_CALL, or 0 (for helper call). If
it is a helper call, then find_kfunc_desc_btf using insn->imm and insn->off
would be a bug.

> +					if (insn->src_reg == BPF_K ||

Why are you comparing it to BPF_K? I think your patch is not going through your
logic in check_helper_call at all.

In your selftest, you declare it using __ksym. This means src_reg will be
encoded as BPF_PSEUDO_KFUNC_CALL (2), this if condition will never be hit
(because BPF_K is 0), and you will do check_kfunc_call for it.

TLDR; I think it is being checked as a normal kfunc call by the verifier.

What am I missing?

> +					   get_mod_helper_proto(desc_btf, insn->imm))
> +						err = check_helper_call(env, insn, &env->insn_idx);
> +					else
> +						err = check_kfunc_call(env, insn, &env->insn_idx);
> +				}
>  				if (err)
>  					return err;
>  			} else if (opcode == BPF_JA) {
> --
> 2.25.1
>