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/[email protected]/
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/[email protected]/
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/[email protected]/
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/[email protected]/
v3 -> v4:
* Drop Fixes-tag from first patch
* Flesh out commit descriptions and messages
v4: https://lore.kernel.org/r/[email protected]
v4 -> v5:
* Reduce lines needed by function calls in key.c
* Add Reviewed-by from Jarkko
v5: https://lore.kernel.org/r/[email protected]
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 <[email protected]>
To: David Woodhouse <[email protected]>
To: Jarkko Sakkinen <[email protected]>
To: Paul Moore <[email protected]>
To: James Morris <[email protected]>
To: "Serge E. Hallyn" <[email protected]>
To: "Mickaël Salaün" <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: Paul Menzel <[email protected]>
Signed-off-by: Thomas Weißschuh <[email protected]>
Cc: Mark Pearson <[email protected]>
---
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 <[email protected]>
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/[email protected]/
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Thomas Weißschuh <[email protected]>
Tested-by: Paul Menzel <[email protected]>
Reviewed-by: Jarkko Sakkinen <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Jarkko Sakkinen <[email protected]>
---
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
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/[email protected]/
>
> 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/[email protected]/
> 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/[email protected]/
> 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/[email protected]/
> v3 -> v4:
> * Drop Fixes-tag from first patch
> * Flesh out commit descriptions and messages
>
> v4: https://lore.kernel.org/r/[email protected]
> v4 -> v5:
> * Reduce lines needed by function calls in key.c
> * Add Reviewed-by from Jarkko
>
> v5: https://lore.kernel.org/r/[email protected]
> 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 <[email protected]>
> To: David Woodhouse <[email protected]>
> To: Jarkko Sakkinen <[email protected]>
> To: Paul Moore <[email protected]>
> To: James Morris <[email protected]>
> To: "Serge E. Hallyn" <[email protected]>
> To: "Micka?l Sala?n" <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: Paul Menzel <[email protected]>
> Signed-off-by: Thomas Wei?schuh <[email protected]>
> Cc: Mark Pearson <[email protected]>
>
> ---
> 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 <[email protected]>
Hi, I'e applied and pushed this now. Thank you.
BR, Jarkko