[PATCH RFC 0/8] Clavis LSM

[PATCH RFC 0/8] Clavis LSM

[Eric Snowberg]

Introduce a new LSM called Clavis (Latin word meaning key).  The motivation
behind this LSM is to provide access control for system keys.  Before spending
more time on this LSM, I am sending this as an RFC to start a discussion to see
if the current direction taken has a possibility of being accepted in the future.

Today the kernel has the following system keyrings: .builtin_trusted_keyring,
.secondary_trusted_keyring, and the .machine.  It also has the .platform keyring
which has limited capabilities; it can only be used to verify a kernel for kexec.

Today the kernel also tracks key usage for verification done with any of these
keys. Current verification usage includes: VERIFYING_MODULE_SIGNATURE,
VERIFYING_FIRMWARE_SIGNATURE, VERIFYING_KEXEC_PE_SIGNATURE, VERIFYING_KEY_SIGNATURE,
VERIFYING_KEY_SELF_SIGNATURE, and VERIFYING_UNSPECIFIED_SIGNATURE. After these usage
types were originally introduced, most additions have typically used the
VERIFYING_UNSPECIFIED_SIGNATURE.

At the moment, besides the usage enforcement for .platform keys, any key
contained within the system keyrings can be used for any verification
purpose.  For example, a key that was originally created to sign kernel
modules could be used for BPF verification.

This new LSM adds the ability to do access control for all system keys. When
enabled, only the .builtin_trusted_keys are available for loading kernel modules
and doing a kexec.  Until an ACL entry is added for a specific key, no other
system key may be used for any other purpose.

Enabling the LSM is done during initial boot by passing in a single asymmetric
key id within a new "clavis=" boot param. The asymmetric key id must match one
already contained within any of the system keyrings.  If a match is found, a
link is created into the new .clavis keyring.  This key shall be used as the
root of trust for any keyring ACL updates afterwards.

On UEFI systems the "clavis" boot param is mirrored into a new UEFI variable
within the EFI stub code. This variable will persist until the next power on
reset.  This same type of functionality is done within shim. Since this variable
is created before ExitBootServices (EBS) it will not have the NVRAM bit set,
signifying it was created during the Boot Services phase. This is being used
so the "clavis" boot param can not be changed via kexec, thereby preventing
a pivot of the root of trust.

As mentioned earlier, this LSM introduces a new .clavis keyring.  Following
boot, no new asymmetric keys can be added to this keyring and only the key
designated via the initial boot param may be used. This LSM can not be started
at any other point in time.  The .clavis keyring also holds the access control
list for system keys. A new key type called clavis_key_acl is being introduced.
This contains the usage followed by the asymmetric key id. To be added to the
clavis keyring, the clavis_key_acl must be S/MIME signed by the sole asymmetric
key contained within it. New ACL additions to the .clavis keyring may be added
at any time.

Currently this LSM does not require new changes or modifications to any user
space tools.  It also does not have a securityfs interface.  Everything is
done using the existing keyctl tool through the new .clavis keyring. The
S/MIME signing can be done with a simple OpenSSL command. If additions or
updates need to be added in the future, new acl key types could be created.
With this approach, maintainability should not be an issue in the future
if missing items are identified.

For setup and usage instructions, the final patch includes an admin-guide.

Future enhancements to this LSM could include:

1. Subsystems that currently use system keys with
   VERIFYING_UNSPECIFIED_SIGNATURE could be updated with their specific
   usage type.  For example, a usage type for IMA, BPF, etc could be
   added.

2. Currently, each clavis_key_acl must be individually signed.  Add the ability
   to sign multiple clavis_key_acl entries within the same file.

3. Currently, this LSM does not place key usage restrictions on the builtin
   keys for kexec and kernel module verification. This was done to prevent a
   regression that could  prevent the kernel from booting.  This could be
   changed if there was a way at compile time to pre-populate the .clavis
   keyring. This would allow the ephemeral key used to sign the kernel
   modules to be included within the .clavis keyring, allowing the kernel
   to boot.

4. UEFI Secure Boot Advanced Targeting (SBAT) support. Since
   the boot param is mirrored into UEFI before EBS is called,
   this LSM could be enhanced to not only enforce key usage,
   but also SBAT levels across kexec.

5. Having the ability to allow platform keys to be on par with
   all other system keys when using this LSM. This would be useful
   for a user that controls their entire UEFI SB DB key chain and
   doesn't want to use MOK keys.

I would appreciate any feedback on this approach. Thanks.

This series is based off lsm/dev commit commit edc6670233a3 ("cred: Use
KMEM_CACHE() instead of kmem_cache_create()")

Eric Snowberg (8):
  certs: Introduce ability to link to a system key
  clavis: Introduce a new system keyring called clavis
  efi: Make clavis boot param persist across kexec
  clavis: Prevent clavis boot param from changing during kexec
  keys: Add new verification type (VERIFYING_CLAVIS_SIGNATURE)
  keys: Add ability to track intended usage of the public key
  clavis: Introduce a new key type called clavis_key_acl
  clavis: Introduce new LSM called clavis

 Documentation/admin-guide/LSM/clavis.rst      | 190 +++++++++++
 MAINTAINERS                                   |   7 +
 certs/blacklist.c                             |   3 +
 certs/system_keyring.c                        |  29 ++
 crypto/asymmetric_keys/asymmetric_type.c      |   1 +
 crypto/asymmetric_keys/pkcs7_trust.c          |  20 ++
 crypto/asymmetric_keys/pkcs7_verify.c         |   5 +
 crypto/asymmetric_keys/signature.c            |   4 +
 drivers/firmware/efi/Kconfig                  |  12 +
 drivers/firmware/efi/libstub/Makefile         |   1 +
 drivers/firmware/efi/libstub/clavis.c         |  33 ++
 .../firmware/efi/libstub/efi-stub-helper.c    |   2 +
 drivers/firmware/efi/libstub/efi-stub.c       |   2 +
 drivers/firmware/efi/libstub/efistub.h        |   8 +
 drivers/firmware/efi/libstub/x86-stub.c       |   2 +
 include/crypto/pkcs7.h                        |   3 +
 include/crypto/public_key.h                   |   4 +
 include/keys/system_keyring.h                 |   7 +-
 include/linux/efi.h                           |   1 +
 include/linux/lsm_hook_defs.h                 |   2 +
 include/linux/security.h                      |  11 +
 include/linux/verification.h                  |   1 +
 include/uapi/linux/lsm.h                      |   1 +
 security/Kconfig                              |  11 +-
 security/Makefile                             |   1 +
 security/clavis/Kconfig                       |   9 +
 security/clavis/Makefile                      |   7 +
 security/clavis/clavis.c                      |  25 ++
 security/clavis/clavis.h                      |  16 +
 security/clavis/clavis_efi.c                  |  50 +++
 security/clavis/clavis_keyring.c              | 313 ++++++++++++++++++
 security/integrity/iint.c                     |   2 +
 security/security.c                           |  16 +-
 33 files changed, 792 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/admin-guide/LSM/clavis.rst
 create mode 100644 drivers/firmware/efi/libstub/clavis.c
 create mode 100644 security/clavis/Kconfig
 create mode 100644 security/clavis/Makefile
 create mode 100644 security/clavis/clavis.c
 create mode 100644 security/clavis/clavis.h
 create mode 100644 security/clavis/clavis_efi.c
 create mode 100644 security/clavis/clavis_keyring.c


base-commit: edc6670233a333ccfd1ec0548f068bd121d209c8
-- 
2.39.3

[PATCH RFC 1/8] certs: Introduce ability to link to a system key

[Eric Snowberg]

Introduce a new function to allow a keyring to link to a key contained
within one of the system keyrings (builtin, secondary, or platform).
Depending on how the kernel is built, if the machine keyring is
available, it will be checked as well, since it is linked to the secondary
keyring. If the asymmetric key id matches a key within one of these
system keyrings, the matching key is linked into the passed in
keyring.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
 certs/system_keyring.c        | 29 +++++++++++++++++++++++++++++
 include/keys/system_keyring.h |  7 ++++++-
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 9de610bf1f4b..b647be49f6e0 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -426,3 +426,32 @@ void __init set_platform_trusted_keys(struct key *keyring)
 	platform_trusted_keys = keyring;
 }
 #endif
+
+/**
+ * system_key_link - Link to a system key
+ * @keyring: The keyring to link into
+ * @id: The asymmetric key id to look for in the system keyring
+ */
+int system_key_link(struct key *keyring, struct asymmetric_key_id *id)
+{
+	struct key *tkey;
+
+#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
+	tkey = find_asymmetric_key(secondary_trusted_keys, id, NULL, NULL, false);
+#else
+	tkey = find_asymmetric_key(builtin_trusted_keys, id, NULL, NULL, false);
+#endif
+	if (!IS_ERR(tkey))
+		goto found;
+
+	tkey = find_asymmetric_key(platform_trusted_keys, id, NULL, NULL, false);
+
+	if (!IS_ERR(tkey))
+		goto found;
+
+	return -1;
+
+found:
+	key_link(keyring, tkey);
+	return 0;
+}
diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
index 8365adf842ef..b47ac8e2001a 100644
--- a/include/keys/system_keyring.h
+++ b/include/keys/system_keyring.h
@@ -9,6 +9,7 @@
 #define _KEYS_SYSTEM_KEYRING_H
 
 #include <linux/key.h>
+struct asymmetric_key_id;
 
 enum blacklist_hash_type {
 	/* TBSCertificate hash */
@@ -28,7 +29,7 @@ int restrict_link_by_digsig_builtin(struct key *dest_keyring,
 				    const union key_payload *payload,
 				    struct key *restriction_key);
 extern __init int load_module_cert(struct key *keyring);
-
+extern int system_key_link(struct key *keyring, struct asymmetric_key_id *id);
 #else
 #define restrict_link_by_builtin_trusted restrict_link_reject
 #define restrict_link_by_digsig_builtin restrict_link_reject
@@ -38,6 +39,10 @@ static inline __init int load_module_cert(struct key *keyring)
 	return 0;
 }
 
+static inline int system_key_link(struct key *keyring, struct asymmetric_key_id *id)
+{
+	return 0;
+}
 #endif
 
 #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
-- 
2.39.3

[PATCH RFC 2/8] clavis: Introduce a new system keyring called clavis

[Eric Snowberg]

Introduce a new system keyring called clavis.  This keyring shall contain a
single asymmetric key. This key shall be a linked to a key already
contained in one of the system keyrings (builtin, secondary, or platform).
The only way to add this key is during boot by passing in the asymmetric
key id within the new "clavis=" boot param.  If a matching key is found in
one of the system keyrings, a link shall be created. This keyring will be
used in the future by the new Clavis LSM.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
 include/linux/security.h         |  4 ++
 security/Kconfig                 |  1 +
 security/Makefile                |  1 +
 security/clavis/Kconfig          |  9 ++++
 security/clavis/Makefile         |  3 ++
 security/clavis/clavis_keyring.c | 90 ++++++++++++++++++++++++++++++++
 security/integrity/iint.c        |  2 +
 7 files changed, 110 insertions(+)
 create mode 100644 security/clavis/Kconfig
 create mode 100644 security/clavis/Makefile
 create mode 100644 security/clavis/clavis_keyring.c

diff --git a/include/linux/security.h b/include/linux/security.h
index 8436f9abf43d..94661398708a 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -508,6 +508,7 @@ int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen);
 int security_locked_down(enum lockdown_reason what);
 int lsm_fill_user_ctx(struct lsm_ctx __user *uctx, size_t *uctx_len,
 		      void *val, size_t val_len, u64 id, u64 flags);
+void late_init_clavis_setup(void);
 #else /* CONFIG_SECURITY */
 
 static inline int call_blocking_lsm_notifier(enum lsm_event event, void *data)
@@ -2223,6 +2224,9 @@ static inline int security_uring_cmd(struct io_uring_cmd *ioucmd)
 {
 	return 0;
 }
