On Thu, 25 Apr 2024 at 17:59, David Vernet <void@xxxxxxxxxxxxx> wrote:
>
> On Wed, Apr 24, 2024 at 11:36:51AM -0700, Alexei Starovoitov wrote:
>
> [...]
>
> > > The OBJ_RELEASE causes check_func_arg_reg_off() to fail to verify if
> > > there's a nonzero offset. In reality, I _think_ we only need to check
> > > for a nonzero offset for KF_RELEASE, and possibly KF_ACQUIRE.
> >
> > Why special case KF_RELEASE/ACQUIRE ?
> > I think they're no different from kfuncs with KF_TRUSTED_ARGS.
> > Should be safe to allow non-zero offset trusted arg in all cases.
>
> Yeah, after thinking about this some more I agree with you. All we need
> to do is verify that the object at the non-zero offset has a
> ref_obj_id > 0 if being passed to KF_RELEASE. No different than at
> offset 0. This will be a nice usability improvement. The offset=0
> restriction really does seem pointless and arbitrary, unless I'm
> completely missing something.
>
It will be mostly ok, especially if a type match (not just type == X,
but for PTR_TO_BTF_ID, btf_struct_ids_match) follows the ref_obj_id
check (regardless of the offset).
Just be careful when such type match won't happen, e.g. with kfuncs
like bpf_obj_drop (taking a void *). In such a case off == 0 is
necessary, otherwise we'll pass the wrong pointer to the free function
in the kfunc.
We've had bugs in absence of type matching with offset != 0, e.g.
64620e0a1e71 ("bpf: Fix out of bounds access for ringbuf helpers")
comes to mind, which slipped through.
> > > > We can allow off!=0 and it won't confuse btf_type_ids_nocast_alias.
> > > >
> > > > struct nf_conn___init {
> > > > int another_field_at_off_zero;
> > > > struct nf_conn ct;
> > > > };
> > > >
> > > > will still trigger strict_type_match as expected.
> > >
> > > Yes, this should continue to just work, but I think we may also have to
> > > be cognizant to not allow this type of pattern:
> > >
> > > struct some_other_type {
> > > int field_at_off_zero;
> > > struct nf_conn___init ct;
> > > };
> > >
> > > In this case, we don't want to allow &other_type->ct to be passed to a
> > > kfunc expecting a struct nf_conn. So we'd also have to compare the type
> > > at the register offset to make sure it's not a nocast alias, not just
> > > the type in the register itself. I'm not sure if this is a problem in
> > > practice. I expect it isn't. struct nf_conn___init exists solely to
> > > allow the struct nf_conn kfuncs to enforce calling semantics so that an
> > > uninitialized struct nf_conn object can't be passed to specific kfuncs
> > > that are expecting an initialized object. I don't see why we'd ever
> > > embed a wrapper type like that inside of another type. But still
> > > something to be cognizant of.
> >
> > Agree that it's not a problem now and I wouldn't proactively
> > complicate the verifier. __init types are in the kernel code and it
> > gets code reviewed. So 'struct some_other_type' won't happen out of
> > nowhere.
>
> Makes sense
