| Message ID | 20220610135916.1285509-2-roberto.sassu@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: Add bpf_verify_signature() helper | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-next-PR | fail | PR summary |
| bpf/vmtest-bpf-next-VM_Test-1 | fail | Logs for Kernel LATEST on ubuntu-latest with gcc |
| bpf/vmtest-bpf-next-VM_Test-2 | fail | Logs for Kernel LATEST on ubuntu-latest with llvm-15 |
| bpf/vmtest-bpf-next-VM_Test-3 | fail | Logs for Kernel LATEST on z15 with gcc |
| netdev/tree_selection | success | Not a local patch, async |
On 6/10/22 3:59 PM, Roberto Sassu wrote: > Add the bpf_verify_signature() helper, to give eBPF security modules the > ability to check the validity of a signature against supplied data, by > using system-provided keys as trust anchor. > > The new helper makes it possible to enforce mandatory policies, as eBPF > programs might be allowed to make security decisions only based on data > sources the system administrator approves. > > The caller should specify the identifier of the keyring containing the keys > for signature verification: 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); > 0xffff for the session keyring (for testing purposes). > > The caller should also specify the type of signature. Currently only PKCS#7 > is supported. > > Since the maximum number of parameters of an eBPF helper is 5, the keyring > and signature types share one (keyring ID: low 16 bits, signature type: > high 16 bits). > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > Reported-by: kernel test robot <lkp@intel.com> (cast warning) > --- > include/uapi/linux/bpf.h | 17 +++++++++++++ > kernel/bpf/bpf_lsm.c | 46 ++++++++++++++++++++++++++++++++++ > tools/include/uapi/linux/bpf.h | 17 +++++++++++++ > 3 files changed, 80 insertions(+) > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > index f4009dbdf62d..97521857e44a 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. > + * > + * long bpf_verify_signature(u8 *data, u32 datalen, u8 *sig, u32 siglen, u32 info) > + * Description > + * Verify a signature of length *siglen* against the supplied data > + * with length *datalen*. *info* contains the keyring identifier > + * (low 16 bits) and the signature type (high 16 bits). The keyring > + * identifier 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); 0xffff for > + * the session keyring (for testing purposes). > + * Return > + * 0 on success, a negative value on error. > */ > #define __BPF_FUNC_MAPPER(FN) \ > FN(unspec), \ > @@ -5455,6 +5471,7 @@ union bpf_attr { > FN(dynptr_read), \ > FN(dynptr_write), \ > FN(dynptr_data), \ > + FN(verify_signature), \ > /* */ > > /* 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..20bd850ea3ee 100644 > --- a/kernel/bpf/bpf_lsm.c > +++ b/kernel/bpf/bpf_lsm.c > @@ -16,6 +16,8 @@ > #include <linux/bpf_local_storage.h> > #include <linux/btf_ids.h> > #include <linux/ima.h> > +#include <linux/verification.h> > +#include <linux/module_signature.h> > > /* For every LSM hook that allows attachment of BPF programs, declare a nop > * function where a BPF program can be attached. > @@ -132,6 +134,46 @@ static const struct bpf_func_proto bpf_get_attach_cookie_proto = { > .arg1_type = ARG_PTR_TO_CTX, > }; > > +#ifdef CONFIG_SYSTEM_DATA_VERIFICATION > +BPF_CALL_5(bpf_verify_signature, u8 *, data, u32, datalen, u8 *, sig, > + u32, siglen, u32, info) > +{ > + unsigned long keyring_id = info & U16_MAX; > + enum pkey_id_type id_type = info >> 16; > + const struct cred *cred = current_cred(); > + struct key *keyring; > + > + if (keyring_id > (unsigned long)VERIFY_USE_PLATFORM_KEYRING && > + keyring_id != U16_MAX) > + return -EINVAL; > + > + keyring = (keyring_id == U16_MAX) ? > + cred->session_keyring : (struct key *)keyring_id; > + > + switch (id_type) { > + case PKEY_ID_PKCS7: > + return verify_pkcs7_signature(data, datalen, sig, siglen, > + keyring, > + VERIFYING_UNSPECIFIED_SIGNATURE, > + NULL, NULL); > + default: > + return -EOPNOTSUPP; Question to you & KP: > Can we keep the helper generic so that it can be extended to more types of > signatures and pass the signature type as an enum? How many different signature types do we expect, say, in the next 6mo, to land here? Just thinking out loud whether it is better to keep it simple as with the last iteration where we have a helper specific to pkcs7, and if needed in future we add others. We only have the last reg as auxillary arg where we need to squeeze all info into it now. What if for other, future signature types this won't suffice? > + } > +} > + > +static const struct bpf_func_proto bpf_verify_signature_proto = { > + .func = bpf_verify_signature, > + .gpl_only = false, > + .ret_type = RET_INTEGER, > + .arg1_type = ARG_PTR_TO_MEM, > + .arg2_type = ARG_CONST_SIZE_OR_ZERO, Can verify_pkcs7_signature() handle null/0 len for data* args? > + .arg3_type = ARG_PTR_TO_MEM, > + .arg4_type = ARG_CONST_SIZE_OR_ZERO, Ditto for sig* args? > + .arg5_type = ARG_ANYTHING, > + .allowed = bpf_ima_inode_hash_allowed, > +}; > +#endif > + > static const struct bpf_func_proto * > bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > { > @@ -158,6 +200,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_SYSTEM_DATA_VERIFICATION > + case BPF_FUNC_verify_signature: > + return prog->aux->sleepable ? &bpf_verify_signature_proto : NULL; > +#endif > default: > return tracing_prog_func_proto(func_id, prog); > }
> From: Daniel Borkmann [mailto:daniel@iogearbox.net] > Sent: Friday, June 10, 2022 4:49 PM > On 6/10/22 3:59 PM, Roberto Sassu wrote: > > Add the bpf_verify_signature() helper, to give eBPF security modules the > > ability to check the validity of a signature against supplied data, by > > using system-provided keys as trust anchor. > > > > The new helper makes it possible to enforce mandatory policies, as eBPF > > programs might be allowed to make security decisions only based on data > > sources the system administrator approves. > > > > The caller should specify the identifier of the keyring containing the keys > > for signature verification: 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); > > 0xffff for the session keyring (for testing purposes). > > > > The caller should also specify the type of signature. Currently only PKCS#7 > > is supported. > > > > Since the maximum number of parameters of an eBPF helper is 5, the keyring > > and signature types share one (keyring ID: low 16 bits, signature type: > > high 16 bits). > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > Reported-by: kernel test robot <lkp@intel.com> (cast warning) > > --- > > include/uapi/linux/bpf.h | 17 +++++++++++++ > > kernel/bpf/bpf_lsm.c | 46 ++++++++++++++++++++++++++++++++++ > > tools/include/uapi/linux/bpf.h | 17 +++++++++++++ > > 3 files changed, 80 insertions(+) > > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > > index f4009dbdf62d..97521857e44a 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. > > + * > > + * long bpf_verify_signature(u8 *data, u32 datalen, u8 *sig, u32 siglen, u32 > info) > > + * Description > > + * Verify a signature of length *siglen* against the supplied data > > + * with length *datalen*. *info* contains the keyring identifier > > + * (low 16 bits) and the signature type (high 16 bits). The keyring > > + * identifier 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); 0xffff for > > + * the session keyring (for testing purposes). > > + * Return > > + * 0 on success, a negative value on error. > > */ > > #define __BPF_FUNC_MAPPER(FN) \ > > FN(unspec), \ > > @@ -5455,6 +5471,7 @@ union bpf_attr { > > FN(dynptr_read), \ > > FN(dynptr_write), \ > > FN(dynptr_data), \ > > + FN(verify_signature), \ > > /* */ > > > > /* 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..20bd850ea3ee 100644 > > --- a/kernel/bpf/bpf_lsm.c > > +++ b/kernel/bpf/bpf_lsm.c > > @@ -16,6 +16,8 @@ > > #include <linux/bpf_local_storage.h> > > #include <linux/btf_ids.h> > > #include <linux/ima.h> > > +#include <linux/verification.h> > > +#include <linux/module_signature.h> > > > > /* For every LSM hook that allows attachment of BPF programs, declare a > nop > > * function where a BPF program can be attached. > > @@ -132,6 +134,46 @@ static const struct bpf_func_proto > bpf_get_attach_cookie_proto = { > > .arg1_type = ARG_PTR_TO_CTX, > > }; > > > > +#ifdef CONFIG_SYSTEM_DATA_VERIFICATION > > +BPF_CALL_5(bpf_verify_signature, u8 *, data, u32, datalen, u8 *, sig, > > + u32, siglen, u32, info) > > +{ > > + unsigned long keyring_id = info & U16_MAX; > > + enum pkey_id_type id_type = info >> 16; > > + const struct cred *cred = current_cred(); > > + struct key *keyring; > > + > > + if (keyring_id > (unsigned long)VERIFY_USE_PLATFORM_KEYRING && > > + keyring_id != U16_MAX) > > + return -EINVAL; > > + > > + keyring = (keyring_id == U16_MAX) ? > > + cred->session_keyring : (struct key *)keyring_id; > > + > > + switch (id_type) { > > + case PKEY_ID_PKCS7: > > + return verify_pkcs7_signature(data, datalen, sig, siglen, > > + keyring, > > + > VERIFYING_UNSPECIFIED_SIGNATURE, > > + NULL, NULL); > > + default: > > + return -EOPNOTSUPP; > > Question to you & KP: > > > Can we keep the helper generic so that it can be extended to more types of > > signatures and pass the signature type as an enum? > > How many different signature types do we expect, say, in the next 6mo, to land > here? Just thinking out loud whether it is better to keep it simple as with the > last iteration where we have a helper specific to pkcs7, and if needed in future > we add others. We only have the last reg as auxillary arg where we need to > squeeze > all info into it now. What if for other, future signature types this won't suffice? I would add at least another for PGP, assuming that the code will be upstreamed. But I agree, the number should not be that high. > > + } > > +} > > + > > +static const struct bpf_func_proto bpf_verify_signature_proto = { > > + .func = bpf_verify_signature, > > + .gpl_only = false, > > + .ret_type = RET_INTEGER, > > + .arg1_type = ARG_PTR_TO_MEM, > > + .arg2_type = ARG_CONST_SIZE_OR_ZERO, > > Can verify_pkcs7_signature() handle null/0 len for data* args? Shouldn't ARG_PTR_TO_MEM require valid memory? 0 len should not be a problem. Thanks Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Yang Xi, Li He > > + .arg3_type = ARG_PTR_TO_MEM, > > + .arg4_type = ARG_CONST_SIZE_OR_ZERO, > > Ditto for sig* args? > > > + .arg5_type = ARG_ANYTHING, > > + .allowed = bpf_ima_inode_hash_allowed, > > +}; > > +#endif > > + > > static const struct bpf_func_proto * > > bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > > { > > @@ -158,6 +200,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_SYSTEM_DATA_VERIFICATION > > + case BPF_FUNC_verify_signature: > > + return prog->aux->sleepable ? &bpf_verify_signature_proto : > NULL; > > +#endif > > default: > > return tracing_prog_func_proto(func_id, prog); > > }
On 6/10/22 4:59 PM, Roberto Sassu wrote: >> From: Daniel Borkmann [mailto:daniel@iogearbox.net] >> Sent: Friday, June 10, 2022 4:49 PM >> On 6/10/22 3:59 PM, Roberto Sassu wrote: >>> Add the bpf_verify_signature() helper, to give eBPF security modules the >>> ability to check the validity of a signature against supplied data, by >>> using system-provided keys as trust anchor. >>> >>> The new helper makes it possible to enforce mandatory policies, as eBPF >>> programs might be allowed to make security decisions only based on data >>> sources the system administrator approves. >>> >>> The caller should specify the identifier of the keyring containing the keys >>> for signature verification: 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); >>> 0xffff for the session keyring (for testing purposes). >>> >>> The caller should also specify the type of signature. Currently only PKCS#7 >>> is supported. >>> >>> Since the maximum number of parameters of an eBPF helper is 5, the keyring >>> and signature types share one (keyring ID: low 16 bits, signature type: >>> high 16 bits). >>> >>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> >>> Reported-by: kernel test robot <lkp@intel.com> (cast warning) >>> --- >>> include/uapi/linux/bpf.h | 17 +++++++++++++ >>> kernel/bpf/bpf_lsm.c | 46 ++++++++++++++++++++++++++++++++++ >>> tools/include/uapi/linux/bpf.h | 17 +++++++++++++ >>> 3 files changed, 80 insertions(+) >>> >>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h >>> index f4009dbdf62d..97521857e44a 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. >>> + * >>> + * long bpf_verify_signature(u8 *data, u32 datalen, u8 *sig, u32 siglen, u32 >> info) >>> + * Description >>> + * Verify a signature of length *siglen* against the supplied data >>> + * with length *datalen*. *info* contains the keyring identifier >>> + * (low 16 bits) and the signature type (high 16 bits). The keyring >>> + * identifier 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); 0xffff for >>> + * the session keyring (for testing purposes). >>> + * Return >>> + * 0 on success, a negative value on error. >>> */ >>> #define __BPF_FUNC_MAPPER(FN) \ >>> FN(unspec), \ >>> @@ -5455,6 +5471,7 @@ union bpf_attr { >>> FN(dynptr_read), \ >>> FN(dynptr_write), \ >>> FN(dynptr_data), \ >>> + FN(verify_signature), \ >>> /* */ >>> >>> /* 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..20bd850ea3ee 100644 >>> --- a/kernel/bpf/bpf_lsm.c >>> +++ b/kernel/bpf/bpf_lsm.c >>> @@ -16,6 +16,8 @@ >>> #include <linux/bpf_local_storage.h> >>> #include <linux/btf_ids.h> >>> #include <linux/ima.h> >>> +#include <linux/verification.h> >>> +#include <linux/module_signature.h> >>> >>> /* For every LSM hook that allows attachment of BPF programs, declare a >> nop >>> * function where a BPF program can be attached. >>> @@ -132,6 +134,46 @@ static const struct bpf_func_proto >> bpf_get_attach_cookie_proto = { >>> .arg1_type = ARG_PTR_TO_CTX, >>> }; >>> >>> +#ifdef CONFIG_SYSTEM_DATA_VERIFICATION >>> +BPF_CALL_5(bpf_verify_signature, u8 *, data, u32, datalen, u8 *, sig, >>> + u32, siglen, u32, info) >>> +{ >>> + unsigned long keyring_id = info & U16_MAX; >>> + enum pkey_id_type id_type = info >> 16; >>> + const struct cred *cred = current_cred(); >>> + struct key *keyring; >>> + >>> + if (keyring_id > (unsigned long)VERIFY_USE_PLATFORM_KEYRING && >>> + keyring_id != U16_MAX) >>> + return -EINVAL; >>> + >>> + keyring = (keyring_id == U16_MAX) ? >>> + cred->session_keyring : (struct key *)keyring_id; >>> + >>> + switch (id_type) { >>> + case PKEY_ID_PKCS7: >>> + return verify_pkcs7_signature(data, datalen, sig, siglen, >>> + keyring, >>> + >> VERIFYING_UNSPECIFIED_SIGNATURE, >>> + NULL, NULL); >>> + default: >>> + return -EOPNOTSUPP; >> >> Question to you & KP: >> >> > Can we keep the helper generic so that it can be extended to more types of >> > signatures and pass the signature type as an enum? >> >> How many different signature types do we expect, say, in the next 6mo, to land >> here? Just thinking out loud whether it is better to keep it simple as with the >> last iteration where we have a helper specific to pkcs7, and if needed in future >> we add others. We only have the last reg as auxillary arg where we need to >> squeeze >> all info into it now. What if for other, future signature types this won't suffice? > > I would add at least another for PGP, assuming that the code will be > upstreamed. But I agree, the number should not be that high. If realistically expected is really just two helpers, what speaks against a bpf_verify_signature_pkcs7() and bpf_verify_signature_pgp() in that case, for sake of better user experience? Maybe one other angle.. if CONFIG_SYSTEM_DATA_VERIFICATION is enabled, it may not be clear whether verify_pkcs7_signature() or a verify_pgp_signature() are both always builtin. And then, we run into the issue again of more complex probing for availability of the algs compared to simple ... #if defined(CONFIG_SYSTEM_DATA_VERIFICATION) && defined(CONFIG_XYZ) case BPF_FUNC_verify_signature_xyz: return ..._proto; #endif ... which bpftool and others easily understand. >>> + } >>> +} >>> + >>> +static const struct bpf_func_proto bpf_verify_signature_proto = { >>> + .func = bpf_verify_signature, >>> + .gpl_only = false, >>> + .ret_type = RET_INTEGER, >>> + .arg1_type = ARG_PTR_TO_MEM, >>> + .arg2_type = ARG_CONST_SIZE_OR_ZERO, >> >> Can verify_pkcs7_signature() handle null/0 len for data* args? > > Shouldn't ARG_PTR_TO_MEM require valid memory? 0 len should > not be a problem. check_helper_mem_access() has: /* Allow zero-byte read from NULL, regardless of pointer type */ if (zero_size_allowed && access_size == 0 && register_is_null(reg)) return 0; So NULL/0 pair can be passed. Maybe good to add these corner cases to the test_progs selftest additions then if it's needed. >>> + .arg3_type = ARG_PTR_TO_MEM, >>> + .arg4_type = ARG_CONST_SIZE_OR_ZERO, >> >> Ditto for sig* args? >> >>> + .arg5_type = ARG_ANYTHING, >>> + .allowed = bpf_ima_inode_hash_allowed, >>> +}; >>> +#endif >>> + >>> static const struct bpf_func_proto * >>> bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) >>> { >>> @@ -158,6 +200,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_SYSTEM_DATA_VERIFICATION >>> + case BPF_FUNC_verify_signature: >>> + return prog->aux->sleepable ? &bpf_verify_signature_proto : >> NULL; >>> +#endif >>> default: >>> return tracing_prog_func_proto(func_id, prog); >>> }
On Fri, Jun 10, 2022 at 6:59 AM Roberto Sassu <roberto.sassu@huawei.com> wrote: > > Since the maximum number of parameters of an eBPF helper is 5, the keyring > and signature types share one (keyring ID: low 16 bits, signature type: > high 16 bits). ... > + * long bpf_verify_signature(u8 *data, u32 datalen, u8 *sig, u32 siglen, u32 info) > + * Description > + * Verify a signature of length *siglen* against the supplied data > + * with length *datalen*. *info* contains the keyring identifier > + * (low 16 bits) and the signature type (high 16 bits). The keyring > + * identifier 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); 0xffff for > + * the session keyring (for testing purposes). Muxing all kinds of info in the 5th arg isn't great. It's better to use dynptr here for data and sig. It will free up two extra arguments.
On Fri, Jun 10, 2022 at 6:59 AM Roberto Sassu <roberto.sassu@huawei.com> wrote: > + keyring = (keyring_id == U16_MAX) ? > + cred->session_keyring : (struct key *)keyring_id; This is too limiting. bpf prog should be able to do what *key syscalls can do. By doing lookup_user_key(id) -> keyring. Maybe it's ok to have a special reserved id that does cred->sessions_keyring as a shortcut, but that's an optimization.
On Fri, Jun 10, 2022 at 5:14 PM Daniel Borkmann <daniel@iogearbox.net> wrote: > > On 6/10/22 4:59 PM, Roberto Sassu wrote: > >> From: Daniel Borkmann [mailto:daniel@iogearbox.net] > >> Sent: Friday, June 10, 2022 4:49 PM > >> On 6/10/22 3:59 PM, Roberto Sassu wrote: > >>> Add the bpf_verify_signature() helper, to give eBPF security modules the > >>> ability to check the validity of a signature against supplied data, by > >>> using system-provided keys as trust anchor. > >>> > >>> The new helper makes it possible to enforce mandatory policies, as eBPF > >>> programs might be allowed to make security decisions only based on data > >>> sources the system administrator approves. > >>> > >>> The caller should specify the identifier of the keyring containing the keys > >>> for signature verification: 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); > >>> 0xffff for the session keyring (for testing purposes). > >>> > >>> The caller should also specify the type of signature. Currently only PKCS#7 > >>> is supported. > >>> > >>> Since the maximum number of parameters of an eBPF helper is 5, the keyring > >>> and signature types share one (keyring ID: low 16 bits, signature type: > >>> high 16 bits). > >>> > >>> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > >>> Reported-by: kernel test robot <lkp@intel.com> (cast warning) > >>> --- > >>> include/uapi/linux/bpf.h | 17 +++++++++++++ > >>> kernel/bpf/bpf_lsm.c | 46 ++++++++++++++++++++++++++++++++++ > >>> tools/include/uapi/linux/bpf.h | 17 +++++++++++++ > >>> 3 files changed, 80 insertions(+) > >>> > >>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > >>> index f4009dbdf62d..97521857e44a 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. > >>> + * > >>> + * long bpf_verify_signature(u8 *data, u32 datalen, u8 *sig, u32 siglen, u32 > >> info) > >>> + * Description > >>> + * Verify a signature of length *siglen* against the supplied data > >>> + * with length *datalen*. *info* contains the keyring identifier > >>> + * (low 16 bits) and the signature type (high 16 bits). The keyring > >>> + * identifier 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); 0xffff for > >>> + * the session keyring (for testing purposes). > >>> + * Return > >>> + * 0 on success, a negative value on error. > >>> */ > >>> #define __BPF_FUNC_MAPPER(FN) \ > >>> FN(unspec), \ > >>> @@ -5455,6 +5471,7 @@ union bpf_attr { > >>> FN(dynptr_read), \ > >>> FN(dynptr_write), \ > >>> FN(dynptr_data), \ > >>> + FN(verify_signature), \ > >>> /* */ > >>> > >>> /* 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..20bd850ea3ee 100644 > >>> --- a/kernel/bpf/bpf_lsm.c > >>> +++ b/kernel/bpf/bpf_lsm.c > >>> @@ -16,6 +16,8 @@ > >>> #include <linux/bpf_local_storage.h> > >>> #include <linux/btf_ids.h> > >>> #include <linux/ima.h> > >>> +#include <linux/verification.h> > >>> +#include <linux/module_signature.h> > >>> > >>> /* For every LSM hook that allows attachment of BPF programs, declare a > >> nop > >>> * function where a BPF program can be attached. > >>> @@ -132,6 +134,46 @@ static const struct bpf_func_proto > >> bpf_get_attach_cookie_proto = { > >>> .arg1_type = ARG_PTR_TO_CTX, > >>> }; > >>> > >>> +#ifdef CONFIG_SYSTEM_DATA_VERIFICATION > >>> +BPF_CALL_5(bpf_verify_signature, u8 *, data, u32, datalen, u8 *, sig, > >>> + u32, siglen, u32, info) > >>> +{ > >>> + unsigned long keyring_id = info & U16_MAX; > >>> + enum pkey_id_type id_type = info >> 16; > >>> + const struct cred *cred = current_cred(); > >>> + struct key *keyring; > >>> + > >>> + if (keyring_id > (unsigned long)VERIFY_USE_PLATFORM_KEYRING && > >>> + keyring_id != U16_MAX) > >>> + return -EINVAL; > >>> + > >>> + keyring = (keyring_id == U16_MAX) ? > >>> + cred->session_keyring : (struct key *)keyring_id; > >>> + > >>> + switch (id_type) { > >>> + case PKEY_ID_PKCS7: > >>> + return verify_pkcs7_signature(data, datalen, sig, siglen, > >>> + keyring, > >>> + > >> VERIFYING_UNSPECIFIED_SIGNATURE, > >>> + NULL, NULL); > >>> + default: > >>> + return -EOPNOTSUPP; > >> > >> Question to you & KP: > >> > >> > Can we keep the helper generic so that it can be extended to more types of > >> > signatures and pass the signature type as an enum? > >> > >> How many different signature types do we expect, say, in the next 6mo, to land > >> here? Just thinking out loud whether it is better to keep it simple as with the > >> last iteration where we have a helper specific to pkcs7, and if needed in future > >> we add others. We only have the last reg as auxillary arg where we need to > >> squeeze > >> all info into it now. What if for other, future signature types this won't suffice? > > > > I would add at least another for PGP, assuming that the code will be > > upstreamed. But I agree, the number should not be that high. > > If realistically expected is really just two helpers, what speaks against a > bpf_verify_signature_pkcs7() and bpf_verify_signature_pgp() in that case, for > sake of better user experience? > > Maybe one other angle.. if CONFIG_SYSTEM_DATA_VERIFICATION is enabled, it may > not be clear whether verify_pkcs7_signature() or a verify_pgp_signature() are > both always builtin. And then, we run into the issue again of more complex probing > for availability of the algs compared to simple ... > > #if defined(CONFIG_SYSTEM_DATA_VERIFICATION) && defined(CONFIG_XYZ) > case BPF_FUNC_verify_signature_xyz: > return ..._proto; > #endif > > ... which bpftool and others easily understand. > > >>> + } > >>> +} > >>> + > >>> +static const struct bpf_func_proto bpf_verify_signature_proto = { > >>> + .func = bpf_verify_signature, > >>> + .gpl_only = false, > >>> + .ret_type = RET_INTEGER, > >>> + .arg1_type = ARG_PTR_TO_MEM, > >>> + .arg2_type = ARG_CONST_SIZE_OR_ZERO, > >> > >> Can verify_pkcs7_signature() handle null/0 len for data* args? > > > > Shouldn't ARG_PTR_TO_MEM require valid memory? 0 len should > > not be a problem. > > check_helper_mem_access() has: > > /* Allow zero-byte read from NULL, regardless of pointer type */ > if (zero_size_allowed && access_size == 0 && > register_is_null(reg)) > return 0; Daniel, makes a fair point here. Alexei, what do you think? I wonder if some "future" signature verification would need even more / different arguments so a unified bpf_verify_signature might get more complex / not easy to extend. > > So NULL/0 pair can be passed. Maybe good to add these corner cases to the test_progs > selftest additions then if it's needed. > > >>> + .arg3_type = ARG_PTR_TO_MEM, > >>> + .arg4_type = ARG_CONST_SIZE_OR_ZERO, > >> > >> Ditto for sig* args? > >> > >>> + .arg5_type = ARG_ANYTHING, > >>> + .allowed = bpf_ima_inode_hash_allowed, > >>> +}; > >>> +#endif > >>> + > >>> static const struct bpf_func_proto * > >>> bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > >>> { > >>> @@ -158,6 +200,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_SYSTEM_DATA_VERIFICATION > >>> + case BPF_FUNC_verify_signature: > >>> + return prog->aux->sleepable ? &bpf_verify_signature_proto : > >> NULL; > >>> +#endif > >>> default: > >>> return tracing_prog_func_proto(func_id, prog); > >>> } >
On Fri, Jun 10, 2022 at 4:53 PM KP Singh <kpsingh@kernel.org> wrote: > > >>> +static const struct bpf_func_proto bpf_verify_signature_proto = { > > >>> + .func = bpf_verify_signature, > > >>> + .gpl_only = false, > > >>> + .ret_type = RET_INTEGER, > > >>> + .arg1_type = ARG_PTR_TO_MEM, > > >>> + .arg2_type = ARG_CONST_SIZE_OR_ZERO, > > >> > > >> Can verify_pkcs7_signature() handle null/0 len for data* args? > > > > > > Shouldn't ARG_PTR_TO_MEM require valid memory? 0 len should > > > not be a problem. > > > > check_helper_mem_access() has: > > > > /* Allow zero-byte read from NULL, regardless of pointer type */ > > if (zero_size_allowed && access_size == 0 && > > register_is_null(reg)) > > return 0; > > Daniel, makes a fair point here. Alexei, what do you think? > > I wonder if some "future" signature verification would need even more > / different arguments so a unified bpf_verify_signature might get more > complex / not easy to extend. You mean a pkcs7 specific helper for now? Makes sense.
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index f4009dbdf62d..97521857e44a 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. + * + * long bpf_verify_signature(u8 *data, u32 datalen, u8 *sig, u32 siglen, u32 info) + * Description + * Verify a signature of length *siglen* against the supplied data + * with length *datalen*. *info* contains the keyring identifier + * (low 16 bits) and the signature type (high 16 bits). The keyring + * identifier 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); 0xffff for + * the session keyring (for testing purposes). + * Return + * 0 on success, a negative value on error. */ #define __BPF_FUNC_MAPPER(FN) \ FN(unspec), \ @@ -5455,6 +5471,7 @@ union bpf_attr { FN(dynptr_read), \ FN(dynptr_write), \ FN(dynptr_data), \ + FN(verify_signature), \ /* */ /* 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..20bd850ea3ee 100644 --- a/kernel/bpf/bpf_lsm.c +++ b/kernel/bpf/bpf_lsm.c @@ -16,6 +16,8 @@ #include <linux/bpf_local_storage.h> #include <linux/btf_ids.h> #include <linux/ima.h> +#include <linux/verification.h> +#include <linux/module_signature.h> /* For every LSM hook that allows attachment of BPF programs, declare a nop * function where a BPF program can be attached. @@ -132,6 +134,46 @@ static const struct bpf_func_proto bpf_get_attach_cookie_proto = { .arg1_type = ARG_PTR_TO_CTX, }; +#ifdef CONFIG_SYSTEM_DATA_VERIFICATION +BPF_CALL_5(bpf_verify_signature, u8 *, data, u32, datalen, u8 *, sig, + u32, siglen, u32, info) +{ + unsigned long keyring_id = info & U16_MAX; + enum pkey_id_type id_type = info >> 16; + const struct cred *cred = current_cred(); + struct key *keyring; + + if (keyring_id > (unsigned long)VERIFY_USE_PLATFORM_KEYRING && + keyring_id != U16_MAX) + return -EINVAL; + + keyring = (keyring_id == U16_MAX) ? + cred->session_keyring : (struct key *)keyring_id; + + switch (id_type) { + case PKEY_ID_PKCS7: + return verify_pkcs7_signature(data, datalen, sig, siglen, + keyring, + VERIFYING_UNSPECIFIED_SIGNATURE, + NULL, NULL); + default: + return -EOPNOTSUPP; + } +} + +static const struct bpf_func_proto bpf_verify_signature_proto = { + .func = bpf_verify_signature, + .gpl_only = false, + .ret_type = RET_INTEGER, + .arg1_type = ARG_PTR_TO_MEM, + .arg2_type = ARG_CONST_SIZE_OR_ZERO, + .arg3_type = ARG_PTR_TO_MEM, + .arg4_type = ARG_CONST_SIZE_OR_ZERO, + .arg5_type = ARG_ANYTHING, + .allowed = bpf_ima_inode_hash_allowed, +}; +#endif + static const struct bpf_func_proto * bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) { @@ -158,6 +200,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_SYSTEM_DATA_VERIFICATION + case BPF_FUNC_verify_signature: + return prog->aux->sleepable ? &bpf_verify_signature_proto : NULL; +#endif default: return tracing_prog_func_proto(func_id, prog); } diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index f4009dbdf62d..97521857e44a 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. + * + * long bpf_verify_signature(u8 *data, u32 datalen, u8 *sig, u32 siglen, u32 info) + * Description + * Verify a signature of length *siglen* against the supplied data + * with length *datalen*. *info* contains the keyring identifier + * (low 16 bits) and the signature type (high 16 bits). The keyring + * identifier 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); 0xffff for + * the session keyring (for testing purposes). + * Return + * 0 on success, a negative value on error. */ #define __BPF_FUNC_MAPPER(FN) \ FN(unspec), \ @@ -5455,6 +5471,7 @@ union bpf_attr { FN(dynptr_read), \ FN(dynptr_write), \ FN(dynptr_data), \ + FN(verify_signature), \ /* */ /* integer value in 'imm' field of BPF_CALL instruction selects which helper
Add the bpf_verify_signature() helper, to give eBPF security modules the ability to check the validity of a signature against supplied data, by using system-provided keys as trust anchor. The new helper makes it possible to enforce mandatory policies, as eBPF programs might be allowed to make security decisions only based on data sources the system administrator approves. The caller should specify the identifier of the keyring containing the keys for signature verification: 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); 0xffff for the session keyring (for testing purposes). The caller should also specify the type of signature. Currently only PKCS#7 is supported. Since the maximum number of parameters of an eBPF helper is 5, the keyring and signature types share one (keyring ID: low 16 bits, signature type: high 16 bits). Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> Reported-by: kernel test robot <lkp@intel.com> (cast warning) --- include/uapi/linux/bpf.h | 17 +++++++++++++ kernel/bpf/bpf_lsm.c | 46 ++++++++++++++++++++++++++++++++++ tools/include/uapi/linux/bpf.h | 17 +++++++++++++ 3 files changed, 80 insertions(+)