Re: [PATCH 1/3] certs: blacklist: define skid: entries and add is_key_blacklisted()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] certs: ignored keyring_search() errors in
  is_hash_blacklisted() [1]
--

commit 82f0c39aa99cf26118fffcb57200171e393b838c
Author: Mimi Zohar <zohar@linux.ibm.com>

certs: blacklist: define skid: entries and add is_key_blacklisted()

This commit defines a "skid:<hex>" blacklist entry type for Subject Key
Identifiers and adds is_key_blacklisted() to look up an asymmetric key's
SKID in the system blacklist keyring.

> diff --git a/certs/blacklist.c b/certs/blacklist.c
> index fa561c12eabfe..9158bd9f9fc90 100644
> --- a/certs/blacklist.c
> +++ b/certs/blacklist.c

[ ... ]

> @@ -244,6 +243,40 @@ int is_hash_blacklisted(const u8 *hash, size_t hash_len,
>  }
>  EXPORT_SYMBOL_GPL(is_hash_blacklisted);
>  
> +#ifdef CONFIG_ASYMMETRIC_KEY_TYPE
> +/**
> + * is_key_blacklisted - Determine if a key is blacklisted by its SKID
> + * @key: The key to be checked, e.g. a trust anchor found in a keyring
> + *
> + * Unlike a "tbs:" entry, which is only compared against certificates being
> + * added or presented in a chain, this checks a key that is already loaded, so
> + * that keys that cannot be unlinked from their keyring can still be revoked.
> + * The identifier is taken from the key itself, not from the caller.
> + *
> + * Returns -EKEYREJECTED if the key's Subject Key Identifier is blacklisted,
> + * 0 if it is not (or the key has no SKID), or another negative error.
> + */
> +int is_key_blacklisted(const struct key *key)
> +{
> +	const struct asymmetric_key_ids *kids;
> +	const struct asymmetric_key_id *skid;
> +
> +	if (!key || key->type != &key_type_asymmetric)
> +		return 0;
> +
> +	kids = asymmetric_key_ids(key);
> +	if (!kids)
> +		return 0;
> +
> +	skid = kids->id[1];
> +	if (!skid || !skid->len)
> +		return 0;
> +
> +	return is_hash_blacklisted(skid->data, skid->len, BLACKLIST_HASH_SKID);
> +}
> +EXPORT_SYMBOL_GPL(is_key_blacklisted);
> +#endif

[Severity: Critical]
This problem wasn't introduced by this patch, but can is_hash_blacklisted()
fail open when keyring_search() returns an error pointer [1]?

certs/blacklist.c:is_hash_blacklisted() {
	...
	kref = keyring_search(make_key_ref(blacklist_keyring, true),
			      &key_type_blacklist, buffer, false);
	if (!IS_ERR(kref)) {
		key_ref_put(kref);
		ret = -EKEYREJECTED;
	}

	kfree(buffer);
	return ret;
}

keyring_search() checks KEY_NEED_SEARCH against current_cred() on
blacklist_keyring and any matching blacklist key.  If a confined task's LSM
policy denies search permission and keyring_search() returns
ERR_PTR(-EACCES), is_hash_blacklisted() discards the error and returns 0,
allowing blacklisted keys, certificates, or binaries to pass verification.

[1] https://sashiko.dev/#/bug/linux-151043b5-dceb-4214-adc8-c004531dcb8d

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106173149.707244-1-zohar@linux.ibm.com?part=1