Messages in this thread Patch in this message | | | Subject | Re: [PATCH v3 1/2] bpf: Add bpf_verify_signature() helper | From | Daniel Borkmann <> | Date | Fri, 10 Jun 2022 17:14:27 +0200 |
| |
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); >>> }
| |