On Fri, Jun 10, 2022 at 5:14 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote: > > On 6/10/22 4:59 PM, Roberto Sassu wrote: > >> From: Daniel Borkmann [mailto:daniel@xxxxxxxxxxxxx] > >> 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@xxxxxxxxxx> > >>> Reported-by: kernel test robot <lkp@xxxxxxxxx> (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); > >>> } >
