[PATCH v2 0/3] certs: Prevent spurious errors on repeated blacklisting

[PATCH v2 0/3] certs: Prevent spurious errors on repeated blacklisting

[Thomas Weißschuh]

When the blacklist keyring was changed to allow updates from the root
user it gained an ->update() function that disallows all updates.
When the a hash is blacklisted multiple times from the builtin or
firmware-provided blacklist this spams prominent logs during boot:

[    0.890814] blacklist: Problem blacklisting hash (-13)

This affects the firmware of various vendors. Reported have been at least:
* Samsung: https://askubuntu.com/questions/1436856/
* Acer: https://ubuntuforums.org/showthread.php?t=2478840
* MSI: https://forum.archlabslinux.com/t/blacklist-problem-blacklisting-hash-13-errors-on-boot/6674/7
* Micro-Star: https://bbs.archlinux.org/viewtopic.php?id=278860

This series is an extension of the following single patch:
https://lore.kernel.org/all/20221104014704.3469-1-linux@weissschuh.net/

Only the first patch has been marked for stable as otherwise the whole of
key_create() would need to be applied to stable.

Thomas Weißschuh (3):
  certs: log more information on blacklist error
  KEYS: Add key_create()
  certs: don't try to update blacklist keys

 certs/blacklist.c   |  23 ++++---
 include/linux/key.h |   8 +++
 security/keys/key.c | 149 +++++++++++++++++++++++++++++++++-----------
 3 files changed, 133 insertions(+), 47 deletions(-)


base-commit: f141df371335645ce29a87d9683a3f79fba7fd67
-- 
2.38.1

[PATCH v2 1/3] certs: log more information on blacklist error

[Thomas Weißschuh]

Without this information these logs are not actionable.

Fixes: 6364d106e041 ("certs: Allow root user to append signed hashes to the blacklist keyring")
Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 certs/blacklist.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/certs/blacklist.c b/certs/blacklist.c
index 41f10601cc72..6e260c4b6a19 100644
--- a/certs/blacklist.c
+++ b/certs/blacklist.c
@@ -192,7 +192,7 @@ static int mark_raw_hash_blacklisted(const char *hash)
 				   KEY_ALLOC_NOT_IN_QUOTA |
 				   KEY_ALLOC_BUILT_IN);
 	if (IS_ERR(key)) {
-		pr_err("Problem blacklisting hash (%ld)\n", PTR_ERR(key));
+		pr_err("Problem blacklisting hash %s: %pe\n", hash, key);
 		return PTR_ERR(key);
 	}
 	return 0;
-- 
2.38.1

Re: [PATCH v2 1/3] certs: log more information on blacklist error

[Jarkko Sakkinen]

On Wed, Nov 09, 2022 at 03:50:17AM +0100, Thomas Weißschuh wrote:
> Without this information these logs are not actionable.

Neither "more information" nor "this information" describe
"what information", right? :-)

BR, Jarkko

[PATCH v2 2/3] KEYS: Add key_create()

[Thomas Weißschuh]

This function works like key_create_or_update() but does not allow
updating an existing key instead returning -EEXIST.

This new function will be used by the blacklist keyring.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 include/linux/key.h |   8 +++
 security/keys/key.c | 149 +++++++++++++++++++++++++++++++++-----------
 2 files changed, 120 insertions(+), 37 deletions(-)

diff --git a/include/linux/key.h b/include/linux/key.h
index d27477faf00d..8dc7f7c3088b 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -386,6 +386,14 @@ extern int wait_for_key_construction(struct key *key, bool intr);
 
 extern int key_validate(const struct key *key);
 
