On Mon, Sep 07, 2020 at 03:46:55PM +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_SOCK_COMMON. For example, it enables us
> to insert such a socket into a sockmap via map_elem_update.
>
> Note that we can't use struct sock* in cases where a function
> expects PTR_TO_SOCKET: not all struct sock* that a tracing program
> may derive are indeed for a full socket, code must check the
> socket state instead.
>
> Teach the verifier that a PTR_TO_BTF_ID for a struct sock is
> equivalent to PTR_TO_SOCK_COMMON. There is one hazard here:
> bpf_sk_release also takes a PTR_TO_SOCK_COMMON, 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..f1f45ce42d60 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_sock_common_ids)
> +BTF_ID(struct, sock)
> +
> static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> struct bpf_call_arg_meta *meta,
> const struct bpf_func_proto *fn)
> @@ -3984,7 +3987,8 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> } else if (arg_type == ARG_PTR_TO_SOCK_COMMON) {
> expected_type = PTR_TO_SOCK_COMMON;
> /* Any sk pointer can be ARG_PTR_TO_SOCK_COMMON */
> - if (!type_is_sk_pointer(type))
> + if (!type_is_sk_pointer(type) &&
> + type != PTR_TO_BTF_ID)
> goto err_type;
> if (reg->ref_obj_id) {
> if (meta->ref_obj_id) {
> @@ -3995,6 +3999,7 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> }
> meta->ref_obj_id = reg->ref_obj_id;
> }
> + meta->btf_id = btf_sock_common_ids[0];
> } else if (arg_type == ARG_PTR_TO_SOCKET ||
> arg_type == ARG_PTR_TO_SOCKET_OR_NULL) {
> expected_type = PTR_TO_SOCKET;
> @@ -4004,33 +4009,9 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> goto err_type;
> }
> } 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;
Same comment as in v3. This is not needed.
