Re: [PATCH v3 01/14] integrity: Introduce a Linux keyring for the Machine Owner Key (MOK)

[Mimi Zohar]

On Thu, 2021-08-12 at 16:36 -0600, Eric Snowberg wrote:
> > On Aug 12, 2021, at 3:31 PM, Mimi Zohar <zohar@xxxxxxxxxxxxx> wrote:
> > 
> > On Wed, 2021-08-11 at 22:18 -0400, Eric Snowberg wrote:
> >> Many UEFI Linux distributions boot using shim.  The UEFI shim provides
> >> what is called Machine Owner Keys (MOK). Shim uses both the UEFI Secure
> >> Boot DB and MOK keys to validate the next step in the boot chain.  The
> >> MOK facility can be used to import user generated keys.  These keys can
> >> be used to sign an end-users development kernel build.  When Linux
> >> boots, both UEFI Secure Boot DB and MOK keys get loaded in the Linux
> >> .platform keyring.
> >> 
> >> Add a new Linux keyring called .mok.  This keyring shall contain just
> >> MOK keys and not the remaining keys in the platform keyring. This new
> >> .mok keyring will be used in follow on patches.  Unlike keys in the
> >> platform keyring, keys contained in the .mok keyring will be trusted
> >> within the kernel if the end-user has chosen to do so.
> >> 
> >> Signed-off-by: Eric Snowberg <eric.snowberg@xxxxxxxxxx>
> >> ---
> >> v1: Initial version
> >> v2: Removed destory keyring code
> >> v3: Unmodified from v2
> >> ---
> >> security/integrity/Makefile                   |  3 ++-
> >> security/integrity/digsig.c                   |  1 +
> >> security/integrity/integrity.h                |  3 ++-
> >> .../integrity/platform_certs/mok_keyring.c    | 21 +++++++++++++++++++
> >> 4 files changed, 26 insertions(+), 2 deletions(-)
> >> create mode 100644 security/integrity/platform_certs/mok_keyring.c
> >> 
> >> diff --git a/security/integrity/Makefile b/security/integrity/Makefile
> >> index 7ee39d66cf16..8e2e98cba1f6 100644
> >> --- a/security/integrity/Makefile
> >> +++ b/security/integrity/Makefile
> >> @@ -9,7 +9,8 @@ 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/mok_keyring.o
> >> integrity-$(CONFIG_LOAD_UEFI_KEYS) += platform_certs/efi_parser.o \
> >> 				      platform_certs/load_uefi.o \
> >> 				      platform_certs/keyring_handler.o
> >> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> >> index 3b06a01bd0fd..e07334504ef1 100644
> >> --- a/security/integrity/digsig.c
> >> +++ b/security/integrity/digsig.c
> >> @@ -30,6 +30,7 @@ static const char * const keyring_name[INTEGRITY_KEYRING_MAX] = {
> >> 	".ima",
> >> #endif
> >> 	".platform",
> >> +	".mok",
> >> };
> >> 
> >> #ifdef CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
> >> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> >> index 547425c20e11..e0e17ccba2e6 100644
> >> --- a/security/integrity/integrity.h
> >> +++ b/security/integrity/integrity.h
> >> @@ -151,7 +151,8 @@ int integrity_kernel_read(struct file *file, loff_t offset,
> >> #define INTEGRITY_KEYRING_EVM		0
> >> #define INTEGRITY_KEYRING_IMA		1
> >> #define INTEGRITY_KEYRING_PLATFORM	2
> >> -#define INTEGRITY_KEYRING_MAX		3
> >> +#define INTEGRITY_KEYRING_MOK		3
> >> +#define INTEGRITY_KEYRING_MAX		4
> >> 
> >> extern struct dentry *integrity_dir;
> >> 
> >> diff --git a/security/integrity/platform_certs/mok_keyring.c b/security/integrity/platform_certs/mok_keyring.c
> >> new file mode 100644
> >> index 000000000000..b1ee45b77731
> >> --- /dev/null
> >> +++ b/security/integrity/platform_certs/mok_keyring.c
> >> @@ -0,0 +1,21 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * MOK keyring routines.
> >> + *
> >> + * Copyright (c) 2021, Oracle and/or its affiliates.
> >> + */
> >> +
> >> +#include "../integrity.h"
> >> +
> >> +static __init int mok_keyring_init(void)
> >> +{
> >> +	int rc;
> >> +
> >> +	rc = integrity_init_keyring(INTEGRITY_KEYRING_MOK);
> >> +	if (rc)
> >> +		return rc;
> >> +
> >> +	pr_notice("MOK Keyring initialized\n");
> >> +	return 0;
> >> +}
> >> +device_initcall(mok_keyring_init);
> > 
> > The ordering of the patches in this patch set is not quite
> > right.  
> 
> I will work on reordering the patches in the next round.
> 
> > Please first introduce the new keyring with the new Kconfig,
> > new restriction, and loading the keys onto the new keyring.  Introduce
> > the builitin_secondary_and_ca_trusted restriction and linking the new
> > keyring to the secondary keyring.  Only after everything is in place,
> > define and use the UEFI mok variable(s).
> > 
> > Originally, I asked you to "Separate each **logical change** into a
> > separate patch."  After re-ordering the patches, see if merging some of
> > them together now makes sense.
> 
> I’ll see if merging some of them together makes sense.
> 
> With the new Kconfig option, should the default be 'y' or ’n' when the secondary 
> is defined?  Thanks.

It definitely needs to be opt in.  Please make it 'n'.

Mimi