+static inline void late_init_clavis_setup(void)
+{
+}
 #endif /* CONFIG_SECURITY */
 #endif /* CONFIG_IO_URING */
 
diff --git a/security/Kconfig b/security/Kconfig
index 52c9af08ad35..7f5a52b7cefd 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -194,6 +194,7 @@ source "security/yama/Kconfig"
 source "security/safesetid/Kconfig"
 source "security/lockdown/Kconfig"
 source "security/landlock/Kconfig"
+source "security/clavis/Kconfig"
 
 source "security/integrity/Kconfig"
 
diff --git a/security/Makefile b/security/Makefile
index 59f238490665..01eb82bdd74f 100644
--- a/security/Makefile
+++ b/security/Makefile
@@ -25,6 +25,7 @@ obj-$(CONFIG_SECURITY_LOCKDOWN_LSM)	+= lockdown/
 obj-$(CONFIG_CGROUPS)			+= device_cgroup.o
 obj-$(CONFIG_BPF_LSM)			+= bpf/
 obj-$(CONFIG_SECURITY_LANDLOCK)		+= landlock/
+obj-$(CONFIG_SECURITY_CLAVIS_KEYRING)	+= clavis/
 
 # Object integrity file lists
 obj-$(CONFIG_INTEGRITY)			+= integrity/
diff --git a/security/clavis/Kconfig b/security/clavis/Kconfig
new file mode 100644
index 000000000000..ca72a9e43089
--- /dev/null
+++ b/security/clavis/Kconfig
@@ -0,0 +1,9 @@
+config SECURITY_CLAVIS_KEYRING
+	bool "Clavis keyring"
+	depends on SECURITY
+	help
+	  Enable the clavis keyring. This keyring shall contain a single asymmetric key.
+	  This key shall be linked to a key already contained in one of the system
+	  keyrings (builtin, secondary, or platform).  The only way to add this key
+	  is during boot by passing in the asymmetric key id within the "clavis=" boot
+	  param.  This keyring is used by the Clavis LSM.
diff --git a/security/clavis/Makefile b/security/clavis/Makefile
new file mode 100644
index 000000000000..ff19c1e240fd
--- /dev/null
+++ b/security/clavis/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0
+
+obj-$(CONFIG_SECURITY_CLAVIS_KEYRING) += clavis_keyring.o
diff --git a/security/clavis/clavis_keyring.c b/security/clavis/clavis_keyring.c
new file mode 100644
index 000000000000..9f1aede81992
--- /dev/null
+++ b/security/clavis/clavis_keyring.c
@@ -0,0 +1,90 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/security.h>
+#include <keys/asymmetric-type.h>
+#include <keys/system_keyring.h>
+
+static struct key *clavis_keyring;
+static struct asymmetric_key_id *setup_keyid;
+
+#define MAX_BIN_KID   32
+
+static struct {
+	struct asymmetric_key_id id;
+	unsigned char data[MAX_BIN_KID];
+} setup_key;
+
+static int restrict_link_for_clavis(struct key *dest_keyring,
+		const struct key_type *type, const union key_payload *payload,
+		struct key *restrict_key)
+{
+	static bool first_pass = true;
+
+	/*
+	 * Allow a single asymmetric key into this keyring. This key is used as the
+	 * root of trust for anything added afterwards.
+	 */
+	if (type == &key_type_asymmetric && dest_keyring == clavis_keyring && first_pass) {
+		first_pass = false;
+		return 0;
+	}
+
+	return -EOPNOTSUPP;
+}
+
+static int __init clavis_param(char *kid)
+{
+	struct asymmetric_key_id *p = &setup_key.id;
+	int error, hex_len, ascii_len = strlen(kid);
+
+	if (!kid)
+		return 1;
+
+	hex_len = ascii_len / 2;
+
+	if (hex_len > sizeof(setup_key.data))
+		return 1;
+
+	p->len = hex_len;
+	error = hex2bin(p->data, kid, p->len);
+
+	if (error < 0)
+		pr_err("Unparsable clavis key id\n");
+	else {
+		setup_keyid = p;
+		pr_info("clavis key id: %s\n", kid);
+	}
+
+	return 1;
+}
+__setup("clavis=", clavis_param);
+
+static int __init clavis_keyring_init(void)
+{
+	struct key_restriction *restriction;
+
+	restriction = kzalloc(sizeof(*restriction), GFP_KERNEL);
+	if (!restriction)
+		panic("Can't allocate clavis keyring restriction\n");
+	restriction->check = restrict_link_for_clavis;
+	clavis_keyring = keyring_alloc(".clavis",
+			      GLOBAL_ROOT_UID, GLOBAL_ROOT_GID, current_cred(), KEY_POS_VIEW |
+			      KEY_POS_READ | KEY_POS_SEARCH | KEY_POS_WRITE | KEY_USR_VIEW |
+			      KEY_USR_READ | KEY_USR_SEARCH | KEY_USR_WRITE,
+			      KEY_ALLOC_NOT_IN_QUOTA | KEY_ALLOC_SET_KEEP,
+			      restriction, NULL);
+
+	if (IS_ERR(clavis_keyring))
+		panic("Can't allocate clavis keyring\n");
+
+	return 0;
+}
+
+void __init late_init_clavis_setup(void)
+{
+	if (!setup_keyid)
+		return;
+
+	clavis_keyring_init();
+	system_key_link(clavis_keyring, setup_keyid);
+}
diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index 068ac6c2ae1e..87a8bfc0662f 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -36,6 +36,8 @@ int integrity_kernel_read(struct file *file, loff_t offset,
  */
 void __init integrity_load_keys(void)
 {
+	late_init_clavis_setup();
+
 	ima_load_x509();
 
 	if (!IS_ENABLED(CONFIG_IMA_LOAD_X509))
-- 
2.39.3

[PATCH RFC 3/8] efi: Make clavis boot param persist across kexec

[Eric Snowberg]

Add the ability for the clavis boot param to persist across kexec.  This is
done by creating a RT variable before ExitBootServices is called. The new
variable is called Clavis with a new GUID
193ccef6-348b-4f1f-a81b-0ea4b899dbf2.  This variable does not have NVRAM
set, signifying it was created during the Boot Services phase.  This
variable will persist across a kexec, however it will not persist across
a power on reset.  This same type of functionality is currently used
within EFI shim to mirror MOK variables into the kernel.  It is being used
here so the clavis boot param can not be changed via kexec.  If a different
clavis boot param is used, the one stored in the RT variable will be used
instead. Enforcement of which boot param to use will be done in a follow
on patch.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
 drivers/firmware/efi/Kconfig                  | 12 +++++++
 drivers/firmware/efi/libstub/Makefile         |  1 +
 drivers/firmware/efi/libstub/clavis.c         | 33 +++++++++++++++++++
 .../firmware/efi/libstub/efi-stub-helper.c    |  2 ++
 drivers/firmware/efi/libstub/efi-stub.c       |  2 ++
 drivers/firmware/efi/libstub/efistub.h        |  8 +++++
 drivers/firmware/efi/libstub/x86-stub.c       |  2 ++
 include/linux/efi.h                           |  1 +
 8 files changed, 61 insertions(+)
 create mode 100644 drivers/firmware/efi/libstub/clavis.c

diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index 72f2537d90ca..8dcb5326d05d 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -186,6 +186,18 @@ config RESET_ATTACK_MITIGATION
 	  have been evicted, since otherwise it will trigger even on clean
 	  reboots.
 
+config EARLY_CLAVIS
+	bool "Early clavis"
+	depends on EFI_STUB
+	help
+	  Allow the clavis boot param to persist across kexec. This will create a
+	  variable called Clavis with a 193ccef6-348b-4f1f-a81b-0ea4b899dbf2 GUID.
+	  This variable does not have NVRAM set, signifying it was created during
+	  the Boot Services phase.  This variable will persist across a kexec,
+	  however it will not persist across a power on reset. During kexec, if
+	  a different clavis boot param is used, the one stored in the RT variable
+	  will be used instead.
+
 config EFI_RCI2_TABLE
 	bool "EFI Runtime Configuration Interface Table Version 2 Support"
 	depends on X86 || COMPILE_TEST
diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
index 06964a3c130f..c4aef94d877a 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -85,6 +85,7 @@ $(obj)/lib-%.o: $(srctree)/lib/%.c FORCE
 lib-$(CONFIG_EFI_GENERIC_STUB)	+= efi-stub.o string.o intrinsics.o systable.o \
 				   screen_info.o efi-stub-entry.o
 
+lib-$(CONFIG_EARLY_CLAVIS)	+= clavis.o
 lib-$(CONFIG_ARM)		+= arm32-stub.o
 lib-$(CONFIG_ARM64)		+= kaslr.o arm64.o arm64-stub.o smbios.o
 lib-$(CONFIG_X86)		+= x86-stub.o
diff --git a/drivers/firmware/efi/libstub/clavis.c b/drivers/firmware/efi/libstub/clavis.c
new file mode 100644
index 000000000000..3a715e87a13a
--- /dev/null
+++ b/drivers/firmware/efi/libstub/clavis.c
@@ -0,0 +1,33 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/efi.h>
+#include <asm/efi.h>
+#include "efistub.h"
+
+#define MAX_PARAM_LENGTH 64
+static const efi_char16_t clavis_param_name[] = L"Clavis";
+static const efi_guid_t clavis_guid = LINUX_EFI_CLAVIS_GUID;
+static unsigned char param_data[MAX_PARAM_LENGTH];
+static size_t param_len;
+
+void efi_parse_clavis(char *option)
+{
+	if (!option)
+		return;
+
+	param_len = strnlen(option, MAX_PARAM_LENGTH);
+	memcpy(param_data, option, param_len);
+}
+
+void efi_setup_clavis(void)
+{
+	efi_status_t error;
+
+	if (param_len) {
+		error = set_efi_var(clavis_param_name, &clavis_guid,
+				    EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS,
+				    param_len, &param_data);
+	}
+
+	if (error)
+		efi_err("Failed to set Clavis\n");
+}
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index bfa30625f5d0..0d9c6ac0a652 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -91,6 +91,8 @@ efi_status_t efi_parse_options(char const *cmdline)
 		} else if (!strcmp(param, "video") &&
 			   val && strstarts(val, "efifb:")) {
 			efi_parse_option_graphics(val + strlen("efifb:"));
+		} else if (!strcmp(param, "clavis") && val) {
+			efi_parse_clavis(val);
 		}
 	}
 	efi_bs_call(free_pool, buf);
diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c
index f9c1e8a2bd1d..16e50a124ce1 100644
--- a/drivers/firmware/efi/libstub/efi-stub.c
+++ b/drivers/firmware/efi/libstub/efi-stub.c
@@ -183,6 +183,8 @@ efi_status_t efi_stub_common(efi_handle_t handle,
 
 	install_memreserve_table();
 
+	efi_setup_clavis();
+
 	status = efi_boot_kernel(handle, image, image_addr, cmdline_ptr);
 
 	free_screen_info(si);
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index 212687c30d79..81a1f013f483 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -1060,6 +1060,14 @@ static inline void
 efi_enable_reset_attack_mitigation(void) { }
 #endif
 
+#ifdef CONFIG_EARLY_CLAVIS
+void efi_parse_clavis(char *option);
+void efi_setup_clavis(void);
+#else
+static inline void efi_parse_clavis(char *option) { }
+static inline void efi_setup_clavis(void) { }
+#endif
+
 void efi_retrieve_tpm2_eventlog(void);
 
 struct screen_info *alloc_screen_info(void);
diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index 0d510c9a06a4..08bd6224d17a 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -928,6 +928,8 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
 
 	setup_unaccepted_memory();
 
+	efi_setup_clavis();
+
 	status = exit_boot(boot_params, handle);
 	if (status != EFI_SUCCESS) {
 		efi_err("exit_boot() failed!\n");
diff --git a/include/linux/efi.h b/include/linux/efi.h
index c74f47711f0b..d0fe291d433e 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -420,6 +420,7 @@ void efi_native_runtime_setup(void);
 #define LINUX_EFI_UNACCEPTED_MEM_TABLE_GUID	EFI_GUID(0xd5d1de3c, 0x105c, 0x44f9,  0x9e, 0xa9, 0xbc, 0xef, 0x98, 0x12, 0x00, 0x31)
 
 #define RISCV_EFI_BOOT_PROTOCOL_GUID		EFI_GUID(0xccd15fec, 0x6f73, 0x4eec,  0x83, 0x95, 0x3e, 0x69, 0xe4, 0xb9, 0x40, 0xbf)
+#define LINUX_EFI_CLAVIS_GUID			EFI_GUID(0x193ccef6, 0x348b, 0x4f1f,  0xa8, 0x1b, 0x0e, 0xa4, 0xb8, 0x99, 0xdb, 0xf2)
 
 /*
  * This GUID may be installed onto the kernel image's handle as a NULL protocol
-- 
2.39.3

[PATCH RFC 4/8] clavis: Prevent clavis boot param from changing during kexec

[Eric Snowberg]

Use the new Clavis EFI RT variable to validate the clavis boot param didn't
change during a reboot. If the boot param is different or missing, use the
one stored in EFI instead. This will prevent a pivot in the root of trust
for the upcoming Clavis LSM.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
 security/clavis/Makefile         |  3 ++
 security/clavis/clavis.h         | 12 ++++++++
 security/clavis/clavis_efi.c     | 50 ++++++++++++++++++++++++++++++++
 security/clavis/clavis_keyring.c | 17 +++++++++--
 4 files changed, 80 insertions(+), 2 deletions(-)
 create mode 100644 security/clavis/clavis.h
 create mode 100644 security/clavis/clavis_efi.c

diff --git a/security/clavis/Makefile b/security/clavis/Makefile
index ff19c1e240fd..c008cb74c762 100644
--- a/security/clavis/Makefile
+++ b/security/clavis/Makefile
@@ -1,3 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 
 obj-$(CONFIG_SECURITY_CLAVIS_KEYRING) += clavis_keyring.o
+ifeq ($(CONFIG_EFI),y)
+obj-$(CONFIG_SECURITY_CLAVIS_KEYRING) += clavis_efi.o
+endif
diff --git a/security/clavis/clavis.h b/security/clavis/clavis.h
new file mode 100644
index 000000000000..3883c390b9e4
--- /dev/null
+++ b/security/clavis/clavis.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+struct asymmetric_key_id;
+
+#ifdef CONFIG_EFI
+int __init clavis_efi_param(struct asymmetric_key_id *kid, int len);
+#else
+static inline int __init clavis_efi_param(struct asymmetric_key_id *kid, int len)
+{
+	return -EINVAL;
+}
+#endif
diff --git a/security/clavis/clavis_efi.c b/security/clavis/clavis_efi.c
new file mode 100644
index 000000000000..7bc8ef03794a
--- /dev/null
+++ b/security/clavis/clavis_efi.c
@@ -0,0 +1,50 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <keys/asymmetric-type.h>
+#include <linux/efi.h>
+#include "clavis.h"
+
+static efi_char16_t clavis_param_name[] = L"Clavis";
+static efi_guid_t clavis_guid = LINUX_EFI_CLAVIS_GUID;
+
+int __init clavis_efi_param(struct asymmetric_key_id *kid, int len)
+{
+	unsigned char buf[64];
+	unsigned long ascii_len = sizeof(buf);
+	efi_status_t error;
+	int hex_len;
+	u32 attr;
+
+	if (!efi_enabled(EFI_BOOT)) {
+		pr_info("efi_enabled(EFI_BOOT) not set");
+		return -EPERM;
+	}
+
+	if (!efi_enabled(EFI_RUNTIME_SERVICES)) {
+		pr_info("%s : EFI runtime services are not enabled\n", __func__);
+		return -EPERM;
+	}
+
+	error = efi.get_variable(clavis_param_name, &clavis_guid, &attr, &ascii_len, &buf);
+
+	if (error) {
+		pr_err("Error reading clavis parm\n");
+		return -EINVAL;
+	}
+
+	if (attr & EFI_VARIABLE_NON_VOLATILE)  {
+		pr_info("Error: NV access set\n");
+		return -EINVAL;
+	} else if (ascii_len > 0) {
+		hex_len = ascii_len / 2;
+
+		if (hex_len > len) {
+			pr_info("invalid length\n");
+			return -EINVAL;
+		}
+		kid->len = hex_len;
+		return hex2bin(kid->data, buf, kid->len);
+	}
+
+	pr_info("Error: invalid size\n");
+	return -EINVAL;
+}
diff --git a/security/clavis/clavis_keyring.c b/security/clavis/clavis_keyring.c
index 9f1aede81992..c5606ed101f6 100644
--- a/security/clavis/clavis_keyring.c
+++ b/security/clavis/clavis_keyring.c
@@ -3,6 +3,7 @@
 #include <linux/security.h>
 #include <keys/asymmetric-type.h>
 #include <keys/system_keyring.h>
+#include "clavis.h"
 
 static struct key *clavis_keyring;
 static struct asymmetric_key_id *setup_keyid;
@@ -82,9 +83,21 @@ static int __init clavis_keyring_init(void)
 
 void __init late_init_clavis_setup(void)
 {
-	if (!setup_keyid)
+	int error;
+	struct {
+		struct asymmetric_key_id id;
+		unsigned char data[MAX_BIN_KID];
+	} efi_keyid;
+	struct asymmetric_key_id *keyid = &efi_keyid.id;
+
+	error = clavis_efi_param(keyid, sizeof(efi_keyid.data));
+
+	if (error && !setup_keyid)
 		return;
 
+	if (error)
+		keyid = setup_keyid;
+
 	clavis_keyring_init();
-	system_key_link(clavis_keyring, setup_keyid);
+	system_key_link(clavis_keyring, keyid);
 }
-- 
2.39.3

[PATCH RFC 5/8] keys: Add new verification type (VERIFYING_CLAVIS_SIGNATURE)

[Eric Snowberg]

Add a new verification type called VERIFYING_CLAVIS_SIGNATURE.  This new
usage will be used for validating keys added to the new clavis lsm keyring.
This will be introduced in a follow-on patch.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
 crypto/asymmetric_keys/asymmetric_type.c | 1 +
 crypto/asymmetric_keys/pkcs7_verify.c    | 1 +
 include/linux/verification.h             | 1 +
 3 files changed, 3 insertions(+)

diff --git a/crypto/asymmetric_keys/asymmetric_type.c b/crypto/asymmetric_keys/asymmetric_type.c
index a5da8ccd353e..7fdc006f18d6 100644
--- a/crypto/asymmetric_keys/asymmetric_type.c
+++ b/crypto/asymmetric_keys/asymmetric_type.c
@@ -25,6 +25,7 @@ const char *const key_being_used_for[NR__KEY_BEING_USED_FOR] = {
 	[VERIFYING_KEY_SIGNATURE]		= "key sig",
 	[VERIFYING_KEY_SELF_SIGNATURE]		= "key self sig",
 	[VERIFYING_UNSPECIFIED_SIGNATURE]	= "unspec sig",
+	[VERIFYING_CLAVIS_SIGNATURE]		= "clavis sig",
 };
 EXPORT_SYMBOL_GPL(key_being_used_for);
 
diff --git a/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c
index f0d4ff3c20a8..1dc80e68ce96 100644
--- a/crypto/asymmetric_keys/pkcs7_verify.c
+++ b/crypto/asymmetric_keys/pkcs7_verify.c
@@ -428,6 +428,7 @@ int pkcs7_verify(struct pkcs7_message *pkcs7,
 		}
 		/* Authattr presence checked in parser */
 		break;
+	case VERIFYING_CLAVIS_SIGNATURE:
 	case VERIFYING_UNSPECIFIED_SIGNATURE:
 		if (pkcs7->data_type != OID_data) {
 			pr_warn("Invalid unspecified sig (not pkcs7-data)\n");
diff --git a/include/linux/verification.h b/include/linux/verification.h
index cb2d47f28091..970f748b5cc9 100644
--- a/include/linux/verification.h
+++ b/include/linux/verification.h
@@ -36,6 +36,7 @@ enum key_being_used_for {
 	VERIFYING_KEY_SIGNATURE,
 	VERIFYING_KEY_SELF_SIGNATURE,
 	VERIFYING_UNSPECIFIED_SIGNATURE,
+	VERIFYING_CLAVIS_SIGNATURE,
 	NR__KEY_BEING_USED_FOR
 };
 extern const char *const key_being_used_for[NR__KEY_BEING_USED_FOR];
-- 
2.39.3

[PATCH RFC 6/8] keys: Add ability to track intended usage of the public key

[Eric Snowberg]

Add two new fields in public_key_signature to track the intended usage of
the signature.  Also add a flag for the revocation pass.  During signiture
validation, two verifications can take place for the same signature.  One
to see if it verifies against something on the .blacklist keyring and
the other to see if it verifies against the supplied keyring. The flag
is used to determine which stage the verification is in.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
 certs/blacklist.c                     |  3 +++
 crypto/asymmetric_keys/pkcs7_trust.c  | 20 ++++++++++++++++++++
 crypto/asymmetric_keys/pkcs7_verify.c |  4 ++++
 include/crypto/pkcs7.h                |  3 +++
 include/crypto/public_key.h           |  4 ++++
 5 files changed, 34 insertions(+)

diff --git a/certs/blacklist.c b/certs/blacklist.c
index 675dd7a8f07a..dd34e56a6362 100644
--- a/certs/blacklist.c
+++ b/certs/blacklist.c
@@ -17,6 +17,7 @@
 #include <linux/uidgid.h>
 #include <keys/asymmetric-type.h>
 #include <keys/system_keyring.h>
+#include <crypto/public_key.h>
 #include "blacklist.h"
 
 /*
@@ -289,7 +290,9 @@ int is_key_on_revocation_list(struct pkcs7_message *pkcs7)
 {
 	int ret;
 
+	pkcs7_set_usage_flag(pkcs7, PKS_REVOCATION_PASS);
 	ret = pkcs7_validate_trust(pkcs7, blacklist_keyring);
+	pkcs7_clear_usage_flag(pkcs7, PKS_REVOCATION_PASS);
 
 	if (ret == 0)
 		return -EKEYREJECTED;
diff --git a/crypto/asymmetric_keys/pkcs7_trust.c b/crypto/asymmetric_keys/pkcs7_trust.c
index 9a87c34ed173..64d70eb68864 100644
--- a/crypto/asymmetric_keys/pkcs7_trust.c
+++ b/crypto/asymmetric_keys/pkcs7_trust.c
@@ -131,6 +131,26 @@ static int pkcs7_validate_trust_one(struct pkcs7_message *pkcs7,
 	return 0;
 }
 
+void pkcs7_clear_usage_flag(struct pkcs7_message *pkcs7, unsigned long usage)
+{
+	struct pkcs7_signed_info *sinfo;
+
+	for (sinfo = pkcs7->signed_infos; sinfo; sinfo = sinfo->next) {
+		if (sinfo->sig)
+			clear_bit(usage, &sinfo->sig->usage_flags);
+	}
+}
+
+void pkcs7_set_usage_flag(struct pkcs7_message *pkcs7, unsigned long usage)
+{
+	struct pkcs7_signed_info *sinfo;
+
+	for (sinfo = pkcs7->signed_infos; sinfo; sinfo = sinfo->next) {
+		if (sinfo->sig)
+			set_bit(usage, &sinfo->sig->usage_flags);
+	}
+}
+
 /**
  * pkcs7_validate_trust - Validate PKCS#7 trust chain
  * @pkcs7: The PKCS#7 certificate to validate
diff --git a/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c
index 1dc80e68ce96..44b8bd0ad4d8 100644
--- a/crypto/asymmetric_keys/pkcs7_verify.c
+++ b/crypto/asymmetric_keys/pkcs7_verify.c
@@ -455,6 +455,10 @@ int pkcs7_verify(struct pkcs7_message *pkcs7,
 			return ret;
 		}
 		actual_ret = 0;
+		if (sinfo->sig) {
+			sinfo->sig->usage = usage;
+			set_bit(PKS_USAGE_SET, &sinfo->sig->usage_flags);
+		}
 	}
 
 	kleave(" = %d", actual_ret);
diff --git a/include/crypto/pkcs7.h b/include/crypto/pkcs7.h
index 38ec7f5f9041..6c3c9061b118 100644
--- a/include/crypto/pkcs7.h
+++ b/include/crypto/pkcs7.h
@@ -32,6 +32,9 @@ extern int pkcs7_get_content_data(const struct pkcs7_message *pkcs7,
 extern int pkcs7_validate_trust(struct pkcs7_message *pkcs7,
 				struct key *trust_keyring);
 
+extern void pkcs7_set_usage_flag(struct pkcs7_message *pkcs7, unsigned long usage);
+extern void pkcs7_clear_usage_flag(struct pkcs7_message *pkcs7, unsigned long usage);
+
 /*
  * pkcs7_verify.c
  */
diff --git a/include/crypto/public_key.h b/include/crypto/public_key.h
index 462f8a34cdf8..3ddfbc20a55f 100644
--- a/include/crypto/public_key.h
+++ b/include/crypto/public_key.h
@@ -48,6 +48,10 @@ struct public_key_signature {
 	const char *pkey_algo;
 	const char *hash_algo;
 	const char *encoding;
+	u32 usage;		/* Intended usage */
+	unsigned long usage_flags;
+#define PKS_USAGE_SET		0
+#define PKS_REVOCATION_PASS	1
 };
 
 extern void public_key_signature_free(struct public_key_signature *sig);
-- 
2.39.3

[PATCH RFC 7/8] clavis: Introduce a new key type called clavis_key_acl

[Eric Snowberg]

Introduce a new key type for keyring access control.  The new key type
is called clavis_key_acl.  The clavis_key_acl contains the subject key
identifer along with the allowed usage type for the key.

The format is as follows:

XX:YYYYYYYYYYY

XX - Single byte of the key type
	VERIFYING_MODULE_SIGNATURE            00
	VERIFYING_FIRMWARE_SIGNATURE          01
	VERIFYING_KEXEC_PE_SIGNATURE          02
	VERIFYING_KEY_SIGNATURE               03
	VERIFYING_KEY_SELF_SIGNATURE          04
	VERIFYING_UNSPECIFIED_SIGNATURE       05
:  - ASCII colon
YY - Even number of hexadecimal characters representing the key id

This key type will be used in the clavis keyring for access control. To
be added to the clavis keyring, the clavis_key_acl must be S/MIME signed
by the sole asymmetric key contained within it.

Below is an example of how this could be used. Within the example, the
key (b360d113c848ace3f1e6a80060b43d1206f0487d) is already in the machine
keyring. The intended usage for this key is to validate a signed kernel
for kexec:

echo "02:b360d113c848ace3f1e6a80060b43d1206f0487d" > kernel-acl.txt

The next step is to sign it:

openssl smime -sign -signer clavis-lsm.x509 -inkey clavis-lsm.priv -in \
	kernel-acl.txt  -out kernel-acl.pkcs7 -binary -outform DER \
	-nodetach -noattr

The final step is how to add the acl to the .clavis keyring:

keyctl padd clavis_key_acl "" %:.clavis < kernel-acl.pkcs7

Afterwards the new clavis_key_acl can be seen in the .clavis keyring:

keyctl show %:.clavis
Keyring
  keyring: .clavis
   \_ asymmetric: Clavis LSM key: 4a00ab9f35c9dc3aed7c225d22bafcbd9285e1e8
   \_ clavis_key_acl: 02:b360d113c848ace3f1e6a80060b43d1206f0487d

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
 security/clavis/clavis_keyring.c | 127 +++++++++++++++++++++++++++++++
 1 file changed, 127 insertions(+)

diff --git a/security/clavis/clavis_keyring.c b/security/clavis/clavis_keyring.c
index c5606ed101f6..0ab2341d47b8 100644
--- a/security/clavis/clavis_keyring.c
+++ b/security/clavis/clavis_keyring.c
@@ -1,13 +1,18 @@
 // SPDX-License-Identifier: GPL-2.0
 
 #include <linux/security.h>
+#include <linux/ctype.h>
 #include <keys/asymmetric-type.h>
+#include <keys/asymmetric-subtype.h>
 #include <keys/system_keyring.h>
+#include <keys/user-type.h>
+#include <crypto/pkcs7.h>
 #include "clavis.h"
 
 static struct key *clavis_keyring;
 static struct asymmetric_key_id *setup_keyid;
 
+#define MAX_ASCII_KID 64
 #define MAX_BIN_KID   32
 
 static struct {
@@ -15,6 +20,123 @@ static struct {
 	unsigned char data[MAX_BIN_KID];
 } setup_key;
 
+static int pkcs7_preparse_content(void *ctx, const void *data, size_t len,
+			      size_t asn1hdrlen)
+{
+	struct key_preparsed_payload *prep = ctx;
+	const void *saved_prep_data;
+	size_t saved_prep_datalen;
+	const char *p;
+	char *desc;
+	int ret, i;
+
+	/* clavis_free_prepase will free this */
+	desc = kmalloc(len, GFP_KERNEL);
+
+	if (!desc)
+		return -ENOMEM;
+	memcpy(desc, data, len);
+
+	/* remove any white space */
+	for (i = 0, p = desc; i < len; i++, p++) {
+		if (isspace(*p))
+			desc[i] = 0;
+	}
+
+	prep->description = desc;
+	saved_prep_data = prep->data;
+	saved_prep_datalen = prep->datalen;
+	prep->data = desc;
+	prep->datalen = len;
+	ret = user_preparse(prep);
+	prep->data = saved_prep_data;
+	prep->datalen = saved_prep_datalen;
+	return ret;
+}
+
+static void key_acl_free_preparse(struct key_preparsed_payload *prep)
+{
+	kfree(prep->description);
+	user_free_preparse(prep);
+}
+
+static int key_acl_preparse(struct key_preparsed_payload *prep)
+{
+	/* Only allow the description to be set via the pkcs7 data contents */
+	if (prep->orig_description)
+		return -EINVAL;
+
+	return verify_pkcs7_signature(NULL, 0, prep->data, prep->datalen, clavis_keyring,
+				      VERIFYING_CLAVIS_SIGNATURE, pkcs7_preparse_content,
+				      prep);
+}
+
+static int key_acl_instantiate(struct key *key, struct key_preparsed_payload *prep)
+{
+	key->perm |= KEY_USR_READ;
+	key->perm |= KEY_USR_SEARCH;
+	set_bit(KEY_FLAG_KEEP, &key->flags);
+	return generic_key_instantiate(key, prep);
+}
+
+static void key_acl_destroy(struct key *key)
+{
+	/* It should not be possible to get here */
+	pr_info("destroy clavis_key_acl denied\n");
+}
+
+static void key_acl_revoke(struct key *key)
+{
+	/* It should not be possible to get here */
+	pr_info("revoke clavis_key_acl denied\n");
+}
+
+static int key_acl_update(struct key *key, struct key_preparsed_payload *prep)
+{
+	return -EPERM;
+}
+
+static int key_acl_vet_description(const char *desc)
+{
+	unsigned char data[MAX_BIN_KID];
+	int ascii_len, hex_len, error;
+
+	ascii_len = strlen(desc);
+
+	/*
+	 * clavis_acl format:
+	 *    xx:yyyyyyyyy...
+	 *
+	 *    xx   - Single byte of the key type
+	 *    :    - Ascii colon
+	 *    yyyy - Even number of hexadecimal characters representing the keyid
+	 */
+	if (ascii_len < 5 || ascii_len > (MAX_ASCII_KID + 3) || desc[2] != ':')
+		return -EINVAL;
+
+	/* move past the colon */
+	ascii_len -= 3;
+	hex_len = ascii_len / 2;
+	error = hex2bin(data, desc+3, hex_len);
+
+	if (error < 0)
+		pr_err("Unparsable clavis key id\n");
+
+	return error;
+}
+
+static struct key_type clavis_key_acl = {
+	.name			= "clavis_key_acl",
+	.preparse		= key_acl_preparse,
+	.free_preparse		= key_acl_free_preparse,
+	.instantiate		= key_acl_instantiate,
+	.update			= key_acl_update,
+	.revoke			= key_acl_revoke,
+	.destroy		= key_acl_destroy,
+	.vet_description	= key_acl_vet_description,
+	.read			= user_read,
+};
+
 static int restrict_link_for_clavis(struct key *dest_keyring,
 		const struct key_type *type, const union key_payload *payload,
 		struct key *restrict_key)
@@ -30,6 +152,8 @@ static int restrict_link_for_clavis(struct key *dest_keyring,
 		return 0;
 	}
 
+	if (type == &clavis_key_acl)
+		return 0;
 	return -EOPNOTSUPP;
 }
 
@@ -64,6 +188,9 @@ static int __init clavis_keyring_init(void)
 {
 	struct key_restriction *restriction;
 
+	if (register_key_type(&clavis_key_acl) < 0)
+		panic("Can't allocate clavis key type\n");
+
 	restriction = kzalloc(sizeof(*restriction), GFP_KERNEL);
 	if (!restriction)
 		panic("Can't allocate clavis keyring restriction\n");
-- 
2.39.3

[PATCH RFC 8/8] clavis: Introduce new LSM called clavis

[Eric Snowberg]

Introduce a new LSM called clavis.  The motivation behind this LSM is to
provide access control for system keys.  The access control list is
contained within a keyring call .clavis.  During boot if the clavis= boot
arg is supplied with a key id contained within any of the current system
keyrings (builtin, secondary, machine, or platform) it shall be used as
the root of trust for validating anything that is added to the ACL list.

The first restriction introduced with this LSM is the ability to enforce
key usage.  The kernel already has a notion of tracking key usage.  This
LSM adds the ability to enforce this usage based on the system owners
configuration.

Each system key may have one or more uses defined within the ACL list.
When this LSM is enabled, only the builtin keys are available for loading
kernel modules and doing a kexec.  Until an entry is added to the .clavis
keyring, no other system key may be used for any other purpose.

In the future it is envisioned this LSM could be enhanced to provide
access control for UEFI Secure Boot Advanced Targeting (SBAT).  Using
the same clavis= boot param and storing the additional contents within
the new RT UEFI var, SBAT restrictions could be maintained across kexec.

Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
---
 Documentation/admin-guide/LSM/clavis.rst | 190 +++++++++++++++++++++++
 MAINTAINERS                              |   7 +
 crypto/asymmetric_keys/signature.c       |   4 +
 include/linux/lsm_hook_defs.h            |   2 +
 include/linux/security.h                 |   7 +
 include/uapi/linux/lsm.h                 |   1 +
 security/Kconfig                         |  10 +-
 security/clavis/Makefile                 |   1 +
 security/clavis/clavis.c                 |  25 +++
 security/clavis/clavis.h                 |   4 +
 security/clavis/clavis_keyring.c         |  83 ++++++++++
 security/security.c                      |  16 +-
 12 files changed, 344 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/admin-guide/LSM/clavis.rst
 create mode 100644 security/clavis/clavis.c

diff --git a/Documentation/admin-guide/LSM/clavis.rst b/Documentation/admin-guide/LSM/clavis.rst
new file mode 100644
index 000000000000..b0a73defb4fc
--- /dev/null
+++ b/Documentation/admin-guide/LSM/clavis.rst
@@ -0,0 +1,190 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=====
+Clavis
+=====
+
+Clavis is a Linux Security Module that provides mandatory access control to
+system kernel keys (i.e. builtin, secondary, machine and platform). These
+restrictions will prohit keys from being used for validation. Upon boot, the
+Clavis LSM is provided a key id as a boot param.  This single key is then
+used as the root of trust for any access control modifications made going
+forward. Access control updates must be signed and validated by this key.
+
+Clavis has its own keyring.  All ACL updates are applied through this keyring.
+The update must be signed by the single root of trust key.
+
+When enabled, all system keys are prohibited from being used until an ACL is
+added for it. There is two exceptions to this rule, builtin keys may be used
+to validate both signed kernels and modules.
+
+Adding system kernel keys can only be performed by the machine owner, this
+could be through the Machine Owner Key (MOK) or the UEFI Secure Boot DB. It
+is possible the machine owner and system administrator may be different
+people. The system administrator will not be able to make ACL updates without
+them being signed by the machine owner.
+
+On UEFI platforms, the root of trust key shall survive a kexec. Trying to
+defeat or change it from the command line is not allowed.  The original boot
+param is stored in UEFI and will always be referenced following a kexec.
+
+The Clavis LSM contains a system keyring call .clavis.  It contains a single
+asymmetric key that is use to validate anything added to it.  This key can only
+be added during boot and must be a preexisting system kernel key.  If the
+clavis= boot param is not used, the keyring does not exist and the feature
+can not be used until the next power on reset.
+
+The only user space components are OpenSSL and the keyctl utility. A new
+key type call clavis_key_acl is used for ACL updates. Any number of signed
+clavis_key_acl entries may be added to the .clavis keyring. The clavis_key_acl
+contains the subject key identifer along with the allowed usage type for
+the key.
+
+The format is as follows:
+::
+
+  XX:YYYYYYYYYYY
+
+  XX - Single byte of the key type
+	VERIFYING_MODULE_SIGNATURE            00
+	VERIFYING_FIRMWARE_SIGNATURE          01
+	VERIFYING_KEXEC_PE_SIGNATURE          02
+	VERIFYING_KEY_SIGNATURE               03
+	VERIFYING_KEY_SELF_SIGNATURE          04
+	VERIFYING_UNSPECIFIED_SIGNATURE       05
+  :  - ASCII colon
+  YY - Even number of hexadecimal characters representing the key id
+
+The clavis_key_acl must be S/MIME signed by the sole asymmetric key contained
+within the .clavis keyring.
+
+In the future if new features are added, new key types could be created.
+
+Usage Examples
+==============
+
+How to create a signing key:
+----------------------------
+
+::
+
+  cat <<EOF > clavis-lsm.genkey
+  [ req ]
+  default_bits = 4096
+  distinguished_name = req_distinguished_name
+  prompt = no
+  string_mask = utf8only
+  x509_extensions = v3_ca
+  [ req_distinguished_name ]
+  O = TEST
+  CN = Clavis LSM key
+  emailAddress = john.doe@foo.com
+  [ v3_ca ]
+  basicConstraints=CA:TRUE
+  subjectKeyIdentifier=hash
+  authorityKeyIdentifier=keyid:always,issuer
+  keyUsage=digitalSignature
+  EOF
+
+  openssl req -new -x509 -utf8 -sha256 -days 3650 -batch \
+        -config clavis-lsm.genkey -outform DER \
+        -out clavis-lsm.x509 -keyout clavis-lsm.priv
+
+How to get the Subject Key Identifier
+-------------------------------------
+::
+
+  openssl x509 -in ./clavis-lsm.x509 -inform der \
+        -ext subjectKeyIdentifier  -nocert \
+        | tail -n +2 | cut -f2 -d '='| tr -d ':'
+  4a00ab9f35c9dc3aed7c225d22bafcbd9285e1e8
+
+How to enroll the signing key into the MOK
+------------------------------------------
+
+The key must now be added to the machine or platform keyrings.  This
+indicates the key was added by the system owner. To add to the machine
+keyring on x86 do:
+::
+
+  mokutil --import ./clavis-lsm.x509
+
+and then reboot and enroll the key through the MokManager.
+
+How to enable the Clavis LSM
+----------------------------
+
+Add the key id to the clavis= boot param.  With the example above the
+key id is the subject key identifer: 4a00ab9f35c9dc3aed7c225d22bafcbd9285e1e8
+
+Add the following boot param:
+::
+
+  clavis=4a00ab9f35c9dc3aed7c225d22bafcbd9285e1e8
+
+After booting there will be a single key contained in the .clavis keyring:
+::
+
+  $ keyctl show %:.clavis
+  Keyring
+    254954913 ----swrv      0     0  keyring: .clavis
+    301905375 ---lswrv      0     0   \_ asymmetric: TEST: Clavis LSM key: 4a00ab9f35c9dc3aed7c225d22bafcbd9285e1e8
+
+The original clavis= boot param will persist across any kexec. Changing it or
+removing it has no effect.
+
+
+How to sign an entry to be added to the .clavis keyring:
+-------------------------------------------------------
+
+In this example we have 3 keys in the machine keyring.  Our Clavis LSM key, a
+key we want to use for kernel verification and a key we want to use for module
+verification.
+::
+
+  $ keyctl show %:.machine
+   Keyring
+    999488265 ---lswrv      0     0  keyring: .machine
+    912608009 ---lswrv      0     0   \_ asymmetric: TEST: Module Key: 17eb8c5bf766364be094c577625213700add9471
+    646229664 ---lswrv      0     0   \_ asymmetric: TEST: Kernel Key: b360d113c848ace3f1e6a80060b43d1206f0487d
+   1073737099 ---lswrv      0     0   \_ asymmetric: TEST: Clavis LSM key: 4a00ab9f35c9dc3aed7c225d22bafcbd9285e1e8
+
+To update the .clavis kerying acl list.  First create a file containing the
+key usage type followed by a colon and the key id that we want to allow to
+validate that usage.  In the first example we are saying key
+17eb8c5bf766364be094c577625213700add9471 is allowed to validate kernel modules.
+In the second example we are saying key b360d113c848ace3f1e6a80060b43d1206f0487d
+is allowed to validate signed kernels.
+
+::
+
+  echo "00:17eb8c5bf766364be094c577625213700add9471" > module-acl.txt
+  echo "02:b360d113c848ace3f1e6a80060b43d1206f0487d" > kernel-acl.txt
+
+Now both these files must be signed by the key contained in the .clavis keyring:
+
+::
+
+  openssl smime -sign -signer clavis-lsm.x509 -inkey clavis-lsm.priv -in module-acl.txt \
+        -out module-acl.pkcs7 -binary -outform DER -nodetach -noattr
+
+  openssl smime -sign -signer clavis-lsm.x509 -inkey clavis-lsm.priv -in kernel-acl.txt \
+        -out kernel-acl.pkcs7 -binary -outform DER -nodetach -noattr
+
+Afterwards the ACL list in the clavis keyring can be updated:
+::
+
+  keyctl padd clavis_key_acl "" %:.clavis < module-acl.pkcs7
+  keyctl padd clavis_key_acl "" %:.clavis < kernel-acl.pkcs7
+
+  keyctl show %:.clavis
+
+  Keyring
+    254954913 ----swrv      0     0  keyring: .clavis
+    301905375 ---lswrv      0     0   \_ asymmetric: TEST: Clavis LSM key: 4a00ab9f35c9dc3aed7c225d22bafcbd9285e1e8
+   1013065475 --alswrv      0     0   \_ clavis_key_acl: 02:b360d113c848ace3f1e6a80060b43d1206f0487d
+    445581284 --alswrv      0     0   \_ clavis_key_acl: 00:17eb8c5bf766364be094c577625213700add9471
+
+Now the 17eb8c5bf766364be094c577625213700add9471 key can be used for
+validating kernel modules and the b360d113c848ace3f1e6a80060b43d1206f0487d
+key can be used to validate signed kernels.
diff --git a/MAINTAINERS b/MAINTAINERS
index 960512bec428..64725ea4f881 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5137,6 +5137,13 @@ F:	scripts/Makefile.clang
 F:	scripts/clang-tools/
 K:	\b(?i:clang|llvm)\b
 
+CLAVIS LINUX SECURITY MODULE
+M:	Eric Snowberg <eric.snowberg@oracle.com>
+L:	linux-security-module@vger.kernel.org
+S:	Maintained
+F:	Documentation/admin-guide/LSM/clavis.rst
+F:	security/clavis
+
 CLK API
 M:	Russell King <linux@armlinux.org.uk>
 L:	linux-clk@vger.kernel.org
diff --git a/crypto/asymmetric_keys/signature.c b/crypto/asymmetric_keys/signature.c
index 398983be77e8..19892e1aa18b 100644
--- a/crypto/asymmetric_keys/signature.c
+++ b/crypto/asymmetric_keys/signature.c
@@ -13,6 +13,7 @@
 #include <linux/err.h>
 #include <linux/slab.h>
 #include <linux/keyctl.h>
+#include <linux/security.h>
 #include <crypto/public_key.h>
 #include <keys/user-type.h>
 #include "asymmetric_keys.h"
@@ -153,6 +154,9 @@ int verify_signature(const struct key *key,
 
 	ret = subtype->verify_signature(key, sig);
 
+	if (!ret)
+		ret = security_key_verify_signature(key, sig);
+
 	pr_devel("<==%s() = %d\n", __func__, ret);
 	return ret;
 }
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 7e4683035d34..d18bf16feb2c 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -408,6 +408,8 @@ LSM_HOOK(int, 0, key_getsecurity, struct key *key, char **buffer)
 LSM_HOOK(void, LSM_RET_VOID, key_post_create_or_update, struct key *keyring,
 	 struct key *key, const void *payload, size_t payload_len,
 	 unsigned long flags, bool create)
+LSM_HOOK(int, 0, key_verify_signature, const struct key *key,
+	 const struct public_key_signature *sig)
 #endif /* CONFIG_KEYS */
 
 #ifdef CONFIG_AUDIT
diff --git a/include/linux/security.h b/include/linux/security.h
index 94661398708a..e672a180714d 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -62,6 +62,7 @@ enum fs_value_type;
 struct watch;
 struct watch_notification;
 struct lsm_ctx;
+struct public_key_signature;
 
 /* Default (no) options for the capable function */
 #define CAP_OPT_NONE 0x0
@@ -2009,6 +2010,7 @@ void security_key_post_create_or_update(struct key *keyring, struct key *key,
 					const void *payload, size_t payload_len,
 					unsigned long flags, bool create);
 
+int security_key_verify_signature(const struct key *key, const struct public_key_signature *sig);
 #else
 
 static inline int security_key_alloc(struct key *key,
@@ -2043,6 +2045,11 @@ static inline void security_key_post_create_or_update(struct key *keyring,
 						      bool create)
 { }
 
+static inline int security_key_verify_signature(const struct key *key,
+						const struct public_key_signature *sig)
+{
+	return 0;
+}
 #endif
 #endif /* CONFIG_KEYS */
 
diff --git a/include/uapi/linux/lsm.h b/include/uapi/linux/lsm.h
index 33d8c9f4aa6b..3a60c4ff5186 100644
--- a/include/uapi/linux/lsm.h
+++ b/include/uapi/linux/lsm.h
@@ -64,6 +64,7 @@ struct lsm_ctx {
 #define LSM_ID_LANDLOCK		110
 #define LSM_ID_IMA		111
 #define LSM_ID_EVM		112
+#define LSM_ID_CLAVIS		113
 
 /*
  * LSM_ATTR_XXX definitions identify different LSM attributes
diff --git a/security/Kconfig b/security/Kconfig
index 7f5a52b7cefd..f158c0e6b8dc 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -234,11 +234,11 @@ endchoice
 
 config LSM
 	string "Ordered list of enabled LSMs"
-	default "landlock,lockdown,yama,loadpin,safesetid,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK
-	default "landlock,lockdown,yama,loadpin,safesetid,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR
-	default "landlock,lockdown,yama,loadpin,safesetid,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO
-	default "landlock,lockdown,yama,loadpin,safesetid,bpf" if DEFAULT_SECURITY_DAC
-	default "landlock,lockdown,yama,loadpin,safesetid,selinux,smack,tomoyo,apparmor,bpf"
+	default "clavis,landlock,lockdown,yama,loadpin,safesetid,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK
+	default "clavis,landlock,lockdown,yama,loadpin,safesetid,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR
+	default "clavis,landlock,lockdown,yama,loadpin,safesetid,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO
+	default "clavis,landlock,lockdown,yama,loadpin,safesetid,bpf" if DEFAULT_SECURITY_DAC
+	default "clavis,landlock,lockdown,yama,loadpin,safesetid,selinux,smack,tomoyo,apparmor,bpf"
 	help
 	  A comma-separated list of LSMs, in initialization order.
 	  Any LSMs left off this list, except for those with order
diff --git a/security/clavis/Makefile b/security/clavis/Makefile
index c008cb74c762..60f72cf96886 100644
--- a/security/clavis/Makefile
+++ b/security/clavis/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 
 obj-$(CONFIG_SECURITY_CLAVIS_KEYRING) += clavis_keyring.o
+obj-$(CONFIG_SECURITY_CLAVIS_KEYRING) += clavis.o
 ifeq ($(CONFIG_EFI),y)
 obj-$(CONFIG_SECURITY_CLAVIS_KEYRING) += clavis_efi.o
 endif
diff --git a/security/clavis/clavis.c b/security/clavis/clavis.c
new file mode 100644
index 000000000000..040337dbd8d9
--- /dev/null
+++ b/security/clavis/clavis.c
@@ -0,0 +1,25 @@
+// SPDX-License-Identifier: GPL-2.0
+//
+#include <linux/lsm_hooks.h>
+#include <uapi/linux/lsm.h>
+#include "clavis.h"
+
+static struct security_hook_list clavis_hooks[] __ro_after_init = {
+	LSM_HOOK_INIT(key_verify_signature, clavis_sig_verify),
+};
+
+const struct lsm_id clavis_lsmid = {
+	.name = "clavis",
+	.id = LSM_ID_CLAVIS,
+};
+
+static int __init clavis_lsm_init(void)
+{
+	security_add_hooks(clavis_hooks, ARRAY_SIZE(clavis_hooks), &clavis_lsmid);
+	return 0;
+};
+
+DEFINE_LSM(clavis) = {
+	.name = "clavis",
+	.init = clavis_lsm_init,
+};
diff --git a/security/clavis/clavis.h b/security/clavis/clavis.h
index 3883c390b9e4..886a85d82518 100644
--- a/security/clavis/clavis.h
+++ b/security/clavis/clavis.h
@@ -1,5 +1,7 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 
+struct key;
+struct public_key_signature;
 struct asymmetric_key_id;
 
 #ifdef CONFIG_EFI
@@ -10,3 +12,5 @@ static inline int __init clavis_efi_param(struct asymmetric_key_id *kid, int len
 	return -EINVAL;
 }
 #endif
+
+int clavis_sig_verify(const struct key *key, const struct public_key_signature *sig);
diff --git a/security/clavis/clavis_keyring.c b/security/clavis/clavis_keyring.c
index 0ab2341d47b8..cc4527b417e1 100644
--- a/security/clavis/clavis_keyring.c
+++ b/security/clavis/clavis_keyring.c
@@ -12,6 +12,7 @@
 static struct key *clavis_keyring;
 static struct asymmetric_key_id *setup_keyid;
 
+static int clavis_init;
 #define MAX_ASCII_KID 64
 #define MAX_BIN_KID   32
 
@@ -227,4 +228,86 @@ void __init late_init_clavis_setup(void)
 
 	clavis_keyring_init();
 	system_key_link(clavis_keyring, keyid);
+	clavis_init = true;
+}
+
+int clavis_sig_verify(const struct key *key, const struct public_key_signature *sig)
+{
+	const struct asymmetric_key_ids *kids = asymmetric_key_ids(key);
+	const struct asymmetric_key_subtype *subtype;
+	const struct asymmetric_key_id *newkid;
+	char *buf_ptr, *ptr;
+	key_ref_t ref;
+	int i, buf_len;
+
+	if (!clavis_init)
+		return 0;
+
+	if (key->type != &key_type_asymmetric)
+		return -EKEYREJECTED;
+	subtype = asymmetric_key_subtype(key);
+	if (!subtype || !key->payload.data[0])
+		return -EKEYREJECTED;
+	if (!subtype->verify_signature)
+		return -EKEYREJECTED;
+
+	/* Allow sig validation when not using a system keyring */
+	if (!test_bit(PKS_USAGE_SET, &sig->usage_flags))
+		return 0;
+
+	if (test_bit(KEY_FLAG_BUILTIN, &key->flags) && sig->usage == VERIFYING_MODULE_SIGNATURE)
+		return 0;
+
+	if (test_bit(KEY_FLAG_BUILTIN, &key->flags) && sig->usage == VERIFYING_KEXEC_PE_SIGNATURE)
+		return 0;
+
+	/* The previous sig validation is enough to get on the clavis keyring */
+	if (sig->usage == VERIFYING_CLAVIS_SIGNATURE)
+		return 0;
+
+	if (test_bit(PKS_REVOCATION_PASS, &sig->usage_flags))
+		return 0;
+
+	for (i = 0, buf_len = 0; i < 3; i++) {
+		if (kids->id[i]) {
+			newkid = (struct asymmetric_key_id *)kids->id[i];
+			if (newkid->len > buf_len)
+				buf_len = newkid->len;
+		}
+	}
+
+	if (!buf_len)
+		return -EKEYREJECTED;
+
+	/* Allocate enough space for the conversion to ascii plus the header. */
+	buf_ptr = kmalloc(buf_len * 2 + 4, GFP_KERNEL | __GFP_ZERO);
+
+	if (!buf_ptr)
+		return -ENOMEM;
+
+	for (i = 0; i < 3; i++) {
+		if (kids->id[i]) {
+			newkid = (struct asymmetric_key_id *)kids->id[i];
+			if (!newkid->len)
+				continue;
+
+			ptr = buf_ptr;
+			ptr = bin2hex(ptr, &sig->usage, 1);
+			*ptr++ = ':';
+			ptr = bin2hex(ptr, newkid->data, newkid->len);
+			*ptr = 0;
+			ref = keyring_search(make_key_ref(clavis_keyring, true), &clavis_key_acl,
+					     buf_ptr, false);
+
+			if (!IS_ERR(ref))
+				break;
+		}
+	}
+
+	kfree(buf_ptr);
+
+	if (IS_ERR(ref))
+		return -EKEYREJECTED;
+
+	return 0;
 }
diff --git a/security/security.c b/security/security.c
index 4cb832b00c40..d1da60a1b7a4 100644
--- a/security/security.c
+++ b/security/security.c
@@ -50,7 +50,8 @@
 	(IS_ENABLED(CONFIG_BPF_LSM) ? 1 : 0) + \
 	(IS_ENABLED(CONFIG_SECURITY_LANDLOCK) ? 1 : 0) + \
 	(IS_ENABLED(CONFIG_IMA) ? 1 : 0) + \
-	(IS_ENABLED(CONFIG_EVM) ? 1 : 0))
+	(IS_ENABLED(CONFIG_EVM) ? 1 : 0) + \
+	(IS_ENABLED(CONFIG_SECURITY_CLAVIS_KEYRING) ? 1 : 0))
 
 /*
  * These are descriptions of the reasons that can be passed to the
@@ -5313,6 +5314,19 @@ void security_key_post_create_or_update(struct key *keyring, struct key *key,
 	call_void_hook(key_post_create_or_update, keyring, key, payload,
 		       payload_len, flags, create);
 }
+
+/**
+ * security_key_verify_signature - verify signature
+ * @key: key
+ * @public_key_signature: signature
+ *
+ * See wheather signature verification is allowed based on the ACL for
+ * key usage.
+ */
+int security_key_verify_signature(const struct key *key, const struct public_key_signature *sig)
+{
+	return call_int_hook(key_verify_signature, key, sig);
+}
 #endif	/* CONFIG_KEYS */
 
 #ifdef CONFIG_AUDIT
-- 
2.39.3

Re: [PATCH RFC 1/8] certs: Introduce ability to link to a system key

[Jarkko Sakkinen]

On Mon Mar 11, 2024 at 6:11 PM EET, Eric Snowberg wrote:
> Introduce a new function to allow a keyring to link to a key contained
> within one of the system keyrings (builtin, secondary, or platform).
> Depending on how the kernel is built, if the machine keyring is
> available, it will be checked as well, since it is linked to the secondary
> keyring. If the asymmetric key id matches a key within one of these
> system keyrings, the matching key is linked into the passed in
> keyring.
>
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> ---
>  certs/system_keyring.c        | 29 +++++++++++++++++++++++++++++
>  include/keys/system_keyring.h |  7 ++++++-
>  2 files changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index 9de610bf1f4b..b647be49f6e0 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -426,3 +426,32 @@ void __init set_platform_trusted_keys(struct key *keyring)
>  	platform_trusted_keys = keyring;
>  }
>  #endif
> +
> +/**
> + * system_key_link - Link to a system key
> + * @keyring: The keyring to link into
> + * @id: The asymmetric key id to look for in the system keyring
> + */
> +int system_key_link(struct key *keyring, struct asymmetric_key_id *id)
> +{
> +	struct key *tkey;

I'd suggest to replace this with just 'tkey'. Single obscure character
does not bring any readability value.

> +
> +#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
> +	tkey = find_asymmetric_key(secondary_trusted_keys, id, NULL, NULL, false);
> +#else
> +	tkey = find_asymmetric_key(builtin_trusted_keys, id, NULL, NULL, false);
> +#endif

I'd have just single call site here and inside ifdef-shenanigans i'd
just set helper "keyring" to point to the appropriate keyring.

> +	if (!IS_ERR(tkey))
> +		goto found;
> +
> +	tkey = find_asymmetric_key(platform_trusted_keys, id, NULL, NULL, false);
> +

Please remove this empty line as the check below is associated with it.

> +	if (!IS_ERR(tkey))
> +		goto found;
> +
> +	return -1;
> +
> +found:
> +	key_link(keyring, tkey);
> +	return 0;
> +}
> diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
> index 8365adf842ef..b47ac8e2001a 100644
> --- a/include/keys/system_keyring.h
> +++ b/include/keys/system_keyring.h
> @@ -9,6 +9,7 @@
>  #define _KEYS_SYSTEM_KEYRING_H
>  
>  #include <linux/key.h>
> +struct asymmetric_key_id;
>  
>  enum blacklist_hash_type {
>  	/* TBSCertificate hash */
> @@ -28,7 +29,7 @@ int restrict_link_by_digsig_builtin(struct key *dest_keyring,
>  				    const union key_payload *payload,
>  				    struct key *restriction_key);
>  extern __init int load_module_cert(struct key *keyring);
> -
> +extern int system_key_link(struct key *keyring, struct asymmetric_key_id *id);
>  #else
>  #define restrict_link_by_builtin_trusted restrict_link_reject
>  #define restrict_link_by_digsig_builtin restrict_link_reject
> @@ -38,6 +39,10 @@ static inline __init int load_module_cert(struct key *keyring)
>  	return 0;
>  }
>  
> +static inline int system_key_link(struct key *keyring, struct asymmetric_key_id *id)
> +{
> +	return 0;
> +}
>  #endif
>  
>  #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING

BR, Jarkko

Re: [PATCH RFC 1/8] certs: Introduce ability to link to a system key

[Jarkko Sakkinen]

On Mon Mar 11, 2024 at 6:11 PM EET, Eric Snowberg wrote:
> +	return -1;

Missed this one: why a magic number?

BR, Jarkko

Re: [PATCH RFC 1/8] certs: Introduce ability to link to a system key

[Eric Snowberg]

> On Mar 11, 2024, at 1:16 PM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> 
> On Mon Mar 11, 2024 at 6:11 PM EET, Eric Snowberg wrote:
>> Introduce a new function to allow a keyring to link to a key contained
>> within one of the system keyrings (builtin, secondary, or platform).
>> Depending on how the kernel is built, if the machine keyring is
>> available, it will be checked as well, since it is linked to the secondary
>> keyring. If the asymmetric key id matches a key within one of these
>> system keyrings, the matching key is linked into the passed in
>> keyring.
>> 
>> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
>> ---
>> certs/system_keyring.c        | 29 +++++++++++++++++++++++++++++
>> include/keys/system_keyring.h |  7 ++++++-
>> 2 files changed, 35 insertions(+), 1 deletion(-)
>> 
>> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
>> index 9de610bf1f4b..b647be49f6e0 100644
>> --- a/certs/system_keyring.c
>> +++ b/certs/system_keyring.c
>> @@ -426,3 +426,32 @@ void __init set_platform_trusted_keys(struct key *keyring)
>> platform_trusted_keys = keyring;
>> }
>> #endif
>> +
>> +/**
>> + * system_key_link - Link to a system key
>> + * @keyring: The keyring to link into
>> + * @id: The asymmetric key id to look for in the system keyring
>> + */
>> +int system_key_link(struct key *keyring, struct asymmetric_key_id *id)
>> +{
>> + struct key *tkey;
> 
> I'd suggest to replace this with just 'tkey'. Single obscure character
> does not bring any readability value.

I assume you mean replace it with "key"?  If so, yes, I'll change that in the next round.

>> +
>> +#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
>> + tkey = find_asymmetric_key(secondary_trusted_keys, id, NULL, NULL, false);
>> +#else
>> + tkey = find_asymmetric_key(builtin_trusted_keys, id, NULL, NULL, false);
>> +#endif
> 
> I'd have just single call site here and inside ifdef-shenanigans i'd
> just set helper "keyring" to point to the appropriate keyring.

I'll also make this change

>> + if (!IS_ERR(tkey))
>> + goto found;
>> +
>> + tkey = find_asymmetric_key(platform_trusted_keys, id, NULL, NULL, false);
>> +
> 
> Please remove this empty line as the check below is associated with it.

and remove the empty line.    Thanks.

Re: [PATCH RFC 1/8] certs: Introduce ability to link to a system key

[Eric Snowberg]

> On Mar 11, 2024, at 1:18 PM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> 
> On Mon Mar 11, 2024 at 6:11 PM EET, Eric Snowberg wrote:
>> + return -1;
> 
> Missed this one: why a magic number?

Good point, I'll change this to return -ENOKEY.  Thanks.

Re: [PATCH RFC 8/8] clavis: Introduce new LSM called clavis

[Randy Dunlap]

On 3/11/24 09:11, Eric Snowberg wrote:
> In the future it is envisioned this LSM could be enhanced to provide
> access control for UEFI Secure Boot Advanced Targeting (SBAT).  Using
> the same clavis= boot param and storing the additional contents within
> the new RT UEFI var, SBAT restrictions could be maintained across kexec.

What does "RT" mean here?

(more below)

> 
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> ---
>  Documentation/admin-guide/LSM/clavis.rst | 190 +++++++++++++++++++++++
>  MAINTAINERS                              |   7 +
>  crypto/asymmetric_keys/signature.c       |   4 +
>  include/linux/lsm_hook_defs.h            |   2 +
>  include/linux/security.h                 |   7 +
>  include/uapi/linux/lsm.h                 |   1 +
>  security/Kconfig                         |  10 +-
>  security/clavis/Makefile                 |   1 +
>  security/clavis/clavis.c                 |  25 +++
>  security/clavis/clavis.h                 |   4 +
>  security/clavis/clavis_keyring.c         |  83 ++++++++++
>  security/security.c                      |  16 +-
>  12 files changed, 344 insertions(+), 6 deletions(-)
>  create mode 100644 Documentation/admin-guide/LSM/clavis.rst
>  create mode 100644 security/clavis/clavis.c
> 


> diff --git a/Documentation/admin-guide/LSM/clavis.rst b/Documentation/admin-guide/LSM/clavis.rst
> new file mode 100644
> index 000000000000..b0a73defb4fc
> --- /dev/null
> +++ b/Documentation/admin-guide/LSM/clavis.rst
> @@ -0,0 +1,190 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +=====
> +Clavis
> +=====
> +
> +Clavis is a Linux Security Module that provides mandatory access control to
> +system kernel keys (i.e. builtin, secondary, machine and platform). These
> +restrictions will prohit keys from being used for validation. Upon boot, the

                     prohibit

> +Clavis LSM is provided a key id as a boot param.  This single key is then
> +used as the root of trust for any access control modifications made going
> +forward. Access control updates must be signed and validated by this key.
> +
> +Clavis has its own keyring.  All ACL updates are applied through this keyring.
> +The update must be signed by the single root of trust key.
> +
> +When enabled, all system keys are prohibited from being used until an ACL is
> +added for it. There is two exceptions to this rule, builtin keys may be used

What is     "it"?  The predecessor seems to be "all system keys" (plural).


> +to validate both signed kernels and modules.
> +
> +Adding system kernel keys can only be performed by the machine owner, this

                                                                  owner;

> +could be through the Machine Owner Key (MOK) or the UEFI Secure Boot DB. It
> +is possible the machine owner and system administrator may be different
> +people. The system administrator will not be able to make ACL updates without
> +them being signed by the machine owner.
> +
> +On UEFI platforms, the root of trust key shall survive a kexec. Trying to
> +defeat or change it from the command line is not allowed.  The original boot
> +param is stored in UEFI and will always be referenced following a kexec.
> +
> +The Clavis LSM contains a system keyring call .clavis.  It contains a single
> +asymmetric key that is use to validate anything added to it.  This key can only
> +be added during boot and must be a preexisting system kernel key.  If the
> +clavis= boot param is not used, the keyring does not exist and the feature
> +can not be used until the next power on reset.

So just a reboot won't cause it to be used?  Must be power off/on?

> +
> +The only user space components are OpenSSL and the keyctl utility. A new
> +key type call clavis_key_acl is used for ACL updates. Any number of signed
> +clavis_key_acl entries may be added to the .clavis keyring. The clavis_key_acl
> +contains the subject key identifer along with the allowed usage type for

                            identifier

> +the key.
> +
> +The format is as follows:
> +::
> +
> +  XX:YYYYYYYYYYY
> +
> +  XX - Single byte of the key type
> +	VERIFYING_MODULE_SIGNATURE            00
> +	VERIFYING_FIRMWARE_SIGNATURE          01
> +	VERIFYING_KEXEC_PE_SIGNATURE          02
> +	VERIFYING_KEY_SIGNATURE               03
> +	VERIFYING_KEY_SELF_SIGNATURE          04
> +	VERIFYING_UNSPECIFIED_SIGNATURE       05
> +  :  - ASCII colon
> +  YY - Even number of hexadecimal characters representing the key id
> +
> +The clavis_key_acl must be S/MIME signed by the sole asymmetric key contained
> +within the .clavis keyring.
> +
> +In the future if new features are added, new key types could be created.
> +
> +Usage Examples
> +==============
> +
> +How to create a signing key:
> +----------------------------
> +
> +::
> +
> +  cat <<EOF > clavis-lsm.genkey
> +  [ req ]
> +  default_bits = 4096
> +  distinguished_name = req_distinguished_name
> +  prompt = no
> +  string_mask = utf8only
> +  x509_extensions = v3_ca
> +  [ req_distinguished_name ]
> +  O = TEST
> +  CN = Clavis LSM key
> +  emailAddress = john.doe@foo.com

There is a foo.com  ;)

> +  [ v3_ca ]
> +  basicConstraints=CA:TRUE
> +  subjectKeyIdentifier=hash
> +  authorityKeyIdentifier=keyid:always,issuer
> +  keyUsage=digitalSignature
> +  EOF
> +
> +  openssl req -new -x509 -utf8 -sha256 -days 3650 -batch \
> +        -config clavis-lsm.genkey -outform DER \
> +        -out clavis-lsm.x509 -keyout clavis-lsm.priv
> +
> +How to get the Subject Key Identifier
> +-------------------------------------
> +::
> +
> +  openssl x509 -in ./clavis-lsm.x509 -inform der \
> +        -ext subjectKeyIdentifier  -nocert \
> +        | tail -n +2 | cut -f2 -d '='| tr -d ':'
> +  4a00ab9f35c9dc3aed7c225d22bafcbd9285e1e8
> +
> +How to enroll the signing key into the MOK
> +------------------------------------------
> +
> +The key must now be added to the machine or platform keyrings.  This
> +indicates the key was added by the system owner. To add to the machine
> +keyring on x86 do:
> +::
> +
> +  mokutil --import ./clavis-lsm.x509
> +
> +and then reboot and enroll the key through the MokManager.
> +
> +How to enable the Clavis LSM
> +----------------------------
> +
> +Add the key id to the clavis= boot param.  With the example above the
> +key id is the subject key identifer: 4a00ab9f35c9dc3aed7c225d22bafcbd9285e1e8

                             identifier:

> +
> +Add the following boot param:
> +::
> +
> +  clavis=4a00ab9f35c9dc3aed7c225d22bafcbd9285e1e8
> +
> +After booting there will be a single key contained in the .clavis keyring:
> +::
> +
> +  $ keyctl show %:.clavis
> +  Keyring
> +    254954913 ----swrv      0     0  keyring: .clavis
> +    301905375 ---lswrv      0     0   \_ asymmetric: TEST: Clavis LSM key: 4a00ab9f35c9dc3aed7c225d22bafcbd9285e1e8
> +
> +The original clavis= boot param will persist across any kexec. Changing it or
> +removing it has no effect.
> +
> +
> +How to sign an entry to be added to the .clavis keyring:
> +-------------------------------------------------------
> +
> +In this example we have 3 keys in the machine keyring.  Our Clavis LSM key, a
> +key we want to use for kernel verification and a key we want to use for module
> +verification.
> +::
> +
> +  $ keyctl show %:.machine
> +   Keyring
> +    999488265 ---lswrv      0     0  keyring: .machine
> +    912608009 ---lswrv      0     0   \_ asymmetric: TEST: Module Key: 17eb8c5bf766364be094c577625213700add9471
> +    646229664 ---lswrv      0     0   \_ asymmetric: TEST: Kernel Key: b360d113c848ace3f1e6a80060b43d1206f0487d
> +   1073737099 ---lswrv      0     0   \_ asymmetric: TEST: Clavis LSM key: 4a00ab9f35c9dc3aed7c225d22bafcbd9285e1e8
> +
> +To update the .clavis kerying acl list.  First create a file containing the

                                 ACL list:
?

> +key usage type followed by a colon and the key id that we want to allow to
> +validate that usage.  In the first example we are saying key
> +17eb8c5bf766364be094c577625213700add9471 is allowed to validate kernel modules.
> +In the second example we are saying key b360d113c848ace3f1e6a80060b43d1206f0487d
> +is allowed to validate signed kernels.
> +
> +::
> +
> +  echo "00:17eb8c5bf766364be094c577625213700add9471" > module-acl.txt
> +  echo "02:b360d113c848ace3f1e6a80060b43d1206f0487d" > kernel-acl.txt
> +
> +Now both these files must be signed by the key contained in the .clavis keyring:
> +
> +::
> +
> +  openssl smime -sign -signer clavis-lsm.x509 -inkey clavis-lsm.priv -in module-acl.txt \
> +        -out module-acl.pkcs7 -binary -outform DER -nodetach -noattr
> +
> +  openssl smime -sign -signer clavis-lsm.x509 -inkey clavis-lsm.priv -in kernel-acl.txt \
> +        -out kernel-acl.pkcs7 -binary -outform DER -nodetach -noattr
> +
> +Afterwards the ACL list in the clavis keyring can be updated:
> +::
> +
> +  keyctl padd clavis_key_acl "" %:.clavis < module-acl.pkcs7
> +  keyctl padd clavis_key_acl "" %:.clavis < kernel-acl.pkcs7
> +
> +  keyctl show %:.clavis
> +
> +  Keyring
> +    254954913 ----swrv      0     0  keyring: .clavis
> +    301905375 ---lswrv      0     0   \_ asymmetric: TEST: Clavis LSM key: 4a00ab9f35c9dc3aed7c225d22bafcbd9285e1e8
> +   1013065475 --alswrv      0     0   \_ clavis_key_acl: 02:b360d113c848ace3f1e6a80060b43d1206f0487d
> +    445581284 --alswrv      0     0   \_ clavis_key_acl: 00:17eb8c5bf766364be094c577625213700add9471
> +
> +Now the 17eb8c5bf766364be094c577625213700add9471 key can be used for
> +validating kernel modules and the b360d113c848ace3f1e6a80060b43d1206f0487d
> +key can be used to validate signed kernels.



> diff --git a/security/security.c b/security/security.c
> index 4cb832b00c40..d1da60a1b7a4 100644
> --- a/security/security.c
> +++ b/security/security.c

> @@ -5313,6 +5314,19 @@ void security_key_post_create_or_update(struct key *keyring, struct key *key,
>  	call_void_hook(key_post_create_or_update, keyring, key, payload,
>  		       payload_len, flags, create);
>  }
> +
> +/**
> + * security_key_verify_signature - verify signature
> + * @key: key
> + * @public_key_signature: signature

Above should be "@sig:".

> + *
> + * See wheather signature verification is allowed based on the ACL for

          whether

> + * key usage.
> + */
> +int security_key_verify_signature(const struct key *key, const struct public_key_signature *sig)
> +{
> +	return call_int_hook(key_verify_signature, key, sig);
> +}
>  #endif	/* CONFIG_KEYS */
>  
>  #ifdef CONFIG_AUDIT

-- 
#Randy

RE: [EXTERNAL] [PATCH RFC 1/8] certs: Introduce ability to link to a system key

[Bharat Bhushan]

> -----Original Message-----
> From: Eric Snowberg <eric.snowberg@oracle.com>
> Sent: Monday, March 11, 2024 9:41 PM
> To: linux-security-module@vger.kernel.org
> Cc: dhowells@redhat.com; dwmw2@infradead.org;
> herbert@gondor.apana.org.au; davem@davemloft.net; ardb@kernel.org;
> jarkko@kernel.org; paul@paul-moore.com; jmorris@namei.org;
> serge@hallyn.com; zohar@linux.ibm.com; roberto.sassu@huawei.com;
> dmitry.kasatkin@gmail.com; mic@digikod.net; casey@schaufler-ca.com;
> stefanb@linux.ibm.com; eric.snowberg@oracle.com; linux-
> kernel@vger.kernel.org; keyrings@vger.kernel.org; linux-
> crypto@vger.kernel.org; linux-efi@vger.kernel.org; linux-
> integrity@vger.kernel.org
> Subject: [EXTERNAL] [PATCH RFC 1/8] certs: Introduce ability to link to a
> system key
> 
> Prioritize security for external emails: Confirm sender and content safety
> before clicking links or opening attachments
> 
> ----------------------------------------------------------------------
> Introduce a new function to allow a keyring to link to a key contained
> within one of the system keyrings (builtin, secondary, or platform).
> Depending on how the kernel is built, if the machine keyring is
> available, it will be checked as well, since it is linked to the secondary
> keyring. If the asymmetric key id matches a key within one of these
> system keyrings, the matching key is linked into the passed in
> keyring.
> 
> Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
> ---
>  certs/system_keyring.c        | 29 +++++++++++++++++++++++++++++
>  include/keys/system_keyring.h |  7 ++++++-
>  2 files changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index 9de610bf1f4b..b647be49f6e0 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -426,3 +426,32 @@ void __init set_platform_trusted_keys(struct key
> *keyring)
>  	platform_trusted_keys = keyring;
>  }
>  #endif
> +
> +/**
> + * system_key_link - Link to a system key
> + * @keyring: The keyring to link into
> + * @id: The asymmetric key id to look for in the system keyring
> + */
> +int system_key_link(struct key *keyring, struct asymmetric_key_id *id)
> +{
> +	struct key *tkey;
> +
> +#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
> +	tkey = find_asymmetric_key(secondary_trusted_keys, id, NULL, NULL,
> false);
> +#else
> +	tkey = find_asymmetric_key(builtin_trusted_keys, id, NULL, NULL,
> false);
> +#endif
> +	if (!IS_ERR(tkey))
> +		goto found;
> +
> +	tkey = find_asymmetric_key(platform_trusted_keys, id, NULL, NULL,
> false);
> +
> +	if (!IS_ERR(tkey))
> +		goto found;
> +
> +	return -1;

Please use -ENOKEY.

Thanks
-Bharat

> +
> +found:
> +	key_link(keyring, tkey);
> +	return 0;
> +}
> diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
> index 8365adf842ef..b47ac8e2001a 100644
> --- a/include/keys/system_keyring.h
> +++ b/include/keys/system_keyring.h
> @@ -9,6 +9,7 @@
>  #define _KEYS_SYSTEM_KEYRING_H
> 
>  #include <linux/key.h>
> +struct asymmetric_key_id;
> 
>  enum blacklist_hash_type {
>  	/* TBSCertificate hash */
> @@ -28,7 +29,7 @@ int restrict_link_by_digsig_builtin(struct key
> *dest_keyring,
>  				    const union key_payload *payload,
>  				    struct key *restriction_key);
>  extern __init int load_module_cert(struct key *keyring);
> -
> +extern int system_key_link(struct key *keyring, struct asymmetric_key_id
> *id);
>  #else
>  #define restrict_link_by_builtin_trusted restrict_link_reject
>  #define restrict_link_by_digsig_builtin restrict_link_reject
> @@ -38,6 +39,10 @@ static inline __init int load_module_cert(struct key
> *keyring)
>  	return 0;
>  }
> 
> +static inline int system_key_link(struct key *keyring, struct asymmetric_key_id
> *id)
> +{
> +	return 0;
> +}
>  #endif
> 
>  #ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
> --
> 2.39.3
> 

