* [RFC] dm-crypt: add ability to use keys from the kernel key retention service @ 2016-08-09 13:56 ` Andrey Ryabinin 0 siblings, 0 replies; 22+ messages in thread From: Andrey Ryabinin @ 2016-08-09 13:56 UTC (permalink / raw) To: Alasdair Kergon, Mike Snitzer, dm-devel Cc: Mikulas Patocka, Milan Broz, Igor Sukhih, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [RFC] dm-crypt: add ability to use keys from the kernel key retention service @ 2016-08-09 13:56 ` Andrey Ryabinin 0 siblings, 0 replies; 22+ messages in thread From: Andrey Ryabinin @ 2016-08-09 13:56 UTC (permalink / raw) To: Alasdair Kergon, Mike Snitzer, dm-devel Cc: Mikulas Patocka, Milan Broz, Igor Sukhih, linux-kernel, 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [dm-devel] [RFC] dm-crypt: add ability to use keys from the kernel key retention service 2016-08-09 13:56 ` Andrey Ryabinin @ 2016-08-10 11:16 ` Ondrej Kozina -1 siblings, 0 replies; 22+ messages in thread From: Ondrej Kozina @ 2016-08-10 11:16 UTC (permalink / raw) To: Andrey Ryabinin, Alasdair Kergon, Mike Snitzer, dm-devel Cc: Igor, Sukhih, linux-kernel, Mikulas Patocka, Milan Broz 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC] dm-crypt: add ability to use keys from the kernel key retention service @ 2016-08-10 11:16 ` Ondrej Kozina 0 siblings, 0 replies; 22+ messages in thread From: Ondrej Kozina @ 2016-08-10 11:16 UTC (permalink / raw) To: Andrey Ryabinin, Alasdair Kergon, Mike Snitzer, dm-devel Cc: Mikulas Patocka, Igor, Sukhih, linux-kernel, Milan Broz 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [dm-devel] [RFC] dm-crypt: add ability to use keys from the kernel key retention service 2016-08-10 11:16 ` Ondrej Kozina @ 2016-08-11 15:01 ` Andrey Ryabinin -1 siblings, 0 replies; 22+ messages in thread From: Andrey Ryabinin @ 2016-08-11 15:01 UTC (permalink / raw) To: Ondrej Kozina, Alasdair Kergon, Mike Snitzer, dm-devel Cc: Igor, Sukhih, linux-kernel, Mikulas Patocka, Milan Broz 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [dm-devel] [RFC] dm-crypt: add ability to use keys from the kernel key retention service @ 2016-08-11 15:01 ` Andrey Ryabinin 0 siblings, 0 replies; 22+ messages in thread From: Andrey Ryabinin @ 2016-08-11 15:01 UTC (permalink / raw) To: Ondrej Kozina, Alasdair Kergon, Mike Snitzer, dm-devel Cc: Igor, Sukhih, linux-kernel, Mikulas Patocka, Milan Broz 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 0/3] Modified kernel keyring support patch 2016-08-09 13:56 ` Andrey Ryabinin (?) (?) @ 2016-11-07 9:38 ` Ondrej Kozina -1 siblings, 0 replies; 22+ messages in thread From: Ondrej Kozina @ 2016-11-07 9:38 UTC (permalink / raw) To: dm-devel; +Cc: Ondrej Kozina, aryabinin, mpatocka, snitzer, mbroz 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/3] dm-crypt: mark key as invalid until properly loaded 2016-08-09 13:56 ` Andrey Ryabinin ` (2 preceding siblings ...) (?) @ 2016-11-07 9:38 ` Ondrej Kozina -1 siblings, 0 replies; 22+ messages in thread From: Ondrej Kozina @ 2016-11-07 9:38 UTC (permalink / raw) To: dm-devel; +Cc: Ondrej Kozina, aryabinin, mpatocka, snitzer, mbroz (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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 2/3] dm-crypt: add ability to use keys from the kernel key retention service 2016-08-09 13:56 ` Andrey Ryabinin ` (3 preceding siblings ...) (?) @ 2016-11-07 9:38 ` Ondrej Kozina -1 siblings, 0 replies; 22+ messages in thread From: Ondrej Kozina @ 2016-11-07 9:38 UTC (permalink / raw) To: dm-devel; +Cc: aryabinin, mpatocka, snitzer, mbroz 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 3/3] dm-crypt: modifications to previous patch 2016-08-09 13:56 ` Andrey Ryabinin ` (4 preceding siblings ...) (?) @ 2016-11-07 9:38 ` Ondrej Kozina 2016-11-13 17:22 ` Milan Broz -1 siblings, 1 reply; 22+ messages in thread From: Ondrej Kozina @ 2016-11-07 9:38 UTC (permalink / raw) To: dm-devel; +Cc: Ondrej Kozina, aryabinin, mpatocka, snitzer, mbroz 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 3/3] dm-crypt: modifications to previous patch 2016-11-07 9:38 ` [PATCH 3/3] dm-crypt: modifications to previous patch Ondrej Kozina @ 2016-11-13 17:22 ` Milan Broz 2016-11-16 20:47 ` [PATCH v2] dm-crypt: add ability to use keys from the kernel key retention service Ondrej Kozina 0 siblings, 1 reply; 22+ messages in thread From: Milan Broz @ 2016-11-13 17:22 UTC (permalink / raw) To: Ondrej Kozina, dm-devel; +Cc: aryabinin, mpatocka, snitzer 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, > ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2] dm-crypt: add ability to use keys from the kernel key retention service 2016-11-13 17:22 ` Milan Broz @ 2016-11-16 20:47 ` Ondrej Kozina 2016-11-17 16:35 ` Andrey Ryabinin 2016-11-21 14:58 ` [PATCH v3] dm-crypt: add ability to use keys from the kernel key retention service Ondrej Kozina 0 siblings, 2 replies; 22+ messages in thread From: Ondrej Kozina @ 2016-11-16 20:47 UTC (permalink / raw) To: dm-devel; +Cc: Ondrej Kozina, aryabinin, mpatocka, snitzer, mbroz (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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v2] dm-crypt: add ability to use keys from the kernel key retention service 2016-11-16 20:47 ` [PATCH v2] dm-crypt: add ability to use keys from the kernel key retention service Ondrej Kozina @ 2016-11-17 16:35 ` Andrey Ryabinin 2016-11-17 19:31 ` Milan Broz 2016-11-17 20:06 ` Ondrej Kozina 2016-11-21 14:58 ` [PATCH v3] dm-crypt: add ability to use keys from the kernel key retention service Ondrej Kozina 1 sibling, 2 replies; 22+ messages in thread From: Andrey Ryabinin @ 2016-11-17 16:35 UTC (permalink / raw) To: Ondrej Kozina, dm-devel; +Cc: mpatocka, snitzer, mbroz 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? ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] dm-crypt: add ability to use keys from the kernel key retention service 2016-11-17 16:35 ` Andrey Ryabinin @ 2016-11-17 19:31 ` Milan Broz 2016-11-17 20:06 ` Ondrej Kozina 1 sibling, 0 replies; 22+ messages in thread From: Milan Broz @ 2016-11-17 19:31 UTC (permalink / raw) To: Andrey Ryabinin, Ondrej Kozina, dm-devel; +Cc: mpatocka, snitzer 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 ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] dm-crypt: add ability to use keys from the kernel key retention service 2016-11-17 16:35 ` Andrey Ryabinin 2016-11-17 19:31 ` Milan Broz @ 2016-11-17 20:06 ` Ondrej Kozina 2016-11-18 16:55 ` Andrey Ryabinin 2016-11-21 12:23 ` Ondrej Kozina 1 sibling, 2 replies; 22+ messages in thread From: Ondrej Kozina @ 2016-11-17 20:06 UTC (permalink / raw) To: Andrey Ryabinin, dm-devel; +Cc: mpatocka, agk, snitzer, mbroz 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] dm-crypt: add ability to use keys from the kernel key retention service 2016-11-17 20:06 ` Ondrej Kozina @ 2016-11-18 16:55 ` Andrey Ryabinin 2016-11-21 12:23 ` Ondrej Kozina 1 sibling, 0 replies; 22+ messages in thread From: Andrey Ryabinin @ 2016-11-18 16:55 UTC (permalink / raw) To: Ondrej Kozina, dm-devel; +Cc: mpatocka, agk, snitzer, mbroz 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2] dm-crypt: add ability to use keys from the kernel key retention service 2016-11-17 20:06 ` Ondrej Kozina 2016-11-18 16:55 ` Andrey Ryabinin @ 2016-11-21 12:23 ` Ondrej Kozina 2016-12-01 17:20 ` [PATCH] dm-crypt: reject key strings containing whitespace chars Ondrej Kozina 1 sibling, 1 reply; 22+ messages in thread From: Ondrej Kozina @ 2016-11-21 12:23 UTC (permalink / raw) To: Andrey Ryabinin, dm-devel; +Cc: mpatocka, snitzer, agk, mbroz 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. ^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH] dm-crypt: reject key strings containing whitespace chars 2016-11-21 12:23 ` Ondrej Kozina @ 2016-12-01 17:20 ` Ondrej Kozina 0 siblings, 0 replies; 22+ messages in thread From: Ondrej Kozina @ 2016-12-01 17:20 UTC (permalink / raw) To: dm-devel; +Cc: snitzer, mpatocka, Ondrej Kozina, aryabinin, agk, mbroz 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v3] dm-crypt: add ability to use keys from the kernel key retention service 2016-11-16 20:47 ` [PATCH v2] dm-crypt: add ability to use keys from the kernel key retention service Ondrej Kozina 2016-11-17 16:35 ` Andrey Ryabinin @ 2016-11-21 14:58 ` Ondrej Kozina 2016-11-21 15:40 ` Mike Snitzer 1 sibling, 1 reply; 22+ messages in thread From: Ondrej Kozina @ 2016-11-21 14:58 UTC (permalink / raw) To: dm-devel; +Cc: Ondrej Kozina, aryabinin, mpatocka, snitzer, mbroz 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v3] dm-crypt: add ability to use keys from the kernel key retention service 2016-11-21 14:58 ` [PATCH v3] dm-crypt: add ability to use keys from the kernel key retention service Ondrej Kozina @ 2016-11-21 15:40 ` Mike Snitzer 2016-11-23 20:51 ` [PATCH] dm-crypt: check key payload pointer not null Ondrej Kozina 2016-11-24 9:28 ` David Howells 0 siblings, 2 replies; 22+ messages in thread From: Mike Snitzer @ 2016-11-21 15:40 UTC (permalink / raw) To: Ondrej Kozina; +Cc: aryabinin, dm-devel, mpatocka, mbroz 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); ^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH] dm-crypt: check key payload pointer not null 2016-11-21 15:40 ` Mike Snitzer @ 2016-11-23 20:51 ` Ondrej Kozina 2016-11-24 9:28 ` David Howells 1 sibling, 0 replies; 22+ messages in thread From: Ondrej Kozina @ 2016-11-23 20:51 UTC (permalink / raw) To: dm-devel; +Cc: dhowells, aryabinin, mpatocka, snitzer, mbroz 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 ^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH] dm-crypt: check key payload pointer not null 2016-11-21 15:40 ` Mike Snitzer 2016-11-23 20:51 ` [PATCH] dm-crypt: check key payload pointer not null Ondrej Kozina @ 2016-11-24 9:28 ` David Howells 1 sibling, 0 replies; 22+ messages in thread From: David Howells @ 2016-11-24 9:28 UTC (permalink / raw) To: Ondrej Kozina; +Cc: snitzer, dhowells, dm-devel, mpatocka, aryabinin, mbroz 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> ^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2016-12-01 17:20 UTC | newest] Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-08-09 13:56 [RFC] dm-crypt: add ability to use keys from the kernel key retention service Andrey Ryabinin 2016-08-09 13:56 ` Andrey Ryabinin 2016-08-10 11:16 ` [dm-devel] " Ondrej Kozina 2016-08-10 11:16 ` Ondrej Kozina 2016-08-11 15:01 ` [dm-devel] " Andrey Ryabinin 2016-08-11 15:01 ` Andrey Ryabinin 2016-11-07 9:38 ` [PATCH 0/3] Modified kernel keyring support patch Ondrej Kozina 2016-11-07 9:38 ` [PATCH 1/3] dm-crypt: mark key as invalid until properly loaded Ondrej Kozina 2016-11-07 9:38 ` [PATCH 2/3] dm-crypt: add ability to use keys from the kernel key retention service Ondrej Kozina 2016-11-07 9:38 ` [PATCH 3/3] dm-crypt: modifications to previous patch Ondrej Kozina 2016-11-13 17:22 ` Milan Broz 2016-11-16 20:47 ` [PATCH v2] dm-crypt: add ability to use keys from the kernel key retention service Ondrej Kozina 2016-11-17 16:35 ` Andrey Ryabinin 2016-11-17 19:31 ` Milan Broz 2016-11-17 20:06 ` Ondrej Kozina 2016-11-18 16:55 ` Andrey Ryabinin 2016-11-21 12:23 ` Ondrej Kozina 2016-12-01 17:20 ` [PATCH] dm-crypt: reject key strings containing whitespace chars Ondrej Kozina 2016-11-21 14:58 ` [PATCH v3] dm-crypt: add ability to use keys from the kernel key retention service Ondrej Kozina 2016-11-21 15:40 ` Mike Snitzer 2016-11-23 20:51 ` [PATCH] dm-crypt: check key payload pointer not null Ondrej Kozina 2016-11-24 9:28 ` David Howells
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.