Re: [PATCH v2 bpf-next 3/4] net: netfilter: add bpf_ct_set_nat_info kfunc helper

[Kumar Kartikeya Dwivedi <memxor@gmail.com>]

On Wed, 7 Sept 2022 at 19:33, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Tue, Sep 6, 2022 at 10:52 PM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> >
> > On Wed, 7 Sept 2022 at 07:15, Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Tue, Sep 6, 2022 at 9:40 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> > > >
> > > > On Wed, 7 Sept 2022 at 06:27, Alexei Starovoitov
> > > > <alexei.starovoitov@gmail.com> wrote:
> > > > >
> > > > > On Mon, Sep 5, 2022 at 6:14 AM Lorenzo Bianconi <lorenzo@kernel.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.
>
> With few minor nits, that I don't want to get into right now,
> all of the above makes sense. It can be a plan of record.
> But all that will be done later.
> The immediate first step I'm proposing is
> to extend the definition of KF_TRUSTED_ARGS to include this
> particular use case of:
> union nf_inet_addr *addr, __be16 *port,
>
> Those won't be PTR_TO_BTF_ID so above plan doesn't affect this case.
> They're PTR_TO_MEM (if I'm reading the selftest in the next patch
> correctly) and we can relax:
>                 if (is_kfunc && trusted_arg && !reg->ref_obj_id) {
>
> Just minimal amount of verifier work to enable this specific
> bpf_ct_set_nat_info kfunc.
>
> I think that's user friendlier approach than __ref suffix
> which forces kfunc writers to understand all of the above
> verifier details (valid-looking-but-uaf, null-but-not-null, etc).

I agree, it seems better from a UX standpoint. I'll relax the
trusted_args check for non-PTR_TO_BTF_ID, and send the patch to flip
it to default as an RFC later.