[RFC] dm-crypt: add ability to use keys from the kernel key retention service

[RFC] dm-crypt: add ability to use keys from the kernel key retention service

[Andrey Ryabinin]

The kernel key service is a generic way to store keys for the use of
other subsystems. Currently there is no way to use kernel keys in dm-crypt.
This patch aims to fix that. Instead of key userspace may pass a key
description with preceding ':'. So message that constructs encryption
mapping now looks like this:

  <cipher> [<key>|:<key description>] <iv_offset> <dev_path> <start> [<#opt_params> <opt_params>]

(I could propose another possible way to distinguish key from the key description.
This would be to add new opt_param, something like 'key_in_keyring'.
So if key_in_keyring is set than we have key description instead of key.)

Note: We haven't implemented userspace part for this yet.

Signed-off-by: Andrey Ryabinin <aryabinin@virtuozzo.com>
---
 drivers/md/dm-crypt.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 86 insertions(+), 9 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 4e9784b..5a7899d 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -12,6 +12,7 @@
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
+#include <linux/key.h>
 #include <linux/bio.h>
 #include <linux/blkdev.h>
 #include <linux/mempool.h>
@@ -29,6 +30,7 @@
 #include <crypto/md5.h>
 #include <crypto/algapi.h>
 #include <crypto/skcipher.h>
+#include <keys/user-type.h>
 
 #include <linux/device-mapper.h>
 
@@ -1489,21 +1491,91 @@ static int crypt_setkey_allcpus(struct crypt_config *cc)
 	return err;
 }
 
+#ifdef CONFIG_KEYS
+static int crypt_set_keyring_key(struct crypt_config *cc, char *key_desc)
+{
+	int ret = 0;
+	struct key *key;
+	const struct user_key_payload *ukp;
+
+	key = request_key(&key_type_user, key_desc, NULL);
+	if (IS_ERR(key))
+		return PTR_ERR(key);
+
+	rcu_read_lock();
+	ret = key_validate(key);
+	if (ret < 0)
+		goto out;
+
+	ukp = user_key_payload(key);
+	if (cc->key_size != ukp->datalen) {
+		ret = -EINVAL;
+		goto out;
+	}
+	memcpy(cc->key, ukp->data, cc->key_size);
+out:
+	rcu_read_unlock();
+	key_put(key);
+	return ret;
+}
+
+static int get_key_size(const char *key_desc)
+{
+	int ret;
+	struct key *key;
+
+	if (key_desc[0] != ':')
+		return strlen(key_desc) >> 1;
+
+	key = request_key(&key_type_user, 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:
+	rcu_read_unlock();
+	key_put(key);
+	return ret;
+}
+#else
+static int crypt_set_keyring_key(struct crypt_config *cc, char *key_desc)
+{
+	return -EINVAL;
+}
+
+static int get_key_size(const char *key)
+{
+	return strlen(key) >> 1;
+}
+#endif
+
 static int crypt_set_key(struct crypt_config *cc, char *key)
 {
 	int r = -EINVAL;
 	int key_string_len = strlen(key);
 
-	/* The key size may not be changed. */
-	if (cc->key_size != (key_string_len >> 1))
-		goto out;
-
 	/* Hyphen (which gives a key_size of zero) means there is no key. */
 	if (!cc->key_size && strcmp(key, "-"))
 		goto out;
 
-	if (cc->key_size && crypt_decode_key(cc->key, key, cc->key_size) < 0)
-		goto out;
+	/* ':' means that the key is in kernel keyring */
+	if (key[0] == ':') {
+		if (crypt_set_keyring_key(cc, key + 1))
+			goto out;
+	} else {
+		/* The key size may not be changed. */
+		if (cc->key_size != (key_string_len >> 1))
+			goto out;
+
+		if (cc->key_size &&
+			crypt_decode_key(cc->key, key, cc->key_size) < 0)
+			goto out;
+	}
 
 	set_bit(DM_CRYPT_KEY_VALID, &cc->flags);
 
@@ -1730,12 +1802,13 @@ bad_mem:
 
 /*
  * Construct an encryption mapping:
- * <cipher> <key> <iv_offset> <dev_path> <start>
+ * <cipher> [<key>|:<key description>] <iv_offset> <dev_path> <start>
  */
 static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 {
 	struct crypt_config *cc;
-	unsigned int key_size, opt_params;
+	int key_size;
+	unsigned int opt_params;
 	unsigned long long tmpll;
 	int ret;
 	size_t iv_size_padding;
@@ -1752,7 +1825,11 @@ static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv)
 		return -EINVAL;
 	}
 
-	key_size = strlen(argv[1]) >> 1;
+	key_size = get_key_size(argv[1]);
+	if (key_size < 0) {
+		ti->error = "Cannot parse key";
+		return -EINVAL;
+	}
 
 	cc = kzalloc(sizeof(*cc) + key_size * sizeof(u8), GFP_KERNEL);
 	if (!cc) {
-- 
2.7.3

Re: [dm-devel] [RFC] dm-crypt: add ability to use keys from the kernel key retention service

[Ondrej Kozina]

On 08/09/2016 03:56 PM, Andrey Ryabinin wrote:

Hi Andrey,

I'm definitely in favour of dm-crypt with support for kernel keyring 
service, but I think this patch do lack in addressing few issues:

Currently the dm-crypt guarantees that on remove ioctl command the 
volume key gets deleted from both crypto layer and device-mapper 
subsystem without exception. I believe we should stick to the guarantee. 
At least let's provide an option that would immediately invalidate the 
key passed via key description on table destruction. Or on last table 
destruction that would reach dm-crypt internal reference count on such 
key equal to zero. Each table key has at least single copy in crypto 
layer anyway... This is no big deal on live system (copy in crypto 
layer) but after proper system shutdown there should be no key in system 
memory (coldboot risk mitigation).

While it may sound contradicting my claim about guaranteed key 
destruction I'd like to be able to perform table load (imagine admin 
wants to resize dm-crypt device) without need of reuploading the key 
every time. Even when such user/admin has no access to already active 
volume key put in a keyring. IOW it doesn't matter what keyring the key 
was originally anchored in. (Re)load of table with valid key description 
should always pass).

