[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

[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: [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

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

[PATCH 0/3] Modified kernel keyring support patch

[Ondrej Kozina]

Hi Andrey,

I'm sorry it took me so long to reply. I've revisited your patch and rebased it on top of my
fix for crypt_set_key(). The last patch in this series adresses my concerns about your original
patch. Would you mind resend your patch including those changes provided it doesn't break your
use case?

I haven't concluded the testing yet but so far cryptsetup testsuite passes with the patch set.

Please consider it still RFC only, I have to write corner-case tests for the kernel keyring bits yet.

With regard to my other suggestion related to guaranteed key erasure on table destruction (even when
provided only via optional parameter) it will require to patch kernel keyring
service so let's postpone it after we get those changes in upstream kernel.

Andrey Ryabinin (1):
  dm-crypt: add ability to use keys from the kernel key retention
    service

Ondrej Kozina (2):
  dm-crypt: mark key as invalid until properly loaded
  dm-crypt: modifications to previous patch

 drivers/md/dm-crypt.c | 147 ++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 132 insertions(+), 15 deletions(-)

-- 
2.7.4

[PATCH 1/3] dm-crypt: mark key as invalid until properly loaded

[Ondrej Kozina]

(just for reference. the patch's been already posted on dm-devel)

on crypt_message() key set code path while loading new key
replacing the old one in case tfm->setkey() fails
we would end having invalid key in crypto layer but
DM_CRYPT_KEY_VALID flag set.

Signed-off-by: Ondrej Kozina <okozina@redhat.com>
---
 drivers/md/dm-crypt.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index a276883..0aedd0e 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -1503,12 +1503,15 @@ static int crypt_set_key(struct crypt_config *cc, char *key)
 	if (!cc->key_size && strcmp(key, "-"))
 		goto out;
 
+	/* clear the flag since following operations may invalidate previously valid key */
+	clear_bit(DM_CRYPT_KEY_VALID, &cc->flags);
+
 	if (cc->key_size && crypt_decode_key(cc->key, key, cc->key_size) < 0)
 		goto out;
 
-	set_bit(DM_CRYPT_KEY_VALID, &cc->flags);
-
 	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. */
-- 
2.7.4

[PATCH 2/3] dm-crypt: add ability to use keys from the kernel key retention service

[Ondrej Kozina]

From: Andrey Ryabinin <aryabinin@virtuozzo.com>

(original patch rebased on top of previous one by Ondrej Kozina)

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 | 93 ++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 85 insertions(+), 8 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 0aedd0e..3b0f2a3 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>
 
@@ -1490,23 +1492,93 @@ 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;
 
+	/* ':' 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;
+	}
+
 	/* clear the flag since following operations may invalidate previously valid key */
 	clear_bit(DM_CRYPT_KEY_VALID, &cc->flags);
 
-	if (cc->key_size && crypt_decode_key(cc->key, key, cc->key_size) < 0)
+	if (key[0] != ':' && cc->key_size &&
+		crypt_decode_key(cc->key, key, cc->key_size) < 0)
 		goto out;
 
 	r = crypt_setkey_allcpus(cc);
@@ -1729,12 +1801,13 @@ static int crypt_ctr_cipher(struct dm_target *ti,
 
 /*
  * 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;
@@ -1751,7 +1824,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.4

[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

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

[Milan Broz]

I think that this patch should be merged with the previous one...

On 11/07/2016 10:38 AM, Ondrej Kozina wrote:
> 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?

I definitely prefer that userspace cannot access the key in keyring once
it is set (this was one of the design goals for keyring use).

Unfortunately key length is determined (in userspace) only by parsing
hexa representation in DM table.
(For example you will check if AES-128/AES-256 is used according the key length.)

I would say we should put expected key length into dm table, something like this:
<cipher> [<raw_hex_key>|:<key_bytes>:<keyring_id>] ...

(Key length is not secret attribute, moreover it was always visible
through hexa string there even if it was zeroed by dmsetup.)

Some notes:

- In previous patch, raw key is copied to cc->key always,
this patch wipes it if keyring key is used
(merging two patches avoid this).

- increase dm-crypt target version (maybe increasing minor is enough, just
old userspace will fail while parsing the new keyring string)

- please add your signed-of-by line :)

- shouldn't be a key description validated before passing it to keyring api?
(so we avoid similar surprises as just mikulas found with '+' in key :)

- please also fix Documentation/device-mapper/dm-crypt.txt

Anyway, I think this is a good start for the keyring use in dm-crypt.

Thanks,
Milan

> ---
>  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,
> 

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

[Ondrej Kozina]

(Please still consider it to be RFC only, I need to modify the uspace teststuite
again due to changes in key_string format. Also the changes to dm-crypt documentation
will follow before final submit. Feature wide I'd consider the patch being complete
unless any bugs would emerge)

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_string>] <iv_offset> <dev_path> <start> [<#opt_params> <opt_params>]

where <key_string> is in format: <key_size>:<key_type>:<key_description>

Currently we only support two elementary key types: 'user' and 'logon'.
Keys may be loaded in dm-crypt either via <key_string> or using
classical method and pass the key in hex representation directly.

dm-crypt device initialised with a key passed in hex representation may be
replaced with key passed in key_string format and vice versa.

(Patch is based on original work by Andrey Ryabinin)

Signed-off-by: Ondrej Kozina <okozina@redhat.com>
---
 drivers/md/dm-crypt.c | 167 ++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 148 insertions(+), 19 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 0aedd0e..f4189ca 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>
 
@@ -140,6 +142,7 @@ struct crypt_config {
 
 	char *cipher;
 	char *cipher_string;
+	char *key_string;
 
 	struct crypt_iv_operations *iv_gen_ops;
 	union {
@@ -1490,29 +1493,138 @@ static int crypt_setkey_allcpus(struct crypt_config *cc)
 	return err;
 }
 
-static int crypt_set_key(struct crypt_config *cc, char *key)
+#ifdef CONFIG_KEYS
+static int crypt_set_keyring_key(struct crypt_config *cc, const char *key_string)
 {
-	int r = -EINVAL;
-	int key_string_len = strlen(key);
+	char *new_key_string, *key_desc;
+	int ret;
+	struct key *key;
+	const struct user_key_payload *ukp;
 
-	/* The key size may not be changed. */
-	if (cc->key_size != (key_string_len >> 1))
+	/* look for next ':' separating key_type from key_description */
+	key_desc = strpbrk(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))
+		return -EINVAL;
+
+	new_key_string = kstrdup(key_string, GFP_KERNEL);
+	if (!new_key_string)
+		return -ENOMEM;
+
+	/*
+	 * FIXME: are there any key descriptions we should disallow users
+	 * from loading to dm-crypt? i.e.: kernel keys starting with '.'
+	 */
+
+	key = request_key(strncmp(key_string, "user", 4) ? &key_type_logon : &key_type_user, key_desc + 1, NULL);
+	if (IS_ERR(key)) {
+		kzfree(new_key_string);
+		return PTR_ERR(key);
+	}
+
+	rcu_read_lock();
+	ret = key_validate(key);
+	if (ret < 0)
 		goto out;
 
-	/* Hyphen (which gives a key_size of zero) means there is no key. */
-	if (!cc->key_size && strcmp(key, "-"))
+	ukp = user_key_payload(key);
+	if (cc->key_size != ukp->datalen) {
+		ret = -EINVAL;
 		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);
 
-	if (cc->key_size && crypt_decode_key(cc->key, key, cc->key_size) < 0)
-		goto out;
+	ret = crypt_setkey_allcpus(cc);
 
-	r = crypt_setkey_allcpus(cc);
-	if (!r)
+	/* 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_string);
+		cc->key_string = new_key_string;
+	} else
+		kzfree(new_key_string);
+
+	return ret;
+out:
+	rcu_read_unlock();
+	key_put(key);
+	kzfree(new_key_string);
+	return ret;
+}
+
+static int get_key_size(char **key_string)
+{
+	char *colon, dummy;
+	int ret;
+
+	if (*key_string[0] != ':')
+		return strlen(*key_string) >> 1;
 
+	/* look for next ':' in key string */
+	colon = strpbrk(*key_string + 1, ":");
+	if (!colon)
+		return -EINVAL;
+
+	if (sscanf(*key_string + 1, "%u%c", &ret, &dummy) != 2 || dummy != ':')
+		return -EINVAL;
+
+	*key_string = colon;
+
+	/* remaining key string should be :<logon|user>:<key_desc> */
+
+	return ret;
+}
+#else
+static int crypt_set_keyring_key(struct crypt_config *cc, const char *key_desc)
+{
+	return -EINVAL;
+}
+
+static int get_key_size(char **key)
+{
+	return (*key[0] == ':') ? -EINVAL : strlen(*key) >> 1;
+}
+#endif
+
+static int crypt_set_key(struct crypt_config *cc, char *key)
+{
+	int r = -EINVAL;
+	int key_string_len = strlen(key);
+
+	/* Hyphen (which gives a key_size of zero) means there is no key. */
+	if (!cc->key_size && strcmp(key, "-"))
+		goto out;
+
+	/* ':' means that the key is in kernel keyring */
+	if (key[0] == ':')
+		r = crypt_set_keyring_key(cc, key + 1);
+	else {
+		/* clear the flag since following operations may invalidate previously valid key */
+		clear_bit(DM_CRYPT_KEY_VALID, &cc->flags);
+
+		/* wipe references to any kernel keyring key */
+		kzfree(cc->key_string);
+		cc->key_string = NULL;
+
+		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);
@@ -1524,6 +1636,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_string);
+	cc->key_string = NULL;
 
 	return crypt_setkey_allcpus(cc);
 }
@@ -1561,6 +1675,7 @@ static void crypt_dtr(struct dm_target *ti)
 
 	kzfree(cc->cipher);
 	kzfree(cc->cipher_string);
+	kzfree(cc->key_string);
 
 	/* Must zero key material before freeing */
 	kzfree(cc);
@@ -1729,12 +1844,13 @@ static int crypt_ctr_cipher(struct dm_target *ti,
 
 /*
  * Construct an encryption mapping:
- * <cipher> <key> <iv_offset> <dev_path> <start>
+ * <cipher> [<key>|:<key_size>:<user|logon>:<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;
@@ -1751,7 +1867,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 size";
+		return -EINVAL;
+	}
 
 	cc = kzalloc(sizeof(*cc) + key_size * sizeof(u8), GFP_KERNEL);
 	if (!cc) {
@@ -1958,10 +2078,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_string)
+				DMEMIT(":%u:%s", cc->key_size, cc->key_string);
+			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,
@@ -2028,6 +2151,12 @@ static int crypt_message(struct dm_target *ti, unsigned argc, char **argv)
 			return -EINVAL;
 		}
 		if (argc == 3 && !strcasecmp(argv[1], "set")) {
+			/* The key size may not be changed. */
+			if (cc->key_size != get_key_size(&argv[2])) {
+				memset(argv[2], '0', strlen(argv[2]));
+				return -EINVAL;
+			}
+
 			ret = crypt_set_key(cc, argv[2]);
 			if (ret)
 				return ret;
@@ -2071,7 +2200,7 @@ static void crypt_io_hints(struct dm_target *ti, struct queue_limits *limits)
 
 static struct target_type crypt_target = {
 	.name   = "crypt",
-	.version = {1, 14, 1},
+	.version = {1, 15, 0},
 	.module = THIS_MODULE,
 	.ctr    = crypt_ctr,
 	.dtr    = crypt_dtr,
-- 
2.7.4

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

[Andrey Ryabinin]

On 11/16/2016 11:47 PM, Ondrej Kozina wrote:
> (Please still consider it to be RFC only, I need to modify the uspace teststuite
> again due to changes in key_string format. Also the changes to dm-crypt documentation
> will follow before final submit. Feature wide I'd consider the patch being complete
> unless any bugs would emerge)
> 
> 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_string>] <iv_offset> <dev_path> <start> [<#opt_params> <opt_params>]
> 
> where <key_string> is in format: <key_size>:<key_type>:<key_description>
> 
> Currently we only support two elementary key types: 'user' and 'logon'.
> Keys may be loaded in dm-crypt either via <key_string> or using
> classical method and pass the key in hex representation directly.
>

I think we need to hexify key description too, because it can contain spaces.
<key_size> seems like unnecessary complication. Kernel knows key_size, it doesn't need
that information from userspace.
Handling different types is probably an overkill too. If it works with logon keys,
why would we need to use 'user' keys?

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

[Milan Broz]

On 11/17/2016 05:35 PM, Andrey Ryabinin wrote:
> On 11/16/2016 11:47 PM, Ondrej Kozina wrote:
>> (Please still consider it to be RFC only, I need to modify the uspace teststuite
>> again due to changes in key_string format. Also the changes to dm-crypt documentation
>> will follow before final submit. Feature wide I'd consider the patch being complete
>> unless any bugs would emerge)
>>
>> 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_string>] <iv_offset> <dev_path> <start> [<#opt_params> <opt_params>]
>>
>> where <key_string> is in format: <key_size>:<key_type>:<key_description>
>>
>> Currently we only support two elementary key types: 'user' and 'logon'.
>> Keys may be loaded in dm-crypt either via <key_string> or using
>> classical method and pass the key in hex representation directly.
>>
> 
> I think we need to hexify key description too, because it can contain spaces.
> <key_size> seems like unnecessary complication. Kernel knows key_size, it doesn't need
> that information from userspace.

Userspace (integration in cryptsetup) need to know key size. It reads DM table and parses
information from it. So it is opposite direction - userspace need to get expected key size
from kernel and if key in keyring with logon type, we cannot get key size from kernel.

Kernel obviously have this information directly from keyring.

> Handling different types is probably an overkill too. If it works with logon keys,
> why would we need to use 'user' keys?

Not sure if it is really needed but I do not think it is overkill, there will be more use cases
than you original patch expected.

Milan

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

[Ondrej Kozina]

On 11/17/2016 05:35 PM, Andrey Ryabinin wrote:
> On 11/16/2016 11:47 PM, Ondrej Kozina wrote:
>> (Please still consider it to be RFC only, I need to modify the uspace teststuite
>> again due to changes in key_string format. Also the changes to dm-crypt documentation
>> will follow before final submit. Feature wide I'd consider the patch being complete
>> unless any bugs would emerge)
>>
>> 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_string>] <iv_offset> <dev_path> <start> [<#opt_params> <opt_params>]
>>
>> where <key_string> is in format: <key_size>:<key_type>:<key_description>
>>
>> Currently we only support two elementary key types: 'user' and 'logon'.
>> Keys may be loaded in dm-crypt either via <key_string> or using
>> classical method and pass the key in hex representation directly.
>>
>
> I think we need to hexify key description too, because it can contain spaces.

I see. You're right the kernel key description may really contain 
whitespace chars, bummer. Well what I'm thinking atm is rejecting any 
keys with descriptions containing whitespaces. But let me ask Mike or 
Alasdair what do they think about it.

> <key_size> seems like unnecessary complication. Kernel knows key_size, it doesn't need
> that information from userspace.

Milan already explained that. I just add that general rule for dm tables 
is what you input via load ioctl() you should get back exactly the same 
via table status ioctl(). And also there's no other way how to get 
dm-crypt key size if you input it via kernel keyring key.

> Handling different types is probably an overkill too. If it works with logon keys,
> why would we need to use 'user' keys?

Well your original patch used 'user' type so I assumed you have good 
reason to use it. But anyway it's not so painful to add option to choose 
from 2 different key types imo. Loading tables is not a hot path.

O.

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

[Andrey Ryabinin]

On 11/17/2016 11:06 PM, Ondrej Kozina wrote:
> On 11/17/2016 05:35 PM, Andrey Ryabinin wrote:
>> On 11/16/2016 11:47 PM, Ondrej Kozina wrote:
>>> (Please still consider it to be RFC only, I need to modify the uspace teststuite
>>> again due to changes in key_string format. Also the changes to dm-crypt documentation
>>> will follow before final submit. Feature wide I'd consider the patch being complete
>>> unless any bugs would emerge)
>>>
>>> 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_string>] <iv_offset> <dev_path> <start> [<#opt_params> <opt_params>]
>>>
>>> where <key_string> is in format: <key_size>:<key_type>:<key_description>
>>>
>>> Currently we only support two elementary key types: 'user' and 'logon'.
>>> Keys may be loaded in dm-crypt either via <key_string> or using
>>> classical method and pass the key in hex representation directly.
>>>
>>
>> I think we need to hexify key description too, because it can contain spaces.
> 
> I see. You're right the kernel key description may really contain whitespace chars, bummer. Well what I'm thinking atm is rejecting any keys with descriptions containing whitespaces. But let me ask Mike or Alasdair what do they think about it.
> 
>> <key_size> seems like unnecessary complication. Kernel knows key_size, it doesn't need
>> that information from userspace.
> 
> Milan already explained that. I just add that general rule for dm tables is what you input via load ioctl() you should get back exactly the same via table status ioctl(). And also there's no other way how to get dm-crypt key size if you input it via kernel keyring key.
> 

Yeah, I get that, but Milan said that we need to *get* that information from the kernel.
My concern is about loading key_size into the kernel.
If I understand you right, we need it just for consistency between table_load and table_status ioctls(). I guess it's ok then.

>> Handling different types is probably an overkill too. If it works with logon keys,
>> why would we need to use 'user' keys?
> 
> Well your original patch used 'user' type so I assumed you have good reason to use it.

I used 'user' because I wasn't sure if userspace requires ability to read keys or not.


> But anyway it's not so painful to add option to choose from 2 different key types imo. Loading tables is not a hot path.
> 

Ok, fair enough. 

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

[Ondrej Kozina]

On 11/17/2016 09:06 PM, Ondrej Kozina wrote:
> On 11/17/2016 05:35 PM, Andrey Ryabinin wrote:
>> On 11/16/2016 11:47 PM, Ondrej Kozina wrote:
>>> (Please still consider it to be RFC only, I need to modify the uspace teststuite
>>> again due to changes in key_string format. Also the changes to dm-crypt documentation
>>> will follow before final submit. Feature wide I'd consider the patch being complete
>>> unless any bugs would emerge)
>>>
>>> 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_string>] <iv_offset> <dev_path> <start> [<#opt_params> <opt_params>]
>>>
>>> where <key_string> is in format: <key_size>:<key_type>:<key_description>
>>>
>>> Currently we only support two elementary key types: 'user' and 'logon'.
>>> Keys may be loaded in dm-crypt either via <key_string> or using
>>> classical method and pass the key in hex representation directly.
>>>
>>
>> I think we need to hexify key description too, because it can contain spaces.
>
> I see. You're right the kernel key description may really contain
> whitespace chars, bummer. Well what I'm thinking atm is rejecting any
> keys with descriptions containing whitespaces. But let me ask Mike or
> Alasdair what do they think about it.

Answering myself:

so I looked at it once again in detail and I'm now convinced we actually 
don't have to do anything about it (provided we'd agree on rejecting any 
key_description containing whitespace):

every table is first processed by dm_split_args() before it's passed to 
any target driver for further processing. That's true also for message 
ioctls. In case you pass table (or message) with whitespace in 
key_description it'll fail to construct such dm-crypt target because 
number of arguments passed will not match the dm-crypt template.

key_string in format: ':32:logon:some:user key' is considered to be 2 
arguments and not single one due to the whitespace.

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

[Ondrej Kozina]

Changes since v2:
  - moved rcu_read_lock() closer to key payload processing (thanks Mikulas)
  - updated dm-crypt documentation
  - updated code comments and unified parameter names in kernel keyring function stubs
    (#CONFIG_KEYS undefined)

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_string>] <iv_offset> <dev_path> <start> [<#opt_params> <opt_params>]

where <key_string> is in format: <key_size>:<key_type>:<key_description>

Currently we only support two elementary key types: 'user' and 'logon'.
Keys may be loaded in dm-crypt either via <key_string> or using
classical method and pass the key in hex representation directly.

dm-crypt device initialised with a key passed in hex representation may be
replaced with key passed in key_string format and vice versa.

(Patch is based on original work by Andrey Ryabinin)

Signed-off-by: Ondrej Kozina <okozina@redhat.com>
---
 Documentation/device-mapper/dm-crypt.txt |  25 ++++-
 drivers/md/dm-crypt.c                    | 169 +++++++++++++++++++++++++++----
 2 files changed, 174 insertions(+), 20 deletions(-)

diff --git a/Documentation/device-mapper/dm-crypt.txt b/Documentation/device-mapper/dm-crypt.txt
index 692171f..6f15fce 100644
--- a/Documentation/device-mapper/dm-crypt.txt
+++ b/Documentation/device-mapper/dm-crypt.txt
@@ -21,13 +21,30 @@ Parameters: <cipher> <key> <iv_offset> <device path> \
     /proc/crypto contains supported crypto modes
 
 <key>
-    Key used for encryption. It is encoded as a hexadecimal number.
+    Key used for encryption. It is encoded either as a hexadecimal number
+    or it can be passed as <key_string> prefixed with single colon
+    character (':') for keys residing in kernel keyring service.
     You can only use key sizes that are valid for the selected cipher
     in combination with the selected iv mode.
     Note that for some iv modes the key string can contain additional
     keys (for example IV seed) so the key contains more parts concatenated
     into a single string.
 
+<key_string>
+    The kernel keyring key is identified by string in following format:
+    <key_size>:<key_type>:<key_description>.
+
+<key_size>
+    The encryption key size in bytes. The kernel key payload size must match
+    the value passed in <key_size>.
+
+<key_type>
+    Either 'logon' or 'user' kernel key type.
+
+<key_description>
+    The kernel keyring key description crypt target should look for
+    when loading key of <key_type>.
+
 <keycount>
     Multi-key compatibility mode. You can define <keycount> keys and
     then sectors are encrypted according to their offsets (sector 0 uses key0;
@@ -90,6 +107,12 @@ dmsetup create crypt1 --table "0 `blockdev --getsize $1` crypt aes-cbc-essiv:sha
 
 [[
 #!/bin/sh
+# Create a crypt device using dmsetup when encryption key is stored in keyring service
+dmsetup create crypt2 --table "0 `blockdev --getsize $1` crypt aes-cbc-essiv:sha256 :32:logon:my_prefix:my_key 0 $1 0"
+]]
+
+[[
+#!/bin/sh
 # Create a crypt device using cryptsetup and LUKS header with default cipher
 cryptsetup luksFormat $1
 cryptsetup luksOpen $1 crypt1
diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 0aedd0e..d61edd8 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>
 
@@ -140,6 +142,7 @@ struct crypt_config {
 
 	char *cipher;
 	char *cipher_string;
+	char *key_string;
 
 	struct crypt_iv_operations *iv_gen_ops;
 	union {
@@ -1490,29 +1493,140 @@ static int crypt_setkey_allcpus(struct crypt_config *cc)
 	return err;
 }
 
+#ifdef CONFIG_KEYS
+static int crypt_set_keyring_key(struct crypt_config *cc, const char *key_string)
+{
+	char *new_key_string, *key_desc;
+	int ret;
+	struct key *key;
+	const struct user_key_payload *ukp;
+
+	/* look for next ':' separating key_type from key_description */
+	key_desc = strpbrk(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))
+		return -EINVAL;
+
+	new_key_string = kstrdup(key_string, GFP_KERNEL);
+	if (!new_key_string)
+		return -ENOMEM;
+
+	/*
+	 * FIXME: are there any key descriptions we should disallow users
+	 * from loading to dm-crypt? i.e.: kernel keys starting with '.'
+	 */
+
+	key = request_key(strncmp(key_string, "user", 4) ? &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) {
+		key_put(key);
+		kzfree(new_key_string);
+		return ret;
+	}
+
+	rcu_read_lock();
+
+	ukp = user_key_payload(key);
+	if (cc->key_size != ukp->datalen) {
+		rcu_read_unlock();
+		key_put(key);
+		kzfree(new_key_string);
+		return -EINVAL;
+	}
+
+	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 copy 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_string);
+		cc->key_string = new_key_string;
+	} else
+		kzfree(new_key_string);
+
+	return ret;
+}
+
+static int get_key_size(char **key_string)
+{
+	char *colon, dummy;
+	int ret;
+
+	if (*key_string[0] != ':')
+		return strlen(*key_string) >> 1;
+
+	/* look for next ':' in key string */
+	colon = strpbrk(*key_string + 1, ":");
+	if (!colon)
+		return -EINVAL;
+
+	if (sscanf(*key_string + 1, "%u%c", &ret, &dummy) != 2 || dummy != ':')
+		return -EINVAL;
+
+	*key_string = colon;
+
+	/* remaining key string should be :<logon|user>:<key_desc> */
+
+	return ret;
+}
+#else
+static int crypt_set_keyring_key(struct crypt_config *cc, const char *key_string)
+{
+	return -EINVAL;
+}
+
+static int get_key_size(char **key_string)
+{
+	return (*key_string[0] == ':') ? -EINVAL : strlen(*key_string) >> 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;
 
-	/* clear the flag since following operations may invalidate previously valid key */
-	clear_bit(DM_CRYPT_KEY_VALID, &cc->flags);
+	/* ':' means that the key is in kernel keyring */
+	if (key[0] == ':')
+		r = crypt_set_keyring_key(cc, key + 1);
+	else {
+		/* clear the flag since following operations may invalidate previously valid key */
+		clear_bit(DM_CRYPT_KEY_VALID, &cc->flags);
 
-	if (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_string);
+		cc->key_string = 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);
@@ -1524,6 +1638,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_string);
+	cc->key_string = NULL;
 
 	return crypt_setkey_allcpus(cc);
 }
@@ -1561,6 +1677,7 @@ static void crypt_dtr(struct dm_target *ti)
 
 	kzfree(cc->cipher);
 	kzfree(cc->cipher_string);
+	kzfree(cc->key_string);
 
 	/* Must zero key material before freeing */
 	kzfree(cc);
@@ -1729,12 +1846,13 @@ static int crypt_ctr_cipher(struct dm_target *ti,
 
 /*
  * Construct an encryption mapping:
- * <cipher> <key> <iv_offset> <dev_path> <start>
+ * <cipher> [<key>|:<key_size>:<user|logon>:<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;
@@ -1751,7 +1869,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 size";
+		return -EINVAL;
+	}
 
 	cc = kzalloc(sizeof(*cc) + key_size * sizeof(u8), GFP_KERNEL);
 	if (!cc) {
@@ -1958,10 +2080,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_string)
+				DMEMIT(":%u:%s", cc->key_size, cc->key_string);
+			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,
@@ -2028,6 +2153,12 @@ static int crypt_message(struct dm_target *ti, unsigned argc, char **argv)
 			return -EINVAL;
 		}
 		if (argc == 3 && !strcasecmp(argv[1], "set")) {
+			/* The key size may not be changed. */
+			if (cc->key_size != get_key_size(&argv[2])) {
+				memset(argv[2], '0', strlen(argv[2]));
+				return -EINVAL;
+			}
+
 			ret = crypt_set_key(cc, argv[2]);
 			if (ret)
 				return ret;
@@ -2071,7 +2202,7 @@ static void crypt_io_hints(struct dm_target *ti, struct queue_limits *limits)
 
 static struct target_type crypt_target = {
 	.name   = "crypt",
-	.version = {1, 14, 1},
+	.version = {1, 15, 0},
 	.module = THIS_MODULE,
 	.ctr    = crypt_ctr,
 	.dtr    = crypt_dtr,
-- 
2.7.4

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

[Mike Snitzer]

On Mon, Nov 21 2016 at  9:58P -0500,
Ondrej Kozina <okozina@redhat.com> wrote:

> Changes since v2:
>   - moved rcu_read_lock() closer to key payload processing (thanks Mikulas)
>   - updated dm-crypt documentation
>   - updated code comments and unified parameter names in kernel keyring function stubs
>     (#CONFIG_KEYS undefined)
> 
> 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_string>] <iv_offset> <dev_path> <start> [<#opt_params> <opt_params>]
> 
> where <key_string> is in format: <key_size>:<key_type>:<key_description>
> 
> Currently we only support two elementary key types: 'user' and 'logon'.
> Keys may be loaded in dm-crypt either via <key_string> or using
> classical method and pass the key in hex representation directly.
> 
> dm-crypt device initialised with a key passed in hex representation may be
> replaced with key passed in key_string format and vice versa.
> 
> (Patch is based on original work by Andrey Ryabinin)
> 
> Signed-off-by: Ondrej Kozina <okozina@redhat.com>

Other than the following cleanup patch that I've put ontop of your v3,
looks good to me.

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 7c2bbef..8fcc924 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -1508,12 +1508,8 @@ static int crypt_set_keyring_key(struct crypt_config *cc, const char *key_string
 	if (!new_key_string)
 		return -ENOMEM;
 
-	/*
-	 * FIXME: are there any key descriptions we should disallow users
-	 * from loading to dm-crypt? i.e.: kernel keys starting with '.'
-	 */
-
-	key = request_key(strncmp(key_string, "user", 4) ? &key_type_logon : &key_type_user, key_desc + 1, NULL);
+	key = request_key(strncmp(key_string, "user", 4) ? &key_type_logon : &key_type_user,
+			  key_desc + 1, NULL);
 	if (IS_ERR(key)) {
 		kzfree(new_key_string);
 		return PTR_ERR(key);
@@ -1602,25 +1598,25 @@ static int crypt_set_key(struct crypt_config *cc, char *key)
 	if (!cc->key_size && strcmp(key, "-"))
 		goto out;
 
-	/* ':' means that the key is in kernel keyring */
-	if (key[0] == ':')
+	/* ':' means the key is in kernel keyring, short-circuit normal key processing */
+	if (key[0] == ':') {
 		r = crypt_set_keyring_key(cc, key + 1);
-	else {
-		/* clear the flag since following operations may invalidate previously valid key */
-		clear_bit(DM_CRYPT_KEY_VALID, &cc->flags);
+		goto out;
+	}
 
-		/* wipe references to any kernel keyring key */
-		kzfree(cc->key_string);
-		cc->key_string = NULL;
+	/* clear the flag since following operations may invalidate previously valid key */
+	clear_bit(DM_CRYPT_KEY_VALID, &cc->flags);
 
-		if (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_string);
+	cc->key_string = NULL;
 
-		r = crypt_setkey(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(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);

[PATCH] dm-crypt: check key payload pointer not null

[Ondrej Kozina]

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(-)

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);
-- 
2.7.4

Re: [PATCH] dm-crypt: check key payload pointer not null

[David Howells]

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>

[PATCH] dm-crypt: reject key strings containing whitespace chars

[Ondrej Kozina]

Well, I was wrong. Unfortunately key_string may theoreticaly contain
whitespace even after it's processed by dm_split_args(). The dm core
driver supports escaping of almost all chars including any whitespace.

if uspace passes key in the kernel in format ":32:logon:my_prefix:my\ key"
the dm-crypt will look up key "my_prefix:my key" in kernel keyring
service. So far everything's fine.

Unfortunately if uspace later calls DM_TABLE_STATUS ioctl, it will not
receive back expected ":32:logon:my_prefix:my\ key" but the unescaped version
instead. Also uspace (most notably cryptsetup) is not ready to parse
single target argument containing (even escaped) whitespace chars and any
whitespace is simply taken as delimiter of another argument.

This efect is mitigated by the fact libdevmapper curently performs
double escaping of '\' char. Any user input in format "x\ x" is transformed
into "x\\ x" before being passed to the kernel. Nonetheless dm-crypt may be
used without go-between libdevmapper. Therefore I propose following patch
to reject any key string containing whitespace.

---
 drivers/md/dm-crypt.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 78ab5e8..11df7a9 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -24,6 +24,7 @@
 #include <linux/atomic.h>
 #include <linux/scatterlist.h>
 #include <linux/rbtree.h>
+#include <linux/ctype.h>
 #include <asm/page.h>
 #include <asm/unaligned.h>
 #include <crypto/hash.h>
@@ -1489,6 +1490,14 @@ static int crypt_setkey(struct crypt_config *cc)
 
 #ifdef CONFIG_KEYS
 
+static int contains_whitespace(const char *str)
+{
+	while (*str)
+		if (isspace(*str++))
+			return 1;
+	return 0;
+}
+
 static int crypt_set_keyring_key(struct crypt_config *cc, const char *key_string)
 {
 	char *new_key_string, *key_desc;
@@ -1496,6 +1505,15 @@ static int crypt_set_keyring_key(struct crypt_config *cc, const char *key_string
 	struct key *key;
 	const struct user_key_payload *ukp;
 
+	/*
+	 * Reject key_string with whitespace. dm core currently lacks code for
+	 * proper whitespace escaping in arguments on DM_TABLE_STATUS path.
+	 */
+	if (contains_whitespace(key_string)) {
+		DMERR("whitespace chars currently not allowed in key string");
+		return -EINVAL;
+	}
+
 	/* look for next ':' separating key_type from key_description */
 	key_desc = strpbrk(key_string, ":");
 	if (!key_desc || key_desc == key_string || !strlen(key_desc + 1))
-- 
2.7.4