On Fri, Nov 18, 2022 at 11:00 AM Kumar Kartikeya Dwivedi
<memxor@xxxxxxxxx> wrote:
>
> Instead of checking bpf_global_ma_set at runtime on each allocation
> inside bpf_obj_new_impl, simply disallow calling the kfunc in case
> bpf_global_ma initialization failed during program verification.
>
> The error generated when bpf_global_ma initialization fails:
> ...
> 21: (18) r1 = 0x7 ; R1_w=7
> 23: (b7) r2 = 0 ; R2_w=0
> 24: (85) call bpf_obj_new_impl#36585
> bpf_global_ma initialization failed, can't call bpf_obj_new_impl
> calling kernel function bpf_obj_new_impl is not allowed
>
> Suggested-by: Alexei Starovoitov <ast@xxxxxxxxxx>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> ---
> kernel/bpf/helpers.c | 2 --
> kernel/bpf/verifier.c | 13 ++++++++++++-
> 2 files changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 212e791d7452..bc02f55adc1f 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -1760,8 +1760,6 @@ void *bpf_obj_new_impl(u64 local_type_id__k, void *meta__ign)
> u64 size = local_type_id__k;
> void *p;
>
> - if (unlikely(!bpf_global_ma_set))
> - return NULL;
> p = bpf_mem_alloc(&bpf_global_ma, size);
> if (!p)
> return NULL;
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 195d24316750..f04bee7934a8 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -8746,6 +8746,17 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
> return 0;
> }
>
> +static bool is_kfunc_disabled(struct bpf_verifier_env *env, const struct btf *btf, u32 func_id)
> +{
> + if (btf != btf_vmlinux)
> + return false;
> + if (!bpf_global_ma_set && func_id == special_kfunc_list[KF_bpf_obj_new_impl]) {
> + verbose(env, "bpf_global_ma initialization failed, can't call bpf_obj_new_impl\n");
> + return true;
> + }
> + return false;
> +}
> +
This is all just unnecessary code bloat for the case
that cannot happen.
When you do:
meta.func_id == special_kfunc_list[KF_bpf_obj_new_impl]
just add
if (!bpf_global_ma_set)
return -ENOMEM;
No need for verbose(). The users will never hit it.
Also please get rid of special_kfunc_set and
and btf_id_set_contains(&special_kfunc_set, meta.func_id)
That additional check is unnecessary as well.
special_kfunc_list is enough.
I'm going to apply patch 2 to make CI green.
