netdev - Re: [PATCH bpf-next v3 05/15] bpf: add bpf_skc_to_tcp6

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 22 Jun 2020 23:39:35 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Yonghong Song <yhs@...com>
Cc:     bpf <bpf@...r.kernel.org>, Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>,
        Martin KaFai Lau <kafai@...com>
Subject: Re: [PATCH bpf-next v3 05/15] bpf: add bpf_skc_to_tcp6_sock() helper

On Mon, Jun 22, 2020 at 5:38 PM Yonghong Song <yhs@...com> wrote:
>
> The helper is used in tracing programs to cast a socket
> pointer to a tcp6_sock pointer.
> The return value could be NULL if the casting is illegal.
>
> A new helper return type RET_PTR_TO_BTF_ID_OR_NULL is added
> so the verifier is able to deduce proper return types for the helper.
>
> Different from the previous BTF_ID based helpers,
> the bpf_skc_to_tcp6_sock() argument can be several possible
> btf_ids. More specifically, all possible socket data structures
> with sock_common appearing in the first in the memory layout.
> This patch only added socket types related to tcp and udp.
>
> All possible argument btf_id and return value btf_id
> for helper bpf_skc_to_tcp6_sock() are pre-calculcated and
> cached. In the future, it is even possible to precompute
> these btf_id's at kernel build time.
>
> Acked-by: Martin KaFai Lau <kafai@...com>
> Signed-off-by: Yonghong Song <yhs@...com>
> ---

Looks good to me as is, but see a few suggestions, they will probably
save me time at some point as well :)

Acked-by: Andrii Nakryiko <andriin@...com>


>  include/linux/bpf.h            | 12 +++++
>  include/uapi/linux/bpf.h       |  9 +++-
>  kernel/bpf/btf.c               |  1 +
>  kernel/bpf/verifier.c          | 43 +++++++++++++-----
>  kernel/trace/bpf_trace.c       |  2 +
>  net/core/filter.c              | 80 ++++++++++++++++++++++++++++++++++
>  scripts/bpf_helpers_doc.py     |  2 +
>  tools/include/uapi/linux/bpf.h |  9 +++-
>  8 files changed, 146 insertions(+), 12 deletions(-)
>

[...]

> @@ -4815,6 +4826,18 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn
>                 regs[BPF_REG_0].type = PTR_TO_MEM_OR_NULL;
>                 regs[BPF_REG_0].id = ++env->id_gen;
>                 regs[BPF_REG_0].mem_size = meta.mem_size;
> +       } else if (fn->ret_type == RET_PTR_TO_BTF_ID_OR_NULL) {
> +               int ret_btf_id;
> +
> +               mark_reg_known_zero(env, regs, BPF_REG_0);
> +               regs[BPF_REG_0].type = PTR_TO_BTF_ID_OR_NULL;
> +               ret_btf_id = *fn->ret_btf_id;

might be a good idea to check fb->ret_btf_id for NULL and print a
warning + return -EFAULT. It's not supposed to happen on properly
configured kernel, but during development this will save a bunch of
time and frustration for next person trying to add something with
RET_PTR_TO_BTF_ID_OR_NULL.

> +               if (ret_btf_id == 0) {

This also has to be struct/union (after typedef/mods stripping, of
course). Or are there other cases?

> +                       verbose(env, "invalid return type %d of func %s#%d\n",
> +                               fn->ret_type, func_id_name(func_id), func_id);
> +                       return -EINVAL;
> +               }
> +               regs[BPF_REG_0].btf_id = ret_btf_id;
>         } else {
>                 verbose(env, "unknown return type %d of func %s#%d\n",
>                         fn->ret_type, func_id_name(func_id), func_id);

[...]

> +void init_btf_sock_ids(struct btf *btf)
> +{
> +       int i, btf_id;
> +
> +       for (i = 0; i < MAX_BTF_SOCK_TYPE; i++) {
> +               btf_id = btf_find_by_name_kind(btf, bpf_sock_types[i],
> +                                              BTF_KIND_STRUCT);
> +               if (btf_id > 0)
> +                       btf_sock_ids[i] = btf_id;
> +       }
> +}

This will hopefully go away with Jiri's work on static BTF IDs, right?
So looking forward to that :)

> +
> +static bool check_arg_btf_id(u32 btf_id, u32 arg)
> +{
> +       int i;
> +
> +       /* only one argument, no need to check arg */
> +       for (i = 0; i < MAX_BTF_SOCK_TYPE; i++)
> +               if (btf_sock_ids[i] == btf_id)
> +                       return true;
> +       return false;
> +}
> +

[...]