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@xxxxxxxxxxxxx>
> > ---
> > 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
> >
