diff mbox

dm-crypt: check key payload pointer not null

Message ID 1479934277-14913-1-git-send-email-okozina@redhat.com (mailing list archive)
State Accepted, archived
Delegated to: Mike Snitzer
Headers show

Commit Message

Ondrej Kozina Nov. 23, 2016, 8:51 p.m. UTC 
Hi Mike,

those are fixes for latest version (including your cleanup patch). I've fixed one
awkward bug the rest is based on David's sugestions he gave me earlier today.

David, may I also kindly ask you for oficial 'Reviewed-by' stamp provided 
it's correct now (with regard to kernel keyring bits)?

bugfix: harden checks for 'user' or 'logon' keywords in key_string (it allowed
to pass with :logo:... or "use:...)

bugfix: we have to check user_key_payload reference after rcu_read_lock().
Otherwise keys revoked between request_key() and rcu_read_lock() calls would
return NULL pointer.

improvement: calling key_validate() right after request_key() is
useless. Exactly same check is performed inside request_key()
routine.

improvement: do not check the whole key_type string again. We already know
it's either 'logon' or 'user'. Read just first char.
---
 drivers/md/dm-crypt.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

Comments

David Howells Nov. 24, 2016, 9:28 a.m. UTC | #1 
Looking at:

	https://git.kernel.org/cgit/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-4.10&id=3b0ef9ade65e299a8c13df5fe70352360c6bd05c

Reviewed-by: David Howells <dhowells@redhat.com>

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
diff mbox

Patch

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index c6ff083..78ab5e8 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -1501,31 +1501,31 @@  static int crypt_set_keyring_key(struct crypt_config *cc, const char *key_string
 	if (!key_desc || key_desc == key_string || !strlen(key_desc + 1))
 		return -EINVAL;
 
-	if (strncmp(key_string, "logon", key_desc - key_string) &&
-	    strncmp(key_string, "user", key_desc - key_string))
+	if (strncmp(key_string, "logon:", key_desc - key_string + 1) &&
+	    strncmp(key_string, "user:", key_desc - key_string + 1))
 		return -EINVAL;
 
 	new_key_string = kstrdup(key_string, GFP_KERNEL);
 	if (!new_key_string)
 		return -ENOMEM;
 
-	key = request_key(strncmp(key_string, "user", 4) ? &key_type_logon : &key_type_user,
+	key = request_key(key_string[0] == 'l' ? &key_type_logon : &key_type_user,
 			  key_desc + 1, NULL);
 	if (IS_ERR(key)) {
 		kzfree(new_key_string);
 		return PTR_ERR(key);
 	}
 
-	ret = key_validate(key);
-	if (ret < 0) {
+	rcu_read_lock();
+
+	ukp = user_key_payload(key);
+	if (!ukp) {
+		rcu_read_unlock();
 		key_put(key);
 		kzfree(new_key_string);
-		return ret;
+		return -EKEYREVOKED;
 	}
 
-	rcu_read_lock();
-
-	ukp = user_key_payload(key);
 	if (cc->key_size != ukp->datalen) {
 		rcu_read_unlock();
 		key_put(key);