2022-11-09 03:37:41

by Thomas Weißschuh

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

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/[email protected]/

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



2022-11-09 03:40:23

by Thomas Weißschuh

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

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 <[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.38.1


2022-11-09 03:44:23

by Thomas Weißschuh

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

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 <[email protected]>
---
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


2022-11-16 00:19:00

by Jarkko Sakkinen

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

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

2022-11-16 01:03:18

by Jarkko Sakkinen

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

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 <[email protected]>
> ---
> 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