Re: [PATCH RFC 8/8] clavis: Introduce new LSM called clavis

[Eric Snowberg]

> On Mar 11, 2024, at 8:45 PM, Randy Dunlap <rdunlap@infradead.org> wrote:
> 
> On 3/11/24 09:11, Eric Snowberg wrote:
>> In the future it is envisioned this LSM could be enhanced to provide
>> access control for UEFI Secure Boot Advanced Targeting (SBAT).  Using
>> the same clavis= boot param and storing the additional contents within
>> the new RT UEFI var, SBAT restrictions could be maintained across kexec.
> 
> What does "RT" mean here?

I will define it in the next round.  It stands for Run-Time.

>> +
>> +Clavis is a Linux Security Module that provides mandatory access control to
>> +system kernel keys (i.e. builtin, secondary, machine and platform). These
>> +restrictions will prohit keys from being used for validation. Upon boot, the
> 
>                     prohibit

I will fix this spelling error and all the others you identified below.

> 
>> +Clavis LSM is provided a key id as a boot param.  This single key is then
>> +used as the root of trust for any access control modifications made going
>> +forward. Access control updates must be signed and validated by this key.
>> +
>> +Clavis has its own keyring.  All ACL updates are applied through this keyring.
>> +The update must be signed by the single root of trust key.
>> +
>> +When enabled, all system keys are prohibited from being used until an ACL is
>> +added for it. There is two exceptions to this rule, builtin keys may be used
> 
> What is     "it"?  The predecessor seems to be "all system keys" (plural).

