[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