RE: [PATCH bpf-next v1 1/2] bpf: Add support for per-parameter trusted args

[Roberto Sassu <roberto.sassu@huawei.com>]

> From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com]
> Sent: Tuesday, August 2, 2022 6:46 AM
> 
> On Thu, Jul 28, 2022 at 2:02 AM Kumar Kartikeya Dwivedi
> <memxor@gmail.com> wrote:
> >
> > On Thu, 28 Jul 2022 at 10:45, Kumar Kartikeya Dwivedi <memxor@gmail.com>
> wrote:
> > >
> > > On Thu, 28 Jul 2022 at 10:18, Roberto Sassu <roberto.sassu@huawei.com>
> wrote:
> > > >
> > > > > From: Roberto Sassu [mailto:roberto.sassu@huawei.com]
> > > > > Sent: Thursday, July 28, 2022 9:46 AM
> > > > > > From: Kumar Kartikeya Dwivedi [mailto:memxor@gmail.com]
> > > > > > Sent: Wednesday, July 27, 2022 10:16 AM
> > > > > > Similar to how we detect mem, size pairs in kfunc, teach verifier to
> > > > > > treat __ref suffix on argument name to imply that it must be a trusted
> > > > > > arg when passed to kfunc, similar to the effect of KF_TRUSTED_ARGS
> flag
> > > > > > but limited to the specific parameter. This is required to ensure that
> > > > > > kfunc that operate on some object only work on acquired pointers and
> not
> > > > > > normal PTR_TO_BTF_ID with same type which can be obtained by
> pointer
> > > > > > walking. Release functions need not specify such suffix on release
> > > > > > arguments as they are already expected to receive one referenced
> > > > > > argument.
> > > > >
> > > > > Thanks, Kumar. I will try it.
> > > >
> > > > Uhm. I realized that I was already using another suffix,
> > > > __maybe_null, to indicate that a caller can pass NULL as
> > > > argument.
> > > >
> > > > Wouldn't probably work well with two suffixes.
> > > >
> > >
> > > Then you can maybe extend it to parse two suffixes at most (for now
> atleast)?
> > >
> > > > Have you considered to extend BTF_ID_FLAGS to take five
> > > > extra arguments, to set flags for each kfunc parameter?
> > > >
> > >
> > > I didn't understand this. Flags parameter is an OR of the flags you
> > > set, why would we want to extend it to take 5 args?
> > > You can just or f1 | f2 | f3 | f4 | f5, as many as you want.
> >
> > Oh, so you mean having 5 more args to indicate flags on each
> > parameter? It is possible, but I think the scheme for now works ok. If
> > you extend it to parse two suffixes, it should be fine. Yes, the
> > variable name would be ugly, but you can just make a copy into a
> > properly named one. This is the best we can do without switching to
> > BTF tags. We can revisit this when we start having 4 or 5 tags on a
> > single parameter.
> >
> > To make it a bit less verbose you could probably call maybe_null just null?
> 
> Thank you for posting the patch.
> It still feels that this extra flexibility gets convoluted.
> I'm not sure Roberto's kfunc actually needs __ref.
> All pointers should be pointers. Hacking -1 and -2 into a pointer
> is something that key infra did, but it doesn't mean that
> we have to carry over it into bpf kfunc.

There is a separate parameter for the keyring IDs that only
verify_pkcs7_signature() understands. Type casting is done
internally in the bpf_verify_pkcs7_signature() kfunc.
 
The other is always a valid struct key pointer or NULL, coming
from bpf_lookup_user_key() (acquire function). I extended
Kumar's patch further to annotate the struct key parameter
with the _ref_null suffix, to accept a referenced pointer or NULL,
instead of just referenced.

Roberto