Re: [RFC] bpf: allowing PTR_TO_BTF_ID | PTR_TRUSTED w/ non-zero fixed offset to selected KF_TRUSTED_ARGS BPF kfuncs

[David Vernet <void@manifault.com>]

[-- Attachment #1: Type: text/plain, Size: 2335 bytes --]

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.

> > > 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]