| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <8b8678505d4fe299a2f3a82133d81f41b2c527b7.camel@kernel.org>
Date: Thu, 16 Sep 2021 17:46:24 +0300
From: Jarkko Sakkinen <jarkko@...nel.org>
To: Nayna Jain <nayna@...ux.ibm.com>, linux-integrity@...r.kernel.org,
keyrings@...r.kernel.org
Cc: dhowells@...hat.com, zohar@...ux.ibm.com,
seth.forshee@...onical.com, linux-security-module@...r.kernel.org,
linux-kernel@...r.kernel.org, kernel test robot <lkp@...el.com>
Subject: Re: [PATCH v2] integrity: support including firmware ".platform"
keys at build time
On Thu, 2021-09-16 at 08:57 -0400, Nayna Jain wrote:
> Some firmware support secureboot by embedding static keys to verify the
> Linux kernel during boot. However, these firmware do not expose an
> interface for the kernel to load firmware keys onto ".platform" keyring.
> This would prevent kernel signature verification on kexec. For those
> environments, allow firmware keys to be compiled into the kernel and
> loaded onto the ".platform" keyring.
"allow" means absolutely nothing. Just tell what your patch does,
and approach taken. Already the patch description should roughly
give idea what and why of code changes. There's nothing here.
>
> Reported-by: kernel test robot <lkp@...el.com>
I don't get this reported-by here.
> Signed-off-by: Nayna Jain <nayna@...ux.ibm.com>
> ---
>
> v2:
> * Fixed the error reported by kernel test robot
> * Updated patch description based on Jarkko's feedback.
>
> certs/Makefile | 3 ++-
> certs/blacklist.c | 1 -
> certs/common.c | 2 +-
> certs/common.h | 9 -------
> certs/system_keyring.c | 1 -
> include/keys/system_keyring.h | 3 +++
> security/integrity/Kconfig | 10 +++++++
> security/integrity/Makefile | 17 +++++++++++-
> security/integrity/digsig.c | 2 +-
> security/integrity/integrity.h | 6 +++++
> .../integrity/platform_certs/platform_cert.S | 23 ++++++++++++++++
> .../platform_certs/platform_keyring.c | 26 +++++++++++++++++++
> 12 files changed, 88 insertions(+), 15 deletions(-)
> delete mode 100644 certs/common.h
> create mode 100644 security/integrity/platform_certs/platform_cert.S
>
> diff --git a/certs/Makefile b/certs/Makefile
> index 279433783b10..64ee37f38b85 100644
> --- a/certs/Makefile
> +++ b/certs/Makefile
> @@ -3,7 +3,8 @@
> # Makefile for the linux kernel signature checking certificates.
> #
>
> -obj-$(CONFIG_SYSTEM_TRUSTED_KEYRING) += system_keyring.o system_certificates.o common.o
> +obj-$(CONFIG_KEYS) += common.o
> +obj-$(CONFIG_SYSTEM_TRUSTED_KEYRING) += system_keyring.o system_certificates.o
> obj-$(CONFIG_SYSTEM_BLACKLIST_KEYRING) += blacklist.o common.o
> obj-$(CONFIG_SYSTEM_REVOCATION_LIST) += revocation_certificates.o
> ifneq ($(CONFIG_SYSTEM_BLACKLIST_HASH_LIST),"")
> diff --git a/certs/blacklist.c b/certs/blacklist.c
> index c9a435b15af4..b95e9b19c42f 100644
> --- a/certs/blacklist.c
> +++ b/certs/blacklist.c
> @@ -17,7 +17,6 @@
> #include <linux/uidgid.h>
> #include <keys/system_keyring.h>
> #include "blacklist.h"
> -#include "common.h"
>
> static struct key *blacklist_keyring;
>
> diff --git a/certs/common.c b/certs/common.c
> index 16a220887a53..41f763415a00 100644
> --- a/certs/common.c
> +++ b/certs/common.c
> @@ -2,7 +2,7 @@
>
> #include <linux/kernel.h>
> #include <linux/key.h>
> -#include "common.h"
Why this include is removed?
You should include to your commit message *also* the approach
you are taking. If you export a function, you should mention
it explicitly.
> +#include <keys/system_keyring.h>
>
> int load_certificate_list(const u8 cert_list[],
> const unsigned long list_size,
> diff --git a/certs/common.h b/certs/common.h
> deleted file mode 100644
> index abdb5795936b..000000000000
> --- a/certs/common.h
> +++ /dev/null
> @@ -1,9 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-or-later */
> -
> -#ifndef _CERT_COMMON_H
> -#define _CERT_COMMON_H
> -
> -int load_certificate_list(const u8 cert_list[], const unsigned long list_size,
> - const struct key *keyring);
> -
> -#endif
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index 692365dee2bd..d130d5a96e09 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -16,7 +16,6 @@
> #include <keys/asymmetric-type.h>
> #include <keys/system_keyring.h>
> #include <crypto/pkcs7.h>
> -#include "common.h"
>
> static struct key *builtin_trusted_keys;
> #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
> diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
> index 6acd3cf13a18..842d770b2a46 100644
> --- a/include/keys/system_keyring.h
> +++ b/include/keys/system_keyring.h
> @@ -10,6 +10,9 @@
>
> #include <linux/key.h>
>
> +extern int load_certificate_list(const u8 cert_list[],
> + const unsigned long list_size,
> + const struct key *keyring);
Remove "extern". It's not needed for functions.
> #ifdef CONFIG_SYSTEM_TRUSTED_KEYRING
>
> extern int restrict_link_by_builtin_trusted(struct key *keyring,
> diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig
> index 71f0177e8716..b2009b792882 100644
> --- a/security/integrity/Kconfig
> +++ b/security/integrity/Kconfig
> @@ -62,6 +62,16 @@ config INTEGRITY_PLATFORM_KEYRING
> provided by the platform for verifying the kexec'ed kerned image
> and, possibly, the initramfs signature.
>
> +config INTEGRITY_PLATFORM_BUILTIN_KEYS
> + string "Builtin X.509 keys for .platform keyring"
> + depends on KEYS
> + depends on ASYMMETRIC_KEY_TYPE
> + depends on INTEGRITY_PLATFORM_KEYRING
> + help
> + If set, this option should be the filename of a PEM-formatted file
> + containing X.509 certificates to be loaded onto the ".platform"
> + keyring.
> +
> config LOAD_UEFI_KEYS
> depends on INTEGRITY_PLATFORM_KEYRING
> depends on EFI
> diff --git a/security/integrity/Makefile b/security/integrity/Makefile
> index 7ee39d66cf16..a45f083589b8 100644
> --- a/security/integrity/Makefile
> +++ b/security/integrity/Makefile
> @@ -3,13 +3,18 @@
> # Makefile for caching inode integrity data (iint)
> #
>
> +quiet_cmd_extract_certs = EXTRACT_CERTS $(patsubst "%",%,$(2))
> + cmd_extract_certs = scripts/extract-cert $(2) $@
> +$(eval $(call config_filename,INTEGRITY_PLATFORM_BUILTIN_KEYS))
> +
> obj-$(CONFIG_INTEGRITY) += integrity.o
>
> integrity-y := iint.o
> integrity-$(CONFIG_INTEGRITY_AUDIT) += integrity_audit.o
> integrity-$(CONFIG_INTEGRITY_SIGNATURE) += digsig.o
> integrity-$(CONFIG_INTEGRITY_ASYMMETRIC_KEYS) += digsig_asymmetric.o
> -integrity-$(CONFIG_INTEGRITY_PLATFORM_KEYRING) += platform_certs/platform_keyring.o
> +integrity-$(CONFIG_INTEGRITY_PLATFORM_KEYRING) += platform_certs/platform_keyring.o \
> + platform_certs/platform_cert.o
> integrity-$(CONFIG_LOAD_UEFI_KEYS) += platform_certs/efi_parser.o \
> platform_certs/load_uefi.o \
> platform_certs/keyring_handler.o
> @@ -19,3 +24,13 @@ integrity-$(CONFIG_LOAD_PPC_KEYS) += platform_certs/efi_parser.o \
> platform_certs/keyring_handler.o
> obj-$(CONFIG_IMA) += ima/
> obj-$(CONFIG_EVM) += evm/
> +
> +
> +$(obj)/platform_certs/platform_cert.o: $(obj)/platform_certs/platform_certificate_list
> +
> +targets += platform_certificate_list
> +
> +$(obj)/platform_certs/platform_certificate_list: scripts/extract-cert $(INTEGRITY_PLATFORM_BUILTIN_KEYS_FILENAME) FORCE
> + $(call if_changed,extract_certs,$(CONFIG_INTEGRITY_PLATFORM_BUILTIN_KEYS))
> +
> +clean-files := platform_certs/platform_certificate_list
> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> index 3b06a01bd0fd..0ea40ed8dfcb 100644
> --- a/security/integrity/digsig.c
> +++ b/security/integrity/digsig.c
> @@ -38,7 +38,7 @@ static const char * const keyring_name[INTEGRITY_KEYRING_MAX] = {
> #define restrict_link_to_ima restrict_link_by_builtin_trusted
> #endif
>
> -static struct key *integrity_keyring_from_id(const unsigned int id)
> +struct key *integrity_keyring_from_id(const unsigned int id)
> {
> if (id >= INTEGRITY_KEYRING_MAX)
> return ERR_PTR(-EINVAL);
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index 547425c20e11..feb84e1b1105 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -167,6 +167,7 @@ int __init integrity_init_keyring(const unsigned int id);
> int __init integrity_load_x509(const unsigned int id, const char *path);
> int __init integrity_load_cert(const unsigned int id, const char *source,
> const void *data, size_t len, key_perm_t perm);
> +struct key *integrity_keyring_from_id(const unsigned int id);
> #else
>
> static inline int integrity_digsig_verify(const unsigned int id,
> @@ -194,6 +195,11 @@ static inline int __init integrity_load_cert(const unsigned int id,
> {
> return 0;
> }
> +
> +static inline struct key *integrity_keyring_from_id(const unsigned int id)
> +{
> + return ERR_PTR(-EOPNOTSUPP);
> +}
> #endif /* CONFIG_INTEGRITY_SIGNATURE */
>
> #ifdef CONFIG_INTEGRITY_ASYMMETRIC_KEYS
> diff --git a/security/integrity/platform_certs/platform_cert.S b/security/integrity/platform_certs/platform_cert.S
> new file mode 100644
> index 000000000000..20bccce5dc5a
> --- /dev/null
> +++ b/security/integrity/platform_certs/platform_cert.S
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#include <linux/export.h>
> +#include <linux/init.h>
> +
> + __INITRODATA
> +
> + .align 8
> +#ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
> + .globl platform_certificate_list
> +platform_certificate_list:
> +__cert_list_start:
> + .incbin "security/integrity/platform_certs/platform_certificate_list"
> +__cert_list_end:
> +#endif
> +
> + .align 8
> + .globl platform_certificate_list_size
> +platform_certificate_list_size:
> +#ifdef CONFIG_64BIT
> + .quad __cert_list_end - __cert_list_start
> +#else
> + .long __cert_list_end - __cert_list_start
> +#endif
> diff --git a/security/integrity/platform_certs/platform_keyring.c b/security/integrity/platform_certs/platform_keyring.c
> index bcafd7387729..17535050d08d 100644
> --- a/security/integrity/platform_certs/platform_keyring.c
> +++ b/security/integrity/platform_certs/platform_keyring.c
> @@ -12,8 +12,12 @@
> #include <linux/cred.h>
> #include <linux/err.h>
> #include <linux/slab.h>
> +#include <keys/system_keyring.h>
> #include "../integrity.h"
>
> +extern __initconst const u8 platform_certificate_list[];
> +extern __initconst const unsigned long platform_certificate_list_size;
> +
> /**
> * add_to_platform_keyring - Add to platform keyring without validation.
> * @source: Source of key
> @@ -37,6 +41,28 @@ void __init add_to_platform_keyring(const char *source, const void *data,
> pr_info("Error adding keys to platform keyring %s\n", source);
> }
>
> +static __init int load_builtin_platform_cert(void)
> +{
> + const u8 *p;
> + unsigned long size;
> + int rc;
> + struct key *keyring;
> +
> + p = platform_certificate_list;
> + size = platform_certificate_list_size;
> +
> + keyring = integrity_keyring_from_id(INTEGRITY_KEYRING_PLATFORM);
> + if (IS_ERR(keyring))
> + return PTR_ERR(keyring);
> +
> + rc = load_certificate_list(p, size, keyring);
> + if (rc)
> + pr_info("Error adding keys to platform keyring %d\n", rc);
> +
> + return rc;
> +}
> +late_initcall(load_builtin_platform_cert);
> +
> /*
> * Create the trusted keyrings.
> */
/Jarkko
Powered by blists - more mailing lists