diff mbox series

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

Message ID 20220608111221.373833-2-roberto.sassu@huawei.com
State New
Headers show
Series bpf: Add bpf_verify_pkcs7_signature() helper | expand

Commit Message

Roberto Sassu June 8, 2022, 11:12 a.m. UTC
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.

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(+)

Comments

Daniel Borkmann June 8, 2022, 2:43 p.m. UTC | #1
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.
> 
> 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
>
KP Singh June 8, 2022, 2:44 p.m. UTC | #2
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.

> >
> > 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
> >
>
kernel test robot June 8, 2022, 2:48 p.m. UTC | #3
Hi Roberto,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on bpf-next/master]
[also build test WARNING on bpf/master]
[cannot apply to horms-ipvs/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Roberto-Sassu/bpf-Add-bpf_verify_pkcs7_signature-helper/20220608-192110
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
config: powerpc-allmodconfig (https://download.01.org/0day-ci/archive/20220608/202206082219.09oAvCwe-lkp@intel.com/config)
compiler: powerpc-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/223914a2278b692d4120315d2fc7a29e3b89512a
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Roberto-Sassu/bpf-Add-bpf_verify_pkcs7_signature-helper/20220608-192110
        git checkout 223914a2278b692d4120315d2fc7a29e3b89512a
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=powerpc SHELL=/bin/bash kernel/bpf/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   kernel/bpf/bpf_lsm.c: In function '____bpf_verify_pkcs7_signature':
>> kernel/bpf/bpf_lsm.c:146:38: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
     146 |                                      (struct key *)keyring,
         |                                      ^


vim +146 kernel/bpf/bpf_lsm.c

   135	
   136	BPF_CALL_5(bpf_verify_pkcs7_signature, u8 *, data, u32, datalen, u8 *, sig,
   137		   u32, siglen, u64, keyring)
   138	{
   139		int ret = -EOPNOTSUPP;
   140	
   141	#ifdef CONFIG_SYSTEM_DATA_VERIFICATION
   142		if (keyring > (unsigned long)VERIFY_USE_PLATFORM_KEYRING)
   143			return -EINVAL;
   144	
   145		ret = verify_pkcs7_signature(data, datalen, sig, siglen,
 > 146					     (struct key *)keyring,
   147					     VERIFYING_UNSPECIFIED_SIGNATURE, NULL,
   148					     NULL);
   149	#endif
   150		return ret;
   151	}
   152
Roberto Sassu June 8, 2022, 3:09 p.m. UTC | #4
> From: Daniel Borkmann [mailto:daniel@iogearbox.net]
> Sent: Wednesday, June 8, 2022 4:43 PM
> 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.
> >
> > 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.	

Hi Daniel

sure, will do.

> > + *	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 ...

Ok.

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

Ok.

I'm currently fixing the test. The challenge is that the module_signature
structure might not be defined. The way I found to fix it is to include
stdlib.h and linux/bpf.h instead of vmlinux.h, and always define the
structure.

Will also skip the test if CONFIG_MODULE_SIG is not defined.

About the CI, I noticed that the kernel configuration file is in the travis
directory. I modified tools/selftests/bpf/config to update the dependencies
for the new helper.

Maybe we should merge both configs at the time the kernel is built?

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Yang Xi, Li He

> >   	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
> >
Roberto Sassu June 8, 2022, 3:13 p.m. UTC | #5
> 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
> > >
> >
diff mbox series

Patch

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*.
+ *	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;
+}
+
+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;
 	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