[PATCH v5 00/12] Enroll kernel keys thru MOK

[PATCH v5 00/12] Enroll kernel keys thru MOK

[Eric Snowberg]

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-user development kernel build.  When Linux boots,
pre-boot keys (both UEFI Secure Boot DB and MOK keys) get loaded in the
Linux .platform keyring.  

Currently, pre-boot keys are not trusted within the Linux trust boundary
[1]. These platform keys can only be used for kexec. If an end-user
wants to use their own key within the Linux trust boundary, they must
either compile it into the kernel themselves or use the insert-sys-cert
script. Both options present a problem. Many end-users do not want to
compile their own kernels. With the insert-sys-cert option, there are
missing upstream changes [2].  Also, with the insert-sys-cert option,
the end-user must re-sign their kernel again with their own key, and
then insert that key into the MOK db. Another problem with
insert-sys-cert is that only a single key can be inserted into a
compressed kernel.

Having the ability to insert a key into the Linux trust boundary opens
up various possibilities.  The end-user can use a pre-built kernel and
sign their own kernel modules.  It also opens up the ability for an
end-user to more easily use digital signature based IMA-appraisal.  To
get a key into the ima keyring, it must be signed by a key within the
Linux trust boundary.

Downstream Linux distros try to have a single signed kernel for each
architecture.  Each end-user may use this kernel in entirely different
ways.  Some downstream kernels have chosen to always trust platform keys
within the Linux trust boundary for kernel module signing.  These
kernels have no way of using digital signature base IMA appraisal.

This series introduces a new Linux kernel keyring containing the Machine
Owner Keys (MOK) called .machine. It also adds a new MOK variable to shim.
This variable allows the end-user to decide if they want to trust keys
enrolled in the MOK within the Linux trust boundary.  By default,
nothing changes; MOK keys are not trusted within the Linux kernel.  They
are only trusted after the end-user makes the decision themselves.  The
end-user would set this through mokutil using a new --trust-mok option
[3]. This would work similar to how the kernel uses MOK variables to
enable/disable signature validation as well as use/ignore the db.

When shim boots, it mirrors the new MokTML Boot Services variable to a
new MokListTrustedRT Runtime Services variable and extends PCR14.
MokListTrustedRT is written without EFI_VARIABLE_NON_VOLATILE set,
preventing an end-user from setting it after booting and doing a kexec.

When the kernel boots, if MokListTrustedRT is set and
EFI_VARIABLE_NON_VOLATILE is not set, the MokListRT is loaded into the
machine keyring instead of the platform keyring. Mimi has suggested that
only CA keys be loaded into this keyring. All other certs will load 
into the platform keyring instead.

The machine keyring contains a new keyring permission that only allows CA
keys to be loaded. If the permission fails, the key is later loaded into
the platform keyring.  After all keys are added into the machine keyring,
they are linked to the secondary trusted keyring.  After the link is 
created, keys contained in the machine keyring will automatically be 
searched when searching the secondary trusted keys.

Secure Boot keys will never be trusted.  They will always be loaded into
the platform keyring.  If an end-user wanted to trust one, they would
need to enroll it into the MOK.

I have included links to both the mokutil [3] and shim [4] changes I
have made to support this new functionality.

V2 changes:
- The .mok keyring persists past boot
- Removed the unrestricted move into the secondary keyring
- Removed the keyring move bypass patch
- Added restrictions to allow the .mok to be linked to either the
  builtin or secondary keyrings
- Secondary keyring dependency has been removed

V3 changes:
- Only CA keys contained in the MOKList are loaded, nothing else
- Support for kernels built without the secondary trusted keyring
  has been dropped.

V4 changes:
- Add new Kconfig INTEGRITY_MOK_KEYRING and move all mok keyring
  code behind it
- Changed patch series ordering
- Consolidated a few patches

V5 changes:
- Rename from mok keyring to machine keyring

[1] https://lore.kernel.org/lkml/1556221605.24945.3.camel@HansenPartnership.com/
[2] https://lore.kernel.org/patchwork/cover/902768/
[3] https://github.com/esnowberg/mokutil/tree/0.3.0-mokvars-v2
[4] https://github.com/esnowberg/shim/tree/mokvars-v2

Eric Snowberg (12):
  integrity: Introduce a Linux keyring called machine
  integrity: Do not allow machine keyring updates following init
  KEYS: CA link restriction
  integrity: restrict INTEGRITY_KEYRING_MACHINE to restrict_link_by_ca
  integrity: add new keyring handler for mok keys
  KEYS: add a reference to machine keyring
  KEYS: Introduce link restriction to include builtin, secondary and
    machine keys
  KEYS: integrity: change link restriction to trust the machine keyring
  KEYS: link secondary_trusted_keys to machine trusted keys
  integrity: store reference to machine keyring
  integrity: Trust MOK keys if MokListTrustedRT found
  integrity: Only use machine keyring when uefi_check_trust_mok_keys is
    true

 certs/system_keyring.c                        | 40 ++++++++-
 crypto/asymmetric_keys/restrict.c             | 40 +++++++++
 include/crypto/public_key.h                   |  5 ++
 include/keys/system_keyring.h                 | 14 +++
 security/integrity/Kconfig                    | 11 +++
 security/integrity/Makefile                   |  1 +
 security/integrity/digsig.c                   | 18 +++-
 security/integrity/integrity.h                | 17 +++-
 .../platform_certs/keyring_handler.c          | 17 +++-
 .../platform_certs/keyring_handler.h          |  5 ++
 security/integrity/platform_certs/load_uefi.c |  4 +-
 .../platform_certs/machine_keyring.c          | 85 +++++++++++++++++++
 12 files changed, 249 insertions(+), 8 deletions(-)
 create mode 100644 security/integrity/platform_certs/machine_keyring.c


base-commit: e22ce8eb631bdc47a4a4ea7ecf4e4ba499db4f93
-- 
2.18.4

[PATCH v5 01/12] integrity: Introduce a Linux keyring called machine

[Eric Snowberg]

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 machine.  This keyring shall contain just
MOK CA keys and not the remaining keys in the platform keyring. This new
machine keyring will be used in follow on patches.  Unlike keys in the
platform keyring, keys contained in the machine keyring will be trusted
within the kernel if the end-user has chosen to do so.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v1: Initial version
v2: Removed destory keyring code
v3: Unmodified from v2
v4: Add Kconfig, merged in "integrity: add add_to_mok_keyring" 
v5: Rename to machine keyring
---
 security/integrity/Kconfig                    | 11 +++++
 security/integrity/Makefile                   |  1 +
 security/integrity/digsig.c                   |  1 +
 security/integrity/integrity.h                | 12 +++++-
 .../platform_certs/machine_keyring.c          | 42 +++++++++++++++++++
 5 files changed, 66 insertions(+), 1 deletion(-)
 create mode 100644 security/integrity/platform_certs/machine_keyring.c

diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig
index 71f0177e8716..52193b86768a 100644
--- a/security/integrity/Kconfig
+++ b/security/integrity/Kconfig
@@ -62,6 +62,17 @@ config INTEGRITY_PLATFORM_KEYRING
          provided by the platform for verifying the kexec'ed kerned image
          and, possibly, the initramfs signature.
 
+config INTEGRITY_MACHINE_KEYRING
+	bool "Provide a keyring to which CA Machine Owner Keys may be added"
+	depends on SECONDARY_TRUSTED_KEYRING
+	depends on INTEGRITY_ASYMMETRIC_KEYS
+	depends on SYSTEM_BLACKLIST_KEYRING
+	help
+	 If set, provide a keyring to which CA Machine Owner Keys (MOK) may
+	 be added. This keyring shall contain just CA MOK keys.  Unlike keys
+	 in the platform keyring, keys contained in the .machine keyring will
+	 be trusted within the kernel.
+
 config LOAD_UEFI_KEYS
        depends on INTEGRITY_PLATFORM_KEYRING
        depends on EFI
diff --git a/security/integrity/Makefile b/security/integrity/Makefile
index 7ee39d66cf16..d0ffe37dc1d6 100644
--- a/security/integrity/Makefile
+++ b/security/integrity/Makefile
@@ -10,6 +10,7 @@ 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_MACHINE_KEYRING) += platform_certs/machine_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..8c315be8ad99 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",
+	".machine",
 };
 
 #ifdef CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 547425c20e11..730771eececd 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_MACHINE	3
+#define INTEGRITY_KEYRING_MAX		4
 
 extern struct dentry *integrity_dir;
 
