netdev - Re: [PATCH RFC bpf-next 2/2] bpf: Enable bpf_skc_to_* sock casting helper to networking prog type

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]

Message-ID: <CACAyw9-rirpChioEaSKiYC5+fLGzL38OawcBvE8Mv+16vNApZA@mail.gmail.com>
Date:   Mon, 14 Sep 2020 10:26:18 +0100
From:   Lorenz Bauer <lmb@...udflare.com>
To:     Martin KaFai Lau <kafai@...com>
Cc:     bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>,
        Networking <netdev@...r.kernel.org>
Subject: Re: [PATCH RFC bpf-next 2/2] bpf: Enable bpf_skc_to_* sock casting
 helper to networking prog type

On Sat, 12 Sep 2020 at 05:59, Martin KaFai Lau <kafai@...com> wrote:
>
> There is a constant need to add more fields into the bpf_tcp_sock
> for the bpf programs running at tc, sock_ops...etc.
>
> A current workaround could be to use bpf_probe_read_kernel().  However,
> other than making another helper call for reading each field and missing
> CO-RE, it is also not as intuitive to use as directly reading
> "tp->lsndtime" for example.  While already having perfmon cap to do
> bpf_probe_read_kernel(), it will be much easier if the bpf prog can
> directly read from the tcp_sock.
>
> This patch tries to do that by using the existing casting-helpers
> bpf_skc_to_*() whose func_proto returns a btf_id.  For example, the
> func_proto of bpf_skc_to_tcp_sock returns the btf_id of the
> kernel "struct tcp_sock".
>
> [ One approach is to make a separate copy of the bpf_skc_to_*
>   func_proto and use ARG_PTR_TO_SOCK_COMMON instead of ARG_PTR_TO_BTF_ID.
>   More on this later (1). ]
>
> This patch modifies the existing bpf_skc_to_* func_proto to take
> ARG_PTR_TO_SOCK_COMMON instead of taking
> "ARG_PTR_TO_BTF_ID + &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON]".
> That will allow tc, sock_ops,...etc to call these casting helpers
> because they already hold the PTR_TO_SOCK_COMMON (or its
> equivalent).  For example:
>
>         sk = sock_ops->sk;
>         if (!sk)
>                 return;
>         tp = bpf_skc_to_tcp_sock(sk);
>         if (!tp)
>                 return;
>         /* Read tp as a PTR_TO_BTF_ID */
>         lsndtime = tp->lsndtime;
>
> To ensure the current bpf prog passing a PTR_TO_BTF_ID to
> bpf_skc_to_*() still works as is, the verifier is modified such that
> ARG_PTR_TO_SOCK_COMMON can accept a reg with reg->type == PTR_TO_BTF_ID
> and reg->btf_id is btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON]
>
> To do that, an idea is borrowed from one of the Lorenz's patch:
> https://lore.kernel.org/bpf/20200904112401.667645-12-lmb@cloudflare.com/ .
> It adds PTR_TO_BTF_ID as one of the acceptable reg->type for
> ARG_PTR_TO_SOCK_COMMON and also specifies what btf_id it can take.
> By doing this, the bpf_skc_to_* will work as before and can still
> take PTR_TO_BTF_ID as the arg.  e.g. The bpf tcp iter will work
> as is.
>
> This will also make other existing helper taking ARG_PTR_TO_SOCK_COMMON
> works with the pointer obtained from bpf_skc_to_*(). For example:

Unfortunately, I think that we need to introduce a new
ARG_PTR_TO_SOCK_COMMON_OR_NULL for this to work. This is because
dereferencing a "tracing" pointer can yield NULL at runtime due to a
page fault, so the helper has to deal with this. Other than that I
think this is a really nice approach: we can gradually move helpers to
PTR_TO_SOCK_COMMON_OR_NULL and in doing so make them compatible with
BTF pointers.

[...]

> @@ -4014,7 +4022,17 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 arg,
>
>  found:
>         if (type == PTR_TO_BTF_ID) {
> -               u32 *expected_btf_id = fn->arg_btf_id[arg];
> +               u32 *expected_btf_id;
> +
> +               if (arg_type == ARG_PTR_TO_BTF_ID) {
> +                       expected_btf_id = fn->arg_btf_id[arg];

Personal preference, but what do you think about moving this to after
the assignment of compatible?

    btf_id = compatible->btf_id;
    if (arg_type == ARG_PTR_TO_BTF_ID)
       btf_id = fn->fn->arg_btf_id[arg];

That makes it clearer that we have to special case ARG_PTR_TO_BTF_ID since
it doesn't make sense to use compatible->btf_id in that case.

-- 
Lorenz Bauer  |  Systems Engineer
6th Floor, County Hall/The Riverside Building, SE1 7PB, UK

www.cloudflare.com