+extern key_ref_t key_create(key_ref_t keyring,
+			    const char *type,
+			    const char *description,
+			    const void *payload,
+			    size_t plen,
+			    key_perm_t perm,
+			    unsigned long flags);
+
 extern key_ref_t key_create_or_update(key_ref_t keyring,
 				      const char *type,
 				      const char *description,
diff --git a/security/keys/key.c b/security/keys/key.c
index c45afdd1dfbb..f84bcd8457f4 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -788,38 +788,18 @@ static inline key_ref_t __key_update(key_ref_t key_ref,
 	goto out;
 }
 
-/**
- * key_create_or_update - Update or create and instantiate a key.
- * @keyring_ref: A pointer to the destination keyring with possession flag.
- * @type: The type of key.
- * @description: The searchable description for the key.
- * @payload: The data to use to instantiate or update the key.
- * @plen: The length of @payload.
- * @perm: The permissions mask for a new key.
- * @flags: The quota flags for a new key.
- *
- * Search the destination keyring for a key of the same description and if one
- * is found, update it, otherwise create and instantiate a new one and create a
- * link to it from that keyring.
- *
- * If perm is KEY_PERM_UNDEF then an appropriate key permissions mask will be
- * concocted.
- *
- * Returns a pointer to the new key if successful, -ENODEV if the key type
- * wasn't available, -ENOTDIR if the keyring wasn't a keyring, -EACCES if the
- * caller isn't permitted to modify the keyring or the LSM did not permit
- * creation of the key.
- *
- * On success, the possession flag from the keyring ref will be tacked on to
- * the key ref before it is returned.
+/*
+ * Create or potentially update a key. The combined logic behind
+ * key_create_or_update() and key_create()
  */
-key_ref_t key_create_or_update(key_ref_t keyring_ref,
-			       const char *type,
-			       const char *description,
-			       const void *payload,
-			       size_t plen,
-			       key_perm_t perm,
-			       unsigned long flags)
+static key_ref_t __key_create_or_update(key_ref_t keyring_ref,
+					const char *type,
+					const char *description,
+					const void *payload,
+					size_t plen,
+					key_perm_t perm,
+					unsigned long flags,
+					bool allow_update)
 {
 	struct keyring_index_key index_key = {
 		.description	= description,
@@ -906,14 +886,23 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
 		goto error_link_end;
 	}
 
-	/* if it's possible to update this type of key, search for an existing
-	 * key of the same type and description in the destination keyring and
-	 * update that instead if possible
+	/* if it's requested and possible to update this type of key, search
+	 * for an existing key of the same type and description in the
+	 * destination keyring and update that instead if possible
 	 */
-	if (index_key.type->update) {
+	if (allow_update) {
+		if (index_key.type->update) {
+			key_ref = find_key_to_update(keyring_ref, &index_key);
+			if (key_ref)
+				goto found_matching_key;
+		}
+	} else {
 		key_ref = find_key_to_update(keyring_ref, &index_key);
-		if (key_ref)
-			goto found_matching_key;
+		if (key_ref) {
+			key_ref_put(key_ref);
+			key_ref = ERR_PTR(-EEXIST);
+			goto error_link_end;
+		}
 	}
 
 	/* if the client doesn't provide, decide on the permissions we want */
@@ -985,8 +974,94 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
 
 	goto error_free_prep;
 }
+
+/**
+ * key_create_or_update - Update or create and instantiate a key.
+ * @keyring_ref: A pointer to the destination keyring with possession flag.
+ * @type: The type of key.
+ * @description: The searchable description for the key.
+ * @payload: The data to use to instantiate or update the key.
+ * @plen: The length of @payload.
+ * @perm: The permissions mask for a new key.
+ * @flags: The quota flags for a new key.
+ *
+ * Search the destination keyring for a key of the same description and if one
+ * is found, update it, otherwise create and instantiate a new one and create a
+ * link to it from that keyring.
+ *
+ * If perm is KEY_PERM_UNDEF then an appropriate key permissions mask will be
+ * concocted.
+ *
+ * Returns a pointer to the new key if successful, -ENODEV if the key type
+ * wasn't available, -ENOTDIR if the keyring wasn't a keyring, -EACCES if the
+ * caller isn't permitted to modify the keyring or the LSM did not permit
+ * creation of the key.
+ *
+ * On success, the possession flag from the keyring ref will be tacked on to
+ * the key ref before it is returned.
+ */
+key_ref_t key_create_or_update(key_ref_t keyring_ref,
+			       const char *type,
+			       const char *description,
+			       const void *payload,
+			       size_t plen,
+			       key_perm_t perm,
+			       unsigned long flags)
+{
+	return __key_create_or_update(keyring_ref,
+				      type,
+				      description,
+				      payload,
+				      plen,
+				      perm,
+				      flags,
+				      true);
+}
 EXPORT_SYMBOL(key_create_or_update);
 
+/**
+ * key_create - Create and instantiate a key.
+ * @keyring_ref: A pointer to the destination keyring with possession flag.
+ * @type: The type of key.
+ * @description: The searchable description for the key.
+ * @payload: The data to use to instantiate or update the key.
+ * @plen: The length of @payload.
+ * @perm: The permissions mask for a new key.
+ * @flags: The quota flags for a new key.
+ *
+ * Create and instantiate a new key and link to it from the destination keyring.
+ *
+ * If perm is KEY_PERM_UNDEF then an appropriate key permissions mask will be
+ * concocted.
+ *
+ * Returns a pointer to the new key if successful, -EEXIST if a key with the
+ * same description already exists, -ENODEV if the key type wasn't available,
+ * -ENOTDIR if the keyring wasn't a keyring, -EACCES if the caller isn't
+ * permitted to modify the keyring or the LSM did not permit creation of the
+ * key.
+ *
+ * On success, the possession flag from the keyring ref will be tacked on to
+ * the key ref before it is returned.
+ */
+key_ref_t key_create(key_ref_t keyring_ref,
+		     const char *type,
+		     const char *description,
+		     const void *payload,
+		     size_t plen,
+		     key_perm_t perm,
+		     unsigned long flags)
+{
+	return __key_create_or_update(keyring_ref,
+				      type,
+				      description,
+				      payload,
+				      plen,
+				      perm,
+				      flags,
+				      false);
+}
+EXPORT_SYMBOL(key_create);
+
 /**
  * key_update - Update a key's contents.
  * @key_ref: The pointer (plus possession flag) to the key.
-- 
2.38.1

Re: [PATCH v2 2/3] KEYS: Add key_create()

[Jarkko Sakkinen]

On Wed, Nov 09, 2022 at 03:50:18AM +0100, Thomas Weißschuh wrote:
> This function works like key_create_or_update() but does not allow
> updating an existing key instead returning -EEXIST.
> 
> This new function will be used by the blacklist keyring.

How?

> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  include/linux/key.h |   8 +++
>  security/keys/key.c | 149 +++++++++++++++++++++++++++++++++-----------
>  2 files changed, 120 insertions(+), 37 deletions(-)
> 
> diff --git a/include/linux/key.h b/include/linux/key.h
> index d27477faf00d..8dc7f7c3088b 100644
> --- a/include/linux/key.h
> +++ b/include/linux/key.h
> @@ -386,6 +386,14 @@ extern int wait_for_key_construction(struct key *key, bool intr);
>  
>  extern int key_validate(const struct key *key);
>  
> +extern key_ref_t key_create(key_ref_t keyring,
> +			    const char *type,
> +			    const char *description,
> +			    const void *payload,
> +			    size_t plen,
> +			    key_perm_t perm,
> +			    unsigned long flags);
> +
>  extern key_ref_t key_create_or_update(key_ref_t keyring,
>  				      const char *type,
>  				      const char *description,
> diff --git a/security/keys/key.c b/security/keys/key.c
> index c45afdd1dfbb..f84bcd8457f4 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -788,38 +788,18 @@ static inline key_ref_t __key_update(key_ref_t key_ref,
>  	goto out;
>  }
>  
> -/**
> - * key_create_or_update - Update or create and instantiate a key.
> - * @keyring_ref: A pointer to the destination keyring with possession flag.
> - * @type: The type of key.
> - * @description: The searchable description for the key.
> - * @payload: The data to use to instantiate or update the key.
> - * @plen: The length of @payload.
> - * @perm: The permissions mask for a new key.
> - * @flags: The quota flags for a new key.
> - *
> - * Search the destination keyring for a key of the same description and if one
> - * is found, update it, otherwise create and instantiate a new one and create a
> - * link to it from that keyring.
> - *
> - * If perm is KEY_PERM_UNDEF then an appropriate key permissions mask will be
> - * concocted.
> - *
> - * Returns a pointer to the new key if successful, -ENODEV if the key type
> - * wasn't available, -ENOTDIR if the keyring wasn't a keyring, -EACCES if the
> - * caller isn't permitted to modify the keyring or the LSM did not permit
> - * creation of the key.
> - *
> - * On success, the possession flag from the keyring ref will be tacked on to
> - * the key ref before it is returned.
> +/*
> + * Create or potentially update a key. The combined logic behind
> + * key_create_or_update() and key_create()
>   */
> -key_ref_t key_create_or_update(key_ref_t keyring_ref,
> -			       const char *type,
> -			       const char *description,
> -			       const void *payload,
> -			       size_t plen,
> -			       key_perm_t perm,
> -			       unsigned long flags)
> +static key_ref_t __key_create_or_update(key_ref_t keyring_ref,
> +					const char *type,
> +					const char *description,
> +					const void *payload,
> +					size_t plen,
> +					key_perm_t perm,
> +					unsigned long flags,
> +					bool allow_update)
>  {
>  	struct keyring_index_key index_key = {
>  		.description	= description,
> @@ -906,14 +886,23 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
>  		goto error_link_end;
>  	}
>  
> -	/* if it's possible to update this type of key, search for an existing
> -	 * key of the same type and description in the destination keyring and
> -	 * update that instead if possible
> +	/* if it's requested and possible to update this type of key, search
> +	 * for an existing key of the same type and description in the
> +	 * destination keyring and update that instead if possible
>  	 */
> -	if (index_key.type->update) {
> +	if (allow_update) {
> +		if (index_key.type->update) {
> +			key_ref = find_key_to_update(keyring_ref, &index_key);
> +			if (key_ref)
> +				goto found_matching_key;
> +		}
> +	} else {
>  		key_ref = find_key_to_update(keyring_ref, &index_key);
> -		if (key_ref)
> -			goto found_matching_key;
> +		if (key_ref) {
> +			key_ref_put(key_ref);
> +			key_ref = ERR_PTR(-EEXIST);
> +			goto error_link_end;
> +		}
>  	}
>  
>  	/* if the client doesn't provide, decide on the permissions we want */
> @@ -985,8 +974,94 @@ key_ref_t key_create_or_update(key_ref_t keyring_ref,
>  
>  	goto error_free_prep;
>  }
> +
> +/**
> + * key_create_or_update - Update or create and instantiate a key.
> + * @keyring_ref: A pointer to the destination keyring with possession flag.
> + * @type: The type of key.
> + * @description: The searchable description for the key.
> + * @payload: The data to use to instantiate or update the key.
> + * @plen: The length of @payload.
> + * @perm: The permissions mask for a new key.
> + * @flags: The quota flags for a new key.
> + *
> + * Search the destination keyring for a key of the same description and if one
> + * is found, update it, otherwise create and instantiate a new one and create a
> + * link to it from that keyring.
> + *
> + * If perm is KEY_PERM_UNDEF then an appropriate key permissions mask will be
> + * concocted.
> + *
> + * Returns a pointer to the new key if successful, -ENODEV if the key type
> + * wasn't available, -ENOTDIR if the keyring wasn't a keyring, -EACCES if the
> + * caller isn't permitted to modify the keyring or the LSM did not permit
> + * creation of the key.
> + *
> + * On success, the possession flag from the keyring ref will be tacked on to
> + * the key ref before it is returned.
> + */
> +key_ref_t key_create_or_update(key_ref_t keyring_ref,
> +			       const char *type,
> +			       const char *description,
> +			       const void *payload,
> +			       size_t plen,
> +			       key_perm_t perm,
> +			       unsigned long flags)
> +{
> +	return __key_create_or_update(keyring_ref,
> +				      type,
> +				      description,
> +				      payload,
> +				      plen,
> +				      perm,
> +				      flags,
> +				      true);
> +}
>  EXPORT_SYMBOL(key_create_or_update);
>  
> +/**
> + * key_create - Create and instantiate a key.
> + * @keyring_ref: A pointer to the destination keyring with possession flag.
> + * @type: The type of key.
> + * @description: The searchable description for the key.
> + * @payload: The data to use to instantiate or update the key.
> + * @plen: The length of @payload.
> + * @perm: The permissions mask for a new key.
> + * @flags: The quota flags for a new key.
> + *
> + * Create and instantiate a new key and link to it from the destination keyring.
> + *
> + * If perm is KEY_PERM_UNDEF then an appropriate key permissions mask will be
> + * concocted.
> + *
> + * Returns a pointer to the new key if successful, -EEXIST if a key with the
> + * same description already exists, -ENODEV if the key type wasn't available,
> + * -ENOTDIR if the keyring wasn't a keyring, -EACCES if the caller isn't
> + * permitted to modify the keyring or the LSM did not permit creation of the
> + * key.
> + *
> + * On success, the possession flag from the keyring ref will be tacked on to
> + * the key ref before it is returned.
> + */
> +key_ref_t key_create(key_ref_t keyring_ref,
> +		     const char *type,
> +		     const char *description,
> +		     const void *payload,
> +		     size_t plen,
> +		     key_perm_t perm,
> +		     unsigned long flags)
> +{
> +	return __key_create_or_update(keyring_ref,
> +				      type,
> +				      description,
> +				      payload,
> +				      plen,
> +				      perm,
> +				      flags,
> +				      false);
> +}
> +EXPORT_SYMBOL(key_create);
> +
>  /**
>   * key_update - Update a key's contents.
>   * @key_ref: The pointer (plus possession flag) to the key.
> -- 
> 2.38.1
> 

BR, Jarkko

[PATCH v2 3/3] certs: don't try to update blacklist keys

[Thomas Weißschuh]

When the same key is blacklisted repeatedly we don't want to log an
error. These duplicates can be provided by buggy firmware. Instead of
spamming the bootlog with errors we use a warning that can still be seen
by OEMs when testing.

Also extend BLACKLIST_KEY_PERM as otherwise the EACCES will shadow the
EEXIST.

Link: https://lore.kernel.org/all/c8c65713-5cda-43ad-8018-20f2e32e4432@t-8ch.de/
Link: https://lore.kernel.org/all/20221104014704.3469-1-linux@weissschuh.net/
Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 certs/blacklist.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/certs/blacklist.c b/certs/blacklist.c
index 6e260c4b6a19..ac8e3166b6d7 100644
--- a/certs/blacklist.c
+++ b/certs/blacklist.c
@@ -26,7 +26,7 @@
  */
 #define MAX_HASH_LEN	128
 
-#define BLACKLIST_KEY_PERM (KEY_POS_SEARCH | KEY_POS_VIEW | \
+#define BLACKLIST_KEY_PERM (KEY_POS_WRITE | KEY_POS_SEARCH | KEY_POS_VIEW | \
 			    KEY_USR_SEARCH | KEY_USR_VIEW)
 
 static const char tbs_prefix[] = "tbs";
@@ -183,16 +183,19 @@ static int mark_raw_hash_blacklisted(const char *hash)
 {
 	key_ref_t key;
 
-	key = key_create_or_update(make_key_ref(blacklist_keyring, true),
-				   "blacklist",
-				   hash,
-				   NULL,
-				   0,
-				   BLACKLIST_KEY_PERM,
-				   KEY_ALLOC_NOT_IN_QUOTA |
-				   KEY_ALLOC_BUILT_IN);
+	key = key_create(make_key_ref(blacklist_keyring, true),
+			 "blacklist",
+			 hash,
+			 NULL,
+			 0,
+			 BLACKLIST_KEY_PERM,
+			 KEY_ALLOC_NOT_IN_QUOTA |
+			 KEY_ALLOC_BUILT_IN);
 	if (IS_ERR(key)) {
-		pr_err("Problem blacklisting hash %s: %pe\n", hash, key);
+		if (PTR_ERR(key) == -EEXIST)
+			pr_warn("Duplicate blacklisted hash %s\n", hash);
+		else
+			pr_err("Problem blacklisting hash %s: %pe\n", hash, key);
 		return PTR_ERR(key);
 	}
 	return 0;
-- 
2.38.1

Re: [PATCH v2 3/3] certs: don't try to update blacklist keys

[Jarkko Sakkinen]

On Wed, Nov 09, 2022 at 03:50:19AM +0100, Thomas Weißschuh wrote:
> When the same key is blacklisted repeatedly we don't want to log an

Who is "we"?

> error. These duplicates can be provided by buggy firmware. Instead of
> spamming the bootlog with errors we use a warning that can still be seen
> by OEMs when testing.
> 
> Also extend BLACKLIST_KEY_PERM as otherwise the EACCES will shadow the
> EEXIST.

How? I.e. please state how you extend it, and why new need the extra bits.

> Link: https://lore.kernel.org/all/c8c65713-5cda-43ad-8018-20f2e32e4432@t-8ch.de/
> Link: https://lore.kernel.org/all/20221104014704.3469-1-linux@weissschuh.net/
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  certs/blacklist.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/certs/blacklist.c b/certs/blacklist.c
> index 6e260c4b6a19..ac8e3166b6d7 100644
> --- a/certs/blacklist.c
> +++ b/certs/blacklist.c
> @@ -26,7 +26,7 @@
>   */
>  #define MAX_HASH_LEN	128
>  
> -#define BLACKLIST_KEY_PERM (KEY_POS_SEARCH | KEY_POS_VIEW | \
> +#define BLACKLIST_KEY_PERM (KEY_POS_WRITE | KEY_POS_SEARCH | KEY_POS_VIEW | \
>  			    KEY_USR_SEARCH | KEY_USR_VIEW)
>  
>  static const char tbs_prefix[] = "tbs";
> @@ -183,16 +183,19 @@ static int mark_raw_hash_blacklisted(const char *hash)
>  {
>  	key_ref_t key;
>  
> -	key = key_create_or_update(make_key_ref(blacklist_keyring, true),
> -				   "blacklist",
> -				   hash,
> -				   NULL,
> -				   0,
> -				   BLACKLIST_KEY_PERM,
> -				   KEY_ALLOC_NOT_IN_QUOTA |
> -				   KEY_ALLOC_BUILT_IN);
> +	key = key_create(make_key_ref(blacklist_keyring, true),
> +			 "blacklist",
> +			 hash,
> +			 NULL,
> +			 0,
> +			 BLACKLIST_KEY_PERM,
> +			 KEY_ALLOC_NOT_IN_QUOTA |
> +			 KEY_ALLOC_BUILT_IN);
>  	if (IS_ERR(key)) {
> -		pr_err("Problem blacklisting hash %s: %pe\n", hash, key);
> +		if (PTR_ERR(key) == -EEXIST)
> +			pr_warn("Duplicate blacklisted hash %s\n", hash);
> +		else
> +			pr_err("Problem blacklisting hash %s: %pe\n", hash, key);
>  		return PTR_ERR(key);
>  	}
>  	return 0;
> -- 
> 2.38.1
> 

BR, Jarkko