Date:   Wed, 7 Sep 2022 07:51:54 +0200
From:   Kumar Kartikeya Dwivedi <memxor@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Lorenzo Bianconi <lorenzo@...nel.org>, bpf <bpf@...r.kernel.org>,
        Network Development <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Pablo Neira Ayuso <pablo@...filter.org>,
        Florian Westphal <fw@...len.de>,
        netfilter-devel <netfilter-devel@...r.kernel.org>,
        Lorenzo Bianconi <lorenzo.bianconi@...hat.com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Toke Høiland-Jørgensen <toke@...hat.com>
Subject: Re: [PATCH v2 bpf-next 3/4] net: netfilter: add bpf_ct_set_nat_info
 kfunc helper

On Wed, 7 Sept 2022 at 07:15, Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> On Tue, Sep 6, 2022 at 9:40 PM Kumar Kartikeya Dwivedi <memxor@...il.com> wrote:
> >
> > On Wed, 7 Sept 2022 at 06:27, Alexei Starovoitov
> > <alexei.starovoitov@...il.com> wrote:
> > >
> > > On Mon, Sep 5, 2022 at 6:14 AM Lorenzo Bianconi <lorenzo@...nel.org> wrote:
> > > > +int bpf_ct_set_nat_info(struct nf_conn___init *nfct__ref,
> > > > +                       union nf_inet_addr *addr, __be16 *port,
> > > > +                       enum nf_nat_manip_type manip)
> > > > +{
> > > ...
> > > > @@ -437,6 +483,7 @@ BTF_ID_FLAGS(func, bpf_ct_set_timeout, KF_TRUSTED_ARGS)
> > > >  BTF_ID_FLAGS(func, bpf_ct_change_timeout, KF_TRUSTED_ARGS)
> > > >  BTF_ID_FLAGS(func, bpf_ct_set_status, KF_TRUSTED_ARGS)
> > > >  BTF_ID_FLAGS(func, bpf_ct_change_status, KF_TRUSTED_ARGS)
> > > > +BTF_ID_FLAGS(func, bpf_ct_set_nat_info)
> > > >  BTF_SET8_END(nf_ct_kfunc_set)
> > >
> > > Instead of __ref and patch 1 and 2 it would be better to
> > > change the meaning of "trusted_args".
> > > In this case "addr" and "port" are just as "trusted".
> > > They're not refcounted per verifier definition,
> > > but they need to be "trusted" by the helper.
> > > At the end the "trusted_args" flags would mean
> > > "this helper can assume that all pointers can be safely
> > > accessed without worrying about lifetime".
> >
> > So you mean it only forces PTR_TO_BTF_ID to have reg->ref_obj_id > 0?
> >
> > But suppose in the future you have a type that has scalars only.
> >
> > struct foo { int a; int b; ... };
> > Just data, and this is acquired from a kfunc and released using another kfunc.
> > Now with this new definition you are proposing, verifier ends up
> > allowing PTR_TO_MEM to also be passed to such helpers for the struct
> > foo *.
> >
> > I guess even reg->ref_obj_id check is not enough, user may also pass
> > PTR_TO_MEM | MEM_ALLOC which can be refcounted.
> >
> > It would be easy to forget such subtle details later.
>
> It may add headaches to the verifier side, but here we have to
> think from pov of other subsystems that add kfuncs.
> They shouldn't need to know the verifier details.
> The internals will change anyway.

Ok, I'll go with making it work for all args for this case.

> Ideally KF_TRUSTED_ARGS will become the default flag that every kfunc
> will use to indicate that the function assumes valid pointers.
> How the verifier recognizes them is irrelevant from kfunc pov.
> People that write bpf progs are not that much different from
> people that write kfuncs that bpf progs use.
> Both should be easy to write.

That is a worthy goal, but it can't become the default unless we
somehow fix how normal PTR_TO_BTF_ID without ref_obj_id is allowed to
be valid, valid-looking-but-uaf pointer, NULL all at the same time
depending on how it was obtained. Currently all helpers, even stable
ones, are broken in this regard. Similarly recently added
cgroup_rstat_flush etc. kfuncs are equally unsafe.

All stable helpers taking PTR_TO_BTF_ID are not even checking for at
least NULL, even though it's right there in bpf.h.
   592         /* PTR_TO_BTF_ID points to a kernel struct that does not need
   593          * to be null checked by the BPF program. This does not imply the
   594          * pointer is _not_ null and in practice this can
easily be a null
   595          * pointer when reading pointer chains. The assumption is program
which just proves how confusing it is right now. And "fixing" that by
adding a NULL check doesn't fix it completely, since it can also be a
seemingly valid looking but freed pointer.

My previous proposal still stands, to accommodate direct PTR_TO_BTF_ID
pointers from loads from PTR_TO_CTX of tracing progs into this
definition of 'trusted', but not those obtained from walking them. It
works for iterator arguments also.

We could limit these restrictions only to kfuncs instead of stable helpers.

It might be possible to instead just whitelist the function BTF IDs as
well, even saying pointers from walks are also safe in this context
for the kfuncs allowed there, or we work on annotating the safe cases
using BTF tags.

There are some problems currently (GCC not supporting BTF tags yet, is
argument really trusted in fexit program in 'xyz_free' function), but
overall it seems like a better state than status quo. It might also
finally push GCC to begin supporting BTF tags.

Mapping of a set of btf_ids can be done to a specific kfunc hook
(instead of current program type), so you are basically composing a
kfunc hook out of a set of btf_ids instead of program type. It
represents a safe context to call those kfuncs in.

It is impossible to know otherwise what case is safe to call a kfunc
for and what is not statically - short of also allowing the unsafe
cases.

Then the kfuncs work on refcounted pointers, and also unrefcounted
ones for known safe cases (basically where the lifetime is guaranteed
by bpf program caller). For arguments it works by default. The only
extra work is annotating things inside structures.
Might not even need that extra annotation in many cases, since kernel
already has __rcu etc. which we can start recognizing like __user to
complain in non-sleepable programs (e.g. without explicit RCU section
which may be added in the future).

Then just flip KF_TRUSTED_ARGS by default, and people have to opt into
'unsafe' instead to make it work for some edge cases, with a big fat
warning for the user of that kfunc.

Powered by blists - more mailing lists