[PATCH] s390/cert_store: Hold the keyring->sem while invalidating keys

[PATCH] s390/cert_store: Hold the keyring->sem while invalidating keys

[Jarkko Sakkinen]

invalidate_keyring_keys() does not hold keyring->sem while calling
keyring_read(), which is contrary to what keyring assumes from its callers,
and can lead to reoccuring bugs.

KEY_FLAG_KEEP alone does not e.g.,  protect from  the following call path:

1. keyctl_keyring_move()
2. key_move()
3. __key_unlink()

Address the issue by guarding the critical section.

Fixes: 8cf57d7217c3 ("s390: add support for user-defined certificates")
Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
This came up as a side product while working on assoc_array to
rhashtable migration and going through various callers of the keyring.
It's made by me but please keep in mind that I don't even have a
toolchain for s390 setup so it is based only on reading code and I have
not even compiled this.

Just wanted to clarify the background for this fix as I'm not a regular
contributor over here. If for any reason it turned out to be false
positive, I'm fine with that too.
 arch/s390/kernel/cert_store.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/arch/s390/kernel/cert_store.c b/arch/s390/kernel/cert_store.c
index 998f2da56175..d35b1ea22507 100644
--- a/arch/s390/kernel/cert_store.c
+++ b/arch/s390/kernel/cert_store.c
@@ -18,6 +18,7 @@
 #include <linux/keyctl.h>
 #include <linux/kobject.h>
 #include <linux/module.h>
+#include <linux/rwsem.h>
 #include <linux/seq_file.h>
 #include <linux/slab.h>
 #include <linux/sysfs.h>
@@ -320,13 +321,17 @@ static int invalidate_keyring_keys(struct key *keyring)
 	struct key *current_key;
 	int rc;

-	keyring_payload_len = key_type_keyring.read(keyring, NULL, 0);
-	num_keys = keyring_payload_len / sizeof(key_serial_t);
-	key_array = kzalloc_objs(key_serial_t, num_keys);
-	if (!key_array)
-		return -ENOMEM;
+	scoped_guard(rwsem_read, &keyring->sem) {
+		keyring_payload_len = key_type_keyring.read(keyring, NULL, 0);
+		num_keys = keyring_payload_len / sizeof(key_serial_t);
+
+		key_array = kzalloc_objs(key_serial_t, num_keys);
+		if (!key_array)
+			return -ENOMEM;
+
+		rc = key_type_keyring.read(keyring, (char *)key_array, keyring_payload_len);
+	}

-	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;
--
2.47.3

Re: [PATCH] s390/cert_store: Hold the keyring->sem while invalidating keys

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] s390/cert_store: scoped_guard() mixed with goto in invalidate_keyring_keys()
--

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

s390/cert_store: Hold the keyring->sem while invalidating keys

This commit acquires keyring->sem for reading via scoped_guard() in
invalidate_keyring_keys() while querying the keyring payload length and
reading the key serial numbers.

> diff --git a/arch/s390/kernel/cert_store.c b/arch/s390/kernel/cert_store.c
> index 998f2da561750..d35b1ea22507d 100644
> --- a/arch/s390/kernel/cert_store.c
> +++ b/arch/s390/kernel/cert_store.c
[ ... ]
> @@ -320,13 +321,17 @@ static int invalidate_keyring_keys(struct key *keyring)
>  	struct key *current_key;
>  	int rc;
>  
> -	keyring_payload_len = key_type_keyring.read(keyring, NULL, 0);
> -	num_keys = keyring_payload_len / sizeof(key_serial_t);
> -	key_array = kzalloc_objs(key_serial_t, num_keys);
> -	if (!key_array)
> -		return -ENOMEM;
> +	scoped_guard(rwsem_read, &keyring->sem) {
> +		keyring_payload_len = key_type_keyring.read(keyring, NULL, 0);
> +		num_keys = keyring_payload_len / sizeof(key_serial_t);
> +
> +		key_array = kzalloc_objs(key_serial_t, num_keys);
> +		if (!key_array)
> +			return -ENOMEM;
> +
> +		rc = key_type_keyring.read(keyring, (char *)key_array, keyring_payload_len);
> +	}
>  
> -	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;

