[dm-devel] [PATCH 3/3] dm-crypt: modifications to previous patch

[Ondrej Kozina]

1) if we load kernel keyring key referenced by key description we
should also return same key description via later status call. First
the table should remain the same, second it's not reasonable to expose
kernel key payload that could already have been be invalidated/expired
by kernel keyring API.

2) search 'logon' key type instead of 'user' key type. 'logon' type
key payloads can't be read from uspace. Not sure this is _the_ correct
way either. Do we want dm-crypt to be able to load arbitrary key
type? If so perhaps we should prefix a key description with key type?
---
 drivers/md/dm-crypt.c | 83 +++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 60 insertions(+), 23 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 3b0f2a3..a038942 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -142,6 +142,7 @@ struct crypt_config {
 
 	char *cipher;
 	char *cipher_string;
+	char *key_description;
 
 	struct crypt_iv_operations *iv_gen_ops;
 	union {
@@ -1495,13 +1496,20 @@ static int crypt_setkey_allcpus(struct crypt_config *cc)
 #ifdef CONFIG_KEYS
 static int crypt_set_keyring_key(struct crypt_config *cc, char *key_desc)
 {
-	int ret = 0;
+	char *new_key_desc;
+	int ret;
 	struct key *key;
 	const struct user_key_payload *ukp;
 
-	key = request_key(&key_type_user, key_desc, NULL);
-	if (IS_ERR(key))
+	new_key_desc = kstrdup(key_desc, GFP_KERNEL);
+	if (!new_key_desc)
+		return -ENOMEM;
+
+	key = request_key(&key_type_logon, key_desc, NULL);
+	if (IS_ERR(key)) {
+		kzfree(new_key_desc);
 		return PTR_ERR(key);
+	}
 
 	rcu_read_lock();
 	ret = key_validate(key);
@@ -1514,9 +1522,30 @@ static int crypt_set_keyring_key(struct crypt_config *cc, char *key_desc)
 		goto out;
 	}
 	memcpy(cc->key, ukp->data, cc->key_size);
+
+	rcu_read_unlock();
+	key_put(key);
+
+	/* clear the flag since following operations may invalidate previously valid key */
+	clear_bit(DM_CRYPT_KEY_VALID, &cc->flags);
+
+	ret = crypt_setkey_allcpus(cc);
+
+	/* wipe the kernel key payload in each case */
+	memset(cc->key, 0, cc->key_size * sizeof(u8));
+
+	if (!ret) {
+		set_bit(DM_CRYPT_KEY_VALID, &cc->flags);
+		kzfree(cc->key_description);
+		cc->key_description = new_key_desc;
+	} else
+		kzfree(new_key_desc);
+
+	return ret;
 out:
 	rcu_read_unlock();
 	key_put(key);
+	kzfree(new_key_desc);
 	return ret;
 }
 
@@ -1528,17 +1557,14 @@ static int get_key_size(const char *key_desc)
 	if (key_desc[0] != ':')
 		return strlen(key_desc) >> 1;
 
-	key = request_key(&key_type_user, key_desc + 1, NULL);
+	key = request_key(&key_type_logon, key_desc + 1, NULL);
 	if (IS_ERR(key))
 		return PTR_ERR(key);
 
 	rcu_read_lock();
 	ret = key_validate(key);
-	if (ret < 0)
-		goto out;
-
-	ret = user_key_payload(key)->datalen;
-out:
+	if (!ret)
+		ret = user_key_payload(key)->datalen;
 	rcu_read_unlock();
 	key_put(key);
 	return ret;
@@ -1566,25 +1592,30 @@ static int crypt_set_key(struct crypt_config *cc, char *key)
 
 	/* ':' means that the key is in kernel keyring */
 	if (key[0] == ':') {
-		if (crypt_set_keyring_key(cc, key + 1))
+		if (key_string_len < 2)
 			goto out;
+
+		r = crypt_set_keyring_key(cc, key + 1);
 	} else {
 		/* The key size may not be changed. */
 		if (cc->key_size != (key_string_len >> 1))
 			goto out;
-	}
 
-	/* clear the flag since following operations may invalidate previously valid key */
-	clear_bit(DM_CRYPT_KEY_VALID, &cc->flags);
+		/* clear the flag since following operations may invalidate previously valid key */
+		clear_bit(DM_CRYPT_KEY_VALID, &cc->flags);
 
-	if (key[0] != ':' && cc->key_size &&
-		crypt_decode_key(cc->key, key, cc->key_size) < 0)
-		goto out;
+		/* wipe references to any kernel keyring key */
+		kzfree(cc->key_description);
+		cc->key_description = NULL;
 
-	r = crypt_setkey_allcpus(cc);
-	if (!r)
-		set_bit(DM_CRYPT_KEY_VALID, &cc->flags);
+		if (cc->key_size &&
+		    crypt_decode_key(cc->key, key, cc->key_size) < 0)
+			goto out;
 
+		r = crypt_setkey_allcpus(cc);
+		if (!r)
+			set_bit(DM_CRYPT_KEY_VALID, &cc->flags);
+	}
 out:
 	/* Hex key string not needed after here, so wipe it. */
 	memset(key, '0', key_string_len);
@@ -1596,6 +1627,8 @@ static int crypt_wipe_key(struct crypt_config *cc)
 {
 	clear_bit(DM_CRYPT_KEY_VALID, &cc->flags);
 	memset(&cc->key, 0, cc->key_size * sizeof(u8));
+	kzfree(cc->key_description);
+	cc->key_description = NULL;
 
 	return crypt_setkey_allcpus(cc);
 }
@@ -1633,6 +1666,7 @@ static void crypt_dtr(struct dm_target *ti)
 
 	kzfree(cc->cipher);
 	kzfree(cc->cipher_string);
+	kzfree(cc->key_description);
 
 	/* Must zero key material before freeing */
 	kzfree(cc);
@@ -2035,10 +2069,13 @@ static void crypt_status(struct dm_target *ti, status_type_t type,
 	case STATUSTYPE_TABLE:
 		DMEMIT("%s ", cc->cipher_string);
 
-		if (cc->key_size > 0)
-			for (i = 0; i < cc->key_size; i++)
-				DMEMIT("%02x", cc->key[i]);
-		else
+		if (cc->key_size > 0) {
+			if (cc->key_description)
+				DMEMIT(":%s", cc->key_description);
+			else
+				for (i = 0; i < cc->key_size; i++)
+					DMEMIT("%02x", cc->key[i]);
+		} else
 			DMEMIT("-");
 
 		DMEMIT(" %llu %s %llu", (unsigned long long)cc->iv_offset,
-- 
2.7.4