> From: Alexei Starovoitov [mailto:alexei.starovoitov@xxxxxxxxx] > Sent: Friday, June 24, 2022 6:50 PM > On Fri, Jun 24, 2022 at 8:32 AM Roberto Sassu <roberto.sassu@xxxxxxxxxx> > wrote: > > > > > From: Alexei Starovoitov [mailto:alexei.starovoitov@xxxxxxxxx] > > > Sent: Thursday, June 23, 2022 10:54 PM > > > On Thu, Jun 23, 2022 at 5:36 AM Roberto Sassu > <roberto.sassu@xxxxxxxxxx> > > > wrote: > > > > > > > > > From: Roberto Sassu [mailto:roberto.sassu@xxxxxxxxxx] > > > > > Sent: Wednesday, June 22, 2022 9:12 AM > > > > > > From: Alexei Starovoitov [mailto:alexei.starovoitov@xxxxxxxxx] > > > > > > Sent: Wednesday, June 22, 2022 12:33 AM > > > > > > On Tue, Jun 21, 2022 at 06:37:54PM +0200, Roberto Sassu wrote: > > > > > > > Add the bpf_lookup_user_key() and bpf_key_put() helpers, to > respectively > > > > > > > search a key with a given serial, and release the reference count of > the > > > > > > > found key. > > > > > > > > > > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx> > > > > > > > --- > > > > > > > include/uapi/linux/bpf.h | 16 ++++++++++++ > > > > > > > kernel/bpf/bpf_lsm.c | 46 > > > ++++++++++++++++++++++++++++++++++ > > > > > > > kernel/bpf/verifier.c | 6 +++-- > > > > > > > scripts/bpf_doc.py | 2 ++ > > > > > > > tools/include/uapi/linux/bpf.h | 16 ++++++++++++ > > > > > > > 5 files changed, 84 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > > > > > > > index e81362891596..7bbcf2cd105d 100644 > > > > > > > --- a/include/uapi/linux/bpf.h > > > > > > > +++ b/include/uapi/linux/bpf.h > > > > > > > @@ -5325,6 +5325,20 @@ union bpf_attr { > > > > > > > * **-EACCES** if the SYN cookie is not valid. > > > > > > > * > > > > > > > * **-EPROTONOSUPPORT** if CONFIG_IPV6 is not builtin. > > > > > > > + * > > > > > > > + * struct key *bpf_lookup_user_key(u32 serial, unsigned long flags) > > > > > > > + * Description > > > > > > > + * Search a key with a given *serial* and the provided > *flags*, > > > and > > > > > > > + * increment the reference count of the key. > > > > > > > > > > > > Why passing 'flags' is ok to do? > > > > > > Please think through every line of the patch. > > > > > > > > > > To be honest, I thought about it. Probably yes, I should do some > > > > > sanitization, like I did for the keyring ID. When I checked > > > > > lookup_user_key(), I saw that flags are checked individually, so > > > > > an arbitrary value passed to the helper should not cause harm. > > > > > Will do sanitization, if you prefer. It is just that we have to keep > > > > > the eBPF code in sync with key flag definition (unless we have > > > > > a 'last' flag). > > > > > > > > I'm not sure that having a helper for lookup_user_key() alone is > > > > correct. By having separate helpers for lookup and usage of the > > > > key, nothing would prevent an eBPF program to ask for a > > > > permission to pass the access control check, and then use the > > > > key for something completely different from what it requested. > > > > > > > > Looking at how lookup_user_key() is used in security/keys/keyctl.c, > > > > it seems clear that it should be used together with the operation > > > > that needs to be performed. Only in this way, the key permission > > > > would make sense. > > > > > > lookup is roughly equivalent to open when all permission checks are done. > > > And using the key is read/write. > > > > For bpf_verify_pkcs7_signature(), we need the search permission > > on the keyring containing the key used for signature verification. > > you mean lookup_user_key(serial, flags, KEY_NEED_SEARCH) ? > > right. and ? what's your point? It is hardcoded. Does not necessarily reflect the operation that will be performed on the key. On the other hand, if I add the permission as parameter to bpf_lookup_user_key(), an eBPF program can pass an arbitrary value, and then do something completely different with the key. Roberto
