netdev - Re: [PATCH v3 bpf-next 02/11] 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 for Android: free password hash cracker in your pocket

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

Message-ID: <20200922182622.zcrqwpzkouvlndbw@kafai-mbp.dhcp.thefacebook.com>
Date:   Tue, 22 Sep 2020 11:26:22 -0700
From:   Martin KaFai Lau <kafai@...com>
To:     Lorenz Bauer <lmb@...udflare.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 v3 bpf-next 02/11] bpf: Enable bpf_skc_to_* sock casting
 helper to networking prog type

On Tue, Sep 22, 2020 at 10:46:41AM +0100, Lorenz Bauer wrote:
> On Tue, 22 Sep 2020 at 08:04, 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".
> >
> > These helpers are also added to is_ptr_cast_function().
> > It ensures the returning reg (BPF_REF_0) will also carries the ref_obj_id.
> > That will keep the ref-tracking works properly.
> >
> > The bpf_skc_to_* helpers are made available to most of the bpf prog
> > types in filter.c. They are limited by perfmon cap.
> >
> > This patch adds a ARG_PTR_TO_BTF_ID_SOCK_COMMON.  The helper accepting
> > this arg can accept a btf-id-ptr (PTR_TO_BTF_ID + &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON])
> > or a legacy-ctx-convert-skc-ptr (PTR_TO_SOCK_COMMON).  The bpf_skc_to_*()
> > helpers are changed to take ARG_PTR_TO_BTF_ID_SOCK_COMMON such that
> > they will accept pointer obtained from skb->sk.
> >
> > PTR_TO_*_OR_NULL is not accepted as an ARG_PTR_TO_BTF_ID_SOCK_COMMON
> > at verification time.  All PTR_TO_*_OR_NULL reg has to do a NULL check
> > first before passing into the helper or else the bpf prog will be
> > rejected by the verifier.
> >
> > [ ARG_PTR_TO_SOCK_COMMON_OR_NULL was attempted earlier.  The _OR_NULL was
> >   needed because the PTR_TO_BTF_ID could be NULL but note that a could be NULL
> >   PTR_TO_BTF_ID is not a scalar NULL to the verifier.  "_OR_NULL" implicitly
> >   gives an expectation that the helper can take a scalar NULL which does
> >   not make sense in most (except one) helpers.  Passing scalar NULL
> >   should be rejected at the verification time.
> 
> What is the benefit of requiring a !sk check from the user if all of
> the helpers know how to deal with a NULL pointer?
I don't see a reason why the verifier should not reject an incorrect
program at load time if it can.

> 
> >
> >   Thus, this patch uses ARG_PTR_TO_BTF_ID_SOCK_COMMON to specify that the
> >   helper can take both the btf-id ptr or the legacy PTR_TO_SOCK_COMMON but
> >   not scalar NULL.  It requires the func_proto to explicitly specify the
> >   arg_btf_id such that there is a very clear expectation that the helper
> >   can handle a NULL PTR_TO_BTF_ID. ]
> 
> I think ARG_PTR_TO_BTF_ID_SOCK_COMMON is actually a misnomer, since
> nothing enforces that arg_btf_id is actually an ID for sock common.
> This is where ARG_PTR_TO_SOCK_COMMON_OR_NULL is much easier to
> understand, even though it's more permissive than it has to be. It
> communicates very clearly what values the argument can take.
_OR_NULL is incorrect which implies a scalar NULL as mentioned in
this commit message.  From verifier pov, _OR_NULL can take
a scalar NULL.

> 
> If you're set on ARG_PTR_TO_BTF_ID_SOCK_COMMON I'd suggest forcing the
> btf_id in struct bpf_reg_types. This avoids the weird case where the
> btf_id doesn't actually point at sock_common, and it also makes my
I have considered the bpf_reg_types option.  I prefer all
arg info (arg_type and arg_btf_id) stay in the same one
place (i.e. func_proto) as much as possible for now
instead of introducing another place to specify/override it
which then depends on a particular arg_type that some arg_type may be
in func_proto while some may be in other places.

The arg_btf_id can be checked in check_btf_id_ok() if it would be a
big concern that it might slip through the review but I think the
chance is pretty low.