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 09:01:33AM IST, Kumar Kartikeya Dwivedi wrote:
> 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 ||
>
> [...]
>

Ah, I think I see what you are doing: BPF_K is zero, so either when it is a
helper call or it is a module helper (which will be a kfunc), you call
check_helper_call. get_mod_helper_proto would return true in that case.

But if it is an in-kernel helper, calling find_kfunc_desc_btf would still be a
bug, since imm encodes func_id.

It's also a bit confusing that check_helper_call is called for a kfunc.

> > +					   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
> >