| Message ID | 154180163107.70506.6894134402057805078.stgit@djiang5-desk3.ch.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Additional patches for nvdimm security support | expand |
On Fri, Nov 9, 2018 at 2:13 PM Dave Jiang <dave.jiang@intel.com> wrote: > > Adding nvdimm key format type to encrypted keys in order to limit the size s/Adding/Add an/ > of the key to 32-bytes. > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > --- > security/keys/encrypted-keys/encrypted.c | 29 ++++++++++++++++++++--------- > 1 file changed, 20 insertions(+), 9 deletions(-) > > diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c > index d92cbf9687c3..182b4f136bdf 100644 > --- a/security/keys/encrypted-keys/encrypted.c > +++ b/security/keys/encrypted-keys/encrypted.c > @@ -45,6 +45,7 @@ static const char hmac_alg[] = "hmac(sha256)"; > static const char blkcipher_alg[] = "cbc(aes)"; > static const char key_format_default[] = "default"; > static const char key_format_ecryptfs[] = "ecryptfs"; > +static const char key_format_nvdimm[] = "nvdimm"; > static unsigned int ivsize; > static int blksize; > > @@ -54,6 +55,7 @@ static int blksize; > #define HASH_SIZE SHA256_DIGEST_SIZE > #define MAX_DATA_SIZE 4096 > #define MIN_DATA_SIZE 20 > +#define KEY_NVDIMM_PAYLOAD_LEN 32 > > static struct crypto_shash *hash_tfm; > > @@ -62,12 +64,13 @@ enum { > }; > > enum { > - Opt_error = -1, Opt_default, Opt_ecryptfs > + Opt_error = -1, Opt_default, Opt_ecryptfs, Opt_nvdimm > }; > > static const match_table_t key_format_tokens = { > {Opt_default, "default"}, > {Opt_ecryptfs, "ecryptfs"}, > + {Opt_nvdimm, "nvdimm"}, > {Opt_error, NULL} > }; > > @@ -195,6 +198,7 @@ static int datablob_parse(char *datablob, const char **format, > key_format = match_token(p, key_format_tokens, args); > switch (key_format) { > case Opt_ecryptfs: > + case Opt_nvdimm: > case Opt_default: > *format = p; > *master_desc = strsep(&datablob, " \t"); > @@ -625,15 +629,22 @@ static struct encrypted_key_payload *encrypted_key_alloc(struct key *key, > format_len = (!format) ? strlen(key_format_default) : strlen(format); > decrypted_datalen = dlen; > payload_datalen = decrypted_datalen; > - if (format && !strcmp(format, key_format_ecryptfs)) { > - if (dlen != ECRYPTFS_MAX_KEY_BYTES) { > - pr_err("encrypted_key: keylen for the ecryptfs format " > - "must be equal to %d bytes\n", > - ECRYPTFS_MAX_KEY_BYTES); > - return ERR_PTR(-EINVAL); > + if (format) { > + if (!strcmp(format, key_format_ecryptfs)) { > + if (dlen != ECRYPTFS_MAX_KEY_BYTES) { > + pr_err("encrypted_key: keylen for the ecryptfs format must be equal to %d bytes\n", > + ECRYPTFS_MAX_KEY_BYTES); > + return ERR_PTR(-EINVAL); > + } > + decrypted_datalen = ECRYPTFS_MAX_KEY_BYTES; > + payload_datalen = sizeof(struct ecryptfs_auth_tok); > + } else if (!strcmp(format, key_format_nvdimm)) { > + if (decrypted_datalen != KEY_NVDIMM_PAYLOAD_LEN) { > + pr_err("encrypted_key: nvdimm key payload incorrect length: %d\n", > + decrypted_datalen); > + return ERR_PTR(-EINVAL); > + } I suspect this may not be the last key type that gets added, but I wonder if we should instead create key-types based on the dlen size. I.e. create a generic 32-byte key-type "enc32"? That way if another 32-byte requirement key comes along we don't need to come touch this routine again.
On 11/27/18 12:20 AM, Dan Williams wrote: > On Fri, Nov 9, 2018 at 2:13 PM Dave Jiang <dave.jiang@intel.com> wrote: >> >> Adding nvdimm key format type to encrypted keys in order to limit the size > > s/Adding/Add an/ > >> of the key to 32-bytes. >> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> >> --- >> security/keys/encrypted-keys/encrypted.c | 29 ++++++++++++++++++++--------- >> 1 file changed, 20 insertions(+), 9 deletions(-) >> >> diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c >> index d92cbf9687c3..182b4f136bdf 100644 >> --- a/security/keys/encrypted-keys/encrypted.c >> +++ b/security/keys/encrypted-keys/encrypted.c >> @@ -45,6 +45,7 @@ static const char hmac_alg[] = "hmac(sha256)"; >> static const char blkcipher_alg[] = "cbc(aes)"; >> static const char key_format_default[] = "default"; >> static const char key_format_ecryptfs[] = "ecryptfs"; >> +static const char key_format_nvdimm[] = "nvdimm"; >> static unsigned int ivsize; >> static int blksize; >> >> @@ -54,6 +55,7 @@ static int blksize; >> #define HASH_SIZE SHA256_DIGEST_SIZE >> #define MAX_DATA_SIZE 4096 >> #define MIN_DATA_SIZE 20 >> +#define KEY_NVDIMM_PAYLOAD_LEN 32 >> >> static struct crypto_shash *hash_tfm; >> >> @@ -62,12 +64,13 @@ enum { >> }; >> >> enum { >> - Opt_error = -1, Opt_default, Opt_ecryptfs >> + Opt_error = -1, Opt_default, Opt_ecryptfs, Opt_nvdimm >> }; >> >> static const match_table_t key_format_tokens = { >> {Opt_default, "default"}, >> {Opt_ecryptfs, "ecryptfs"}, >> + {Opt_nvdimm, "nvdimm"}, >> {Opt_error, NULL} >> }; >> >> @@ -195,6 +198,7 @@ static int datablob_parse(char *datablob, const char **format, >> key_format = match_token(p, key_format_tokens, args); >> switch (key_format) { >> case Opt_ecryptfs: >> + case Opt_nvdimm: >> case Opt_default: >> *format = p; >> *master_desc = strsep(&datablob, " \t"); >> @@ -625,15 +629,22 @@ static struct encrypted_key_payload *encrypted_key_alloc(struct key *key, >> format_len = (!format) ? strlen(key_format_default) : strlen(format); >> decrypted_datalen = dlen; >> payload_datalen = decrypted_datalen; >> - if (format && !strcmp(format, key_format_ecryptfs)) { >> - if (dlen != ECRYPTFS_MAX_KEY_BYTES) { >> - pr_err("encrypted_key: keylen for the ecryptfs format " >> - "must be equal to %d bytes\n", >> - ECRYPTFS_MAX_KEY_BYTES); >> - return ERR_PTR(-EINVAL); >> + if (format) { >> + if (!strcmp(format, key_format_ecryptfs)) { >> + if (dlen != ECRYPTFS_MAX_KEY_BYTES) { >> + pr_err("encrypted_key: keylen for the ecryptfs format must be equal to %d bytes\n", >> + ECRYPTFS_MAX_KEY_BYTES); >> + return ERR_PTR(-EINVAL); >> + } >> + decrypted_datalen = ECRYPTFS_MAX_KEY_BYTES; >> + payload_datalen = sizeof(struct ecryptfs_auth_tok); >> + } else if (!strcmp(format, key_format_nvdimm)) { >> + if (decrypted_datalen != KEY_NVDIMM_PAYLOAD_LEN) { >> + pr_err("encrypted_key: nvdimm key payload incorrect length: %d\n", >> + decrypted_datalen); >> + return ERR_PTR(-EINVAL); >> + } > > I suspect this may not be the last key type that gets added, but I > wonder if we should instead create key-types based on the dlen size. > I.e. create a generic 32-byte key-type "enc32"? That way if another > 32-byte requirement key comes along we don't need to come touch this > routine again. > I'm ok with that if Mimi is.
On Tue, 2018-11-27 at 09:20 -0700, Dave Jiang wrote: > > On 11/27/18 12:20 AM, Dan Williams wrote: > > On Fri, Nov 9, 2018 at 2:13 PM Dave Jiang <dave.jiang@intel.com> wrote: > >> > >> Adding nvdimm key format type to encrypted keys in order to limit the size > > > > s/Adding/Add an/ > > > >> of the key to 32-bytes. > >> > >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> > >> --- > >> security/keys/encrypted-keys/encrypted.c | 29 ++++++++++++++++++++--------- > >> 1 file changed, 20 insertions(+), 9 deletions(-) > >> > >> diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c > >> index d92cbf9687c3..182b4f136bdf 100644 > >> --- a/security/keys/encrypted-keys/encrypted.c > >> +++ b/security/keys/encrypted-keys/encrypted.c > >> @@ -45,6 +45,7 @@ static const char hmac_alg[] = "hmac(sha256)"; > >> static const char blkcipher_alg[] = "cbc(aes)"; > >> static const char key_format_default[] = "default"; > >> static const char key_format_ecryptfs[] = "ecryptfs"; > >> +static const char key_format_nvdimm[] = "nvdimm"; > >> static unsigned int ivsize; > >> static int blksize; > >> > >> @@ -54,6 +55,7 @@ static int blksize; > >> #define HASH_SIZE SHA256_DIGEST_SIZE > >> #define MAX_DATA_SIZE 4096 > >> #define MIN_DATA_SIZE 20 > >> +#define KEY_NVDIMM_PAYLOAD_LEN 32 > >> > >> static struct crypto_shash *hash_tfm; > >> > >> @@ -62,12 +64,13 @@ enum { > >> }; > >> > >> enum { > >> - Opt_error = -1, Opt_default, Opt_ecryptfs > >> + Opt_error = -1, Opt_default, Opt_ecryptfs, Opt_nvdimm > >> }; > >> > >> static const match_table_t key_format_tokens = { > >> {Opt_default, "default"}, > >> {Opt_ecryptfs, "ecryptfs"}, > >> + {Opt_nvdimm, "nvdimm"}, > >> {Opt_error, NULL} > >> }; > >> > >> @@ -195,6 +198,7 @@ static int datablob_parse(char *datablob, const char **format, > >> key_format = match_token(p, key_format_tokens, args); > >> switch (key_format) { > >> case Opt_ecryptfs: > >> + case Opt_nvdimm: > >> case Opt_default: > >> *format = p; > >> *master_desc = strsep(&datablob, " \t"); > >> @@ -625,15 +629,22 @@ static struct encrypted_key_payload *encrypted_key_alloc(struct key *key, > >> format_len = (!format) ? strlen(key_format_default) : strlen(format); > >> decrypted_datalen = dlen; > >> payload_datalen = decrypted_datalen; > >> - if (format && !strcmp(format, key_format_ecryptfs)) { > >> - if (dlen != ECRYPTFS_MAX_KEY_BYTES) { > >> - pr_err("encrypted_key: keylen for the ecryptfs format " > >> - "must be equal to %d bytes\n", > >> - ECRYPTFS_MAX_KEY_BYTES); > >> - return ERR_PTR(-EINVAL); > >> + if (format) { > >> + if (!strcmp(format, key_format_ecryptfs)) { > >> + if (dlen != ECRYPTFS_MAX_KEY_BYTES) { > >> + pr_err("encrypted_key: keylen for the ecryptfs format must be equal to %d bytes\n", > >> + ECRYPTFS_MAX_KEY_BYTES); > >> + return ERR_PTR(-EINVAL); > >> + } > >> + decrypted_datalen = ECRYPTFS_MAX_KEY_BYTES; > >> + payload_datalen = sizeof(struct ecryptfs_auth_tok); > >> + } else if (!strcmp(format, key_format_nvdimm)) { > >> + if (decrypted_datalen != KEY_NVDIMM_PAYLOAD_LEN) { > >> + pr_err("encrypted_key: nvdimm key payload incorrect length: %d\n", > >> + decrypted_datalen); > >> + return ERR_PTR(-EINVAL); > >> + } > > > > I suspect this may not be the last key type that gets added, but I > > wonder if we should instead create key-types based on the dlen size. > > I.e. create a generic 32-byte key-type "enc32"? That way if another > > 32-byte requirement key comes along we don't need to come touch this > > routine again. > > > > I'm ok with that if Mimi is. If the usage (eg. format: ecryptfs, nvdimm) limits the dlen size(s), how will defining generic key-types help? If there are no usage size limitations, then there would be no usage specific definition. I must be missing something. Mimi
On Tue, Nov 27, 2018 at 10:24 AM Mimi Zohar <zohar@linux.ibm.com> wrote: > > On Tue, 2018-11-27 at 09:20 -0700, Dave Jiang wrote: > > > > On 11/27/18 12:20 AM, Dan Williams wrote: > > > On Fri, Nov 9, 2018 at 2:13 PM Dave Jiang <dave.jiang@intel.com> wrote: > > >> > > >> Adding nvdimm key format type to encrypted keys in order to limit the size > > > > > > s/Adding/Add an/ > > > > > >> of the key to 32-bytes. > > >> > > >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> > > >> --- > > >> security/keys/encrypted-keys/encrypted.c | 29 ++++++++++++++++++++--------- > > >> 1 file changed, 20 insertions(+), 9 deletions(-) > > >> > > >> diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c > > >> index d92cbf9687c3..182b4f136bdf 100644 > > >> --- a/security/keys/encrypted-keys/encrypted.c > > >> +++ b/security/keys/encrypted-keys/encrypted.c > > >> @@ -45,6 +45,7 @@ static const char hmac_alg[] = "hmac(sha256)"; > > >> static const char blkcipher_alg[] = "cbc(aes)"; > > >> static const char key_format_default[] = "default"; > > >> static const char key_format_ecryptfs[] = "ecryptfs"; > > >> +static const char key_format_nvdimm[] = "nvdimm"; > > >> static unsigned int ivsize; > > >> static int blksize; > > >> > > >> @@ -54,6 +55,7 @@ static int blksize; > > >> #define HASH_SIZE SHA256_DIGEST_SIZE > > >> #define MAX_DATA_SIZE 4096 > > >> #define MIN_DATA_SIZE 20 > > >> +#define KEY_NVDIMM_PAYLOAD_LEN 32 > > >> > > >> static struct crypto_shash *hash_tfm; > > >> > > >> @@ -62,12 +64,13 @@ enum { > > >> }; > > >> > > >> enum { > > >> - Opt_error = -1, Opt_default, Opt_ecryptfs > > >> + Opt_error = -1, Opt_default, Opt_ecryptfs, Opt_nvdimm > > >> }; > > >> > > >> static const match_table_t key_format_tokens = { > > >> {Opt_default, "default"}, > > >> {Opt_ecryptfs, "ecryptfs"}, > > >> + {Opt_nvdimm, "nvdimm"}, > > >> {Opt_error, NULL} > > >> }; > > >> > > >> @@ -195,6 +198,7 @@ static int datablob_parse(char *datablob, const char **format, > > >> key_format = match_token(p, key_format_tokens, args); > > >> switch (key_format) { > > >> case Opt_ecryptfs: > > >> + case Opt_nvdimm: > > >> case Opt_default: > > >> *format = p; > > >> *master_desc = strsep(&datablob, " \t"); > > >> @@ -625,15 +629,22 @@ static struct encrypted_key_payload *encrypted_key_alloc(struct key *key, > > >> format_len = (!format) ? strlen(key_format_default) : strlen(format); > > >> decrypted_datalen = dlen; > > >> payload_datalen = decrypted_datalen; > > >> - if (format && !strcmp(format, key_format_ecryptfs)) { > > >> - if (dlen != ECRYPTFS_MAX_KEY_BYTES) { > > >> - pr_err("encrypted_key: keylen for the ecryptfs format " > > >> - "must be equal to %d bytes\n", > > >> - ECRYPTFS_MAX_KEY_BYTES); > > >> - return ERR_PTR(-EINVAL); > > >> + if (format) { > > >> + if (!strcmp(format, key_format_ecryptfs)) { > > >> + if (dlen != ECRYPTFS_MAX_KEY_BYTES) { > > >> + pr_err("encrypted_key: keylen for the ecryptfs format must be equal to %d bytes\n", > > >> + ECRYPTFS_MAX_KEY_BYTES); > > >> + return ERR_PTR(-EINVAL); > > >> + } > > >> + decrypted_datalen = ECRYPTFS_MAX_KEY_BYTES; > > >> + payload_datalen = sizeof(struct ecryptfs_auth_tok); > > >> + } else if (!strcmp(format, key_format_nvdimm)) { > > >> + if (decrypted_datalen != KEY_NVDIMM_PAYLOAD_LEN) { > > >> + pr_err("encrypted_key: nvdimm key payload incorrect length: %d\n", > > >> + decrypted_datalen); > > >> + return ERR_PTR(-EINVAL); > > >> + } > > > > > > I suspect this may not be the last key type that gets added, but I > > > wonder if we should instead create key-types based on the dlen size. > > > I.e. create a generic 32-byte key-type "enc32"? That way if another > > > 32-byte requirement key comes along we don't need to come touch this > > > routine again. > > > > > > > I'm ok with that if Mimi is. > > If the usage (eg. format: ecryptfs, nvdimm) limits the dlen size(s), > how will defining generic key-types help? If there are no usage size > limitations, then there would be no usage specific definition. > > I must be missing something. ...or I did a poor job of describing the problem. I'm just looking ahead to another potential encrypted-key use case, but instead of nvdimms this would be to lock / unlock persistent memory "namespace" objects. If it turns out the namespace key is just another 32-byte encrypted key should the encrypted-keys core grow support for a new "namespace" key type, should it reuse the "nvdimm" key type, or should we define a generic 32-byte payload-size-requirement key type?
On Tue, 2018-11-27 at 11:10 -0800, Dan Williams wrote: > On Tue, Nov 27, 2018 at 10:24 AM Mimi Zohar <zohar@linux.ibm.com> wrote: > > > > On Tue, 2018-11-27 at 09:20 -0700, Dave Jiang wrote: > > > > > > On 11/27/18 12:20 AM, Dan Williams wrote: > > > > On Fri, Nov 9, 2018 at 2:13 PM Dave Jiang <dave.jiang@intel.com> wrote: > > > >> > > > >> Adding nvdimm key format type to encrypted keys in order to limit the size > > > > > > > > s/Adding/Add an/ > > > > > > > >> of the key to 32-bytes. > > > >> > > > >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> > > > >> --- > > > >> security/keys/encrypted-keys/encrypted.c | 29 ++++++++++++++++++++--------- > > > >> 1 file changed, 20 insertions(+), 9 deletions(-) > > > >> > > > >> diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c > > > >> index d92cbf9687c3..182b4f136bdf 100644 > > > >> --- a/security/keys/encrypted-keys/encrypted.c > > > >> +++ b/security/keys/encrypted-keys/encrypted.c > > > >> @@ -45,6 +45,7 @@ static const char hmac_alg[] = "hmac(sha256)"; > > > >> static const char blkcipher_alg[] = "cbc(aes)"; > > > >> static const char key_format_default[] = "default"; > > > >> static const char key_format_ecryptfs[] = "ecryptfs"; > > > >> +static const char key_format_nvdimm[] = "nvdimm"; > > > >> static unsigned int ivsize; > > > >> static int blksize; > > > >> > > > >> @@ -54,6 +55,7 @@ static int blksize; > > > >> #define HASH_SIZE SHA256_DIGEST_SIZE > > > >> #define MAX_DATA_SIZE 4096 > > > >> #define MIN_DATA_SIZE 20 > > > >> +#define KEY_NVDIMM_PAYLOAD_LEN 32 > > > >> > > > >> static struct crypto_shash *hash_tfm; > > > >> > > > >> @@ -62,12 +64,13 @@ enum { > > > >> }; > > > >> > > > >> enum { > > > >> - Opt_error = -1, Opt_default, Opt_ecryptfs > > > >> + Opt_error = -1, Opt_default, Opt_ecryptfs, Opt_nvdimm > > > >> }; > > > >> > > > >> static const match_table_t key_format_tokens = { > > > >> {Opt_default, "default"}, > > > >> {Opt_ecryptfs, "ecryptfs"}, > > > >> + {Opt_nvdimm, "nvdimm"}, > > > >> {Opt_error, NULL} > > > >> }; > > > >> > > > >> @@ -195,6 +198,7 @@ static int datablob_parse(char *datablob, const char **format, > > > >> key_format = match_token(p, key_format_tokens, args); > > > >> switch (key_format) { > > > >> case Opt_ecryptfs: > > > >> + case Opt_nvdimm: > > > >> case Opt_default: > > > >> *format = p; > > > >> *master_desc = strsep(&datablob, " \t"); > > > >> @@ -625,15 +629,22 @@ static struct encrypted_key_payload *encrypted_key_alloc(struct key *key, > > > >> format_len = (!format) ? strlen(key_format_default) : strlen(format); > > > >> decrypted_datalen = dlen; > > > >> payload_datalen = decrypted_datalen; > > > >> - if (format && !strcmp(format, key_format_ecryptfs)) { > > > >> - if (dlen != ECRYPTFS_MAX_KEY_BYTES) { > > > >> - pr_err("encrypted_key: keylen for the ecryptfs format " > > > >> - "must be equal to %d bytes\n", > > > >> - ECRYPTFS_MAX_KEY_BYTES); > > > >> - return ERR_PTR(-EINVAL); > > > >> + if (format) { > > > >> + if (!strcmp(format, key_format_ecryptfs)) { > > > >> + if (dlen != ECRYPTFS_MAX_KEY_BYTES) { > > > >> + pr_err("encrypted_key: keylen for the ecryptfs format must be equal to %d bytes\n", > > > >> + ECRYPTFS_MAX_KEY_BYTES); > > > >> + return ERR_PTR(-EINVAL); > > > >> + } > > > >> + decrypted_datalen = ECRYPTFS_MAX_KEY_BYTES; > > > >> + payload_datalen = sizeof(struct ecryptfs_auth_tok); > > > >> + } else if (!strcmp(format, key_format_nvdimm)) { > > > >> + if (decrypted_datalen != KEY_NVDIMM_PAYLOAD_LEN) { > > > >> + pr_err("encrypted_key: nvdimm key payload incorrect length: %d\n", > > > >> + decrypted_datalen); > > > >> + return ERR_PTR(-EINVAL); > > > >> + } > > > > > > > > I suspect this may not be the last key type that gets added, but I > > > > wonder if we should instead create key-types based on the dlen size. > > > > I.e. create a generic 32-byte key-type "enc32"? That way if another > > > > 32-byte requirement key comes along we don't need to come touch this > > > > routine again. > > > > > > > > > > I'm ok with that if Mimi is. > > > > If the usage (eg. format: ecryptfs, nvdimm) limits the dlen size(s), > > how will defining generic key-types help? If there are no usage size > > limitations, then there would be no usage specific definition. > > > > I must be missing something. > > ...or I did a poor job of describing the problem. I'm just looking > ahead to another potential encrypted-key use case, but instead of > nvdimms this would be to lock / unlock persistent memory "namespace" > objects. If it turns out the namespace key is just another 32-byte > encrypted key should the encrypted-keys core grow support for a new > "namespace" key type, should it reuse the "nvdimm" key type, or should > we define a generic 32-byte payload-size-requirement key type? Even with a generic length, there will be format (eg. ecryptfs, nvdimm, namespace) specific code. Otherwise you wouldn't be defining a new format type. switch(dlen) { case enc32: if (ecryptfs) goto fail; ... break; case enc64: if (nvdimm || ns) goto fail; ... break; default: } Mimi
On Tue, Nov 27, 2018 at 11:35 AM Mimi Zohar <zohar@linux.ibm.com> wrote: > > On Tue, 2018-11-27 at 11:10 -0800, Dan Williams wrote: > > On Tue, Nov 27, 2018 at 10:24 AM Mimi Zohar <zohar@linux.ibm.com> wrote: > > > > > > On Tue, 2018-11-27 at 09:20 -0700, Dave Jiang wrote: > > > > > > > > On 11/27/18 12:20 AM, Dan Williams wrote: > > > > > On Fri, Nov 9, 2018 at 2:13 PM Dave Jiang <dave.jiang@intel.com> wrote: > > > > >> > > > > >> Adding nvdimm key format type to encrypted keys in order to limit the size > > > > > > > > > > s/Adding/Add an/ > > > > > > > > > >> of the key to 32-bytes. > > > > >> > > > > >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> > > > > >> --- > > > > >> security/keys/encrypted-keys/encrypted.c | 29 ++++++++++++++++++++--------- > > > > >> 1 file changed, 20 insertions(+), 9 deletions(-) > > > > >> > > > > >> diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c > > > > >> index d92cbf9687c3..182b4f136bdf 100644 > > > > >> --- a/security/keys/encrypted-keys/encrypted.c > > > > >> +++ b/security/keys/encrypted-keys/encrypted.c > > > > >> @@ -45,6 +45,7 @@ static const char hmac_alg[] = "hmac(sha256)"; > > > > >> static const char blkcipher_alg[] = "cbc(aes)"; > > > > >> static const char key_format_default[] = "default"; > > > > >> static const char key_format_ecryptfs[] = "ecryptfs"; > > > > >> +static const char key_format_nvdimm[] = "nvdimm"; > > > > >> static unsigned int ivsize; > > > > >> static int blksize; > > > > >> > > > > >> @@ -54,6 +55,7 @@ static int blksize; > > > > >> #define HASH_SIZE SHA256_DIGEST_SIZE > > > > >> #define MAX_DATA_SIZE 4096 > > > > >> #define MIN_DATA_SIZE 20 > > > > >> +#define KEY_NVDIMM_PAYLOAD_LEN 32 > > > > >> > > > > >> static struct crypto_shash *hash_tfm; > > > > >> > > > > >> @@ -62,12 +64,13 @@ enum { > > > > >> }; > > > > >> > > > > >> enum { > > > > >> - Opt_error = -1, Opt_default, Opt_ecryptfs > > > > >> + Opt_error = -1, Opt_default, Opt_ecryptfs, Opt_nvdimm > > > > >> }; > > > > >> > > > > >> static const match_table_t key_format_tokens = { > > > > >> {Opt_default, "default"}, > > > > >> {Opt_ecryptfs, "ecryptfs"}, > > > > >> + {Opt_nvdimm, "nvdimm"}, > > > > >> {Opt_error, NULL} > > > > >> }; > > > > >> > > > > >> @@ -195,6 +198,7 @@ static int datablob_parse(char *datablob, const char **format, > > > > >> key_format = match_token(p, key_format_tokens, args); > > > > >> switch (key_format) { > > > > >> case Opt_ecryptfs: > > > > >> + case Opt_nvdimm: > > > > >> case Opt_default: > > > > >> *format = p; > > > > >> *master_desc = strsep(&datablob, " \t"); > > > > >> @@ -625,15 +629,22 @@ static struct encrypted_key_payload *encrypted_key_alloc(struct key *key, > > > > >> format_len = (!format) ? strlen(key_format_default) : strlen(format); > > > > >> decrypted_datalen = dlen; > > > > >> payload_datalen = decrypted_datalen; > > > > >> - if (format && !strcmp(format, key_format_ecryptfs)) { > > > > >> - if (dlen != ECRYPTFS_MAX_KEY_BYTES) { > > > > >> - pr_err("encrypted_key: keylen for the ecryptfs format " > > > > >> - "must be equal to %d bytes\n", > > > > >> - ECRYPTFS_MAX_KEY_BYTES); > > > > >> - return ERR_PTR(-EINVAL); > > > > >> + if (format) { > > > > >> + if (!strcmp(format, key_format_ecryptfs)) { > > > > >> + if (dlen != ECRYPTFS_MAX_KEY_BYTES) { > > > > >> + pr_err("encrypted_key: keylen for the ecryptfs format must be equal to %d bytes\n", > > > > >> + ECRYPTFS_MAX_KEY_BYTES); > > > > >> + return ERR_PTR(-EINVAL); > > > > >> + } > > > > >> + decrypted_datalen = ECRYPTFS_MAX_KEY_BYTES; > > > > >> + payload_datalen = sizeof(struct ecryptfs_auth_tok); > > > > >> + } else if (!strcmp(format, key_format_nvdimm)) { > > > > >> + if (decrypted_datalen != KEY_NVDIMM_PAYLOAD_LEN) { > > > > >> + pr_err("encrypted_key: nvdimm key payload incorrect length: %d\n", > > > > >> + decrypted_datalen); > > > > >> + return ERR_PTR(-EINVAL); > > > > >> + } > > > > > > > > > > I suspect this may not be the last key type that gets added, but I > > > > > wonder if we should instead create key-types based on the dlen size. > > > > > I.e. create a generic 32-byte key-type "enc32"? That way if another > > > > > 32-byte requirement key comes along we don't need to come touch this > > > > > routine again. > > > > > > > > > > > > > I'm ok with that if Mimi is. > > > > > > If the usage (eg. format: ecryptfs, nvdimm) limits the dlen size(s), > > > how will defining generic key-types help? If there are no usage size > > > limitations, then there would be no usage specific definition. > > > > > > I must be missing something. > > > > ...or I did a poor job of describing the problem. I'm just looking > > ahead to another potential encrypted-key use case, but instead of > > nvdimms this would be to lock / unlock persistent memory "namespace" > > objects. If it turns out the namespace key is just another 32-byte > > encrypted key should the encrypted-keys core grow support for a new > > "namespace" key type, should it reuse the "nvdimm" key type, or should > > we define a generic 32-byte payload-size-requirement key type? > > Even with a generic length, there will be format (eg. ecryptfs, > nvdimm, namespace) specific code. Otherwise you wouldn't be defining > a new format type. > > switch(dlen) { > case enc32: > if (ecryptfs) goto fail; > ... > break; > case enc64: > if (nvdimm || ns) goto fail; > ... > break; > default: > } > I was thinking that the generic-length *is* the format. This does not work for ecyptfs because it has that: payload_datalen = sizeof(struct ecryptfs_auth_tok); ...detail that is ecryptfs specific. For nvdimm the only detail of the format is the decrypted-data-length. However, I get the feeling I'm proposing a solution to a problem that does not exist yet. Let's just go with the "nvdimm" format for now.
On Tue, 2018-11-27 at 11:48 -0800, Dan Williams wrote: > I was thinking that the generic-length *is* the format. This does not > work for ecyptfs because it has that: > > payload_datalen = sizeof(struct ecryptfs_auth_tok); > > ...detail that is ecryptfs specific. For nvdimm the only detail of the > format is the decrypted-data-length. However, I get the feeling I'm > proposing a solution to a problem that does not exist yet. Let's just > go with the "nvdimm" format for now. Ah, that makes more sense now. Defining "Opt_nvdimm" or the generic "Opt_enc32" is fine. Missing from this patch is the update to Documentation/security/keys/trusted-encrypted.rst. Otherwise this patch looks fine. Mimi
On 11/27/18 1:10 PM, Mimi Zohar wrote: > On Tue, 2018-11-27 at 11:48 -0800, Dan Williams wrote: > >> I was thinking that the generic-length *is* the format. This does not >> work for ecyptfs because it has that: >> >> payload_datalen = sizeof(struct ecryptfs_auth_tok); >> >> ...detail that is ecryptfs specific. For nvdimm the only detail of the >> format is the decrypted-data-length. However, I get the feeling I'm >> proposing a solution to a problem that does not exist yet. Let's just >> go with the "nvdimm" format for now. > > Ah, that makes more sense now. Defining "Opt_nvdimm" or the generic > "Opt_enc32" is fine. Missing from this patch is the update to > Documentation/security/keys/trusted-encrypted.rst. Otherwise this > patch looks fine. I'll go with enc32 and update the doc and resubmit with the series. Thanks Mimi. > > Mimi >
diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c index d92cbf9687c3..182b4f136bdf 100644 --- a/security/keys/encrypted-keys/encrypted.c +++ b/security/keys/encrypted-keys/encrypted.c @@ -45,6 +45,7 @@ static const char hmac_alg[] = "hmac(sha256)"; static const char blkcipher_alg[] = "cbc(aes)"; static const char key_format_default[] = "default"; static const char key_format_ecryptfs[] = "ecryptfs"; +static const char key_format_nvdimm[] = "nvdimm"; static unsigned int ivsize; static int blksize; @@ -54,6 +55,7 @@ static int blksize; #define HASH_SIZE SHA256_DIGEST_SIZE #define MAX_DATA_SIZE 4096 #define MIN_DATA_SIZE 20 +#define KEY_NVDIMM_PAYLOAD_LEN 32 static struct crypto_shash *hash_tfm; @@ -62,12 +64,13 @@ enum { }; enum { - Opt_error = -1, Opt_default, Opt_ecryptfs + Opt_error = -1, Opt_default, Opt_ecryptfs, Opt_nvdimm }; static const match_table_t key_format_tokens = { {Opt_default, "default"}, {Opt_ecryptfs, "ecryptfs"}, + {Opt_nvdimm, "nvdimm"}, {Opt_error, NULL} }; @@ -195,6 +198,7 @@ static int datablob_parse(char *datablob, const char **format, key_format = match_token(p, key_format_tokens, args); switch (key_format) { case Opt_ecryptfs: + case Opt_nvdimm: case Opt_default: *format = p; *master_desc = strsep(&datablob, " \t"); @@ -625,15 +629,22 @@ static struct encrypted_key_payload *encrypted_key_alloc(struct key *key, format_len = (!format) ? strlen(key_format_default) : strlen(format); decrypted_datalen = dlen; payload_datalen = decrypted_datalen; - if (format && !strcmp(format, key_format_ecryptfs)) { - if (dlen != ECRYPTFS_MAX_KEY_BYTES) { - pr_err("encrypted_key: keylen for the ecryptfs format " - "must be equal to %d bytes\n", - ECRYPTFS_MAX_KEY_BYTES); - return ERR_PTR(-EINVAL); + if (format) { + if (!strcmp(format, key_format_ecryptfs)) { + if (dlen != ECRYPTFS_MAX_KEY_BYTES) { + pr_err("encrypted_key: keylen for the ecryptfs format must be equal to %d bytes\n", + ECRYPTFS_MAX_KEY_BYTES); + return ERR_PTR(-EINVAL); + } + decrypted_datalen = ECRYPTFS_MAX_KEY_BYTES; + payload_datalen = sizeof(struct ecryptfs_auth_tok); + } else if (!strcmp(format, key_format_nvdimm)) { + if (decrypted_datalen != KEY_NVDIMM_PAYLOAD_LEN) { + pr_err("encrypted_key: nvdimm key payload incorrect length: %d\n", + decrypted_datalen); + return ERR_PTR(-EINVAL); + } } - decrypted_datalen = ECRYPTFS_MAX_KEY_BYTES; - payload_datalen = sizeof(struct ecryptfs_auth_tok); } encrypted_datalen = roundup(decrypted_datalen, blksize);
Adding nvdimm key format type to encrypted keys in order to limit the size of the key to 32bytes. Signed-off-by: Dave Jiang <dave.jiang@intel.com> --- security/keys/encrypted-keys/encrypted.c | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-)