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

[PATCH v3 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/

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

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 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(-)


base-commit: 84368d882b9688bfac77ce48d33b1e20a4e4a787
-- 
2.38.1

[PATCH v3 1/3] certs: log hash value on blacklist error

[Thomas Weißschuh]

Without this information these logs are not actionable.

For example on duplicate blacklisted hashes reported by the system
firmware users should be able to report the erroneous hashes to their
system vendors.

While we are at it use the dedicated format string for ERR_PTR.

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 v3 1/3] certs: log hash value on blacklist error

[Jarkko Sakkinen]

"Make blacklisted hash available in klog"

On Fri, Nov 18, 2022 at 05:03:41AM +0100, Thomas Weißschuh wrote:
> Without this information these logs are not actionable.

Without blacklisted hash?

> For example on duplicate blacklisted hashes reported by the system
> firmware users should be able to report the erroneous hashes to their
> system vendors.
> 
> While we are at it use the dedicated format string for ERR_PTR.

Lacks the beef so saying "while we are at it" makes no sense.

> Fixes: 6364d106e041 ("certs: Allow root user to append signed hashes to the blacklist keyring")

Why does this count as a bug?

> 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
> 

BR, Jarkko

Re: [PATCH v3 1/3] certs: log hash value on blacklist error

[Thomas Weißschuh]

On 2022-11-28 03:11+0200, Jarkko Sakkinen wrote:
> "Make blacklisted hash available in klog"
> 
> On Fri, Nov 18, 2022 at 05:03:41AM +0100, Thomas Weißschuh wrote:
> > Without this information these logs are not actionable.
> 
> Without blacklisted hash?
> 
> > For example on duplicate blacklisted hashes reported by the system
> > firmware users should be able to report the erroneous hashes to their
> > system vendors.
> > 
> > While we are at it use the dedicated format string for ERR_PTR.
> 
> Lacks the beef so saying "while we are at it" makes no sense.

What about this:

  [PATCH] certs: make blacklisted hash available in klog

  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.

> > Fixes: 6364d106e041 ("certs: Allow root user to append signed hashes to the blacklist keyring")
> 
> Why does this count as a bug?

These error logs are confusing to users, prompting them to waste time
investigating them and even mess with their firmware settings.
(As indicated in the threads linked from the cover letter)

The most correct fix would be patches 2 and 3 from this series.

I was not sure if patch 2 would be acceptable for stable as it introduces new
infrastructure code.
So patch 1 enables users to report the issue to their firmware vendors and get
the spurious logs resolved that way.

If these assumptions are incorrect I can fold patch 1 into patch 3.

But are patch 2 and 3 material for stable?

> > 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
> > 
> 
> BR, Jarkko

Re: [PATCH v3 1/3] certs: log hash value on blacklist error

[Jarkko Sakkinen]

On Mon, Nov 28, 2022 at 02:59:20AM +0100, Thomas Weißschuh wrote:
> On 2022-11-28 03:11+0200, Jarkko Sakkinen wrote:
> > "Make blacklisted hash available in klog"
> > 
> > On Fri, Nov 18, 2022 at 05:03:41AM +0100, Thomas Weißschuh wrote:
> > > Without this information these logs are not actionable.
> > 
> > Without blacklisted hash?
> > 
> > > For example on duplicate blacklisted hashes reported by the system
> > > firmware users should be able to report the erroneous hashes to their
> > > system vendors.
> > > 
> > > While we are at it use the dedicated format string for ERR_PTR.
> > 
> > Lacks the beef so saying "while we are at it" makes no sense.
> 
> What about this:
> 
>   [PATCH] certs: make blacklisted hash available in klog
> 
>   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.

Looks looks a lot better thank you!

> > > Fixes: 6364d106e041 ("certs: Allow root user to append signed hashes to the blacklist keyring")
> > 
> > Why does this count as a bug?
> 
> These error logs are confusing to users, prompting them to waste time
> investigating them and even mess with their firmware settings.
> (As indicated in the threads linked from the cover letter)
> 
> The most correct fix would be patches 2 and 3 from this series.
> 
> I was not sure if patch 2 would be acceptable for stable as it introduces new
> infrastructure code.
> So patch 1 enables users to report the issue to their firmware vendors and get
> the spurious logs resolved that way.
> 
> If these assumptions are incorrect I can fold patch 1 into patch 3.
> 
> But are patch 2 and 3 material for stable?

I cannot say anything conclusive to this before seeing updated version of
the patch set.

BR, Jarkko

[PATCH v3 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 to handle EEXIST
errors specially by logging a different message with lower severity.

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 v3 2/3] KEYS: Add key_create()

[Jarkko Sakkinen]

On Fri, Nov 18, 2022 at 05:03:42AM +0100, Thomas Weißschuh wrote:
> This function works like key_create_or_update() but does not allow
> updating an existing key, instead returning -EEXIST.

What is "this"??

> This new function will be used by the blacklist keyring to handle EEXIST

Ditto.

BR, Jarkko

[PATCH v3 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 enduser 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>
---
 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.38.1

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

[Paul Menzel]

Dear Thomas,


Am 18.11.22 um 05:03 schrieb Thomas Weißschuh:

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

After updating the UEFI firmware of the MSI B350M-MORTAR [1] from BIOS 
1.MV 06/23/2020 to BIOS 1.O6 07/13/2022 (7A37v1O6 (Beta version)), the 
same (uninformative) errors were logged by Linux. With your patches, the 
errors are gone.

Tested-by: Paul Menzel <pmenzel@molgen.mpg.de>


Kind regards,

Paul


[1]: https://de.msi.com/Motherboard/B350M-MORTAR/support