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