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

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

> From: Roberto Sassu [mailto:roberto.sassu@huawei.com]
> Sent: Tuesday, August 2, 2022 6:01 PM
> > From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com]
> > Sent: Tuesday, August 2, 2022 5:06 PM
> > On Tue, Aug 2, 2022 at 3:07 AM Roberto Sassu <roberto.sassu@huawei.com>
> > wrote:
> > >
> > > > 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.
> >
> > I don't think it's a good tradeoff complexity wise.
> > !=null check can be done in runtime by the helper.
> 
> Not sure I follow. bpf_lookup_user_key() might not find
> the key you asked for, so it will return NULL.
> 
> Did you mean that I should not set KF_RET_NULL for
> bpf_lookup_user_key()?
> 
> That anyway won’t help if you use the system keyring,
> the alternative parameter. The user keyring is the other
> one, returned by bpf_lookup_user_key().
> 
> When you specify a system keyring, the user keyring should
> be NULL, to indicate that the parameter should not be
> used. This was suggested by both John and Daniel.
> 
> So, it seems unavoidable to annotate the keyring parameter
> with __ref_null, or at least with __null. __ref_null would be
> better, to require a referenced parameter.
> 
> > The type cast is a sign of something fishy in the design.
> 
> Since this is what verify_pkcs7_signature() accepts, I guess
> it is the only option.

I added this topic for the agenda of BPF office hours tomorrow.

Roberto