| Message ID | 20220621163757.760304-3-roberto.sassu@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: Add bpf_verify_pkcs7_signature() helper | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-next-VM_Test-1 | success | Logs for Kernel LATEST on ubuntu-latest with gcc |
| bpf/vmtest-bpf-next-VM_Test-2 | success | Logs for Kernel LATEST on ubuntu-latest with llvm-15 |
| bpf/vmtest-bpf-next-PR | fail | PR summary |
| bpf/vmtest-bpf-next-VM_Test-3 | fail | Logs for Kernel LATEST on z15 with gcc |
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply, async |
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@huawei.com> > --- > 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.
> From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com] > 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@huawei.com> > > --- > > 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). Thanks Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Yang Xi, Li He
> From: Roberto Sassu [mailto:roberto.sassu@huawei.com] > Sent: Wednesday, June 22, 2022 9:12 AM > > From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com] > > 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@huawei.com> > > > --- > > > 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. What do you think (also David)? Thanks Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Yang Xi, Li He
On Thu, Jun 23, 2022 at 5:36 AM Roberto Sassu <roberto.sassu@huawei.com> wrote: > > > From: Roberto Sassu [mailto:roberto.sassu@huawei.com] > > Sent: Wednesday, June 22, 2022 9:12 AM > > > From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com] > > > 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@huawei.com> > > > > --- > > > > 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. > What do you think (also David)? > > Thanks > > Roberto > > HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 > Managing Director: Li Peng, Yang Xi, Li He Please use a different email server and get rid of this.
> From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com] > Sent: Thursday, June 23, 2022 10:54 PM > On Thu, Jun 23, 2022 at 5:36 AM Roberto Sassu <roberto.sassu@huawei.com> > wrote: > > > > > From: Roberto Sassu [mailto:roberto.sassu@huawei.com] > > > Sent: Wednesday, June 22, 2022 9:12 AM > > > > From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com] > > > > 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@huawei.com> > > > > > --- > > > > > 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. Thanks Roberto (I was not told otherwise, I use my corporate email to send work to outside, so I have to keep the notice) HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Yang Xi, Li He
> From: Roberto Sassu > Sent: Friday, June 24, 2022 5:33 PM > > From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com] > > Sent: Thursday, June 23, 2022 10:54 PM > > On Thu, Jun 23, 2022 at 5:36 AM Roberto Sassu <roberto.sassu@huawei.com> > > wrote: > > > > > > > From: Roberto Sassu [mailto:roberto.sassu@huawei.com] > > > > Sent: Wednesday, June 22, 2022 9:12 AM > > > > > From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com] > > > > > 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@huawei.com> > > > > > > --- > > > > > > 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. > > Thanks > > Roberto > > (I was not told otherwise, I use my corporate email to send > work to outside, so I have to keep the notice) I can remove it. Just to let you know. Roberto
On Fri, Jun 24, 2022 at 8:32 AM Roberto Sassu <roberto.sassu@huawei.com> wrote: > > > From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com] > > Sent: Thursday, June 23, 2022 10:54 PM > > On Thu, Jun 23, 2022 at 5:36 AM Roberto Sassu <roberto.sassu@huawei.com> > > wrote: > > > > > > > From: Roberto Sassu [mailto:roberto.sassu@huawei.com] > > > > Sent: Wednesday, June 22, 2022 9:12 AM > > > > > From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com] > > > > > 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@huawei.com> > > > > > > --- > > > > > > 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?
> From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com] > Sent: Friday, June 24, 2022 6:50 PM > On Fri, Jun 24, 2022 at 8:32 AM Roberto Sassu <roberto.sassu@huawei.com> > wrote: > > > > > From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com] > > > Sent: Thursday, June 23, 2022 10:54 PM > > > On Thu, Jun 23, 2022 at 5:36 AM Roberto Sassu > <roberto.sassu@huawei.com> > > > wrote: > > > > > > > > > From: Roberto Sassu [mailto:roberto.sassu@huawei.com] > > > > > Sent: Wednesday, June 22, 2022 9:12 AM > > > > > > From: Alexei Starovoitov [mailto:alexei.starovoitov@gmail.com] > > > > > > 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@huawei.com> > > > > > > > --- > > > > > > > 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
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. + * Return + * A key pointer if the key is found, a NULL pointer otherwise. + * + * void bpf_key_put(struct key *key) + * Description + * Decrement the reference count of the key obtained with the + * bpf_lookup_user_key() helper. + * Return + * 0 */ #define __BPF_FUNC_MAPPER(FN) \ FN(unspec), \ @@ -5535,6 +5549,8 @@ union bpf_attr { FN(tcp_raw_gen_syncookie_ipv6), \ FN(tcp_raw_check_syncookie_ipv4), \ FN(tcp_raw_check_syncookie_ipv6), \ + FN(lookup_user_key), \ + FN(key_put), \ /* */ /* integer value in 'imm' field of BPF_CALL instruction selects which helper diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c index c1351df9f7ee..bbbf9640f391 100644 --- a/kernel/bpf/bpf_lsm.c +++ b/kernel/bpf/bpf_lsm.c @@ -132,6 +132,46 @@ static const struct bpf_func_proto bpf_get_attach_cookie_proto = { .arg1_type = ARG_PTR_TO_CTX, }; +#ifdef CONFIG_KEYS +BTF_ID_LIST_SINGLE(btf_key_ids, struct, key) + +BPF_CALL_2(bpf_lookup_user_key, u32, serial, unsigned long, flags) +{ + key_ref_t key_ref; + + key_ref = lookup_user_key(serial, flags, KEY_NEED_SEARCH); + if (IS_ERR(key_ref)) + return (unsigned long)NULL; + + return (unsigned long)key_ref_to_ptr(key_ref); +} + +static const struct bpf_func_proto bpf_lookup_user_key_proto = { + .func = bpf_lookup_user_key, + .gpl_only = false, + .ret_type = RET_PTR_TO_BTF_ID_OR_NULL, + .ret_btf_id = &btf_key_ids[0], + .arg1_type = ARG_ANYTHING, + .arg2_type = ARG_ANYTHING, + .allowed = bpf_ima_inode_hash_allowed, +}; + +BPF_CALL_1(bpf_key_put, struct key *, key) +{ + key_put(key); + return 0; +} + +static const struct bpf_func_proto bpf_key_put_proto = { + .func = bpf_key_put, + .gpl_only = false, + .ret_type = RET_VOID, + .arg1_type = ARG_PTR_TO_BTF_ID | OBJ_RELEASE, + .arg1_btf_id = &btf_key_ids[0], + .allowed = bpf_ima_inode_hash_allowed, +}; +#endif /* CONFIG_KEYS */ + static const struct bpf_func_proto * bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) { @@ -158,6 +198,12 @@ bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) return prog->aux->sleepable ? &bpf_ima_file_hash_proto : NULL; case BPF_FUNC_get_attach_cookie: return bpf_prog_has_trampoline(prog) ? &bpf_get_attach_cookie_proto : NULL; +#ifdef CONFIG_KEYS + case BPF_FUNC_lookup_user_key: + return &bpf_lookup_user_key_proto; + case BPF_FUNC_key_put: + return &bpf_key_put_proto; +#endif /* CONFIG_KEYS */ default: return tracing_prog_func_proto(func_id, prog); } diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index bf72dc511df6..12f06ca649a4 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -483,7 +483,8 @@ static bool may_be_acquire_function(enum bpf_func_id func_id) func_id == BPF_FUNC_sk_lookup_udp || func_id == BPF_FUNC_skc_lookup_tcp || func_id == BPF_FUNC_map_lookup_elem || - func_id == BPF_FUNC_ringbuf_reserve; + func_id == BPF_FUNC_ringbuf_reserve || + func_id == BPF_FUNC_lookup_user_key; } static bool is_acquire_function(enum bpf_func_id func_id, @@ -495,7 +496,8 @@ static bool is_acquire_function(enum bpf_func_id func_id, func_id == BPF_FUNC_sk_lookup_udp || func_id == BPF_FUNC_skc_lookup_tcp || func_id == BPF_FUNC_ringbuf_reserve || - func_id == BPF_FUNC_kptr_xchg) + func_id == BPF_FUNC_kptr_xchg || + func_id == BPF_FUNC_lookup_user_key) return true; if (func_id == BPF_FUNC_map_lookup_elem && diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py index a0ec321469bd..3d5a7ad6f493 100755 --- a/scripts/bpf_doc.py +++ b/scripts/bpf_doc.py @@ -637,6 +637,7 @@ class PrinterHelpers(Printer): 'struct bpf_dynptr', 'struct iphdr', 'struct ipv6hdr', + 'struct key', ] known_types = { '...', @@ -690,6 +691,7 @@ class PrinterHelpers(Printer): 'struct bpf_dynptr', 'struct iphdr', 'struct ipv6hdr', + 'struct key', } mapped_types = { 'u8': '__u8', diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index e81362891596..7bbcf2cd105d 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/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. + * Return + * A key pointer if the key is found, a NULL pointer otherwise. + * + * void bpf_key_put(struct key *key) + * Description + * Decrement the reference count of the key obtained with the + * bpf_lookup_user_key() helper. + * Return + * 0 */ #define __BPF_FUNC_MAPPER(FN) \ FN(unspec), \ @@ -5535,6 +5549,8 @@ union bpf_attr { FN(tcp_raw_gen_syncookie_ipv6), \ FN(tcp_raw_check_syncookie_ipv4), \ FN(tcp_raw_check_syncookie_ipv6), \ + FN(lookup_user_key), \ + FN(key_put), \ /* */ /* integer value in 'imm' field of BPF_CALL instruction selects which helper
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@huawei.com> --- 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(-)