[Severity: Low]
In invalidate_keyring_keys() in arch/s390/kernel/cert_store.c, this patch
introduces scoped_guard(rwsem_read, &keyring->sem) while the function still
uses goto out to free key_array when key_type_keyring.read() returns a length
mismatch:

	if (rc != keyring_payload_len) {
		pr_dbf_msg("Couldn't read keyring payload");
		goto out;
	}
	...
out:
	kfree(key_array);
	return rc;

include/linux/cleanup.h notes that goto-based cleanup and scope-based cleanup
helpers should not be mixed within the same function. Could key_array also be
declared with __free(kfree) = NULL so the out label and goto can be dropped,
or should explicit down_read() and up_read() calls be used instead?

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

Re: [PATCH] s390/cert_store: Hold the keyring->sem while invalidating keys

[Jarkko Sakkinen]

On Sat, Oct 24, 2026 at 02:58:25PM +0000, sashiko-bot@kernel.org wrote: 
> include/linux/cleanup.h notes that goto-based cleanup and scope-based cleanup
> helpers should not be mixed within the same function. Could key_array also be
> declared with __free(kfree) = NULL so the out label and goto can be dropped,
> or should explicit down_read() and up_read() calls be used instead?

This was actually a "design choice" as it is out of scope for the bug
fix.

Br, Jarkko

Re: [PATCH] s390/cert_store: Hold the keyring->sem while invalidating keys

[Heiko Carstens]

On Sat, Oct 24, 2026 at 05:42:25PM +0300, Jarkko Sakkinen wrote:
> invalidate_keyring_keys() does not hold keyring->sem while calling
> keyring_read(), which is contrary to what keyring assumes from its callers,
> and can lead to reoccuring bugs.
> 
> KEY_FLAG_KEEP alone does not e.g.,  protect from  the following call path:
> 
> 1. keyctl_keyring_move()
> 2. key_move()
> 3. __key_unlink()
> 
> Address the issue by guarding the critical section.
> 
> Fixes: 8cf57d7217c3 ("s390: add support for user-defined certificates")
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
> This came up as a side product while working on assoc_array to
> rhashtable migration and going through various callers of the keyring.
> It's made by me but please keep in mind that I don't even have a
> toolchain for s390 setup so it is based only on reading code and I have
> not even compiled this.
> 
> Just wanted to clarify the background for this fix as I'm not a regular
> contributor over here. If for any reason it turned out to be false
> positive, I'm fine with that too.

It is not a false positive. There seem to be more bugs in the code.
I'll take your patch and create a series with other fixes.

Thanks a lot!

Re: [PATCH] s390/cert_store: Hold the keyring->sem while invalidating keys

[Jarkko Sakkinen]

On Wed, Oct 28, 2026 at 04:34:19PM +0100, Heiko Carstens wrote:
> On Sat, Oct 24, 2026 at 05:42:25PM +0300, Jarkko Sakkinen wrote:
> > invalidate_keyring_keys() does not hold keyring->sem while calling
> > keyring_read(), which is contrary to what keyring assumes from its callers,
> > and can lead to reoccuring bugs.
> > 
> > KEY_FLAG_KEEP alone does not e.g.,  protect from  the following call path:
> > 
> > 1. keyctl_keyring_move()
> > 2. key_move()
> > 3. __key_unlink()
> > 
> > Address the issue by guarding the critical section.
> > 
> > Fixes: 8cf57d7217c3 ("s390: add support for user-defined certificates")
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > ---
> > This came up as a side product while working on assoc_array to
> > rhashtable migration and going through various callers of the keyring.
> > It's made by me but please keep in mind that I don't even have a
> > toolchain for s390 setup so it is based only on reading code and I have
> > not even compiled this.
> > 
> > Just wanted to clarify the background for this fix as I'm not a regular
> > contributor over here. If for any reason it turned out to be false
> > positive, I'm fine with that too.
> 
> It is not a false positive. There seem to be more bugs in the code.
> I'll take your patch and create a series with other fixes.

OK, great :-)

> 
> Thanks a lot!

You're welcome!

Br, Jarkko