[PATCH bpf] bpf: Fix struct_meta lookup for bpf_obj_free_fields kfunc call

[PATCH bpf] bpf: Fix struct_meta lookup for bpf_obj_free_fields kfunc call

[Dave Marchevsky]

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

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(&regs[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);
-- 
2.34.1

Re: [PATCH bpf] bpf: Fix struct_meta lookup for bpf_obj_free_fields kfunc call

[Alexei Starovoitov]

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.

Re: [PATCH bpf] bpf: Fix struct_meta lookup for bpf_obj_free_fields kfunc call

[Dave Marchevsky]

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.