| Message ID | 20230403173125.1056883-1-davemarchevsky@fb.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf] bpf: Fix struct_meta lookup for bpf_obj_free_fields kfunc call | expand |
On Mon, Apr 3, 2023 at 10:31 AM Dave Marchevsky <davemarchevsky@fb.com> wrote: > > bpf_obj_drop_impl has a void return type. In check_kfunc_call, the "else > if" which sets kptr_struct_meta for bpf_obj_drop_impl is > surrounded by a larger if statement which checks btf_type_is_ptr. As a > result: > > * The bpf_obj_drop_impl-specific code will never execute > * The btf_struct_meta input to bpf_obj_drop is always NULL > * bpf_obj_drop_impl will always see a NULL btf_record when called > from BPF program, and won't call bpf_obj_free_fields > * program-allocated kptrs which have fields that should be cleaned up > by bpf_obj_free_fields may instead leak resources > > This patch adds a btf_type_is_void branch to the larger if and moves > special handling for bpf_obj_drop_impl there, fixing the issue. > > Fixes: ac9f06050a35 ("bpf: Introduce bpf_obj_drop") > Cc: Kumar Kartikeya Dwivedi <memxor@gmail.com> > Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com> > --- > I can send a version of this patch which applies on bpf-next as well, > but think this makes sense in bpf as the issue exists there too. I'd like to avoid the merge conflict in this tricky area of the verifier. Let's get it through bpf-next first and then we can backport it to stable after bpf-next gets pulled during the merge window.
On 4/3/23 1:31 PM, Dave Marchevsky wrote: > bpf_obj_drop_impl has a void return type. In check_kfunc_call, the "else > if" which sets kptr_struct_meta for bpf_obj_drop_impl is > surrounded by a larger if statement which checks btf_type_is_ptr. As a > result: > > * The bpf_obj_drop_impl-specific code will never execute > * The btf_struct_meta input to bpf_obj_drop is always NULL > * bpf_obj_drop_impl will always see a NULL btf_record when called > from BPF program, and won't call bpf_obj_free_fields > * program-allocated kptrs which have fields that should be cleaned up > by bpf_obj_free_fields may instead leak resources > > This patch adds a btf_type_is_void branch to the larger if and moves > special handling for bpf_obj_drop_impl there, fixing the issue. > > Fixes: ac9f06050a35 ("bpf: Introduce bpf_obj_drop") > Cc: Kumar Kartikeya Dwivedi <memxor@gmail.com> > Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com> > --- > I can send a version of this patch which applies on bpf-next as well, > but think this makes sense in bpf as the issue exists there too. Alexei and I talked offline, I'll send bpf-next version of this shortly. This can be ignored.
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index d517d13878cf..753f652914da 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -9965,10 +9965,6 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn, env->insn_aux_data[insn_idx].obj_new_size = ret_t->size; env->insn_aux_data[insn_idx].kptr_struct_meta = btf_find_struct_meta(ret_btf, ret_btf_id); - } else if (meta.func_id == special_kfunc_list[KF_bpf_obj_drop_impl]) { - env->insn_aux_data[insn_idx].kptr_struct_meta = - btf_find_struct_meta(meta.arg_obj_drop.btf, - meta.arg_obj_drop.btf_id); } else if (meta.func_id == special_kfunc_list[KF_bpf_list_pop_front] || meta.func_id == special_kfunc_list[KF_bpf_list_pop_back]) { struct btf_field *field = meta.arg_list_head.field; @@ -10053,7 +10049,15 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn, if (reg_may_point_to_spin_lock(®s[BPF_REG_0]) && !regs[BPF_REG_0].id) regs[BPF_REG_0].id = ++env->id_gen; - } /* else { add_kfunc_call() ensures it is btf_type_is_void(t) } */ + } else if (btf_type_is_void(t)) { + if (meta.btf == btf_vmlinux && btf_id_set_contains(&special_kfunc_set, meta.func_id)) { + if (meta.func_id == special_kfunc_list[KF_bpf_obj_drop_impl]) { + env->insn_aux_data[insn_idx].kptr_struct_meta = + btf_find_struct_meta(meta.arg_obj_drop.btf, + meta.arg_obj_drop.btf_id); + } + } + } nargs = btf_type_vlen(func_proto); args = (const struct btf_param *)(func_proto + 1);
bpf_obj_drop_impl has a void return type. In check_kfunc_call, the "else if" which sets kptr_struct_meta for bpf_obj_drop_impl is surrounded by a larger if statement which checks btf_type_is_ptr. As a result: * The bpf_obj_drop_impl-specific code will never execute * The btf_struct_meta input to bpf_obj_drop is always NULL * bpf_obj_drop_impl will always see a NULL btf_record when called from BPF program, and won't call bpf_obj_free_fields * program-allocated kptrs which have fields that should be cleaned up by bpf_obj_free_fields may instead leak resources This patch adds a btf_type_is_void branch to the larger if and moves special handling for bpf_obj_drop_impl there, fixing the issue. Fixes: ac9f06050a35 ("bpf: Introduce bpf_obj_drop") Cc: Kumar Kartikeya Dwivedi <memxor@gmail.com> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com> --- I can send a version of this patch which applies on bpf-next as well, but think this makes sense in bpf as the issue exists there too. kernel/bpf/verifier.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)