* Re: [PATCH v4 1/3] certs: make blacklisted hash available in klog
[not found] ` <20221212-keys-blacklist-v4-1-00afeb3137fb@weissschuh.net>
@ 2023-01-04 12:31 ` , Jarkko Sakkinen
0 siblings, 0 replies; 3+ messages in thread
From: , Jarkko Sakkinen @ 2023-01-04 12:31 UTC (permalink / raw)
To: Thomas Weißschuh
Cc: David Howells, David Woodhouse, Paul Moore, James Morris,
Serge E. Hallyn, Mickaël Salaün, keyrings,
linux-kernel, linux-security-module, Paul Menzel, Mark Pearson
On Wed, Dec 21, 2022 at 02:08:22AM +0000, Thomas Weißschuh wrote:
> 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>
> ---
> 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
Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>
BR, Jarkko
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v4 2/3] KEYS: Add new function key_create()
[not found] ` <20221212-keys-blacklist-v4-2-00afeb3137fb@weissschuh.net>
@ 2023-01-04 12:34 ` , Jarkko Sakkinen
0 siblings, 0 replies; 3+ messages in thread
From: , Jarkko Sakkinen @ 2023-01-04 12:34 UTC (permalink / raw)
To: Thomas Weißschuh
Cc: David Howells, David Woodhouse, Paul Moore, James Morris,
Serge E. Hallyn, Mickaël Salaün, keyrings,
linux-kernel, linux-security-module, Paul Menzel, Mark Pearson
On Wed, Dec 21, 2022 at 02:08:23AM +0000, Thomas Weißschuh wrote:
> 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 | 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);
I'd consider:
return __key_create_or_update(keyring_ref, type, description, payload, plen, perm, flags,
false);
I don't see any logical reason to spread this to 8 lines.
> +}
> +EXPORT_SYMBOL(key_create);
> +
> /**
> * key_update - Update a key's contents.
> * @key_ref: The pointer (plus possession flag) to the key.
>
> --
> 2.39.0
BR, Jarkko
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v4 3/3] certs: don't try to update blacklist keys
[not found] ` <20221212-keys-blacklist-v4-3-00afeb3137fb@weissschuh.net>
@ 2023-01-04 12:35 ` , Jarkko Sakkinen
0 siblings, 0 replies; 3+ messages in thread
From: , Jarkko Sakkinen @ 2023-01-04 12:35 UTC (permalink / raw)
To: Thomas Weißschuh
Cc: David Howells, David Woodhouse, Paul Moore, James Morris,
Serge E. Hallyn, Mickaël Salaün, keyrings,
linux-kernel, linux-security-module, Paul Menzel, Mark Pearson
On Wed, Dec 21, 2022 at 02:08:24AM +0000, Thomas Weißschuh wrote:
> 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>
> ---
> 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
Reviewed-by: Jarkko Sakkinen <jarko@kernel.org>
BR, Jarkko
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-01-04 12:35 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20221212-keys-blacklist-v4-0-00afeb3137fb@weissschuh.net>
[not found] ` <20221212-keys-blacklist-v4-1-00afeb3137fb@weissschuh.net>
2023-01-04 12:31 ` [PATCH v4 1/3] certs: make blacklisted hash available in klog , Jarkko Sakkinen
[not found] ` <20221212-keys-blacklist-v4-2-00afeb3137fb@weissschuh.net>
2023-01-04 12:34 ` [PATCH v4 2/3] KEYS: Add new function key_create() , Jarkko Sakkinen
[not found] ` <20221212-keys-blacklist-v4-3-00afeb3137fb@weissschuh.net>
2023-01-04 12:35 ` [PATCH v4 3/3] certs: don't try to update blacklist keys , Jarkko Sakkinen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).