The word "for" should be "to" above.  The sentence should be:

When enabled, all system keys are prohibited from being used until an ACL is
added to it.

>> +
>> +The Clavis LSM contains a system keyring call .clavis.  It contains a single
>> +asymmetric key that is use to validate anything added to it.  This key can only
>> +be added during boot and must be a preexisting system kernel key.  If the
>> +clavis= boot param is not used, the keyring does not exist and the feature
>> +can not be used until the next power on reset.
> 
> So just a reboot won't cause it to be used?  Must be power off/on?

A reboot would too.  I will rework this sentence.  I just want to show that doing a kexec 
can not be used to make changes to the root of trust.

>> diff --git a/security/security.c b/security/security.c
>> index 4cb832b00c40..d1da60a1b7a4 100644
>> --- a/security/security.c
>> +++ b/security/security.c
> 
>> @@ -5313,6 +5314,19 @@ void security_key_post_create_or_update(struct key *keyring, struct key *key,
>> call_void_hook(key_post_create_or_update, keyring, key, payload,
>>       payload_len, flags, create);
>> }
>> +
>> +/**
>> + * security_key_verify_signature - verify signature
>> + * @key: key
>> + * @public_key_signature: signature
> 
> Above should be "@sig:".

I will fix that too.  Thanks for your review.

Re: [PATCH RFC 1/8] certs: Introduce ability to link to a system key

[Jarkko Sakkinen]

On Mon Mar 11, 2024 at 11:31 PM EET, Eric Snowberg wrote:
>
>
> > On Mar 11, 2024, at 1:18 PM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> > 
> > On Mon Mar 11, 2024 at 6:11 PM EET, Eric Snowberg wrote:
> >> + return -1;
> > 
> > Missed this one: why a magic number?
>
> Good point, I'll change this to return -ENOKEY.  Thanks.

Either that or a boolean function, which ever fits to the overall
flow better... The upside of error code is less branching in the call
sites. The upside of boolean is that caller exactly knows all the
values that ever should come out as a result.

Your choice ofc.

BR, Jarkko

Re: [PATCH RFC 1/8] certs: Introduce ability to link to a system key

[Mimi Zohar]

Hi Eric,

> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index 9de610bf1f4b..b647be49f6e0 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -426,3 +426,32 @@ void __init set_platform_trusted_keys(struct key
> *keyring)
>  	platform_trusted_keys = keyring;
>  }
>  #endif
> +
> +/**
> + * system_key_link - Link to a system key
> + * @keyring: The keyring to link into
> + * @id: The asymmetric key id to look for in the system keyring
> + */
> +int system_key_link(struct key *keyring, struct asymmetric_key_id *id)
> +{
> +	struct key *tkey;
> +
> +#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
> +	tkey = find_asymmetric_key(secondary_trusted_keys, id, NULL, NULL,
> false);
> +#else
> +	tkey = find_asymmetric_key(builtin_trusted_keys, id, NULL, NULL, false);
> +#endif
> +	if (!IS_ERR(tkey))
> +		goto found;
> +
> +	tkey = find_asymmetric_key(platform_trusted_keys, id, NULL, NULL,
> false);
> +
> +	if (!IS_ERR(tkey))
> +		goto found;
> +
> +	return -1;

Normally "goto" is for the error.  Invert the logic. 

Mimi

> +
> +found:
> +	key_link(keyring, tkey);
> +	return 0;
> +}
> 

