Re: [PATCH v11 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:
- [High] s390/cert_store: error pointer dereference in
  invalidate_keyring_keys()
- [Medium] s390/cert_store: stale positive return value in
  invalidate_keyring_keys()
--

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

s390/cert_store: Skip keys that disappeared during invalidation

This commit adds a NULL check after key_lookup() in
invalidate_keyring_keys() to skip keys that were unlinked before lookup.

> diff --git a/arch/s390/kernel/cert_store.c b/arch/s390/kernel/cert_store.c
> index 0a10b31d68af3..bc682216fa7fd 100644
> --- a/arch/s390/kernel/cert_store.c
> +++ b/arch/s390/kernel/cert_store.c
> @@ -333,6 +333,9 @@ static int invalidate_keyring_keys(struct key *keyring)
>  
>  	for (key_index = 0; key_index < num_keys; key_index++) {
>  		current_key = key_lookup(key_array[key_index]);
> +		if (!current_key)
> +			continue;
> +
>  		pr_dbf_msg("Invalidating key %08x", current_key->serial);
>  
>  		key_invalidate(current_key);

[Severity: High]
Should this check IS_ERR(current_key) instead of !current_key?  When a key
is not found or its usage count has dropped to zero, key_lookup() returns
ERR_PTR(-ENOKEY) rather than NULL, so current_key->serial and
key_invalidate(current_key) will still dereference an error pointer.

[Severity: Medium]
Can skipping keys via continue cause invalidate_keyring_keys() to return a
positive byte count on success?  Before the loop, rc is set to
keyring_payload_len by key_type_keyring.read() and is only updated by
key_unlink():

arch/s390/kernel/cert_store.c:invalidate_keyring_keys() {
	...
	rc = key_type_keyring.read(keyring, (char *)key_array, keyring_payload_len);
	if (rc != keyring_payload_len) {
		pr_dbf_msg("Couldn't read keyring payload");
		goto out;
	}

	for (key_index = 0; key_index < num_keys; key_index++) {
		...
		rc = key_unlink(keyring, current_key);
	...
out:
	kfree(key_array);
	return rc;
}

If all keys in key_array have disappeared and are skipped, rc remains
keyring_payload_len, causing cleanup_cs_keys() to treat the call as a
failure and unregister key_type_cert_store_key.

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