Re: [PATCH v2 1/3] bpf: Add bpf_verify_pkcs7_signature() helper

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

> >
> > 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
> >
>



[Index of Archives]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux