Re: [PATCH v12 3/6] s390/cert_store: Skip keys that disappeared during invalidation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] s390/cert_store: incomplete handling of unlinked keys in
  invalidate_keyring_keys()
- [Low] s390/cert_store: commit message claims key_lookup() returns NULL
--

commit 725043b976271178c0635971476e80ff473d9670
Author: Jarkko Sakkinen <jarkko@kernel.org>

s390/cert_store: Skip keys that disappeared during invalidation

This commit updates invalidate_keyring_keys() to initialize rc to 0 and
skip keys when key_lookup() returns an error pointer during keyring
invalidation.

> key_lookup() can return NULL for an unlinked key. Skip such keys instead.

[Severity: Low]
Is this commit message description accurate?  CONFIG_CERT_STORE depends on
CONFIG_KEYS, and when CONFIG_KEYS is enabled, key_lookup() returns
ERR_PTR(-ENOKEY) rather than NULL (which matches the IS_ERR(current_key)
check in the diff).

> diff --git a/arch/s390/kernel/cert_store.c b/arch/s390/kernel/cert_store.c
> index 0a10b31d68af3..03391b379dad3 100644
> --- a/arch/s390/kernel/cert_store.c
> +++ b/arch/s390/kernel/cert_store.c
> @@ -331,8 +331,13 @@ static int invalidate_keyring_keys(struct key *keyring)
>  		goto out;
>  	}
>  
> +	rc = 0;
> +
>  	for (key_index = 0; key_index < num_keys; key_index++) {
>  		current_key = key_lookup(key_array[key_index]);
> +		if (IS_ERR(current_key))
> +			continue;
> +
>  		pr_dbf_msg("Invalidating key %08x", current_key->serial);
>  
>  		key_invalidate(current_key);

[Severity: Medium]
Does skipping only key_lookup() failures still leave unlinked keys unhandled
later in invalidate_keyring_keys()?

When a key is unlinked from a keyring, dropping its reference is deferred to
an RCU callback (or another reference may exist), so key_lookup() still
succeeds; key_invalidate(current_key) also schedules keyring_gc(), which can
unlink current_key before key_unlink() runs:

arch/s390/kernel/cert_store.c:invalidate_keyring_keys() {
	...
		key_invalidate(current_key);
		key_put(current_key);
		rc = key_unlink(keyring, current_key);
		if (rc) {
			pr_dbf_msg("Couldn't unlink key %08x: %d", current_key->serial, rc);
			break;
		}
}

In either case, key_unlink() returns -ENOENT, aborting the loop before the
remaining keys in key_array are invalidated and causing cleanup_cs_keys() to
fall back to unregister_key_type(&key_type_cert_store_key).  Note also that
key_put(current_key) drops the caller's reference before key_unlink() and
current_key->serial are used.

Similarly, if a key is unlinked between the two key_type_keyring.read() calls
earlier in invalidate_keyring_keys(), the second call returns a smaller byte
count than keyring_payload_len and jumps to out without invalidating the
remaining keys, returning 0 if the last key was unlinked or a positive byte
count otherwise.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114164139.429814-1-jarkko@kernel.org?part=3