> On Dec 23, 2024, at 5:01 PM, Mimi Zohar <zohar@xxxxxxxxxxxxx> wrote:
>
> On Thu, 2024-10-17 at 09:55 -0600, Eric Snowberg wrote:
>> Introduce a new system keyring called clavis. This keyring shall contain
>> a single asymmetric key. This key may be a linked to a key already
>> contained in one of the system keyrings (builtin, secondary, or platform).
>
> Although "This key may be a linked to ..." is might be correct. Being
> introduced in this patch is only the ability of loading a key by specifying it
> on the boot command line. In this case, the key must be on one of the system
> keyrings.
I'll reword this
>> One way to add this key into this keyring 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@xxxxxxxxxx>
>> ---
>> .../admin-guide/kernel-parameters.txt | 6 +
>> include/linux/integrity.h | 8 ++
>> security/Kconfig | 1 +
>> security/Makefile | 1 +
>> security/clavis/Kconfig | 11 ++
>> security/clavis/Makefile | 3 +
>> security/clavis/clavis.h | 13 ++
>> security/clavis/clavis_keyring.c | 115 ++++++++++++++++++
>> security/integrity/iint.c | 2 +
>> 9 files changed, 160 insertions(+)
>> create mode 100644 security/clavis/Kconfig
>> create mode 100644 security/clavis/Makefile
>> create mode 100644 security/clavis/clavis.h
>> create mode 100644 security/clavis/clavis_keyring.c
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index 1518343bbe22..d71397e7d254 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -645,6 +645,12 @@
>> cio_ignore= [S390]
>> See Documentation/arch/s390/common_io.rst for details.
>>
>> + clavis= [SECURITY,EARLY]
>> + Identifies a specific key contained in one of the system
>> + keyrings (builtin, secondary, or platform) to be used as
>> + the Clavis root of trust.
>> + Format: { <keyid> }
>
> Include .machine keyring here.
and I'll add this too.
>> +
>> clearcpuid=X[,X...] [X86]
>> Disable CPUID feature X for the kernel. See
>> arch/x86/include/asm/cpufeatures.h for the valid bit
>> diff --git a/include/linux/integrity.h b/include/linux/integrity.h
>> index f5842372359b..837c52e1d83b 100644
>> --- a/include/linux/integrity.h
>> +++ b/include/linux/integrity.h
>> @@ -23,6 +23,14 @@ enum integrity_status {
>> #ifdef CONFIG_INTEGRITY
>> extern void __init integrity_load_keys(void);
>>
>> +#ifdef CONFIG_SECURITY_CLAVIS
>> +void __init late_init_clavis_setup(void);
>> +#else
>> +static inline void late_init_clavis_setup(void)
>> +{
>> +}
>> +#endif
>> +
>> #else
>> static inline void integrity_load_keys(void)
>> {
>> diff --git a/security/Kconfig b/security/Kconfig
>> index 28e685f53bd1..714ec08dda96 100644
>> --- a/security/Kconfig
>> +++ b/security/Kconfig
>> @@ -225,6 +225,7 @@ source "security/safesetid/Kconfig"
>> source "security/lockdown/Kconfig"
>> source "security/landlock/Kconfig"
>> source "security/ipe/Kconfig"
>> +source "security/clavis/Kconfig"
>>
>> source "security/integrity/Kconfig"
>>
>> diff --git a/security/Makefile b/security/Makefile
>> index cc0982214b84..69576551007a 100644
>> --- a/security/Makefile
>> +++ b/security/Makefile
>> @@ -26,6 +26,7 @@ obj-$(CONFIG_CGROUPS) += device_cgroup.o
>> obj-$(CONFIG_BPF_LSM) += bpf/
>> obj-$(CONFIG_SECURITY_LANDLOCK) += landlock/
>> obj-$(CONFIG_SECURITY_IPE) += ipe/
>> +obj-$(CONFIG_SECURITY_CLAVIS) += 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..04f7565f2e2b
>> --- /dev/null
>> +++ b/security/clavis/Kconfig
>> @@ -0,0 +1,11 @@
>> +config SECURITY_CLAVIS
>> + bool "Clavis keyring"
>
> Isn't SECURITY_CLAVIS the new LSM? Why is the bool defined as just "Clavis
> keyring"?
>
>> + depends on SECURITY
>> + select SYSTEM_DATA_VERIFICATION
>> + select CRYPTO_SHA256
>> + 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). One way to add this key
>> + is during boot by passing in the asymmetric key id within the "clavis=" boot
>> + param. This keyring is required by the Clavis LSM.
>
> If SECURITY_CLAVIS is a new LSM, the 'help' shouldn't be limited to just the
> clavis keyring, but written at a higher level describing the new LSM. For
> example,
>
> This option enables the Clavis LSM, which provides the ability to configure and
> enforce the usage of keys contained on the system keyrings -
> .builtin_trusted_keys, .secondary_trusted_keys, .machine, and .platform
> keyrings. The clavis LSM defines a keyring named "clavis", which contains a
> single asymmetric key and the key usage rules.
>
> The single asymmetric key may be specified on the boot command line ...
>
> [The patch that introduces the key usage rules would add additional info here.]
>
> [The patch that adds the Documentatoin would add a reference here.]
I went the route of creating the keyring in this patch and then introducing the
LSM which uses it in a later patch. My reasoning was it can be tested
independently. Also, I thought it would make it easier to review, since
everything isn't contained within a single patch. I could look at combining
them together if you think that would be better.
>
>> diff --git a/security/clavis/Makefile b/security/clavis/Makefile
>> new file mode 100644
>> index 000000000000..16c451f45f37
>> --- /dev/null
>> +++ b/security/clavis/Makefile
>> @@ -0,0 +1,3 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +
>> +obj-$(CONFIG_SECURITY_CLAVIS) += clavis_keyring.o
>> diff --git a/security/clavis/clavis.h b/security/clavis/clavis.h
>> new file mode 100644
>> index 000000000000..5e397b55a60a
>> --- /dev/null
>> +++ b/security/clavis/clavis.h
>> @@ -0,0 +1,13 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef _SECURITY_CLAVIS_H_
>> +#define _SECURITY_CLAVIS_H_
>> +#include <keys/asymmetric-type.h>
>> +
>> +/* Max length for the asymmetric key id contained on the boot param */
>> +#define CLAVIS_BIN_KID_MAX 32
>> +
>> +struct asymmetric_setup_kid {
>> + struct asymmetric_key_id id;
>> + unsigned char data[CLAVIS_BIN_KID_MAX];
>> +};
>> +#endif /* _SECURITY_CLAVIS_H_ */
>> diff --git a/security/clavis/clavis_keyring.c b/security/clavis/clavis_keyring.c
>> new file mode 100644
>> index 000000000000..400ed455a3a2
>> --- /dev/null
>> +++ b/security/clavis/clavis_keyring.c
>> @@ -0,0 +1,115 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#include <linux/security.h>
>> +#include <linux/integrity.h>
>> +#include <keys/asymmetric-type.h>
>> +#include <keys/system_keyring.h>
>> +#include "clavis.h"
>> +
>> +static struct key *clavis_keyring;
>> +static struct asymmetric_key_id *clavis_boot_akid;
>> +static struct asymmetric_setup_kid clavis_setup_akid;
>> +static bool clavis_enforced;
>> +
>> +static bool clavis_acl_enforced(void)
>> +{
>> + return clavis_enforced;
>> +}
>
> Add blank line between functions.
I'll add the blank line in the next round. Thanks.