@@ -283,3 +284,12 @@ static inline void __init add_to_platform_keyring(const char *source,
 {
 }
 #endif
+
+#ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
+void __init add_to_machine_keyring(const char *source, const void *data, size_t len);
+#else
+static inline void __init add_to_machine_keyring(const char *source,
+						  const void *data, size_t len)
+{
+}
+#endif
diff --git a/security/integrity/platform_certs/machine_keyring.c b/security/integrity/platform_certs/machine_keyring.c
new file mode 100644
index 000000000000..948ec6c738c8
--- /dev/null
+++ b/security/integrity/platform_certs/machine_keyring.c
@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Machine keyring routines.
+ *
+ * Copyright (c) 2021, Oracle and/or its affiliates.
+ */
+
+#include "../integrity.h"
+
+static __init int machine_keyring_init(void)
+{
+	int rc;
+
+	rc = integrity_init_keyring(INTEGRITY_KEYRING_MACHINE);
+	if (rc)
+		return rc;
+
+	pr_notice("Machine keyring initialized\n");
+	return 0;
+}
+device_initcall(machine_keyring_init);
+
+void __init add_to_machine_keyring(const char *source, const void *data, size_t len)
+{
+	key_perm_t perm;
+	int rc;
+
+	perm = (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW;
+	rc = integrity_load_cert(INTEGRITY_KEYRING_MACHINE, source, data, len, perm);
+
+	/*
+	 * Some MOKList keys may not pass the machine keyring restrictions.
+	 * If the restriction check does not pass and the platform keyring
+	 * is configured, try to add it into that keyring instead.
+	 */
+	if (rc)
+		rc = integrity_load_cert(INTEGRITY_KEYRING_PLATFORM, source,
+					 data, len, perm);
+
+	if (rc)
+		pr_info("Error adding keys to machine keyring %s\n", source);
+}
-- 
2.18.4

Re: [PATCH v5 01/12] integrity: Introduce a Linux keyring called machine

[Jarkko Sakkinen]

On Tue, 2021-09-07 at 12:00 -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 machine.  This keyring shall contain just
> MOK CA keys and not the remaining keys in the platform keyring. This new
> machine keyring will be used in follow on patches.  Unlike keys in the
> platform keyring, keys contained in the machine keyring will be trusted
> within the kernel if the end-user has chosen to do so.
> 
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>

I guess this is somewhat sealed so

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

/Jarkko

Re: [PATCH v5 01/12] integrity: Introduce a Linux keyring called machine

[Mimi Zohar]

On Tue, 2021-09-07 at 12:00 -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 machine.  This keyring shall contain just

^Define

> MOK CA keys and not the remaining keys in the platform keyring. This new
> machine keyring will be used in follow on patches.  Unlike keys in the
> platform keyring, keys contained in the machine keyring will be trusted
> within the kernel if the end-user has chosen to do so.
> 
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> ---
> v1: Initial version
> v2: Removed destory keyring code
> v3: Unmodified from v2
> v4: Add Kconfig, merged in "integrity: add add_to_mok_keyring" 
> v5: Rename to machine keyring
> ---
>  security/integrity/Kconfig                    | 11 +++++
>  security/integrity/Makefile                   |  1 +
>  security/integrity/digsig.c                   |  1 +
>  security/integrity/integrity.h                | 12 +++++-
>  .../platform_certs/machine_keyring.c          | 42 +++++++++++++++++++
>  5 files changed, 66 insertions(+), 1 deletion(-)
>  create mode 100644 security/integrity/platform_certs/machine_keyring.c
> 
> diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig
> index 71f0177e8716..52193b86768a 100644
> --- a/security/integrity/Kconfig
> +++ b/security/integrity/Kconfig
> @@ -62,6 +62,17 @@ config INTEGRITY_PLATFORM_KEYRING
>           provided by the platform for verifying the kexec'ed kerned image
>           and, possibly, the initramfs signature.
>  
> +config INTEGRITY_MACHINE_KEYRING
> +	bool "Provide a keyring to which CA Machine Owner Keys may be added"
> +	depends on SECONDARY_TRUSTED_KEYRING
> +	depends on INTEGRITY_ASYMMETRIC_KEYS
> +	depends on SYSTEM_BLACKLIST_KEYRING
> +	help
> +	 If set, provide a keyring to which CA Machine Owner Keys (MOK) may
> +	 be added. This keyring shall contain just CA MOK keys.  Unlike keys
> +	 in the platform keyring, keys contained in the .machine keyring will
> +	 be trusted within the kernel.

No sense in creating the ".machine" keyring, unless it is possible to
safely load CA certificates on it.  At least for the time being, this
should also be dependent on EFI.

<snip>

> +++ b/security/integrity/platform_certs/machine_keyring.c
> @@ -0,0 +1,42 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Machine keyring routines.
> + *
> + * Copyright (c) 2021, Oracle and/or its affiliates.
> + */
> +
> +#include "../integrity.h"
> +
> +static __init int machine_keyring_init(void)
> +{
> +	int rc;
> +
> +	rc = integrity_init_keyring(INTEGRITY_KEYRING_MACHINE);
> +	if (rc)
> +		return rc;
> +
> +	pr_notice("Machine keyring initialized\n");
> +	return 0;
> +}
> +device_initcall(machine_keyring_init);
> +
> +void __init add_to_machine_keyring(const char *source, const void *data, size_t len)
> +{
> +	key_perm_t perm;
> +	int rc;
> +
> +	perm = (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW;
> +	rc = integrity_load_cert(INTEGRITY_KEYRING_MACHINE, source, data, len, perm);
> +
> +	/*
> +	 * Some MOKList keys may not pass the machine keyring restrictions.
> +	 * If the restriction check does not pass and the platform keyring
> +	 * is configured, try to add it into that keyring instead.
> +	 */
> +	if (rc)

In addition to the comment, also test to see if the ".platform" keyring
is configured.

thanks,

Mimi

> +		rc = integrity_load_cert(INTEGRITY_KEYRING_PLATFORM, source,
> +					 data, len, perm);
> +
> +	if (rc)
> +		pr_info("Error adding keys to machine keyring %s\n", source);
> +}

Re: [PATCH v5 01/12] integrity: Introduce a Linux keyring called machine

[Eric Snowberg]

> On Sep 9, 2021, at 9:19 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> On Tue, 2021-09-07 at 12:00 -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 machine.  This keyring shall contain just
> 
> ^Define

I’ll change this in the next round.

> 
>> MOK CA keys and not the remaining keys in the platform keyring. This new
>> machine keyring will be used in follow on patches.  Unlike keys in the
>> platform keyring, keys contained in the machine keyring will be trusted
>> within the kernel if the end-user has chosen to do so.
>> 
>> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
>> ---
>> v1: Initial version
>> v2: Removed destory keyring code
>> v3: Unmodified from v2
>> v4: Add Kconfig, merged in "integrity: add add_to_mok_keyring" 
>> v5: Rename to machine keyring
>> ---
>> security/integrity/Kconfig                    | 11 +++++
>> security/integrity/Makefile                   |  1 +
>> security/integrity/digsig.c                   |  1 +
>> security/integrity/integrity.h                | 12 +++++-
>> .../platform_certs/machine_keyring.c          | 42 +++++++++++++++++++
>> 5 files changed, 66 insertions(+), 1 deletion(-)
>> create mode 100644 security/integrity/platform_certs/machine_keyring.c
>> 
>> diff --git a/security/integrity/Kconfig b/security/integrity/Kconfig
>> index 71f0177e8716..52193b86768a 100644
>> --- a/security/integrity/Kconfig
>> +++ b/security/integrity/Kconfig
>> @@ -62,6 +62,17 @@ config INTEGRITY_PLATFORM_KEYRING
>>          provided by the platform for verifying the kexec'ed kerned image
>>          and, possibly, the initramfs signature.
>> 
>> +config INTEGRITY_MACHINE_KEYRING
>> +	bool "Provide a keyring to which CA Machine Owner Keys may be added"
>> +	depends on SECONDARY_TRUSTED_KEYRING
>> +	depends on INTEGRITY_ASYMMETRIC_KEYS
>> +	depends on SYSTEM_BLACKLIST_KEYRING
>> +	help
>> +	 If set, provide a keyring to which CA Machine Owner Keys (MOK) may
>> +	 be added. This keyring shall contain just CA MOK keys.  Unlike keys
>> +	 in the platform keyring, keys contained in the .machine keyring will
>> +	 be trusted within the kernel.
> 
> No sense in creating the ".machine" keyring, unless it is possible to
> safely load CA certificates on it.  At least for the time being, this
> should also be dependent on EFI.
> 

Will also add a depends for EFI

>> +++ b/security/integrity/platform_certs/machine_keyring.c
>> @@ -0,0 +1,42 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Machine keyring routines.
>> + *
>> + * Copyright (c) 2021, Oracle and/or its affiliates.
>> + */
>> +
>> +#include "../integrity.h"
>> +
>> +static __init int machine_keyring_init(void)
>> +{
>> +	int rc;
>> +
>> +	rc = integrity_init_keyring(INTEGRITY_KEYRING_MACHINE);
>> +	if (rc)
>> +		return rc;
>> +
>> +	pr_notice("Machine keyring initialized\n");
>> +	return 0;
>> +}
>> +device_initcall(machine_keyring_init);
>> +
>> +void __init add_to_machine_keyring(const char *source, const void *data, size_t len)
>> +{
>> +	key_perm_t perm;
>> +	int rc;
>> +
>> +	perm = (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW;
>> +	rc = integrity_load_cert(INTEGRITY_KEYRING_MACHINE, source, data, len, perm);
>> +
>> +	/*
>> +	 * Some MOKList keys may not pass the machine keyring restrictions.
>> +	 * If the restriction check does not pass and the platform keyring
>> +	 * is configured, try to add it into that keyring instead.
>> +	 */
>> +	if (rc)
> 
> In addition to the comment, also test to see if the ".platform" keyring
> is configured.

and will add this too. Thanks.

[PATCH v5 02/12] integrity: Do not allow machine keyring updates following init

[Eric Snowberg]

The machine keyring is setup during init.  No additional keys should be
allowed to be added afterwards.  Leave the permission as read only.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v2: Initial version
v4: Unmodified from v2
v5: Rename to machine keyring
---
 security/integrity/digsig.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 8c315be8ad99..5a75ac2c4dbe 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -140,7 +140,8 @@ int __init integrity_init_keyring(const unsigned int id)
 		return -ENOMEM;
 
 	restriction->check = restrict_link_to_ima;
-	perm |= KEY_USR_WRITE;
+	if (id != INTEGRITY_KEYRING_MACHINE)
+		perm |= KEY_USR_WRITE;
 
 out:
 	return __integrity_init_keyring(id, perm, restriction);
-- 
2.18.4

Re: [PATCH v5 02/12] integrity: Do not allow machine keyring updates following init

[Jarkko Sakkinen]

On Tue, 2021-09-07 at 12:01 -0400, Eric Snowberg wrote:
> The machine keyring is setup during init.  No additional keys should be
> allowed to be added afterwards.  Leave the permission as read only.
> 
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> ---
> v2: Initial version
> v4: Unmodified from v2
> v5: Rename to machine keyring
> ---
>  security/integrity/digsig.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> index 8c315be8ad99..5a75ac2c4dbe 100644
> --- a/security/integrity/digsig.c
> +++ b/security/integrity/digsig.c
> @@ -140,7 +140,8 @@ int __init integrity_init_keyring(const unsigned int id)
>  		return -ENOMEM;
>  
>  	restriction->check = restrict_link_to_ima;
> -	perm |= KEY_USR_WRITE;

I would add here inline comment to say mostly the same as the commit
message does.

> +	if (id != INTEGRITY_KEYRING_MACHINE)
> +		perm |= KEY_USR_WRITE;
>  
>  out:
>  	return __integrity_init_keyring(id, perm, restriction);

I checked patch 01 but I lost the email somewhere. The keyring definition
looks now sane.

/Jarkko

[PATCH v5 03/12] KEYS: CA link restriction

[Eric Snowberg]

Add a new link restriction.  Restrict the addition of keys in a keyring
based on the key to be added being a CA (self-signed).

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v1: Initial version
v2: Removed secondary keyring references
v3: Removed restrict_link_by_system_trusted_or_ca
    Simplify restrict_link_by_ca - only see if the key is a CA
    Did not add __init in front of restrict_link_by_ca in case
      restriction could be resued in the future
v5: Unmodified from v3
---
 crypto/asymmetric_keys/restrict.c | 40 +++++++++++++++++++++++++++++++
 include/crypto/public_key.h       |  5 ++++
 2 files changed, 45 insertions(+)

diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
index 84cefe3b3585..9ae43d3f862b 100644
--- a/crypto/asymmetric_keys/restrict.c
+++ b/crypto/asymmetric_keys/restrict.c
@@ -108,6 +108,46 @@ int restrict_link_by_signature(struct key *dest_keyring,
 	return ret;
 }
 
+/**
+ * restrict_link_by_ca - Restrict additions to a ring of CA keys
+ * @dest_keyring: Keyring being linked to.
+ * @type: The type of key being added.
+ * @payload: The payload of the new key.
+ * @trusted: Unused.
+ *
+ * Check if the new certificate is a CA. If it is a CA, then mark the new
+ * certificate as being ok to link.
+ *
+ * Returns 0 if the new certificate was accepted, -ENOKEY if we could not find
+ * a matching parent certificate in the trusted list.  -ENOPKG if the signature
+ * uses unsupported crypto, or some other error if there is a matching
+ * certificate  but the signature check cannot be performed.
+ */
+int restrict_link_by_ca(struct key *dest_keyring,
+			const struct key_type *type,
+			const union key_payload *payload,
+			struct key *trust_keyring)
+{
+	const struct public_key_signature *sig;
+	const struct public_key *pkey;
+
+	if (type != &key_type_asymmetric)
+		return -EOPNOTSUPP;
+
+	sig = payload->data[asym_auth];
+	if (!sig)
+		return -ENOPKG;
+
+	if (!sig->auth_ids[0] && !sig->auth_ids[1])
+		return -ENOKEY;
+
+	pkey = payload->data[asym_crypto];
+	if (!pkey)
+		return -ENOPKG;
+
+	return public_key_verify_signature(pkey, sig);
+}
+
 static bool match_either_id(const struct asymmetric_key_ids *pair,
 			    const struct asymmetric_key_id *single)
 {
diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h
index 47accec68cb0..545af1ea57de 100644
--- a/include/crypto/public_key.h
+++ b/include/crypto/public_key.h
@@ -71,6 +71,11 @@ extern int restrict_link_by_key_or_keyring_chain(struct key *trust_keyring,
 						 const union key_payload *payload,
 						 struct key *trusted);
 
+extern int restrict_link_by_ca(struct key *dest_keyring,
+			       const struct key_type *type,
+			       const union key_payload *payload,
+			       struct key *trust_keyring);
+
 extern int query_asymmetric_key(const struct kernel_pkey_params *,
 				struct kernel_pkey_query *);
 
-- 
2.18.4

[PATCH v5 04/12] integrity: restrict INTEGRITY_KEYRING_MACHINE to restrict_link_by_ca

[Eric Snowberg]

Set the restriction check for INTEGRITY_KEYRING_MACHINE keys to
restrict_link_by_ca.  This will only allow CA keys into the machine
keyring.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v1: Initial version
v2: Added !IS_ENABLED(CONFIG_INTEGRITY_TRUSTED_KEYRING check so mok
    keyring gets created even when it isn't enabled
v3: Rename restrict_link_by_system_trusted_or_ca to restrict_link_by_ca
v4: removed unnecessary restriction->check set
v5: Rename to machine keyring
---
 security/integrity/digsig.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 5a75ac2c4dbe..2b75bbbd9e0e 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -132,14 +132,18 @@ int __init integrity_init_keyring(const unsigned int id)
 		goto out;
 	}
 
-	if (!IS_ENABLED(CONFIG_INTEGRITY_TRUSTED_KEYRING))
+	if (!IS_ENABLED(CONFIG_INTEGRITY_TRUSTED_KEYRING) && id != INTEGRITY_KEYRING_MACHINE)
 		return 0;
 
 	restriction = kzalloc(sizeof(struct key_restriction), GFP_KERNEL);
 	if (!restriction)
 		return -ENOMEM;
 
-	restriction->check = restrict_link_to_ima;
+	if (id == INTEGRITY_KEYRING_MACHINE)
+		restriction->check = restrict_link_by_ca;
+	else
+		restriction->check = restrict_link_to_ima;
+
 	if (id != INTEGRITY_KEYRING_MACHINE)
 		perm |= KEY_USR_WRITE;
 
-- 
2.18.4

Re: [PATCH v5 04/12] integrity: restrict INTEGRITY_KEYRING_MACHINE to restrict_link_by_ca

[Jarkko Sakkinen]

On Tue, 2021-09-07 at 12:01 -0400, Eric Snowberg wrote:
> Set the restriction check for INTEGRITY_KEYRING_MACHINE keys to
> restrict_link_by_ca.  This will only allow CA keys into the machine
> keyring.
> 
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> ---
> v1: Initial version
> v2: Added !IS_ENABLED(CONFIG_INTEGRITY_TRUSTED_KEYRING check so mok
>     keyring gets created even when it isn't enabled
> v3: Rename restrict_link_by_system_trusted_or_ca to restrict_link_by_ca
> v4: removed unnecessary restriction->check set
> v5: Rename to machine keyring
> ---
>  security/integrity/digsig.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> index 5a75ac2c4dbe..2b75bbbd9e0e 100644
> --- a/security/integrity/digsig.c
> +++ b/security/integrity/digsig.c
> @@ -132,14 +132,18 @@ int __init integrity_init_keyring(const unsigned int id)
>  		goto out;
>  	}
>  
> -	if (!IS_ENABLED(CONFIG_INTEGRITY_TRUSTED_KEYRING))
> +	if (!IS_ENABLED(CONFIG_INTEGRITY_TRUSTED_KEYRING) && id != INTEGRITY_KEYRING_MACHINE)
>  		return 0;
>  
>  	restriction = kzalloc(sizeof(struct key_restriction), GFP_KERNEL);
>  	if (!restriction)
>  		return -ENOMEM;
>  
> -	restriction->check = restrict_link_to_ima;
> +	if (id == INTEGRITY_KEYRING_MACHINE)
> +		restriction->check = restrict_link_by_ca;
> +	else
> +		restriction->check = restrict_link_to_ima;
> +
>  	if (id != INTEGRITY_KEYRING_MACHINE)
>  		perm |= KEY_USR_WRITE;
>  

03 and 04 look sane. As said, the patches seem to be already in nice shape.

/Jarkko

Re: [PATCH v5 04/12] integrity: restrict INTEGRITY_KEYRING_MACHINE to restrict_link_by_ca

[Mimi Zohar]

On Tue, 2021-09-07 at 12:01 -0400, Eric Snowberg wrote:
> Set the restriction check for INTEGRITY_KEYRING_MACHINE keys to
> restrict_link_by_ca.  This will only allow CA keys into the machine
> keyring.
> 
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>

Normally the new function, in this case the restriction, and usage
should be defined together.  Any reason why 3/12 and 4/12 are two
separate patches?  I would squash them together.

> ---
> v1: Initial version
> v2: Added !IS_ENABLED(CONFIG_INTEGRITY_TRUSTED_KEYRING check so mok
>     keyring gets created even when it isn't enabled
> v3: Rename restrict_link_by_system_trusted_or_ca to restrict_link_by_ca
> v4: removed unnecessary restriction->check set
> v5: Rename to machine keyring
> ---
>  security/integrity/digsig.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> index 5a75ac2c4dbe..2b75bbbd9e0e 100644
> --- a/security/integrity/digsig.c
> +++ b/security/integrity/digsig.c
> @@ -132,14 +132,18 @@ int __init integrity_init_keyring(const unsigned int id)
>  		goto out;
>  	}
>  
> -	if (!IS_ENABLED(CONFIG_INTEGRITY_TRUSTED_KEYRING))
> +	if (!IS_ENABLED(CONFIG_INTEGRITY_TRUSTED_KEYRING) && id != INTEGRITY_KEYRING_MACHINE)

Over 80 chars, please split the line.

thanks,

Mimi

>  		return 0;
>  
>  	restriction = kzalloc(sizeof(struct key_restriction), GFP_KERNEL);
>  	if (!restriction)
>  		return -ENOMEM;
>  
> -	restriction->check = restrict_link_to_ima;
> +	if (id == INTEGRITY_KEYRING_MACHINE)
> +		restriction->check = restrict_link_by_ca;
> +	else
> +		restriction->check = restrict_link_to_ima;
> +
>  	if (id != INTEGRITY_KEYRING_MACHINE)
>  		perm |= KEY_USR_WRITE;
>  

Re: [PATCH v5 04/12] integrity: restrict INTEGRITY_KEYRING_MACHINE to restrict_link_by_ca

[Eric Snowberg]

> On Sep 9, 2021, at 11:25 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> On Tue, 2021-09-07 at 12:01 -0400, Eric Snowberg wrote:
>> Set the restriction check for INTEGRITY_KEYRING_MACHINE keys to
>> restrict_link_by_ca.  This will only allow CA keys into the machine
>> keyring.
>> 
>> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> 
> Normally the new function, in this case the restriction, and usage
> should be defined together.  Any reason why 3/12 and 4/12 are two
> separate patches?  

I split them since they cross subsystems.

> I would squash them together.

But I can squash them together in the next round.

> 
>> ---
>> v1: Initial version
>> v2: Added !IS_ENABLED(CONFIG_INTEGRITY_TRUSTED_KEYRING check so mok
>>    keyring gets created even when it isn't enabled
>> v3: Rename restrict_link_by_system_trusted_or_ca to restrict_link_by_ca
>> v4: removed unnecessary restriction->check set
>> v5: Rename to machine keyring
>> ---
>> security/integrity/digsig.c | 8 ++++++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
>> 
>> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
>> index 5a75ac2c4dbe..2b75bbbd9e0e 100644
>> --- a/security/integrity/digsig.c
>> +++ b/security/integrity/digsig.c
>> @@ -132,14 +132,18 @@ int __init integrity_init_keyring(const unsigned int id)
>> 		goto out;
>> 	}
>> 
>> -	if (!IS_ENABLED(CONFIG_INTEGRITY_TRUSTED_KEYRING))
>> +	if (!IS_ENABLED(CONFIG_INTEGRITY_TRUSTED_KEYRING) && id != INTEGRITY_KEYRING_MACHINE)
> 
> Over 80 chars, please split the line

I thought the 80 char limit was relaxed?  But if it hasn’t
I can change this too.  Thanks.

Re: [PATCH v5 04/12] integrity: restrict INTEGRITY_KEYRING_MACHINE to restrict_link_by_ca

[Mimi Zohar]

On Thu, 2021-09-09 at 11:53 -0600, Eric Snowberg wrote:
> > On Sep 9, 2021, at 11:25 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > 
> > On Tue, 2021-09-07 at 12:01 -0400, Eric Snowberg wrote:
> >> Set the restriction check for INTEGRITY_KEYRING_MACHINE keys to
> >> restrict_link_by_ca.  This will only allow CA keys into the machine
> >> keyring.
> >> 
> >> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> > 
> > Normally the new function, in this case the restriction, and usage
> > should be defined together.  Any reason why 3/12 and 4/12 are two
> > separate patches?  
> 
> I split them since they cross subsystems.

That makes sense.

thanks,

Mimi

[PATCH v5 05/12] integrity: add new keyring handler for mok keys

[Eric Snowberg]

Currently both Secure Boot DB and Machine Owner Keys (MOK) go through
the same keyring handler (get_handler_for_db). With the addition of the
new machine keyring, the end-user may choose to trust MOK keys.

Introduce a new keyring handler specific for MOK keys.  If MOK keys are
trusted by the end-user, use the new keyring handler instead.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v1: Initial version
v3: Only change the keyring handler if the secondary is enabled
v4: Removed trust_moklist check
v5: Rename to machine keyring
---
 .../integrity/platform_certs/keyring_handler.c  | 17 ++++++++++++++++-
 .../integrity/platform_certs/keyring_handler.h  |  5 +++++
 security/integrity/platform_certs/load_uefi.c   |  4 ++--
 3 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/security/integrity/platform_certs/keyring_handler.c b/security/integrity/platform_certs/keyring_handler.c
index 5604bd57c990..445d413aec74 100644
--- a/security/integrity/platform_certs/keyring_handler.c
+++ b/security/integrity/platform_certs/keyring_handler.c
@@ -66,7 +66,7 @@ static __init void uefi_revocation_list_x509(const char *source,
 
 /*
  * Return the appropriate handler for particular signature list types found in
- * the UEFI db and MokListRT tables.
+ * the UEFI db tables.
  */
 __init efi_element_handler_t get_handler_for_db(const efi_guid_t *sig_type)
 {
@@ -75,6 +75,21 @@ __init efi_element_handler_t get_handler_for_db(const efi_guid_t *sig_type)
 	return 0;
 }
 
+/*
+ * Return the appropriate handler for particular signature list types found in
+ * the MokListRT tables.
+ */
+__init efi_element_handler_t get_handler_for_mok(const efi_guid_t *sig_type)
+{
+	if (efi_guidcmp(*sig_type, efi_cert_x509_guid) == 0) {
+		if (IS_ENABLED(CONFIG_INTEGRITY_MACHINE_KEYRING))
+			return add_to_machine_keyring;
+		else
+			return add_to_platform_keyring;
+	}
+	return 0;
+}
+
 /*
  * Return the appropriate handler for particular signature list types found in
  * the UEFI dbx and MokListXRT tables.
diff --git a/security/integrity/platform_certs/keyring_handler.h b/security/integrity/platform_certs/keyring_handler.h
index 2462bfa08fe3..284558f30411 100644
--- a/security/integrity/platform_certs/keyring_handler.h
+++ b/security/integrity/platform_certs/keyring_handler.h
@@ -24,6 +24,11 @@ void blacklist_binary(const char *source, const void *data, size_t len);
  */
 efi_element_handler_t get_handler_for_db(const efi_guid_t *sig_type);
 
+/*
+ * Return the handler for particular signature list types found in the mok.
+ */
+efi_element_handler_t get_handler_for_mok(const efi_guid_t *sig_type);
+
 /*
  * Return the handler for particular signature list types found in the dbx.
  */
diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
index f290f78c3f30..c1bfd1cd7cc3 100644
--- a/security/integrity/platform_certs/load_uefi.c
+++ b/security/integrity/platform_certs/load_uefi.c
@@ -94,7 +94,7 @@ static int __init load_moklist_certs(void)
 		rc = parse_efi_signature_list("UEFI:MokListRT (MOKvar table)",
 					      mokvar_entry->data,
 					      mokvar_entry->data_size,
-					      get_handler_for_db);
+					      get_handler_for_mok);
 		/* All done if that worked. */
 		if (!rc)
 			return rc;
@@ -109,7 +109,7 @@ static int __init load_moklist_certs(void)
 	mok = get_cert_list(L"MokListRT", &mok_var, &moksize, &status);
 	if (mok) {
 		rc = parse_efi_signature_list("UEFI:MokListRT",
-					      mok, moksize, get_handler_for_db);
+					      mok, moksize, get_handler_for_mok);
 		kfree(mok);
 		if (rc)
 			pr_err("Couldn't parse MokListRT signatures: %d\n", rc);
-- 
2.18.4

[PATCH v5 06/12] KEYS: add a reference to machine keyring

[Eric Snowberg]

Expose the .machine keyring created in integrity code by adding
a reference.  This makes the machine keyring accessible for keyring
restrictions in the future.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v2: Initial version
v3: set_mok_trusted_keys only available when secondary is enabled
v4: Moved code under CONFIG_INTEGRITY_MOK_KEYRING
v5: Rename to machine keyring
---
 certs/system_keyring.c        | 9 +++++++++
 include/keys/system_keyring.h | 8 ++++++++
 2 files changed, 17 insertions(+)

diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 692365dee2bd..08ea542c8096 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -22,6 +22,9 @@ static struct key *builtin_trusted_keys;
 #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
 static struct key *secondary_trusted_keys;
 #endif
+#ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
+static struct key *machine_trusted_keys;
+#endif
 #ifdef CONFIG_INTEGRITY_PLATFORM_KEYRING
 static struct key *platform_trusted_keys;
 #endif
@@ -91,6 +94,12 @@ static __init struct key_restriction *get_builtin_and_secondary_restriction(void
 	return restriction;
 }
 #endif
+#ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
+void __init set_machine_trusted_keys(struct key *keyring)
+{
+	machine_trusted_keys = keyring;
+}
+#endif
 
 /*
  * Create the trusted keyrings
diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
index 6acd3cf13a18..98c9b10cdc17 100644
--- a/include/keys/system_keyring.h
+++ b/include/keys/system_keyring.h
@@ -38,6 +38,14 @@ extern int restrict_link_by_builtin_and_secondary_trusted(
 #define restrict_link_by_builtin_and_secondary_trusted restrict_link_by_builtin_trusted
 #endif
 
+#ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
+extern void __init set_machine_trusted_keys(struct key *keyring);
+#else
+static inline void __init set_machine_trusted_keys(struct key *keyring)
+{
+}
+#endif
+
 extern struct pkcs7_message *pkcs7;
 #ifdef CONFIG_SYSTEM_BLACKLIST_KEYRING
 extern int mark_hash_blacklisted(const char *hash);
-- 
2.18.4

[PATCH v5 07/12] KEYS: Introduce link restriction to include builtin, secondary and machine keys

[Eric Snowberg]

Introduce a new link restriction that includes the trusted builtin,
secondary and machine keys. The restriction is based on the key to be added
being vouched for by a key in any of these three keyrings.

Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v3: Initial version
v4: moved code under CONFIG_INTEGRITY_MOK_KEYRING
v5: Rename to machine keyring
---
 certs/system_keyring.c        | 23 +++++++++++++++++++++++
 include/keys/system_keyring.h |  6 ++++++
 2 files changed, 29 insertions(+)

diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 08ea542c8096..955bd57815f4 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -99,6 +99,29 @@ void __init set_machine_trusted_keys(struct key *keyring)
 {
 	machine_trusted_keys = keyring;
 }
+
+/**
+ * restrict_link_by_builtin_secondary_and_ca_trusted
+ *
+ * Restrict the addition of keys into a keyring based on the key-to-be-added
+ * being vouched for by a key in either the built-in, the secondary, or
+ * the machine keyrings.
+ */
+int restrict_link_by_builtin_secondary_and_ca_trusted(
+	struct key *dest_keyring,
+	const struct key_type *type,
+	const union key_payload *payload,
+	struct key *restrict_key)
+{
+	if (machine_trusted_keys && type == &key_type_keyring &&
+	    dest_keyring == secondary_trusted_keys &&
+	    payload == &machine_trusted_keys->payload)
+		/* Allow the machine keyring to be added to the secondary */
+		return 0;
+
+	return restrict_link_by_builtin_and_secondary_trusted(dest_keyring, type,
+							      payload, restrict_key);
+}
 #endif
 
 /*
diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
index 98c9b10cdc17..fe4be10e66ef 100644
--- a/include/keys/system_keyring.h
+++ b/include/keys/system_keyring.h
@@ -39,8 +39,14 @@ extern int restrict_link_by_builtin_and_secondary_trusted(
 #endif
 
 #ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
+extern int restrict_link_by_builtin_secondary_and_ca_trusted(
+	struct key *dest_keyring,
+	const struct key_type *type,
+	const union key_payload *payload,
+	struct key *restrict_key);
 extern void __init set_machine_trusted_keys(struct key *keyring);
 #else
+#define restrict_link_by_builtin_secondary_and_ca_trusted restrict_link_by_builtin_trusted
 static inline void __init set_machine_trusted_keys(struct key *keyring)
 {
 }
-- 
2.18.4

Re: [PATCH v5 07/12] KEYS: Introduce link restriction to include builtin, secondary and machine keys

[Mimi Zohar]

Hi Eric,

The subject line above is too long.   According to
Documentation/process/submitting-patches.rst the "the ``summary`` must
be no more than 70-75 characters".

On Tue, 2021-09-07 at 12:01 -0400, Eric Snowberg wrote:
> Introduce a new link restriction that includes the trusted builtin,
> secondary and machine keys. The restriction is based on the key to be added
> being vouched for by a key in any of these three keyrings.
> 
> Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> ---
> v3: Initial version
> v4: moved code under CONFIG_INTEGRITY_MOK_KEYRING
> v5: Rename to machine keyring
> ---
>  certs/system_keyring.c        | 23 +++++++++++++++++++++++
>  include/keys/system_keyring.h |  6 ++++++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index 08ea542c8096..955bd57815f4 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -99,6 +99,29 @@ void __init set_machine_trusted_keys(struct key *keyring)
>  {
>  	machine_trusted_keys = keyring;
>  }
> +
> +/**
> + * restrict_link_by_builtin_secondary_and_ca_trusted

Sorry for the patch churn.  With the keyring name change to ".machine",
the restriction name should also reflect this change.

thanks,

Mimi

Re: [PATCH v5 07/12] KEYS: Introduce link restriction to include builtin, secondary and machine keys

[Eric Snowberg]

> On Sep 9, 2021, at 11:26 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> Hi Eric,
> 
> The subject line above is too long.   According to
> Documentation/process/submitting-patches.rst the "the ``summary`` must
> be no more than 70-75 characters".
> 
> On Tue, 2021-09-07 at 12:01 -0400, Eric Snowberg wrote:
>> Introduce a new link restriction that includes the trusted builtin,
>> secondary and machine keys. The restriction is based on the key to be added
>> being vouched for by a key in any of these three keyrings.
>> 
>> Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
>> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
>> ---
>> v3: Initial version
>> v4: moved code under CONFIG_INTEGRITY_MOK_KEYRING
>> v5: Rename to machine keyring
>> ---
>> certs/system_keyring.c        | 23 +++++++++++++++++++++++
>> include/keys/system_keyring.h |  6 ++++++
>> 2 files changed, 29 insertions(+)
>> 
>> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
>> index 08ea542c8096..955bd57815f4 100644
>> --- a/certs/system_keyring.c
>> +++ b/certs/system_keyring.c
>> @@ -99,6 +99,29 @@ void __init set_machine_trusted_keys(struct key *keyring)
>> {
>> 	machine_trusted_keys = keyring;
>> }
>> +
>> +/**
>> + * restrict_link_by_builtin_secondary_and_ca_trusted
> 
> Sorry for the patch churn.  With the keyring name change to ".machine",
> the restriction name should also reflect this change.

Yes, I can change that. Should it be renamed to 
restrict_link_by_builtin_secondary_and_machine_trusted? That seems a little
long though.  Thanks.

Re: [PATCH v5 07/12] KEYS: Introduce link restriction to include builtin, secondary and machine keys

[Mimi Zohar]

On Thu, 2021-09-09 at 12:03 -0600, Eric Snowberg wrote:
> > On Sep 9, 2021, at 11:26 AM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > 
> > Hi Eric,
> > 
> > The subject line above is too long.   According to
> > Documentation/process/submitting-patches.rst the "the ``summary`` must
> > be no more than 70-75 characters".
> > 
> > On Tue, 2021-09-07 at 12:01 -0400, Eric Snowberg wrote:
> >> Introduce a new link restriction that includes the trusted builtin,
> >> secondary and machine keys. The restriction is based on the key to be added
> >> being vouched for by a key in any of these three keyrings.
> >> 
> >> Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
> >> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> >> ---
> >> v3: Initial version
> >> v4: moved code under CONFIG_INTEGRITY_MOK_KEYRING
> >> v5: Rename to machine keyring
> >> ---
> >> certs/system_keyring.c        | 23 +++++++++++++++++++++++
> >> include/keys/system_keyring.h |  6 ++++++
> >> 2 files changed, 29 insertions(+)
> >> 
> >> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> >> index 08ea542c8096..955bd57815f4 100644
> >> --- a/certs/system_keyring.c
> >> +++ b/certs/system_keyring.c
> >> @@ -99,6 +99,29 @@ void __init set_machine_trusted_keys(struct key *keyring)
> >> {
> >> 	machine_trusted_keys = keyring;
> >> }
> >> +
> >> +/**
> >> + * restrict_link_by_builtin_secondary_and_ca_trusted
> > 
> > Sorry for the patch churn.  With the keyring name change to ".machine",
> > the restriction name should also reflect this change.
> 
> Yes, I can change that. Should it be renamed to 
> restrict_link_by_builtin_secondary_and_machine_trusted? That seems a little
> long though.  Thanks.

The existing name is long too.  Not sure it makes much of a difference,
but dropping "and" and/or "trusted" might help.

Mimi

[PATCH v5 08/12] KEYS: integrity: change link restriction to trust the machine keyring

[Eric Snowberg]

With the introduction of the machine keyring, the end-user may choose to
trust Machine Owner Keys (MOK) within the kernel. If they have chosen to
trust them, the .machine keyring will contain these keys.  If not, the
machine keyring will always be empty.  Update the restriction check to
allow the secondary trusted keyring and ima keyring to also trust
machine keys.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v4: Initial version (consolidated two previous patches)
v5: Rename to machine keyring
---
 certs/system_keyring.c      | 5 ++++-
 security/integrity/digsig.c | 4 ++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 955bd57815f4..747f0c528fec 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -89,7 +89,10 @@ static __init struct key_restriction *get_builtin_and_secondary_restriction(void
 	if (!restriction)
 		panic("Can't allocate secondary trusted keyring restriction\n");
 
-	restriction->check = restrict_link_by_builtin_and_secondary_trusted;
+	if (IS_ENABLED(CONFIG_INTEGRITY_MACHINE_KEYRING))
+		restriction->check = restrict_link_by_builtin_secondary_and_ca_trusted;
+	else
+		restriction->check = restrict_link_by_builtin_and_secondary_trusted;
 
 	return restriction;
 }
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 2b75bbbd9e0e..c3c1939be2f1 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -34,7 +34,11 @@ static const char * const keyring_name[INTEGRITY_KEYRING_MAX] = {
 };
 
 #ifdef CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY
+#ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
+#define restrict_link_to_ima restrict_link_by_builtin_secondary_and_ca_trusted
+#else
 #define restrict_link_to_ima restrict_link_by_builtin_and_secondary_trusted
+#endif
 #else
 #define restrict_link_to_ima restrict_link_by_builtin_trusted
 #endif
-- 
2.18.4

Re: [PATCH v5 08/12] KEYS: integrity: change link restriction to trust the machine keyring

[Mimi Zohar]

Hi Eric,

On Tue, 2021-09-07 at 12:01 -0400, Eric Snowberg wrote:

> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index 955bd57815f4..747f0c528fec 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -89,7 +89,10 @@ static __init struct key_restriction *get_builtin_and_secondary_restriction(void
>  	if (!restriction)
>  		panic("Can't allocate secondary trusted keyring restriction\n");
>  
> -	restriction->check = restrict_link_by_builtin_and_secondary_trusted;
> +	if (IS_ENABLED(CONFIG_INTEGRITY_MACHINE_KEYRING))
> +		restriction->check = restrict_link_by_builtin_secondary_and_ca_trusted;

Returning the new restriction in a function named
get_builtin_and_secondary_restriction() is kind of weird.  Renaming the
function to get_secondary_restriction() would be clearer.

thanks,

Mimi

[PATCH v5 09/12] KEYS: link secondary_trusted_keys to machine trusted keys

[Eric Snowberg]

Allow the .machine keyring to be linked to the secondary_trusted_keys.
After the link is created, keys contained in the .machine keyring will
automatically be searched when searching secondary_trusted_keys.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v3: Initial version
v4: Unmodified from v3
v5: Rename to machine keyring
---
 certs/system_keyring.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 747f0c528fec..e414b80f2135 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -101,6 +101,9 @@ static __init struct key_restriction *get_builtin_and_secondary_restriction(void
 void __init set_machine_trusted_keys(struct key *keyring)
 {
 	machine_trusted_keys = keyring;
+
+	if (key_link(secondary_trusted_keys, machine_trusted_keys) < 0)
+		panic("Can't link (machine) trusted keyrings\n");
 }
 
 /**
-- 
2.18.4

[PATCH v5 10/12] integrity: store reference to machine keyring

[Eric Snowberg]

Store a reference to the machine keyring in system keyring code. The
system keyring code needs this to complete the keyring link to
to machine keyring.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v2: Initial version
v3: Unmodified from v2
v4: Removed trust_moklist check
v5: Rename to machine keyring
---
 security/integrity/digsig.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index c3c1939be2f1..0dce2775f3c2 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -116,6 +116,8 @@ static int __init __integrity_init_keyring(const unsigned int id,
 	} else {
 		if (id == INTEGRITY_KEYRING_PLATFORM)
 			set_platform_trusted_keys(keyring[id]);
+		if (id == INTEGRITY_KEYRING_MACHINE)
+			set_machine_trusted_keys(keyring[id]);
 		if (id == INTEGRITY_KEYRING_IMA)
 			load_module_cert(keyring[id]);
 	}
-- 
2.18.4

[PATCH v5 11/12] integrity: Trust MOK keys if MokListTrustedRT found

[Eric Snowberg]

A new Machine Owner Key (MOK) variable called MokListTrustedRT has been
introduced in shim. When this UEFI variable is set, it indicates the
end-user has made the decision themself that they wish to trust MOK keys
within the Linux trust boundary.  It is not an error if this variable
does not exist. If it does not exist, the MOK keys should not be trusted
within the kernel.

MOK variables are mirrored from Boot Services to Runtime Services.  When
shim sees the new MokTML BS variable, it will create a new variable
(before Exit Boot Services is called) called MokListTrustedRT without
EFI_VARIABLE_NON_VOLATILE set.  Following Exit Boot Services, UEFI
variables can only be set and created with SetVariable if both
EFI_VARIABLE_RUNTIME_ACCESS & EFI_VARIABLE_NON_VOLATILE are set.
Therefore, this can not be defeated by simply creating a
MokListTrustedRT variable from Linux, the existence of
EFI_VARIABLE_NON_VOLATILE will cause uefi_check_trust_machine_keys to
return false.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v1: Initial version
v2: Removed mok_keyring_trust_setup function
v4: Unmodified from v2
v5: Rename to machine keyring
---
 .../platform_certs/machine_keyring.c          | 27 +++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/security/integrity/platform_certs/machine_keyring.c b/security/integrity/platform_certs/machine_keyring.c
index 948ec6c738c8..635ab2b9e289 100644
--- a/security/integrity/platform_certs/machine_keyring.c
+++ b/security/integrity/platform_certs/machine_keyring.c
@@ -5,6 +5,7 @@
  * Copyright (c) 2021, Oracle and/or its affiliates.
  */
 
+#include <linux/efi.h>
 #include "../integrity.h"
 
 static __init int machine_keyring_init(void)
@@ -40,3 +41,29 @@ void __init add_to_machine_keyring(const char *source, const void *data, size_t
 	if (rc)
 		pr_info("Error adding keys to machine keyring %s\n", source);
 }
+
+/*
+ * Try to load the MokListTrustedRT UEFI variable to see if we should trust
+ * the mok keys within the kernel. It is not an error if this variable
+ * does not exist.  If it does not exist, mok keys should not be trusted
+ * within the machine keyring.
+ */
+static __init bool uefi_check_trust_mok_keys(void)
+{
+	efi_status_t status;
+	unsigned int mtrust = 0;
+	unsigned long size = sizeof(mtrust);
+	efi_guid_t guid = EFI_SHIM_LOCK_GUID;
+	u32 attr;
+
+	status = efi.get_variable(L"MokListTrustedRT", &guid, &attr, &size, &mtrust);
+
+	/*
+	 * The EFI_VARIABLE_NON_VOLATILE check is to verify MokListTrustedRT
+	 * was set thru shim mirrioring and not by a user from the host os.
+	 * According to the UEFI spec, once EBS is performed, SetVariable()
+	 * will succeed only when both EFI_VARIABLE_RUNTIME_ACCESS &
+	 * EFI_VARIABLE_NON_VOLATILE are set.
+	 */
+	return (status == EFI_SUCCESS && (!(attr & EFI_VARIABLE_NON_VOLATILE)));
+}
-- 
2.18.4

[PATCH v5 12/12] integrity: Only use machine keyring when uefi_check_trust_mok_keys is true

[Eric Snowberg]

With the introduction of uefi_check_trust_mok_keys, it signifies the end-
user wants to trust the machine keyring as trusted keys.  If they have
chosen to trust the machine keyring, load the qualifying keys into it
during boot, then link it to the secondary keyring .  If the user has not
chosen to trust the machine keyring, it will be empty and not linked to
the secondary keyring.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
v4: Initial version
v5: Rename to machine keyring
---
 security/integrity/digsig.c                      |  2 +-
 security/integrity/integrity.h                   |  5 +++++
 .../integrity/platform_certs/keyring_handler.c   |  2 +-
 .../integrity/platform_certs/machine_keyring.c   | 16 ++++++++++++++++
 4 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 0dce2775f3c2..d495c4e49240 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -116,7 +116,7 @@ static int __init __integrity_init_keyring(const unsigned int id,
 	} else {
 		if (id == INTEGRITY_KEYRING_PLATFORM)
 			set_platform_trusted_keys(keyring[id]);
-		if (id == INTEGRITY_KEYRING_MACHINE)
+		if (id == INTEGRITY_KEYRING_MACHINE && trust_moklist())
 			set_machine_trusted_keys(keyring[id]);
 		if (id == INTEGRITY_KEYRING_IMA)
 			load_module_cert(keyring[id]);
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 730771eececd..2e214c761158 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -287,9 +287,14 @@ static inline void __init add_to_platform_keyring(const char *source,
 
 #ifdef CONFIG_INTEGRITY_MACHINE_KEYRING
 void __init add_to_machine_keyring(const char *source, const void *data, size_t len);
+bool __init trust_moklist(void);
 #else
 static inline void __init add_to_machine_keyring(const char *source,
 						  const void *data, size_t len)
 {
 }
+static inline bool __init trust_moklist(void)
+{
+	return false;
+}
 #endif
diff --git a/security/integrity/platform_certs/keyring_handler.c b/security/integrity/platform_certs/keyring_handler.c
index 445d413aec74..d78dd66fb048 100644
--- a/security/integrity/platform_certs/keyring_handler.c
+++ b/security/integrity/platform_certs/keyring_handler.c
@@ -82,7 +82,7 @@ __init efi_element_handler_t get_handler_for_db(const efi_guid_t *sig_type)
 __init efi_element_handler_t get_handler_for_mok(const efi_guid_t *sig_type)
 {
 	if (efi_guidcmp(*sig_type, efi_cert_x509_guid) == 0) {
-		if (IS_ENABLED(CONFIG_INTEGRITY_MACHINE_KEYRING))
+		if (IS_ENABLED(CONFIG_INTEGRITY_MACHINE_KEYRING) && trust_moklist())
 			return add_to_machine_keyring;
 		else
 			return add_to_platform_keyring;
diff --git a/security/integrity/platform_certs/machine_keyring.c b/security/integrity/platform_certs/machine_keyring.c
index 635ab2b9e289..eaef1efdb261 100644
--- a/security/integrity/platform_certs/machine_keyring.c
+++ b/security/integrity/platform_certs/machine_keyring.c
@@ -8,6 +8,8 @@
 #include <linux/efi.h>
 #include "../integrity.h"
 
+bool trust_mok;
+
 static __init int machine_keyring_init(void)
 {
 	int rc;
@@ -67,3 +69,17 @@ static __init bool uefi_check_trust_mok_keys(void)
 	 */
 	return (status == EFI_SUCCESS && (!(attr & EFI_VARIABLE_NON_VOLATILE)));
 }
+
+bool __init trust_moklist(void)
+{
+	static bool initialized;
+
+	if (!initialized) {
+		initialized = true;
+
+		if (uefi_check_trust_mok_keys())
+			trust_mok = true;
+	}
+
+	return trust_mok;
+}
-- 
2.18.4

Re: [PATCH v5 12/12] integrity: Only use machine keyring when uefi_check_trust_mok_keys is true

[Jarkko Sakkinen]

On Tue, 2021-09-07 at 12:01 -0400, Eric Snowberg wrote:
> With the introduction of uefi_check_trust_mok_keys, it signifies the end-
> user wants to trust the machine keyring as trusted keys.  If they have
> chosen to trust the machine keyring, load the qualifying keys into it
> during boot, then link it to the secondary keyring .  If the user has not
> chosen to trust the machine keyring, it will be empty and not linked to
> the secondary keyring.
> 
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>

I would not worry too much applying the code changes if the story
part made sense (to *almost anyone*) in the cover letter.

/Jarkko

Re: [PATCH v5 00/12] Enroll kernel keys thru MOK

[Jarkko Sakkinen]

On Tue, 2021-09-07 at 12:00 -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-user development kernel build.  When Linux boots,
> pre-boot keys (both UEFI Secure Boot DB and MOK keys) get loaded in the
> Linux .platform keyring.  
> 
> Currently, pre-boot keys are not trusted within the Linux trust boundary
> [1]. These platform keys can only be used for kexec. If an end-user

What exactly is "trust boundary"? And what do you mean when you say that
Linux "trusts" something? AFAIK, software does not have feelings. Please,
just speak about exact things.

That's the whole point of the cover letter. It's better to not have cover
letter at all, than a confusing cover letter that reads like a white paper.
Code changes at least tell the exact story, and not speak about feelings.

> wants to use their own key within the Linux trust boundary, they must
> either compile it into the kernel themselves or use the insert-sys-cert
> script. Both options present a problem. Many end-users do not want to
> compile their own kernels. With the insert-sys-cert option, there are
> missing upstream changes [2].  Also, with the insert-sys-cert option,
> the end-user must re-sign their kernel again with their own key, and
> then insert that key into the MOK db. Another problem with
> insert-sys-cert is that only a single key can be inserted into a
> compressed kernel.

I use a pre-compiled kernel in my desktop: https://liquorix.net/. When
a new version comes up it requires a sbsign one-liner to sign it for
secure boot. I'm wondering what is the problem I'm facing because I do
not see it.

If there are something missing changes that you use as a rationale for
this large patch set, you should at least broadly explain what we are
missing. How I conclude this paragraph is that, since there is only an
xref, they are not really "that important" changes, which are missing.

> Having the ability to insert a key into the Linux trust boundary opens
> up various possibilities.  The end-user can use a pre-built kernel and
> sign their own kernel modules.  It also opens up the ability for an

Which both can be done by end-user as of today, or I'm misreading this.

> end-user to more easily use digital signature based IMA-appraisal.  To
> get a key into the ima keyring, it must be signed by a key within the
> Linux trust boundary.

What is IMA appraisal? I just don't know it because I don't use IMA.
Again, this trust boundary is really something I do not. Looking at
code changes, you could just speak about exact assets in the kernel.

> Downstream Linux distros try to have a single signed kernel for each
> architecture.  Each end-user may use this kernel in entirely different
> ways.  Some downstream kernels have chosen to always trust platform keys
> within the Linux trust boundary for kernel module signing.  These
> kernels have no way of using digital signature base IMA appraisal.
> 
> This series introduces a new Linux kernel keyring containing the Machine
> Owner Keys (MOK) called .machine. It also adds a new MOK variable to shim.
> This variable allows the end-user to decide if they want to trust keys
> enrolled in the MOK within the Linux trust boundary.  By default,
> nothing changes; MOK keys are not trusted within the Linux kernel.  They
> are only trusted after the end-user makes the decision themselves.  The
> end-user would set this through mokutil using a new --trust-mok option
> [3]. This would work similar to how the kernel uses MOK variables to
> enable/disable signature validation as well as use/ignore the db.

OK, changes are described here (again speaking about trusting tho). The
motivation part is missing. The text before this is more like confusion
part. When you describe motivation to do something you should really be in
grass roots, e.g. "when you have this feature in the kernel, look, I can
do now this". It's not that hard. E.g. with an usage example it is quite
quick accomplish this.

/Jarkko

Re: [PATCH v5 00/12] Enroll kernel keys thru MOK

[Jarkko Sakkinen]

On Wed, 2021-09-08 at 19:03 +0300, Jarkko Sakkinen wrote:
> On Tue, 2021-09-07 at 12:00 -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-user development kernel build.  When Linux boots,
> > pre-boot keys (both UEFI Secure Boot DB and MOK keys) get loaded in the
> > Linux .platform keyring.  
> > 
> > Currently, pre-boot keys are not trusted within the Linux trust boundary
> > [1]. These platform keys can only be used for kexec. If an end-user
> 
> What exactly is "trust boundary"? And what do you mean when you say that
> Linux "trusts" something? AFAIK, software does not have feelings. Please,
> just speak about exact things.
> 
> That's the whole point of the cover letter. It's better to not have cover
> letter at all, than a confusing cover letter that reads like a white paper.
> Code changes at least tell the exact story, and not speak about feelings.
> 
> > wants to use their own key within the Linux trust boundary, they must
> > either compile it into the kernel themselves or use the insert-sys-cert
> > script. Both options present a problem. Many end-users do not want to
> > compile their own kernels. With the insert-sys-cert option, there are
> > missing upstream changes [2].  Also, with the insert-sys-cert option,
> > the end-user must re-sign their kernel again with their own key, and
> > then insert that key into the MOK db. Another problem with
> > insert-sys-cert is that only a single key can be inserted into a
> > compressed kernel.
> 
> I use a pre-compiled kernel in my desktop: https://liquorix.net/. When
> a new version comes up it requires a sbsign one-liner to sign it for
> secure boot. I'm wondering what is the problem I'm facing because I do
> not see it.
> 
> If there are something missing changes that you use as a rationale for
> this large patch set, you should at least broadly explain what we are
> missing. How I conclude this paragraph is that, since there is only an
> xref, they are not really "that important" changes, which are missing.
> 
> > Having the ability to insert a key into the Linux trust boundary opens
> > up various possibilities.  The end-user can use a pre-built kernel and
> > sign their own kernel modules.  It also opens up the ability for an
> 
> Which both can be done by end-user as of today, or I'm misreading this.
> 
> > end-user to more easily use digital signature based IMA-appraisal.  To
> > get a key into the ima keyring, it must be signed by a key within the
> > Linux trust boundary.
> 
> What is IMA appraisal? I just don't know it because I don't use IMA.
> Again, this trust boundary is really something I do not. Looking at
> code changes, you could just speak about exact assets in the kernel.
> 
> > Downstream Linux distros try to have a single signed kernel for each
> > architecture.  Each end-user may use this kernel in entirely different
> > ways.  Some downstream kernels have chosen to always trust platform keys
> > within the Linux trust boundary for kernel module signing.  These
> > kernels have no way of using digital signature base IMA appraisal.
> > 
> > This series introduces a new Linux kernel keyring containing the Machine
> > Owner Keys (MOK) called .machine. It also adds a new MOK variable to shim.
> > This variable allows the end-user to decide if they want to trust keys
> > enrolled in the MOK within the Linux trust boundary.  By default,
> > nothing changes; MOK keys are not trusted within the Linux kernel.  They
> > are only trusted after the end-user makes the decision themselves.  The
> > end-user would set this through mokutil using a new --trust-mok option
> > [3]. This would work similar to how the kernel uses MOK variables to
> > enable/disable signature validation as well as use/ignore the db.
> 
> OK, changes are described here (again speaking about trusting tho). The
> motivation part is missing. The text before this is more like confusion
> part. When you describe motivation to do something you should really be in
> grass roots, e.g. "when you have this feature in the kernel, look, I can
> do now this". It's not that hard. E.g. with an usage example it is quite
> quick accomplish this.

The code changes overally make sense but this motivotional part is the
problem. E.g. if you do a pull request, it is completely *unusable* in
that context. In that case I would have to write something that should
have been the cover letter. It's 12 patches, so it is perfectly sensible
to ask a better one.

/Jarkko

Re: [PATCH v5 00/12] Enroll kernel keys thru MOK

[Eric Snowberg]

> On Sep 8, 2021, at 10:49 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> 
> On Wed, 2021-09-08 at 19:03 +0300, Jarkko Sakkinen wrote:
>> 
>>> Downstream Linux distros try to have a single signed kernel for each
>>> architecture.  Each end-user may use this kernel in entirely different
>>> ways.  Some downstream kernels have chosen to always trust platform keys
>>> within the Linux trust boundary for kernel module signing.  These
>>> kernels have no way of using digital signature base IMA appraisal.
>>> 
>>> This series introduces a new Linux kernel keyring containing the Machine
>>> Owner Keys (MOK) called .machine. It also adds a new MOK variable to shim.
>>> This variable allows the end-user to decide if they want to trust keys
>>> enrolled in the MOK within the Linux trust boundary.  By default,
>>> nothing changes; MOK keys are not trusted within the Linux kernel.  They
>>> are only trusted after the end-user makes the decision themselves.  The
>>> end-user would set this through mokutil using a new --trust-mok option
>>> [3]. This would work similar to how the kernel uses MOK variables to
>>> enable/disable signature validation as well as use/ignore the db.
>> 
>> OK, changes are described here (again speaking about trusting tho). The
>> motivation part is missing. The text before this is more like confusion
>> part. When you describe motivation to do something you should really be in
>> grass roots, e.g. "when you have this feature in the kernel, look, I can
>> do now this". It's not that hard. E.g. with an usage example it is quite
>> quick accomplish this.
> 
> The code changes overally make sense but this motivotional part is the
> problem. E.g. if you do a pull request, it is completely *unusable* in
> that context. In that case I would have to write something that should
> have been the cover letter. It's 12 patches, so it is perfectly sensible
> to ask a better one.

Would this be a more appropriate cover letter that includes a better
motivation?

Back in 2013 Linus requested a feature to allow end-users to have the 
ability "to add their own keys and sign modules they trust".  This was his 
*second* order outlined here [1].  There have been many attempts over the
years to solve this problem, all have been rejected.  Many distributions 
carry one of these rejected attempts. This series tries to solve this problem 
with a solution that takes into account all the problems brought up in the 
previous attempts.

This series introduces a new Linux kernel keyring containing the Machine
Owner Keys (MOK) called .machine. It also adds a new MOK variable to shim.
This variable allows the end-user to decide if they want to load keys
enrolled in the MOK within the Linux kernel.  By default, nothing changes; 
MOK keys are not loaded within the Linux kernel.  They are only loaded after
the end-user makes the decision themselves.  The end-user would set this 
through mokutil using a new --trust-mok option [2]. This would work similar 
to how the kernel uses MOK variables to enable/disable signature validation 
as well as use/ignore the db. Mimi has suggested that only CA keys be loaded 
into this keyring. All other certs will load into the platform keyring instead.

Secure Boot keys will never be loaded.  They will always be loaded into
the platform keyring.  If an end-user wanted to load one, they would
need to enroll it into the MOK.

Steps required by the end user:

Sign kernel module with user created key:
$ /usr/src/kernels/$(uname -r)/scripts/sign-file sha512 signing_key.priv \
    signing_key.x509 my_module.ko

Import the key into the MOK
$ mokutil --import signing_key.x509

Setup the kernel to load MOK keys into the .machine keyring
$ mokutil —trust-mok

Then reboot, the MokManager will load and ask if you want to trust the MOK 
key and enroll the MOK into the MOKList.  Afterwards the signed kernel module 
will load.

I have included links to both the mokutil [2] and shim [3] changes I
have made to support this new functionality.

[1] https://marc.info/?l=linux-kernel&m=136185386310140&w=2
[2] https://github.com/esnowberg/mokutil/tree/0.3.0-mokvars-v2
[3] https://github.com/esnowberg/shim/tree/mokvars-v2

Re: [PATCH v5 00/12] Enroll kernel keys thru MOK

[Mimi Zohar]

On Wed, 2021-09-08 at 16:25 -0600, Eric Snowberg wrote:
> > On Sep 8, 2021, at 10:49 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > 
> > On Wed, 2021-09-08 at 19:03 +0300, Jarkko Sakkinen wrote:
> >> 
> >>> Downstream Linux distros try to have a single signed kernel for each
> >>> architecture.  Each end-user may use this kernel in entirely different
> >>> ways.  Some downstream kernels have chosen to always trust platform keys
> >>> within the Linux trust boundary for kernel module signing.  These
> >>> kernels have no way of using digital signature base IMA appraisal.
> >>> 
> >>> This series introduces a new Linux kernel keyring containing the Machine
> >>> Owner Keys (MOK) called .machine. It also adds a new MOK variable to shim.
> >>> This variable allows the end-user to decide if they want to trust keys
> >>> enrolled in the MOK within the Linux trust boundary.  By default,
> >>> nothing changes; MOK keys are not trusted within the Linux kernel.  They
> >>> are only trusted after the end-user makes the decision themselves.  The
> >>> end-user would set this through mokutil using a new --trust-mok option
> >>> [3]. This would work similar to how the kernel uses MOK variables to
> >>> enable/disable signature validation as well as use/ignore the db.
> >> 
> >> OK, changes are described here (again speaking about trusting tho). The
> >> motivation part is missing. The text before this is more like confusion
> >> part. When you describe motivation to do something you should really be in
> >> grass roots, e.g. "when you have this feature in the kernel, look, I can
> >> do now this". It's not that hard. E.g. with an usage example it is quite
> >> quick accomplish this.
> > 
> > The code changes overally make sense but this motivotional part is the
> > problem. E.g. if you do a pull request, it is completely *unusable* in
> > that context. In that case I would have to write something that should
> > have been the cover letter. It's 12 patches, so it is perfectly sensible
> > to ask a better one.
> 
> Would this be a more appropriate cover letter that includes a better
> motivation?
> 
> Back in 2013 Linus requested a feature to allow end-users to have the 
> ability "to add their own keys and sign modules they trust".  This was his 
> *second* order outlined here [1].  There have been many attempts over the
> years to solve this problem, all have been rejected.  Many distributions 
> carry one of these rejected attempts. This series tries to solve this problem 
> with a solution that takes into account all the problems brought up in the 
> previous attempts.

Instead of making the reviewer look up the failed attempts, please
summarize why they failed (e.g. all preboot firmware keys were
trusted), and then continue, like below, with how this attempt differs.

> 
> This series introduces a new Linux kernel keyring containing the Machine
> Owner Keys (MOK) called .machine.

Other archs will also want to allow loading "end-users" key.  Please
prefix this paragraph with something like "On UEFI based systems".

>  It also adds a new MOK variable to shim.

Replace "adds" with "defines".

> This variable allows the end-user to decide if they want to load keys
> enrolled in the MOK within the Linux kernel.  By default, nothing changes; 
> MOK keys are not loaded within the Linux kernel.  They are only loaded after
> the end-user makes the decision themselves.  The end-user would set this 
> through mokutil using a new --trust-mok option [2]. This would work similar 
> to how the kernel uses MOK variables to enable/disable signature validation 
> as well as use/ignore the db. Mimi has suggested that only CA keys be loaded 
> into this keyring. All other certs will load into the platform keyring instead.

Thank you for crediting me for limiting loading only the CA keys stored
in the MOK db onto the "machine" keyring, but the limitation should be
better integrated in the paragraph.

> Secure Boot keys will never be loaded.  They will always be loaded into
> the platform keyring.  If an end-user wanted to load one, they would
> need to enroll it into the MOK.
> 
> Steps required by the end user:
> 
> Sign kernel module with user created key:
> $ /usr/src/kernels/$(uname -r)/scripts/sign-file sha512 signing_key.priv \
>     signing_key.x509 my_module.ko
> 
> Import the key into the MOK
> $ mokutil --import signing_key.x509

To differentiate this "signing_key" from others, perhaps name the file
"machine_sigining_key" or "local_signing_key".

thanks,

Mimi

> 
> Setup the kernel to load MOK keys into the .machine keyring
> $ mokutil —trust-mok
> 
> Then reboot, the MokManager will load and ask if you want to trust the MOK 
> key and enroll the MOK into the MOKList.  Afterwards the signed kernel module 
> will load.
> 
> I have included links to both the mokutil [2] and shim [3] changes I
> have made to support this new functionality.
> 
> [1] https://marc.info/?l=linux-kernel&m=136185386310140&w=2
> [2] https://github.com/esnowberg/mokutil/tree/0.3.0-mokvars-v2
> [3] https://github.com/esnowberg/shim/tree/mokvars-v2

Re: [PATCH v5 00/12] Enroll kernel keys thru MOK

[Eric Snowberg]

> On Sep 8, 2021, at 10:03 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> 
> On Tue, 2021-09-07 at 12:00 -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-user development kernel build.  When Linux boots,
>> pre-boot keys (both UEFI Secure Boot DB and MOK keys) get loaded in the
>> Linux .platform keyring.  
>> 
>> Currently, pre-boot keys are not trusted within the Linux trust boundary
>> [1]. These platform keys can only be used for kexec. If an end-user
> 
> What exactly is "trust boundary"? And what do you mean when you say that
> Linux "trusts" something? AFAIK, software does not have feelings. Please,
> just speak about exact things.

I am using terminology used previously by others when addressing this issue.  
If I should be using different terminology, please advise. The kernel does not 
trust pre-boot keys within it, meaning these pre-boot keys can not be used to 
validate items within the kernel. This is the “trust boundary”. Preboot keys are
on one side of the boundary, compiled-in keys are on the other.  MOK keys are 
pre-boot keys.  Currently they can not be used to validate things within the 
kernel itself (kernel modules, IMA keys, etc).