[01/11] keys-encrypted: add nvdimm key format type to encrypted keys
diff mbox series

Message ID 154180163107.70506.6894134402057805078.stgit@djiang5-desk3.ch.intel.com
State New, archived
Headers show
Series
  • Additional patches for nvdimm security support
Related show

Commit Message

Dave Jiang Nov. 9, 2018, 10:13 p.m. UTC
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(-)

Comments

Dan Williams Nov. 27, 2018, 7:20 a.m. UTC | #1
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.
Dave Jiang Nov. 27, 2018, 4:20 p.m. UTC | #2
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.
Mimi Zohar Nov. 27, 2018, 6:24 p.m. UTC | #3
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
Dan Williams Nov. 27, 2018, 7:10 p.m. UTC | #4
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?
Mimi Zohar Nov. 27, 2018, 7:35 p.m. UTC | #5
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
Dan Williams Nov. 27, 2018, 7:48 p.m. UTC | #6
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.
Mimi Zohar Nov. 27, 2018, 8:10 p.m. UTC | #7
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
Dave Jiang Nov. 27, 2018, 8:15 p.m. UTC | #8
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
>

Patch
diff mbox series

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