On Fri, Sep 04, 2020 at 10:58:59AM +0100, Lorenz Bauer wrote:
> Tracing programs can derive struct sock pointers from a variety
> of sources, e.g. a bpf_iter for sk_storage maps receives one as
> part of the context. It's desirable to be able to pass these to
> functions that expect PTR_TO_SOCKET. For example, it enables us
> to insert such a socket into a sockmap via map_elem_update.
>
> Teach the verifier that a PTR_TO_BTF_ID for a struct sock is
> equivalent to PTR_TO_SOCKET. There is one hazard here:
> bpf_sk_release also takes a PTR_TO_SOCKET, but expects it to be
> refcounted. Since this isn't the case for pointers derived from
> BTF we must prevent them from being passed to the function.
> Luckily, we can simply check that the ref_obj_id is not zero
> in release_reference, and return an error otherwise.
>
> Signed-off-by: Lorenz Bauer <lmb@xxxxxxxxxxxxxx>
> ---
> kernel/bpf/verifier.c | 61 +++++++++++++++++++++++++------------------
> 1 file changed, 36 insertions(+), 25 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index b4e9c56b8b32..509754c3aa7d 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -3908,6 +3908,9 @@ static int resolve_map_arg_type(struct bpf_verifier_env *env,
> return 0;
> }
>
> +BTF_ID_LIST(btf_fullsock_ids)
> +BTF_ID(struct, sock)
It may be fine for the sockmap iter case to treat the "struct sock" BTF_ID
as a fullsock (i.e. PTR_TO_SOCKET).
This is a generic verifier change though. For tracing, it is not always the
case. It cannot always assume that the "struct sock *" in the function being
traced is always a fullsock.
Currently, the func_proto taking ARG_PTR_TO_SOCKET can safely
assume it must be a fullsock. If it is allowed to also take BTF_ID
"struct sock" in verification time, I think the sk_fullsock()
check in runtime is needed and this check should be pretty
cheap to do.
> +
> static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> struct bpf_call_arg_meta *meta,
> const struct bpf_func_proto *fn)
> @@ -4000,37 +4003,15 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> expected_type = PTR_TO_SOCKET;
> if (!(register_is_null(reg) &&
> arg_type == ARG_PTR_TO_SOCKET_OR_NULL)) {
> - if (type != expected_type)
> + if (type != expected_type &&
> + type != PTR_TO_BTF_ID)
> goto err_type;
> }
> + meta->btf_id = btf_fullsock_ids[0];
> } else if (arg_type == ARG_PTR_TO_BTF_ID) {
> - bool ids_match = false;
> -
> expected_type = PTR_TO_BTF_ID;
> if (type != expected_type)
> goto err_type;
> - if (!fn->check_btf_id) {
> - if (reg->btf_id != meta->btf_id) {
> - ids_match = btf_struct_ids_match(&env->log, reg->off, reg->btf_id,
> - meta->btf_id);
> - if (!ids_match) {
> - verbose(env, "Helper has type %s got %s in R%d\n",
> - kernel_type_name(meta->btf_id),
> - kernel_type_name(reg->btf_id), regno);
> - return -EACCES;
> - }
> - }
> - } else if (!fn->check_btf_id(reg->btf_id, arg)) {
> - verbose(env, "Helper does not support %s in R%d\n",
> - kernel_type_name(reg->btf_id), regno);
> -
> - return -EACCES;
> - }
> - if ((reg->off && !ids_match) || !tnum_is_const(reg->var_off) || reg->var_off.value) {
> - verbose(env, "R%d is a pointer to in-kernel struct with non-zero offset\n",
> - regno);
> - return -EACCES;
> - }
> } else if (arg_type == ARG_PTR_TO_SPIN_LOCK) {
> if (meta->func_id == BPF_FUNC_spin_lock) {
> if (process_spin_lock(env, regno, true))
> @@ -4085,6 +4066,33 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> return -EFAULT;
> }
>
> + if (type == PTR_TO_BTF_ID) {
> + bool ids_match = false;
> +
> + if (!fn->check_btf_id) {
> + if (reg->btf_id != meta->btf_id) {
> + ids_match = btf_struct_ids_match(&env->log, reg->off, reg->btf_id,
> + meta->btf_id);
> + if (!ids_match) {
> + verbose(env, "Helper has type %s got %s in R%d\n",
> + kernel_type_name(meta->btf_id),
> + kernel_type_name(reg->btf_id), regno);
> + return -EACCES;
> + }
> + }
> + } else if (!fn->check_btf_id(reg->btf_id, arg)) {
> + verbose(env, "Helper does not support %s in R%d\n",
> + kernel_type_name(reg->btf_id), regno);
> +
> + return -EACCES;
> + }
> + if ((reg->off && !ids_match) || !tnum_is_const(reg->var_off) || reg->var_off.value) {
> + verbose(env, "R%d is a pointer to in-kernel struct with non-zero offset\n",
> + regno);
> + return -EACCES;
> + }
> + }
> +
> if (arg_type == ARG_CONST_MAP_PTR) {
> /* bpf_map_xxx(map_ptr) call: remember that map_ptr */
> meta->map_ptr = reg->map_ptr;
> @@ -4561,6 +4569,9 @@ static int release_reference(struct bpf_verifier_env *env,
> int err;
> int i;
>
> + if (!ref_obj_id)
> + return -EINVAL;
hmm...... Is it sure this is needed? If it was, it seems there was
an existing bug in release_reference_state() below which could not catch
the case where "bpf_sk_release()" is called on a pointer that has no
reference acquired before.
Can you write a verifier test to demonstrate the issue?
> +
> err = release_reference_state(cur_func(env), ref_obj_id);
> if (err)
> return err;
> --
> 2.25.1
>
