On Tue, Sep 6, 2022 at 10:52 PM Kumar Kartikeya Dwivedi
<memxor@xxxxxxxxx> wrote:
>
> On Wed, 7 Sept 2022 at 07:15, Alexei Starovoitov
> <alexei.starovoitov@xxxxxxxxx> wrote:
> >
> > On Tue, Sep 6, 2022 at 9:40 PM Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> wrote:
> > >
> > > On Wed, 7 Sept 2022 at 06:27, Alexei Starovoitov
> > > <alexei.starovoitov@xxxxxxxxx> wrote:
> > > >
> > > > On Mon, Sep 5, 2022 at 6:14 AM Lorenzo Bianconi <lorenzo@xxxxxxxxxx> 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).
