> From: KP Singh [mailto:kpsingh@xxxxxxxxxx] > Sent: Wednesday, June 8, 2022 4:45 PM > On Wed, Jun 8, 2022 at 4:43 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> > wrote: > > > > On 6/8/22 1:12 PM, Roberto Sassu wrote: > > > Add the bpf_verify_pkcs7_signature() helper, to give the ability to eBPF > > > security modules to check the validity of a PKCS#7 signature against > > > supplied data. > > 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? > > bpf_verify_signature and a type SIG_PKCS7 or something. Hi KP makes sense. Otherwise, we have to add a new helper every time a new signature verification function is introduced (for example one would be needed for PGP). I will reuse enum pkey_id_type in module_signature.h Thanks Roberto HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063 Managing Director: Li Peng, Yang Xi, Li He > > > Use the 'keyring' parameter to select the keyring containing the > > > verification key: 0 for the primary keyring, 1 for the primary and > > > secondary keyrings, 2 for the platform keyring. > > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx> > > > --- > > > include/uapi/linux/bpf.h | 8 ++++++++ > > > kernel/bpf/bpf_lsm.c | 32 ++++++++++++++++++++++++++++++++ > > > tools/include/uapi/linux/bpf.h | 8 ++++++++ > > > 3 files changed, 48 insertions(+) > > > > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > > > index f4009dbdf62d..40d0fc0d9493 100644 > > > --- a/include/uapi/linux/bpf.h > > > +++ b/include/uapi/linux/bpf.h > > > @@ -5249,6 +5249,13 @@ 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_pkcs7_signature(u8 *data, u32 datalen, u8 *sig, u32 > siglen, u64 keyring) > > > + * Description > > > + * Verify the PKCS#7 *sig* with length *siglen*, on *data* with > > > + * length *datalen*, with key in *keyring*. > > > > Could you also add a description for users about the keyring argument and > guidance on when > > they should use which in their programs? Above is a bit too terse, imho. > > > > > + * Return > > > + * 0 on success, a negative value on error. > > > */ > > > #define __BPF_FUNC_MAPPER(FN) \ > > > FN(unspec), \ > > > @@ -5455,6 +5462,7 @@ union bpf_attr { > > > FN(dynptr_read), \ > > > FN(dynptr_write), \ > > > FN(dynptr_data), \ > > > + FN(verify_pkcs7_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..1cda43cb541a 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,35 @@ static const struct bpf_func_proto > bpf_get_attach_cookie_proto = { > > > .arg1_type = ARG_PTR_TO_CTX, > > > }; > > > > > > +BPF_CALL_5(bpf_verify_pkcs7_signature, u8 *, data, u32, datalen, u8 *, sig, > > > + u32, siglen, u64, keyring) > > > +{ > > > + int ret = -EOPNOTSUPP; > > > + > > > +#ifdef CONFIG_SYSTEM_DATA_VERIFICATION > > > + if (keyring > (unsigned long)VERIFY_USE_PLATFORM_KEYRING) > > > + return -EINVAL; > > > + > > > + ret = verify_pkcs7_signature(data, datalen, sig, siglen, > > > + (struct key *)keyring, > > > + VERIFYING_UNSPECIFIED_SIGNATURE, NULL, > > > + NULL); > > > +#endif > > > + return ret; > > > +} > > > > Looks great! One small nit, I would move all of the BPF_CALL and _proto under > the > > #ifdef CONFIG_SYSTEM_DATA_VERIFICATION ... > > > > > +static const struct bpf_func_proto bpf_verify_pkcs7_signature_proto = { > > > + .func = bpf_verify_pkcs7_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, > > > +}; > > > + > > > static const struct bpf_func_proto * > > > bpf_lsm_func_proto(enum bpf_func_id func_id, const struct bpf_prog > *prog) > > > { > > > @@ -158,6 +188,8 @@ 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; > > > + case BPF_FUNC_verify_pkcs7_signature: > > > + return prog->aux->sleepable ? &bpf_verify_pkcs7_signature_proto : > NULL; > > > > ... same here: > > > > #ifdef CONFIG_SYSTEM_DATA_VERIFICATION > > case BPF_FUNC_verify_pkcs7_signature: > > return prog->aux->sleepable ? &bpf_verify_pkcs7_signature_proto : > NULL; > > #endif > > > > So that bpftool or other feature probes can check for its availability. > Otherwise, apps have > > a hard time checking whether bpf_verify_pkcs7_signature() helper is available > for use or not. > > > > > 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..40d0fc0d9493 100644 > > > --- a/tools/include/uapi/linux/bpf.h > > > +++ b/tools/include/uapi/linux/bpf.h > > > @@ -5249,6 +5249,13 @@ 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_pkcs7_signature(u8 *data, u32 datalen, u8 *sig, u32 > siglen, u64 keyring) > > > + * Description > > > + * Verify the PKCS#7 *sig* with length *siglen*, on *data* with > > > + * length *datalen*, with key in *keyring*. > > > + * Return > > > + * 0 on success, a negative value on error. > > > */ > > > #define __BPF_FUNC_MAPPER(FN) \ > > > FN(unspec), \ > > > @@ -5455,6 +5462,7 @@ union bpf_attr { > > > FN(dynptr_read), \ > > > FN(dynptr_write), \ > > > FN(dynptr_data), \ > > > + FN(verify_pkcs7_signature), \ > > > /* */ > > > > > > /* integer value in 'imm' field of BPF_CALL instruction selects which helper > > > > >