The uspace part is about to land in cryptsetup 2.0 hopefully later this 
year. Most probably the kernel keyring will be used with other features 
of 2.0 release apart from loading dm-crypt mappings.

Last but not least: Mind me asking where do you plan to use it? In case 
we come with different implementation I'd like to reassure it'll be 
still of use to you.

Regards Ondrej

Re: [dm-devel] [RFC] dm-crypt: add ability to use keys from the kernel key retention service

[Andrey Ryabinin]

On 08/10/2016 02:16 PM, Ondrej Kozina wrote:
> On 08/09/2016 03:56 PM, Andrey Ryabinin wrote:
> 
> Hi Andrey,
> 
> I'm definitely in favour of dm-crypt with support for kernel keyring service, but I think this patch do lack in addressing few issues:
> 
> Currently the dm-crypt guarantees that on remove ioctl command the volume key gets deleted from both crypto layer and device-mapper subsystem without exception. I believe we should stick to the guarantee. At least let's provide an option that would immediately invalidate the key passed via key description on table destruction. Or on last table destruction that would reach dm-crypt internal reference count on such key equal to zero. Each table key has at least single copy in crypto layer anyway... This is no big deal on live system (copy in crypto layer) but after proper system shutdown there should be no key in system memory (coldboot risk mitigation).
> 

Hmm... I don't think that the kernel should do this (delete keys from keyring).
It makes sense that remove command wipes the keys added by create command.
But create command doesn't add keys to the keyring, so it would be odd if remove command clear such key.
It sound like a userspace's responsibility to manage such keys.

> While it may sound contradicting my claim about guaranteed key destruction I'd like to be able to perform table load (imagine admin wants to resize dm-crypt device) without need of reuploading the key every time. Even when such user/admin has no access to already active volume key put in a keyring. IOW it doesn't matter what keyring the key was originally anchored in. (Re)load of table with valid key description should always pass).
> 

Well, at first we'll  need to understand how to perform table load without asking the key every time.
But, for that case, it shouldn't matter whether the key passed directly or placed in keyring.


> The uspace part is about to land in cryptsetup 2.0 hopefully later this year. Most probably the kernel keyring will be used with other features of 2.0 release apart from loading dm-crypt mappings.
> 
> Last but not least: Mind me asking where do you plan to use it? In case we come with different implementation I'd like to reassure it'll be still of use to you.
> 

Our plan is to use this for encrypting containers.

> Regards Ondrej