[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

[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