Re: [PATCH RFC 1/8] certs: Introduce ability to link to a system key

[Eric Snowberg]

> On Apr 4, 2024, at 4:40 PM, Mimi Zohar <zohar@linux.ibm.com> wrote:
> 
> Hi Eric,
> 
>> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
>> index 9de610bf1f4b..b647be49f6e0 100644
>> --- a/certs/system_keyring.c
>> +++ b/certs/system_keyring.c
>> @@ -426,3 +426,32 @@ void __init set_platform_trusted_keys(struct key
>> *keyring)
>> platform_trusted_keys = keyring;
>> }
>> #endif
>> +
>> +/**
>> + * system_key_link - Link to a system key
>> + * @keyring: The keyring to link into
>> + * @id: The asymmetric key id to look for in the system keyring
>> + */
>> +int system_key_link(struct key *keyring, struct asymmetric_key_id *id)
>> +{
>> + struct key *tkey;
>> +
>> +#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
>> + tkey = find_asymmetric_key(secondary_trusted_keys, id, NULL, NULL,
>> false);
>> +#else
>> + tkey = find_asymmetric_key(builtin_trusted_keys, id, NULL, NULL, false);
>> +#endif
>> + if (!IS_ERR(tkey))
>> + goto found;
>> +
>> + tkey = find_asymmetric_key(platform_trusted_keys, id, NULL, NULL,
>> false);
>> +
>> + if (!IS_ERR(tkey))
>> + goto found;
>> +
>> + return -1;
> 
> Normally "goto" is for the error.  Invert the logic. 

Ok, I'll change that in the next version.  Thanks.