diff mbox series

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

Message ID 20240531003945.44594-2-eric.snowberg@oracle.com
State New
Headers show
Series [RFC,v2,1/8] certs: Introduce ability to link to a system key | expand

Commit Message

Eric Snowberg May 31, 2024, 12:39 a.m. UTC
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        | 31 +++++++++++++++++++++++++++++++
 include/keys/system_keyring.h |  7 ++++++-
 2 files changed, 37 insertions(+), 1 deletion(-)

Comments

Jarkko Sakkinen June 4, 2024, 6:08 p.m. UTC | #1
On Fri May 31, 2024 at 3:39 AM EEST, 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).

"Introduce system_key_link(), a new function..."

I hate when the exact thing added is not immediately transparent from
the commit message ;-) Helps a lot when bisecting for instance.

> 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        | 31 +++++++++++++++++++++++++++++++
>  include/keys/system_keyring.h |  7 ++++++-
>  2 files changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
> index 9de610bf1f4b..94e47b6b3333 100644
> --- a/certs/system_keyring.c
> +++ b/certs/system_keyring.c
> @@ -426,3 +426,34 @@ void __init set_platform_trusted_keys(struct key *keyring)
>  	platform_trusted_keys = keyring;
>  }
>  #endif
> +
> +/**
> + * system_key_link - Link to a system key

"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
> + */

Really could use some overview keyrings traversed just as a reminder.

> +int system_key_link(struct key *keyring, struct asymmetric_key_id *id)
> +{
> +	struct key *system_keyring;
> +	struct key *key;
> +
> +#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
> +	system_keyring = secondary_trusted_keys;
> +#else
> +	system_keyring = builtin_trusted_keys;
> +#endif

Why not simply make secondary_trusted_keys in the first place be alias
to builtin_trusted_keys when it is not enabled?

> +
> +	key = find_asymmetric_key(system_keyring, id, NULL, NULL, false);
> +	if (!IS_ERR(key))
> +		goto found;
> +
> +	key = find_asymmetric_key(platform_trusted_keys, id, NULL, NULL, false);
> +	if (!IS_ERR(key))
> +		goto found;
> +
> +	return -ENOKEY;
> +
> +found:

"link:"?

Then you could see already from goto statement what will happen next
(your call anyway).

> +	key_link(keyring, key);
> +	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
Eric Snowberg June 5, 2024, 8:36 p.m. UTC | #2
> On Jun 4, 2024, at 12:08 PM, Jarkko Sakkinen <jarkko@kernel.org> wrote:
> 
> On Fri May 31, 2024 at 3:39 AM EEST, 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).
> 
> "Introduce system_key_link(), a new function..."
> 
> I hate when the exact thing added is not immediately transparent from
> the commit message ;-) Helps a lot when bisecting for instance.
> 
>> 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        | 31 +++++++++++++++++++++++++++++++
>> include/keys/system_keyring.h |  7 ++++++-
>> 2 files changed, 37 insertions(+), 1 deletion(-)
>> 
>> diff --git a/certs/system_keyring.c b/certs/system_keyring.c
>> index 9de610bf1f4b..94e47b6b3333 100644
>> --- a/certs/system_keyring.c
>> +++ b/certs/system_keyring.c
>> @@ -426,3 +426,34 @@ void __init set_platform_trusted_keys(struct key *keyring)
>> platform_trusted_keys = keyring;
>> }
>> #endif
>> +
>> +/**
>> + * system_key_link - Link to a system key
> 
> "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
>> + */
> 
> Really could use some overview keyrings traversed just as a reminder.

Sure, I will make the three changes above in the next round.

>> +int system_key_link(struct key *keyring, struct asymmetric_key_id *id)
>> +{
>> + struct key *system_keyring;
>> + struct key *key;
>> +
>> +#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
>> + system_keyring = secondary_trusted_keys;
>> +#else
>> + system_keyring = builtin_trusted_keys;
>> +#endif
> 
> Why not simply make secondary_trusted_keys in the first place be alias
> to builtin_trusted_keys when it is not enabled?

I'll change that in the next round and remove the #ifdef completely from within this 
function.  I'll add a clean up patch first that removes this same pattern elsewhere
in the file.  I think I see how the goto can be removed now.  And I'll also take care 
of the case where the kernel is built without the platform keyring enabled.  Which I
now see is a problem with this current version. Thanks.
diff mbox series

Patch

diff --git a/certs/system_keyring.c b/certs/system_keyring.c
index 9de610bf1f4b..94e47b6b3333 100644
--- a/certs/system_keyring.c
+++ b/certs/system_keyring.c
@@ -426,3 +426,34 @@  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 *system_keyring;
+	struct key *key;
+
+#ifdef CONFIG_SECONDARY_TRUSTED_KEYRING
+	system_keyring = secondary_trusted_keys;
+#else
+	system_keyring = builtin_trusted_keys;
+#endif
+
+	key = find_asymmetric_key(system_keyring, id, NULL, NULL, false);
+	if (!IS_ERR(key))
+		goto found;
+
+	key = find_asymmetric_key(platform_trusted_keys, id, NULL, NULL, false);
+	if (!IS_ERR(key))
+		goto found;
+
+	return -ENOKEY;
+
+found:
+	key_link(keyring, key);
+	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