From: Roberto Sassu <roberto.sassu@huawei.com>
To: KP Singh <kpsingh@kernel.org>, Daniel Borkmann <daniel@iogearbox.net>
Cc: "ast@kernel.org" <ast@kernel.org>,
"andrii@kernel.org" <andrii@kernel.org>,
"bpf@vger.kernel.org" <bpf@vger.kernel.org>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
"linux-kselftest@vger.kernel.org"
<linux-kselftest@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"john.fastabend@gmail.com" <john.fastabend@gmail.com>
Subject: RE: [PATCH v2 1/3] bpf: Add bpf_verify_pkcs7_signature() helper
Date: Wed, 8 Jun 2022 15:13:40 +0000 [thread overview]
Message-ID: <48b67de1c99f4b9f97a12016e6e99081@huawei.com> (raw)
In-Reply-To: <CACYkzJ4VU+JQzsCZHgeAY9Aej5W_k7bJFSeDP93Nq=uM_v7c8Q@mail.gmail.com>
> From: KP Singh [mailto:kpsingh@kernel.org]
> Sent: Wednesday, June 8, 2022 4:45 PM
> On Wed, Jun 8, 2022 at 4:43 PM Daniel Borkmann <daniel@iogearbox.net>
> 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@huawei.com>
> > > ---
> > > 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
> > >
> >
next prev parent reply other threads:[~2022-06-08 15:21 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-08 11:12 [PATCH v2 0/3] bpf: Add bpf_verify_pkcs7_signature() helper Roberto Sassu
2022-06-08 11:12 ` [PATCH v2 1/3] " Roberto Sassu
2022-06-08 14:43 ` Daniel Borkmann
2022-06-08 14:44 ` KP Singh
2022-06-08 15:13 ` Roberto Sassu [this message]
2022-06-08 15:09 ` Roberto Sassu
2022-06-08 14:48 ` kernel test robot
2022-06-08 11:12 ` [PATCH v2 2/3] selftests/bpf: Add test_progs opts for sign-file and kernel priv key + cert Roberto Sassu
2022-06-09 0:12 ` Alexei Starovoitov
2022-06-09 9:00 ` Roberto Sassu
2022-06-09 15:38 ` Alexei Starovoitov
2022-06-10 12:10 ` Roberto Sassu
2022-06-08 11:12 ` [PATCH v2 3/3] selftests/bpf: Add test for bpf_verify_pkcs7_signature() helper Roberto Sassu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=48b67de1c99f4b9f97a12016e6e99081@huawei.com \
--to=roberto.sassu@huawei.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=john.fastabend@gmail.com \
--cc=kpsingh@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).