On Tue, Jul 19, 2022 at 11:59 AM Dave Marchevsky <davemarchevsky@xxxxxx> wrote:
>
> Discussion around a recently-submitted patch provided historical
> context for check_refcount_ok [0]. Specifically, the function and its
> helpers - may_be_acquire_function and arg_type_may_be_refcounted -
> predate the OBJ_RELEASE type flag and the addition of many more helpers
> with acquire/release semantics.
>
> The purpose of check_refcount_ok is to ensure:
> 1) Helper doesn't have multiple uses of return reg's ref_obj_id
> 2) Helper with release semantics only has one arg needing to be
> released, since that's tracked using meta->ref_obj_id
>
> With current verifier, it's safe to remove check_refcount_ok and its
> helpers. Since addition of OBJ_RELEASE type flag, case 2) has been
> handled by the arg_type_is_release check in check_func_arg. To ensure
> case 1) won't result in verifier silently prioritizing one use of
> ref_obj_id, this patch adds a helper_multiple_ref_obj_use check which
> fails loudly if a helper passes > 1 test for use of ref_obj_id.
>
> [0]: lore.kernel.org/bpf/20220713234529.4154673-1-davemarchevsky@xxxxxx
>
> Signed-off-by: Dave Marchevsky <davemarchevsky@xxxxxx>
> ---
> No extant helpers fail the helper_multiple_ref_obj_use check (as
> expected). I validated this by adding BPF_FUNC_dynptr_data to
> is_acquire_function check and observing that dynptr selftests failed
> with expected error, then doing the same for is_ptr_cast_function.
>
> kernel/bpf/verifier.c | 72 +++++++++++++++++--------------------------
> 1 file changed, 28 insertions(+), 44 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index c59c3df0fea6..b3e057a9384d 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -467,25 +467,11 @@ static bool type_is_rdonly_mem(u32 type)
> return type & MEM_RDONLY;
> }
>
> -static bool arg_type_may_be_refcounted(enum bpf_arg_type type)
> -{
> - return type == ARG_PTR_TO_SOCK_COMMON;
> -}
> -
> static bool type_may_be_null(u32 type)
> {
> return type & PTR_MAYBE_NULL;
> }
>
> -static bool may_be_acquire_function(enum bpf_func_id func_id)
> -{
> - return func_id == BPF_FUNC_sk_lookup_tcp ||
> - func_id == BPF_FUNC_sk_lookup_udp ||
> - func_id == BPF_FUNC_skc_lookup_tcp ||
> - func_id == BPF_FUNC_map_lookup_elem ||
> - func_id == BPF_FUNC_ringbuf_reserve;
> -}
> -
> static bool is_acquire_function(enum bpf_func_id func_id,
> const struct bpf_map *map)
> {
> @@ -518,6 +504,26 @@ static bool is_ptr_cast_function(enum bpf_func_id func_id)
> func_id == BPF_FUNC_skc_to_tcp_request_sock;
> }
>
> +static bool is_dynptr_acquire_function(enum bpf_func_id func_id)
nit: for the function name, bpf_dynptr_data doesn't acquire a
reference state, it tracks the dynptr's existing reference state so
that when the dynptr is invalidated, the data slice is invalidated as
well. This is my fault for the misleading comment about "finding the
id of the dynptr we're acquiring a reference to". Maybe something like
"is_dynptr_ref_tracking" would work better as a name?
> +{
> + return func_id == BPF_FUNC_dynptr_data;
> +}
> +
> +static bool helper_multiple_ref_obj_use(enum bpf_func_id func_id,
> + const struct bpf_map *map)
> +{
> + int ref_obj_uses = 0;
> +
> + if (is_ptr_cast_function(func_id))
> + ref_obj_uses++;
> + if (is_acquire_function(func_id, map))
> + ref_obj_uses++;
> + if (is_dynptr_acquire_function(func_id))
> + ref_obj_uses++;
> +
> + return ref_obj_uses > 1;
> +}
> +
> static bool is_cmpxchg_insn(const struct bpf_insn *insn)
> {
> return BPF_CLASS(insn->code) == BPF_STX &&
> @@ -6453,33 +6459,6 @@ static bool check_arg_pair_ok(const struct bpf_func_proto *fn)
> return true;
> }
>
> -static bool check_refcount_ok(const struct bpf_func_proto *fn, int func_id)
> -{
> - int count = 0;
> -
> - if (arg_type_may_be_refcounted(fn->arg1_type))
> - count++;
> - if (arg_type_may_be_refcounted(fn->arg2_type))
> - count++;
> - if (arg_type_may_be_refcounted(fn->arg3_type))
> - count++;
> - if (arg_type_may_be_refcounted(fn->arg4_type))
> - count++;
> - if (arg_type_may_be_refcounted(fn->arg5_type))
> - count++;
> -
> - /* A reference acquiring function cannot acquire
> - * another refcounted ptr.
> - */
> - if (may_be_acquire_function(func_id) && count)
> - return false;
> -
> - /* We only support one arg being unreferenced at the moment,
> - * which is sufficient for the helper functions we have right now.
> - */
> - return count <= 1;
> -}
> -
> static bool check_btf_id_ok(const struct bpf_func_proto *fn)
> {
> int i;
> @@ -6503,8 +6482,7 @@ static int check_func_proto(const struct bpf_func_proto *fn, int func_id,
> {
> return check_raw_mode_ok(fn) &&
> check_arg_pair_ok(fn) &&
> - check_btf_id_ok(fn) &&
> - check_refcount_ok(fn, func_id) ? 0 : -EINVAL;
> + check_btf_id_ok(fn) ? 0 : -EINVAL;
> }
>
> /* Packet data might have moved, any old PTR_TO_PACKET[_META,_END]
> @@ -7457,6 +7435,12 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
> if (type_may_be_null(regs[BPF_REG_0].type))
> regs[BPF_REG_0].id = ++env->id_gen;
>
> + if (helper_multiple_ref_obj_use(func_id, meta.map_ptr)) {
> + verbose(env, "verifier internal error: func %s#%d sets ref_obj_id more than once\n",
> + func_id_name(func_id), func_id);
> + return -EINVAL;
nit: I think -EFAULT here makes more sense because this is an error in
the internal helper function definition
> + }
> +
> if (is_ptr_cast_function(func_id)) {
> /* For release_reference() */
> regs[BPF_REG_0].ref_obj_id = meta.ref_obj_id;
> @@ -7469,7 +7453,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
> regs[BPF_REG_0].id = id;
> /* For release_reference() */
> regs[BPF_REG_0].ref_obj_id = id;
> - } else if (func_id == BPF_FUNC_dynptr_data) {
> + } else if (is_dynptr_acquire_function(func_id)) {
> int dynptr_id = 0, i;
>
> /* Find the id of the dynptr we're acquiring a reference to */
Do you mind modifying this to "Find the id of the dynptr we're
tracking the reference of"?
> --
> 2.30.2
>
