| Message ID | 20220614130621.1976089-3-roberto.sassu@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: Add bpf_verify_pkcs7_signature() helper | expand |
On Tue, Jun 14, 2022 at 03:06:19PM +0200, Roberto Sassu wrote: > Add the bpf_request_key_by_id() helper, so that an eBPF program can obtain > a suitable key pointer to pass to the bpf_verify_pkcs7_signature() helper, > to be introduced in a later patch. > > The passed identifier can have the following values: 0 for the primary > keyring (immutable keyring of system keys); 1 for both the primary and > secondary keyring (where keys can be added only if they are vouched for by > existing keys in those keyrings); 2 for the platform keyring (primarily > used by the integrity subsystem to verify a kexec'ed kerned image and, > possibly, the initramfs signature); ULONG_MAX for the session keyring (for > testing purposes). > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > --- > include/uapi/linux/bpf.h | 17 +++++++++++++++++ > kernel/bpf/bpf_lsm.c | 30 ++++++++++++++++++++++++++++++ > scripts/bpf_doc.py | 2 ++ > tools/include/uapi/linux/bpf.h | 17 +++++++++++++++++ > 4 files changed, 66 insertions(+) > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > index f4009dbdf62d..dfd93e0e0759 100644 > --- a/include/uapi/linux/bpf.h > +++ b/include/uapi/linux/bpf.h > @@ -5249,6 +5249,22 @@ union bpf_attr { > * Pointer to the underlying dynptr data, NULL if the dynptr is > * read-only, if the dynptr is invalid, or if the offset and length > * is out of bounds. > + * > + * struct key *bpf_request_key_by_id(unsigned long id) > + * Description > + * Request a keyring by *id*. > + * > + * *id* can have the following values (some defined in > + * verification.h): 0 for the primary keyring (immutable keyring of > + * system keys); 1 for both the primary and secondary keyring > + * (where keys can be added only if they are vouched for by > + * existing keys in those keyrings); 2 for the platform keyring > + * (primarily used by the integrity subsystem to verify a kexec'ed > + * kerned image and, possibly, the initramfs signature); ULONG_MAX > + * for the session keyring (for testing purposes). It's never ok to add something like this to uapi 'for testing purposes'. If it's not useful in general it should not be a part of api. > + * Return > + * A non-NULL pointer if *id* is valid and not 0, a NULL pointer > + * otherwise. > */ > #define __BPF_FUNC_MAPPER(FN) \ > FN(unspec), \ > @@ -5455,6 +5471,7 @@ union bpf_attr { > FN(dynptr_read), \ > FN(dynptr_write), \ > FN(dynptr_data), \ > + FN(request_key_by_id), \ > /* */ > > /* 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..e1911812398b 100644 > --- a/kernel/bpf/bpf_lsm.c > +++ b/kernel/bpf/bpf_lsm.c > @@ -16,6 +16,7 @@ > #include <linux/bpf_local_storage.h> > #include <linux/btf_ids.h> > #include <linux/ima.h> > +#include <linux/verification.h> > > /* For every LSM hook that allows attachment of BPF programs, declare a nop > * function where a BPF program can be attached. > @@ -132,6 +133,31 @@ static const struct bpf_func_proto bpf_get_attach_cookie_proto = { > .arg1_type = ARG_PTR_TO_CTX, > }; > > +#ifdef CONFIG_KEYS > +BTF_ID_LIST_SINGLE(bpf_request_key_by_id_btf_ids, struct, key) > + > +BPF_CALL_1(bpf_request_key_by_id, unsigned long, id) > +{ > + const struct cred *cred = current_cred(); > + > + if (id > (unsigned long)VERIFY_USE_PLATFORM_KEYRING && id != ULONG_MAX) > + return (unsigned long)NULL; > + > + if (id == ULONG_MAX) > + return (unsigned long)cred->session_keyring; > + > + return id; It needs to do a proper lookup. Why cannot it do lookup_user_key ? The helper needs 'flags' arg too. Please think hard of extensibility and long term usefulness of api. At present this api feels like it was 'let me just hack something quickly'. Not ok.
> From: Alexei Starovoitov <alexei.starovoitov@gmail.com> > Sent: Friday, June 17, 2022 5:46 AM Adding in CC the keyring mailing list and David. Sort summary: we are adding an eBPF helper, to let eBPF programs verify PKCS#7 signatures. The helper simply calls verify_pkcs7_signature(). The problem is how to pass the key for verification. For hardcoded keyring IDs, it is easy, pass 0, 1 or 2 for respectively the built-in, secondary and platform keyring. If you want to pass another keyring, you need to do a lookup, which returns a key with reference count increased. While in the kernel you can call key_put() to decrease the reference count, that is not guaranteed with an eBPF program, if the developer forgets about it. What probably is necessary, is to add the capability to the verifier to check whether the reference count is decreased, or adding a callback mechanism to call automatically key_put() when the eBPF program is terminated. Is there an alternative solution? Thanks Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Yang Xi, Li He > On Tue, Jun 14, 2022 at 03:06:19PM +0200, Roberto Sassu wrote: > > Add the bpf_request_key_by_id() helper, so that an eBPF program can > > obtain a suitable key pointer to pass to the > > bpf_verify_pkcs7_signature() helper, to be introduced in a later patch. > > > > The passed identifier can have the following values: 0 for the primary > > keyring (immutable keyring of system keys); 1 for both the primary and > > secondary keyring (where keys can be added only if they are vouched > > for by existing keys in those keyrings); 2 for the platform keyring > > (primarily used by the integrity subsystem to verify a kexec'ed kerned > > image and, possibly, the initramfs signature); ULONG_MAX for the > > session keyring (for testing purposes). > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > --- > > include/uapi/linux/bpf.h | 17 +++++++++++++++++ > > kernel/bpf/bpf_lsm.c | 30 ++++++++++++++++++++++++++++++ > > scripts/bpf_doc.py | 2 ++ > > tools/include/uapi/linux/bpf.h | 17 +++++++++++++++++ > > 4 files changed, 66 insertions(+) > > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index > > f4009dbdf62d..dfd93e0e0759 100644 > > --- a/include/uapi/linux/bpf.h > > +++ b/include/uapi/linux/bpf.h > > @@ -5249,6 +5249,22 @@ union bpf_attr { > > * Pointer to the underlying dynptr data, NULL if the dynptr is > > * read-only, if the dynptr is invalid, or if the offset and length > > * is out of bounds. > > + * > > + * struct key *bpf_request_key_by_id(unsigned long id) > > + * Description > > + * Request a keyring by *id*. > > + * > > + * *id* can have the following values (some defined in > > + * verification.h): 0 for the primary keyring (immutable keyring > of > > + * system keys); 1 for both the primary and secondary keyring > > + * (where keys can be added only if they are vouched for by > > + * existing keys in those keyrings); 2 for the platform keyring > > + * (primarily used by the integrity subsystem to verify a > kexec'ed > > + * kerned image and, possibly, the initramfs signature); > ULONG_MAX > > + * for the session keyring (for testing purposes). > > It's never ok to add something like this to uapi 'for testing purposes'. > If it's not useful in general it should not be a part of api. > > > + * Return > > + * A non-NULL pointer if *id* is valid and not 0, a NULL pointer > > + * otherwise. > > */ > > #define __BPF_FUNC_MAPPER(FN) \ > > FN(unspec), \ > > @@ -5455,6 +5471,7 @@ union bpf_attr { > > FN(dynptr_read), \ > > FN(dynptr_write), \ > > FN(dynptr_data), \ > > + FN(request_key_by_id), \ > > /* */ > > > > /* 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..e1911812398b 100644 > > --- a/kernel/bpf/bpf_lsm.c > > +++ b/kernel/bpf/bpf_lsm.c > > @@ -16,6 +16,7 @@ > > #include <linux/bpf_local_storage.h> > > #include <linux/btf_ids.h> > > #include <linux/ima.h> > > +#include <linux/verification.h> > > > > /* For every LSM hook that allows attachment of BPF programs, declare a > nop > > * function where a BPF program can be attached. > > @@ -132,6 +133,31 @@ static const struct bpf_func_proto > bpf_get_attach_cookie_proto = { > > .arg1_type = ARG_PTR_TO_CTX, > > }; > > > > +#ifdef CONFIG_KEYS > > +BTF_ID_LIST_SINGLE(bpf_request_key_by_id_btf_ids, struct, key) > > + > > +BPF_CALL_1(bpf_request_key_by_id, unsigned long, id) { > > + const struct cred *cred = current_cred(); > > + > > + if (id > (unsigned long)VERIFY_USE_PLATFORM_KEYRING && id != > ULONG_MAX) > > + return (unsigned long)NULL; > > + > > + if (id == ULONG_MAX) > > + return (unsigned long)cred->session_keyring; > > + > > + return id; > > It needs to do a proper lookup. > Why cannot it do lookup_user_key ? > The helper needs 'flags' arg too. > Please think hard of extensibility and long term usefulness of api. > At present this api feels like it was 'let me just hack something quickly'. Not > ok.
On Fri, Jun 17, 2022 at 2:11 AM Roberto Sassu <roberto.sassu@huawei.com> wrote: > > > From: Alexei Starovoitov <alexei.starovoitov@gmail.com> > > Sent: Friday, June 17, 2022 5:46 AM > > Adding in CC the keyring mailing list and David. > > Sort summary: we are adding an eBPF helper, to let eBPF programs > verify PKCS#7 signatures. The helper simply calls verify_pkcs7_signature(). > > The problem is how to pass the key for verification. > > For hardcoded keyring IDs, it is easy, pass 0, 1 or 2 for respectively > the built-in, secondary and platform keyring. > > If you want to pass another keyring, you need to do a lookup, > which returns a key with reference count increased. > > While in the kernel you can call key_put() to decrease the > reference count, that is not guaranteed with an eBPF program, > if the developer forgets about it. What probably is necessary, > is to add the capability to the verifier to check whether the > reference count is decreased, or adding a callback mechanism > to call automatically key_put() when the eBPF program is > terminated. Nothing special here. See acquire/release logic in the verifier and relevant helpers. Like bpf_sk_lookup_tcp and others. > Is there an alternative solution? > > Thanks > > Roberto > > HUAWEI TECHNOLOGIES Duesseldorf GmbH, > HRB 56063 Managing Director: Li Peng, Yang Xi, Li He Please remove this footer from your emails.
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index f4009dbdf62d..dfd93e0e0759 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -5249,6 +5249,22 @@ union bpf_attr { * Pointer to the underlying dynptr data, NULL if the dynptr is * read-only, if the dynptr is invalid, or if the offset and length * is out of bounds. + * + * struct key *bpf_request_key_by_id(unsigned long id) + * Description + * Request a keyring by *id*. + * + * *id* can have the following values (some defined in + * verification.h): 0 for the primary keyring (immutable keyring of + * system keys); 1 for both the primary and secondary keyring + * (where keys can be added only if they are vouched for by + * existing keys in those keyrings); 2 for the platform keyring + * (primarily used by the integrity subsystem to verify a kexec'ed + * kerned image and, possibly, the initramfs signature); ULONG_MAX + * for the session keyring (for testing purposes). + * Return + * A non-NULL pointer if *id* is valid and not 0, a NULL pointer + * otherwise. */ #define __BPF_FUNC_MAPPER(FN) \ FN(unspec), \ @@ -5455,6 +5471,7 @@ union bpf_attr { FN(dynptr_read), \ FN(dynptr_write), \ FN(dynptr_data), \ + FN(request_key_by_id), \ /* */ /* 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..e1911812398b 100644 --- a/kernel/bpf/bpf_lsm.c +++ b/kernel/bpf/bpf_lsm.c @@ -16,6 +16,7 @@ #include <linux/bpf_local_storage.h> #include <linux/btf_ids.h> #include <linux/ima.h> +#include <linux/verification.h> /* For every LSM hook that allows attachment of BPF programs, declare a nop * function where a BPF program can be attached. @@ -132,6 +133,31 @@ static const struct bpf_func_proto bpf_get_attach_cookie_proto = { .arg1_type = ARG_PTR_TO_CTX, }; +#ifdef CONFIG_KEYS +BTF_ID_LIST_SINGLE(bpf_request_key_by_id_btf_ids, struct, key) + +BPF_CALL_1(bpf_request_key_by_id, unsigned long, id) +{ + const struct cred *cred = current_cred(); + + if (id > (unsigned long)VERIFY_USE_PLATFORM_KEYRING && id != ULONG_MAX) + return (unsigned long)NULL; + + if (id == ULONG_MAX) + return (unsigned long)cred->session_keyring; + + return id; +} + +static const struct bpf_func_proto bpf_request_key_by_id_proto = { + .func = bpf_request_key_by_id, + .gpl_only = false, + .ret_type = RET_PTR_TO_BTF_ID_OR_NULL, + .ret_btf_id = &bpf_request_key_by_id_btf_ids[0], + .arg1_type = ARG_ANYTHING, +}; +#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 +184,10 @@ 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_request_key_by_id: + return &bpf_request_key_by_id_proto; +#endif /* CONFIG_KEYS */ default: return tracing_prog_func_proto(func_id, prog); } diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py index 855b937e7585..176917df0ac0 100755 --- a/scripts/bpf_doc.py +++ b/scripts/bpf_doc.py @@ -635,6 +635,7 @@ class PrinterHelpers(Printer): 'struct bpf_timer', 'struct mptcp_sock', 'struct bpf_dynptr', + 'struct key', ] known_types = { '...', @@ -686,6 +687,7 @@ class PrinterHelpers(Printer): 'struct bpf_timer', 'struct mptcp_sock', 'struct bpf_dynptr', + 'struct key', } mapped_types = { 'u8': '__u8', diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index f4009dbdf62d..dfd93e0e0759 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/include/uapi/linux/bpf.h @@ -5249,6 +5249,22 @@ union bpf_attr { * Pointer to the underlying dynptr data, NULL if the dynptr is * read-only, if the dynptr is invalid, or if the offset and length * is out of bounds. + * + * struct key *bpf_request_key_by_id(unsigned long id) + * Description + * Request a keyring by *id*. + * + * *id* can have the following values (some defined in + * verification.h): 0 for the primary keyring (immutable keyring of + * system keys); 1 for both the primary and secondary keyring + * (where keys can be added only if they are vouched for by + * existing keys in those keyrings); 2 for the platform keyring + * (primarily used by the integrity subsystem to verify a kexec'ed + * kerned image and, possibly, the initramfs signature); ULONG_MAX + * for the session keyring (for testing purposes). + * Return + * A non-NULL pointer if *id* is valid and not 0, a NULL pointer + * otherwise. */ #define __BPF_FUNC_MAPPER(FN) \ FN(unspec), \ @@ -5455,6 +5471,7 @@ union bpf_attr { FN(dynptr_read), \ FN(dynptr_write), \ FN(dynptr_data), \ + FN(request_key_by_id), \ /* */ /* integer value in 'imm' field of BPF_CALL instruction selects which helper
Add the bpf_request_key_by_id() helper, so that an eBPF program can obtain a suitable key pointer to pass to the bpf_verify_pkcs7_signature() helper, to be introduced in a later patch. The passed identifier can have the following values: 0 for the primary keyring (immutable keyring of system keys); 1 for both the primary and secondary keyring (where keys can be added only if they are vouched for by existing keys in those keyrings); 2 for the platform keyring (primarily used by the integrity subsystem to verify a kexec'ed kerned image and, possibly, the initramfs signature); ULONG_MAX for the session keyring (for testing purposes). Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> --- include/uapi/linux/bpf.h | 17 +++++++++++++++++ kernel/bpf/bpf_lsm.c | 30 ++++++++++++++++++++++++++++++ scripts/bpf_doc.py | 2 ++ tools/include/uapi/linux/bpf.h | 17 +++++++++++++++++ 4 files changed, 66 insertions(+)