| Message ID | 1445690562-11405-1-git-send-email-jarkko.sakkinen@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
On Sat, Oct 24, 2015 at 03:42:42PM +0300, Jarkko Sakkinen wrote: > Added 'hashalg=' option for selecting the hash algorithm. > > Currently available options are: > > * sha1 > * sha256 > * sha384 > * sha512 > * sm3_256 > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> Things that came to mind after sending this: * Documentation update is needed. * Maybe it would be a good idea to return -EINVAL in the following conditions: * TPM1 chip * Explicit hashalg option where hashalg is something different than sha1. Also one question: should I split this into three patches: * Support for 'hashalg' in drivers/char/tpm. * Parsing of hashalg in security/keys. * Documentation update. Anyway, this patch is good for testing. No functional updates are needed except maybe that update for TPM1 chips. /Jarkko > --- > drivers/char/tpm/tpm.h | 5 ++++- > drivers/char/tpm/tpm2-cmd.c | 34 ++++++++++++++++++++++++++++++++++ > include/keys/trusted-type.h | 2 ++ > security/keys/trusted.c | 8 +++++++- > 4 files changed, 47 insertions(+), 2 deletions(-) > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h > index a4257a3..4c18f46 100644 > --- a/drivers/char/tpm/tpm.h > +++ b/drivers/char/tpm/tpm.h > @@ -92,7 +92,10 @@ enum tpm2_algorithms { > TPM2_ALG_SHA1 = 0x0004, > TPM2_ALG_KEYEDHASH = 0x0008, > TPM2_ALG_SHA256 = 0x000B, > - TPM2_ALG_NULL = 0x0010 > + TPM2_ALG_SHA384 = 0x000C, > + TPM2_ALG_SHA512 = 0x000D, > + TPM2_ALG_NULL = 0x0010, > + TPM2_ALG_SM3_256 = 0x0012, > }; > > enum tpm2_command_codes { > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c > index bd7039f..0704bd6 100644 > --- a/drivers/char/tpm/tpm2-cmd.c > +++ b/drivers/char/tpm/tpm2-cmd.c > @@ -104,6 +104,22 @@ struct tpm2_cmd { > union tpm2_cmd_params params; > } __packed; > > +struct tpm2_hashalg { > + char name[MAX_HASHALG_SIZE]; > + u32 id; > +}; > + > +struct tpm2_hashalg tpm2_hashalg_map[] = { > + {"sha1", TPM2_ALG_SHA1}, > + {"sha256", TPM2_ALG_SHA256}, > + {"sm3_256", TPM2_ALG_SM3_256}, > + {"sha384", TPM2_ALG_SHA384}, > + {"sha512", TPM2_ALG_SHA512}, > +}; > + > +#define TPM2_HASHALG_COUNT \ > + (sizeof(tpm2_hashalg_map) / sizeof(tpm2_hashalg_map[1])) > + > /* > * Array with one entry per ordinal defining the maximum amount > * of time the chip could take to return the result. The values > @@ -429,8 +445,26 @@ int tpm2_seal_trusted(struct tpm_chip *chip, > { > unsigned int blob_len; > struct tpm_buf buf; > + u32 hashalg = TPM2_ALG_SHA256; > + int i; > int rc; > > + if (strlen(options->hashalg) > 0) { > + for (i = 0; i < TPM2_HASHALG_COUNT; i++) { > + if (!strcmp(options->hashalg, > + tpm2_hashalg_map[i].name)) { > + hashalg = tpm2_hashalg_map[i].id; > + dev_dbg(chip->pdev, "%s: hashalg: %s 0x%08X\n", > + __func__, tpm2_hashalg_map[i].name, > + hashalg); > + break; > + } > + } > + > + if (i == TPM2_HASHALG_COUNT) > + return -EINVAL; > + } > + > rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_CREATE); > if (rc) > return rc; > diff --git a/include/keys/trusted-type.h b/include/keys/trusted-type.h > index f91ecd9..a545733 100644 > --- a/include/keys/trusted-type.h > +++ b/include/keys/trusted-type.h > @@ -18,6 +18,7 @@ > #define MAX_KEY_SIZE 128 > #define MAX_BLOB_SIZE 512 > #define MAX_PCRINFO_SIZE 64 > +#define MAX_HASHALG_SIZE 16 > > struct trusted_key_payload { > struct rcu_head rcu; > @@ -36,6 +37,7 @@ struct trusted_key_options { > uint32_t pcrinfo_len; > unsigned char pcrinfo[MAX_PCRINFO_SIZE]; > int pcrlock; > + unsigned char hashalg[MAX_HASHALG_SIZE]; > }; > > extern struct key_type key_type_trusted; > diff --git a/security/keys/trusted.c b/security/keys/trusted.c > index d3633cf..9e7564d 100644 > --- a/security/keys/trusted.c > +++ b/security/keys/trusted.c > @@ -710,7 +710,8 @@ enum { > Opt_err = -1, > Opt_new, Opt_load, Opt_update, > Opt_keyhandle, Opt_keyauth, Opt_blobauth, > - Opt_pcrinfo, Opt_pcrlock, Opt_migratable > + Opt_pcrinfo, Opt_pcrlock, Opt_migratable, > + Opt_hashalg, > }; > > static const match_table_t key_tokens = { > @@ -723,6 +724,7 @@ static const match_table_t key_tokens = { > {Opt_pcrinfo, "pcrinfo=%s"}, > {Opt_pcrlock, "pcrlock=%s"}, > {Opt_migratable, "migratable=%s"}, > + {Opt_hashalg, "hashalg=%s"}, > {Opt_err, NULL} > }; > > @@ -787,6 +789,10 @@ static int getoptions(char *c, struct trusted_key_payload *pay, > return -EINVAL; > opt->pcrlock = lock; > break; > + case Opt_hashalg: > + strncpy(opt->hashalg, args[0].from, > + MAX_HASHALG_SIZE - 1); > + break; > default: > return -EINVAL; > } > -- > 2.5.0 > ------------------------------------------------------------------------------
On Sat, 2015-10-24 at 15:42 +0300, Jarkko Sakkinen wrote: > Added 'hashalg=' option for selecting the hash algorithm. > > Currently available options are: > > * sha1 > * sha256 > * sha384 > * sha512 > * sm3_256 Please consider using crypto/hash_info.c: hash_algo_name[], which already define the algorithm string names. Use include/crypto/hash_info.c to include a reference to this array. Boot command line options should be prefixed with the subsystem name. So instead of hashalg, please use tpm_hashalg. The boot command line option needs to be documented in Documentation/kernel-parameters.txt. Mimi ------------------------------------------------------------------------------
On Sun, Oct 25, 2015 at 03:21:31PM -0400, Mimi Zohar wrote: > On Sat, 2015-10-24 at 15:42 +0300, Jarkko Sakkinen wrote: > > Added 'hashalg=' option for selecting the hash algorithm. > > > > Currently available options are: > > > > * sha1 > > * sha256 > > * sha384 > > * sha512 > > * sm3_256 > > Please consider using crypto/hash_info.c: hash_algo_name[], which > already define the algorithm string names. Use > include/crypto/hash_info.c to include a reference to this array. It wold work for me. I did ad-hoc because first example that I looked at was EcryptFS. I need to add sm3_256 to that array. I've found three different ways to write it: * sm3256 (various google hits) * sm3-256 (various google hits) * sm3_256 (TPM 2.0 Structures specification) Maybe the second option would be the most appropriate? > Boot command line options should be prefixed with the subsystem name. > So instead of hashalg, please use tpm_hashalg. The boot command line > option needs to be documented in Documentation/kernel-parameters.txt. I see. My commit message is clearly inadequate. It's an option for the keyring syscalls. > Mimi /Jarkko ------------------------------------------------------------------------------
n Mon, Oct 26, 2015 at 07:44:39AM +0200, Jarkko Sakkinen wrote: > On Sun, Oct 25, 2015 at 03:21:31PM -0400, Mimi Zohar wrote: > > On Sat, 2015-10-24 at 15:42 +0300, Jarkko Sakkinen wrote: > > > Added 'hashalg=' option for selecting the hash algorithm. > > > > > > Currently available options are: > > > > > > * sha1 > > > * sha256 > > > * sha384 > > > * sha512 > > > * sm3_256 > > > > Please consider using crypto/hash_info.c: hash_algo_name[], which > > already define the algorithm string names. Use > > include/crypto/hash_info.c to include a reference to this array. > > It wold work for me. I did ad-hoc because first example that I looked > at was EcryptFS. > > I need to add sm3_256 to that array. > > I've found three different ways to write it: > > * sm3256 (various google hits) > * sm3-256 (various google hits) > * sm3_256 (TPM 2.0 Structures specification) > > Maybe the second option would be the most appropriate? > > > Boot command line options should be prefixed with the subsystem name. > > So instead of hashalg, please use tpm_hashalg. The boot command line > > option needs to be documented in Documentation/kernel-parameters.txt. > > I see. My commit message is clearly inadequate. It's an option for the > keyring syscalls. BTW, in IMA I see you have the hash algorithm as a boot parameter. I guess it makes sense there because it works implicitly in the background? Sealing a trusted key is an explicit operation. That's why I thought it'd be better to have it as an option for the syscall. Does this logic make sense to your or not? > > Mimi PS. Hey one more thing: this was supposed to be RFC, forgot to add --subject-prefix="PATCH RFC". Sorry about that. > /Jarkko /Jarkko ------------------------------------------------------------------------------
On Tue, 2015-10-27 at 12:42 +0200, Jarkko Sakkinen wrote: > n Mon, Oct 26, 2015 at 07:44:39AM +0200, Jarkko Sakkinen wrote: > > On Sun, Oct 25, 2015 at 03:21:31PM -0400, Mimi Zohar wrote: > > > On Sat, 2015-10-24 at 15:42 +0300, Jarkko Sakkinen wrote: > > > > Added 'hashalg=' option for selecting the hash algorithm. > > > > > > > > Currently available options are: > > > > > > > > * sha1 > > > > * sha256 > > > > * sha384 > > > > * sha512 > > > > * sm3_256 > > > > > > Please consider using crypto/hash_info.c: hash_algo_name[], which > > > already define the algorithm string names. Use > > > include/crypto/hash_info.c to include a reference to this array. > > > > It wold work for me. I did ad-hoc because first example that I looked > > at was EcryptFS. After EVM, EcryptFS was the first subsystem to use trusted keys. Support for larger digests was later added to IMA. > > I need to add sm3_256 to that array. Unless there is kernel crypto support for this algorithm, I would conditionally include the algorithm, probably based on a Kconfig option. > > I've found three different ways to write it: > > > > * sm3256 (various google hits) > > * sm3-256 (various google hits) > > * sm3_256 (TPM 2.0 Structures specification) > > > > Maybe the second option would be the most appropriate? Right, If there aren't any standards, use the second option for the string and an underscore for the variable name. > > > Boot command line options should be prefixed with the subsystem name. > > > So instead of hashalg, please use tpm_hashalg. The boot command line > > > option needs to be documented in Documentation/kernel-parameters.txt. > > > > I see. My commit message is clearly inadequate. It's an option for the > > keyring syscalls. Sorry for the misunderstanding. > BTW, in IMA I see you have the hash algorithm as a boot parameter. I > guess it makes sense there because it works implicitly in the > background? The default hash algorithm is defined using the Kconfig IMA_DEFAULT_HASH option, but can be specified on the boot command line using "ima_hash=". > Sealing a trusted key is an explicit operation. That's why I thought > it'd be better to have it as an option for the syscall. Does this logic > make sense to your or not? It does. > > PS. Hey one more thing: this was supposed to be RFC, forgot to add > --subject-prefix="PATCH RFC". Sorry about that. :) Mimi ------------------------------------------------------------------------------
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index a4257a3..4c18f46 100644 --- a/drivers/char/tpm/tpm.h +++ b/drivers/char/tpm/tpm.h @@ -92,7 +92,10 @@ enum tpm2_algorithms { TPM2_ALG_SHA1 = 0x0004, TPM2_ALG_KEYEDHASH = 0x0008, TPM2_ALG_SHA256 = 0x000B, - TPM2_ALG_NULL = 0x0010 + TPM2_ALG_SHA384 = 0x000C, + TPM2_ALG_SHA512 = 0x000D, + TPM2_ALG_NULL = 0x0010, + TPM2_ALG_SM3_256 = 0x0012, }; enum tpm2_command_codes { diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c index bd7039f..0704bd6 100644 --- a/drivers/char/tpm/tpm2-cmd.c +++ b/drivers/char/tpm/tpm2-cmd.c @@ -104,6 +104,22 @@ struct tpm2_cmd { union tpm2_cmd_params params; } __packed; +struct tpm2_hashalg { + char name[MAX_HASHALG_SIZE]; + u32 id; +}; + +struct tpm2_hashalg tpm2_hashalg_map[] = { + {"sha1", TPM2_ALG_SHA1}, + {"sha256", TPM2_ALG_SHA256}, + {"sm3_256", TPM2_ALG_SM3_256}, + {"sha384", TPM2_ALG_SHA384}, + {"sha512", TPM2_ALG_SHA512}, +}; + +#define TPM2_HASHALG_COUNT \ + (sizeof(tpm2_hashalg_map) / sizeof(tpm2_hashalg_map[1])) + /* * Array with one entry per ordinal defining the maximum amount * of time the chip could take to return the result. The values @@ -429,8 +445,26 @@ int tpm2_seal_trusted(struct tpm_chip *chip, { unsigned int blob_len; struct tpm_buf buf; + u32 hashalg = TPM2_ALG_SHA256; + int i; int rc; + if (strlen(options->hashalg) > 0) { + for (i = 0; i < TPM2_HASHALG_COUNT; i++) { + if (!strcmp(options->hashalg, + tpm2_hashalg_map[i].name)) { + hashalg = tpm2_hashalg_map[i].id; + dev_dbg(chip->pdev, "%s: hashalg: %s 0x%08X\n", + __func__, tpm2_hashalg_map[i].name, + hashalg); + break; + } + } + + if (i == TPM2_HASHALG_COUNT) + return -EINVAL; + } + rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_CREATE); if (rc) return rc; diff --git a/include/keys/trusted-type.h b/include/keys/trusted-type.h index f91ecd9..a545733 100644 --- a/include/keys/trusted-type.h +++ b/include/keys/trusted-type.h @@ -18,6 +18,7 @@ #define MAX_KEY_SIZE 128 #define MAX_BLOB_SIZE 512 #define MAX_PCRINFO_SIZE 64 +#define MAX_HASHALG_SIZE 16 struct trusted_key_payload { struct rcu_head rcu; @@ -36,6 +37,7 @@ struct trusted_key_options { uint32_t pcrinfo_len; unsigned char pcrinfo[MAX_PCRINFO_SIZE]; int pcrlock; + unsigned char hashalg[MAX_HASHALG_SIZE]; }; extern struct key_type key_type_trusted; diff --git a/security/keys/trusted.c b/security/keys/trusted.c index d3633cf..9e7564d 100644 --- a/security/keys/trusted.c +++ b/security/keys/trusted.c @@ -710,7 +710,8 @@ enum { Opt_err = -1, Opt_new, Opt_load, Opt_update, Opt_keyhandle, Opt_keyauth, Opt_blobauth, - Opt_pcrinfo, Opt_pcrlock, Opt_migratable + Opt_pcrinfo, Opt_pcrlock, Opt_migratable, + Opt_hashalg, }; static const match_table_t key_tokens = { @@ -723,6 +724,7 @@ static const match_table_t key_tokens = { {Opt_pcrinfo, "pcrinfo=%s"}, {Opt_pcrlock, "pcrlock=%s"}, {Opt_migratable, "migratable=%s"}, + {Opt_hashalg, "hashalg=%s"}, {Opt_err, NULL} }; @@ -787,6 +789,10 @@ static int getoptions(char *c, struct trusted_key_payload *pay, return -EINVAL; opt->pcrlock = lock; break; + case Opt_hashalg: + strncpy(opt->hashalg, args[0].from, + MAX_HASHALG_SIZE - 1); + break; default: return -EINVAL; }
Added 'hashalg=' option for selecting the hash algorithm. Currently available options are: * sha1 * sha256 * sha384 * sha512 * sm3_256 Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> --- drivers/char/tpm/tpm.h | 5 ++++- drivers/char/tpm/tpm2-cmd.c | 34 ++++++++++++++++++++++++++++++++++ include/keys/trusted-type.h | 2 ++ security/keys/trusted.c | 8 +++++++- 4 files changed, 47 insertions(+), 2 deletions(-)