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

[PATCH RESEND v6 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
* Lenovo: https://lore.kernel.org/lkml/c8c65713-5cda-43ad-8018-20f2e32e4432@t-8ch.de/

Note: In the meantime I lost access to the machine exhibiting the
problematic behavior. If larger changes are required to this series
somebody else would have to validate them or take over the series.

Changelog:

v1: https://lore.kernel.org/all/20221104014704.3469-1-linux@weissschuh.net/
v1 -> v2:
 * Improve logging message to include the failed hash
 * Add key_create() function without update semantics
 * Use key_create() from mark_raw_hash_blacklisted() and log specific message
   on -EEXIST

v2: https://lore.kernel.org/lkml/20221109025019.1855-1-linux@weissschuh.net/
v2 -> v3:
 * Clarify commit titles and messages
 * Drop the change to BLACKLIST_KEY_PERM from patch 3, as it was an artifact
   of some obsolete version of the patch and not needed

v3: https://lore.kernel.org/lkml/20221118040343.2958-1-linux@weissschuh.net/
v3 -> v4:
 * Drop Fixes-tag from first patch
 * Flesh out commit descriptions and messages

v4: https://lore.kernel.org/r/20221212-keys-blacklist-v4-0-00afeb3137fb@weissschuh.net
v4 -> v5:
 * Reduce lines needed by function calls in key.c
 * Add Reviewed-by from Jarkko

v5: https://lore.kernel.org/r/20221212-keys-blacklist-v5-0-52e9eb5a8827@weissschuh.net
v5 -> v6:
 * Correct Jarkkos email in Reviewed-by tags
 * Resend to hopefully reach @kernel.org recipients

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

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

--
2.38.1

To: David Howells <dhowells@redhat.com>
To: David Woodhouse <dwmw2@infradead.org>
To: Jarkko Sakkinen <jarkko@kernel.org>
To: Paul Moore <paul@paul-moore.com>
To: James Morris <jmorris@namei.org>
To: "Serge E. Hallyn" <serge@hallyn.com>
To: "Mickaël Salaün" <mic@digikod.net>
Cc: keyrings@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-security-module@vger.kernel.org
Cc: Paul Menzel <pmenzel@molgen.mpg.de>
Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
Cc: Mark Pearson <markpearson@lenovo.com>

---
Thomas Weißschuh (3):
      certs: make blacklisted hash available in klog
      KEYS: Add new function key_create()
      certs: don't try to update blacklist keys

 certs/blacklist.c   |  21 ++++----
 include/linux/key.h |   8 +++
 security/keys/key.c | 137 ++++++++++++++++++++++++++++++++++++++--------------
 3 files changed, 120 insertions(+), 46 deletions(-)
---
base-commit: 512dee0c00ad9e9c7ae9f11fc6743702ea40caff
change-id: 20221212-keys-blacklist-2c79a64667c9

Best regards,
-- 
Thomas Weißschuh <linux@weissschuh.net>

[PATCH RESEND v6 1/3] certs: make blacklisted hash available in klog

[Thomas Weißschuh]

One common situation triggering this log statement are duplicate hashes
reported by the system firmware.

These duplicates should be removed from the firmware.

Without logging the blacklisted hash triggering the issue however the users
can not report it properly to the firmware vendors and the firmware vendors
can not easily see which specific hash is duplicated.

While changing the log message also use the dedicated ERR_PTR format
placeholder for the returned error value.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 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.39.0

[PATCH RESEND v6 2/3] KEYS: Add new function key_create()

[Thomas Weißschuh]

key_create() works like key_create_or_update() but does not allow
updating an existing key, instead returning ERR_PTR(-EEXIST).

key_create() will be used by the blacklist keyring which should not
create duplicate entries or update existing entries.
Instead a dedicated message with appropriate severity will be logged.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 include/linux/key.h |   8 +++
 security/keys/key.c | 137 ++++++++++++++++++++++++++++++++++++++--------------
 2 files changed, 108 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..5c0c7df833f8 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,82 @@ 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.39.0

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

[Thomas Weißschuh]

When the same key is blacklisted repeatedly logging at pr_err() level is
excessive as no functionality is impaired.
When these duplicates are provided by buggy firmware there is nothing
the user can do to fix the situation.
Instead of spamming the bootlog with errors we use a warning that can
still be seen by OEMs when testing their firmware.

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>
Tested-by: Paul Menzel <pmenzel@molgen.mpg.de>
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 certs/blacklist.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/certs/blacklist.c b/certs/blacklist.c
index 6e260c4b6a19..675dd7a8f07a 100644
--- a/certs/blacklist.c
+++ b/certs/blacklist.c
@@ -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.39.0

Re: [PATCH RESEND v6 0/3] certs: Prevent spurious errors on repeated blacklisting

[Jarkko Sakkinen]

On Mon, Jan 09, 2023 at 11:59:41PM +0000, Thomas Weißschuh wrote:
> 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
> * Lenovo: https://lore.kernel.org/lkml/c8c65713-5cda-43ad-8018-20f2e32e4432@t-8ch.de/
> 
> Note: In the meantime I lost access to the machine exhibiting the
> problematic behavior. If larger changes are required to this series
> somebody else would have to validate them or take over the series.
> 
> Changelog:
> 
> v1: https://lore.kernel.org/all/20221104014704.3469-1-linux@weissschuh.net/
> v1 -> v2:
>  * Improve logging message to include the failed hash
>  * Add key_create() function without update semantics
>  * Use key_create() from mark_raw_hash_blacklisted() and log specific message
>    on -EEXIST
> 
> v2: https://lore.kernel.org/lkml/20221109025019.1855-1-linux@weissschuh.net/
> v2 -> v3:
>  * Clarify commit titles and messages
>  * Drop the change to BLACKLIST_KEY_PERM from patch 3, as it was an artifact
>    of some obsolete version of the patch and not needed
> 
> v3: https://lore.kernel.org/lkml/20221118040343.2958-1-linux@weissschuh.net/
> v3 -> v4:
>  * Drop Fixes-tag from first patch
>  * Flesh out commit descriptions and messages
> 
> v4: https://lore.kernel.org/r/20221212-keys-blacklist-v4-0-00afeb3137fb@weissschuh.net
> v4 -> v5:
>  * Reduce lines needed by function calls in key.c
>  * Add Reviewed-by from Jarkko
> 
> v5: https://lore.kernel.org/r/20221212-keys-blacklist-v5-0-52e9eb5a8827@weissschuh.net
> v5 -> v6:
>  * Correct Jarkkos email in Reviewed-by tags
>  * Resend to hopefully reach @kernel.org recipients
> 
> Thomas Weißschuh (3):
>   certs: log hash value on blacklist error
>   KEYS: Add key_create()
>   certs: don't try to update blacklist keys
> 
>  certs/blacklist.c   |  21 ++++---
>  include/linux/key.h |   8 +++
>  security/keys/key.c | 149 +++++++++++++++++++++++++++++++++-----------
>  3 files changed, 132 insertions(+), 46 deletions(-)
> 
> --
> 2.38.1
> 
> To: David Howells <dhowells@redhat.com>
> To: David Woodhouse <dwmw2@infradead.org>
> To: Jarkko Sakkinen <jarkko@kernel.org>
> To: Paul Moore <paul@paul-moore.com>
> To: James Morris <jmorris@namei.org>
> To: "Serge E. Hallyn" <serge@hallyn.com>
> To: "Mickaël Salaün" <mic@digikod.net>
> Cc: keyrings@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-security-module@vger.kernel.org
> Cc: Paul Menzel <pmenzel@molgen.mpg.de>
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> Cc: Mark Pearson <markpearson@lenovo.com>
> 
> ---
> Thomas Weißschuh (3):
>       certs: make blacklisted hash available in klog
>       KEYS: Add new function key_create()
>       certs: don't try to update blacklist keys
> 
>  certs/blacklist.c   |  21 ++++----
>  include/linux/key.h |   8 +++
>  security/keys/key.c | 137 ++++++++++++++++++++++++++++++++++++++--------------
>  3 files changed, 120 insertions(+), 46 deletions(-)
> ---
> base-commit: 512dee0c00ad9e9c7ae9f11fc6743702ea40caff
> change-id: 20221212-keys-blacklist-2c79a64667c9
> 
> Best regards,
> -- 
> Thomas Weißschuh <linux@weissschuh.net>

Hi, I'e applied and pushed this now. Thank you.

BR, Jarkko