Apologies about the delayed response on this thread, I've been on
parental leave for the last couple of weeks and only just catching up
on everything now.
On Thu, Apr 25, 2024 at 06:06:00PM +0200, Kumar Kartikeya Dwivedi wrote:
> 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
OK, so based on what has been discussed within this thread, I've
understood that we're OK with generally relaxing the reg->off
restriction that is currently in place for
KF_ACQUIRE/KF_RELEASE/KF_TRUSTED_ARGS/KF_RCU flagged BPF kfuncs?
This is providing that we substitute the current !reg->off checks with
the following instead moving forward:
* For arguments passed to BPF kfuncs that makes use of the
KF_RELEASE/KF_TRUSTED_ARGS/KF_RCU flag, ensure that the backing reg
has a reg->ref_obj_id > 0, irrespective of the value held by
reg->off.
* Perform a type match check against reg and the arguments of the
KF_RELEASE/KF_TRUSTED_ARGS/KF_RCU based BPF kfunc, following the
preceding reg->ref_obj_id > 0 check.
* If we fail to type match, because the
KF_RELEASE/KF_TRUSTED_ARGS/KF_RCU based BPF kfunc takes a void
pointer, fallback to enforcing the !reg->off that was previously in
place.
Please do correct me if I've misunderstood something.
/M
