[tpmdd-devel,v2,1/3] keys, trusted: select the hash algorithm
diff mbox

Message ID 1446204910-29948-2-git-send-email-jarkko.sakkinen@linux.intel.com
State New
Headers show

Commit Message

Jarkko Sakkinen Oct. 30, 2015, 11:35 a.m. UTC
Added 'hash=' option for selecting the hash algorithm for add_key()
syscall and documentation for it.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 Documentation/security/keys-trusted-encrypted.txt |  3 +++
 include/keys/trusted-type.h                       |  1 +
 security/keys/Kconfig                             |  1 +
 security/keys/trusted.c                           | 20 +++++++++++++++++++-
 4 files changed, 24 insertions(+), 1 deletion(-)

Comments

Mimi Zohar Nov. 2, 2015, 12:16 p.m. UTC | #1
On Fri, 2015-10-30 at 13:35 +0200, Jarkko Sakkinen wrote:

> @@ -787,6 +791,20 @@ static int getoptions(char *c, struct trusted_key_payload *pay,
>  				return -EINVAL;
>  			opt->pcrlock = lock;
>  			break;
> +		case Opt_hash:
> +			for (i = 0; i < HASH_ALGO__LAST; i++) {
> +				if (!strcmp(args[0].from, hash_algo_name[i])) {
> +					opt->hash = i;
> +					break;
> +				}
> +			}
> +			res = tpm_is_tpm2(TPM_ANY_NUM);

While looking at this, I wanted to verify that chips are still added to
the tail of the tpm_chip_list.  Unfortunately, commit "afb5abc tpm:
two-phase chip management functions" reverted David Howell's commit
"770ab65 TPM: Add new TPMs to the tail of the list to prevent
inadvertent change of dev".

> +			if (res < 0)
> +				return res;
> +			if (i == HASH_ALGO__LAST ||
> +			    (!res && i != HASH_ALGO_SHA1))
> +				return -EINVAL;
> +			break;

If the first TPM registered is a TPM 1.2, then changing the default TPM
2.0 hash algorithm will fail.

Mimi

>  		default:
>  			return -EINVAL;
>  		}
 


------------------------------------------------------------------------------
Jarkko Sakkinen Nov. 2, 2015, 5:40 p.m. UTC | #2
On Mon, Nov 02, 2015 at 07:16:49AM -0500, Mimi Zohar wrote:
> On Fri, 2015-10-30 at 13:35 +0200, Jarkko Sakkinen wrote:
> 
> > @@ -787,6 +791,20 @@ static int getoptions(char *c, struct trusted_key_payload *pay,
> >  				return -EINVAL;
> >  			opt->pcrlock = lock;
> >  			break;
> > +		case Opt_hash:
> > +			for (i = 0; i < HASH_ALGO__LAST; i++) {
> > +				if (!strcmp(args[0].from, hash_algo_name[i])) {
> > +					opt->hash = i;
> > +					break;
> > +				}
> > +			}
> > +			res = tpm_is_tpm2(TPM_ANY_NUM);
> 
> While looking at this, I wanted to verify that chips are still added to
> the tail of the tpm_chip_list.  Unfortunately, commit "afb5abc tpm:
> two-phase chip management functions" reverted David Howell's commit
> "770ab65 TPM: Add new TPMs to the tail of the list to prevent
> inadvertent change of dev".

Ouch. I'll send a fix that reverts the behavior. Good catch and
apologies.  Platforms that I've used BIOS let choose either dTPM 1.2 or
fTPM and platforms that have dTPM 2.0 do not have fTPM option at all.
That's why it went unnoticed.

> > +			if (res < 0)
> > +				return res;
> > +			if (i == HASH_ALGO__LAST ||
> > +			    (!res && i != HASH_ALGO_SHA1))
> > +				return -EINVAL;
> > +			break;
> 
> If the first TPM registered is a TPM 1.2, then changing the default TPM
> 2.0 hash algorithm will fail.

Yup.

> Mimi
> 
> >  		default:
> >  			return -EINVAL;
> >  		}

/Jarkko

------------------------------------------------------------------------------
Jarkko Sakkinen Nov. 3, 2015, 7:39 a.m. UTC | #3
On Mon, Nov 02, 2015 at 07:16:49AM -0500, Mimi Zohar wrote:
> On Fri, 2015-10-30 at 13:35 +0200, Jarkko Sakkinen wrote:
> 
> > @@ -787,6 +791,20 @@ static int getoptions(char *c, struct trusted_key_payload *pay,
> >  				return -EINVAL;
> >  			opt->pcrlock = lock;
> >  			break;
> > +		case Opt_hash:
> > +			for (i = 0; i < HASH_ALGO__LAST; i++) {
> > +				if (!strcmp(args[0].from, hash_algo_name[i])) {
> > +					opt->hash = i;
> > +					break;
> > +				}
> > +			}
> > +			res = tpm_is_tpm2(TPM_ANY_NUM);
> 
> While looking at this, I wanted to verify that chips are still added to
> the tail of the tpm_chip_list.  Unfortunately, commit "afb5abc tpm:
> two-phase chip management functions" reverted David Howell's commit
> "770ab65 TPM: Add new TPMs to the tail of the list to prevent
> inadvertent change of dev".
> 
> > +			if (res < 0)
> > +				return res;
> > +			if (i == HASH_ALGO__LAST ||
> > +			    (!res && i != HASH_ALGO_SHA1))
> > +				return -EINVAL;
> > +			break;
> 
> If the first TPM registered is a TPM 1.2, then changing the default TPM
> 2.0 hash algorithm will fail.

Now that we are going fix this issue in 4.3 and 4.4 do you find this
patch otherwise acceptable?

PS. In other options that we don't support in TPM2 I'm planning to
submit a fix that they will return -EINVAL (like pcrinfo).

> Mimi

/Jarkko

------------------------------------------------------------------------------
Mimi Zohar Nov. 3, 2015, 3:39 p.m. UTC | #4
On Tue, 2015-11-03 at 09:39 +0200, Jarkko Sakkinen wrote:
> On Mon, Nov 02, 2015 at 07:16:49AM -0500, Mimi Zohar wrote:
> > On Fri, 2015-10-30 at 13:35 +0200, Jarkko Sakkinen wrote:
> > 
> > > @@ -787,6 +791,20 @@ static int getoptions(char *c, struct trusted_key_payload *pay,
> > >  				return -EINVAL;
> > >  			opt->pcrlock = lock;
> > >  			break;
> > > +		case Opt_hash:
> > > +			for (i = 0; i < HASH_ALGO__LAST; i++) {
> > > +				if (!strcmp(args[0].from, hash_algo_name[i])) {
> > > +					opt->hash = i;
> > > +					break;
> > > +				}
> > > +			}
> > > +			res = tpm_is_tpm2(TPM_ANY_NUM);
> > 
> > While looking at this, I wanted to verify that chips are still added to
> > the tail of the tpm_chip_list.  Unfortunately, commit "afb5abc tpm:
> > two-phase chip management functions" reverted David Howell's commit
> > "770ab65 TPM: Add new TPMs to the tail of the list to prevent
> > inadvertent change of dev".
> > 
> > > +			if (res < 0)
> > > +				return res;
> > > +			if (i == HASH_ALGO__LAST ||
> > > +			    (!res && i != HASH_ALGO_SHA1))
> > > +				return -EINVAL;
> > > +			break;
> > 
> > If the first TPM registered is a TPM 1.2, then changing the default TPM
> > 2.0 hash algorithm will fail.
> 
> Now that we are going fix this issue in 4.3 and 4.4 do you find this
> patch otherwise acceptable?
> 
> PS. In other options that we don't support in TPM2 I'm planning to
> submit a fix that they will return -EINVAL (like pcrinfo).

I don't have a problem failing the request, but I do suggest adding some
sort of error message.  Different systems might behavior differently
without any explanation.

Mimi


------------------------------------------------------------------------------
Jarkko Sakkinen Nov. 3, 2015, 4:12 p.m. UTC | #5
On Tue, Nov 03, 2015 at 10:39:11AM -0500, Mimi Zohar wrote:
> On Tue, 2015-11-03 at 09:39 +0200, Jarkko Sakkinen wrote:
> > On Mon, Nov 02, 2015 at 07:16:49AM -0500, Mimi Zohar wrote:
> > > On Fri, 2015-10-30 at 13:35 +0200, Jarkko Sakkinen wrote:
> > > 
> > > > @@ -787,6 +791,20 @@ static int getoptions(char *c, struct trusted_key_payload *pay,
> > > >  				return -EINVAL;
> > > >  			opt->pcrlock = lock;
> > > >  			break;
> > > > +		case Opt_hash:
> > > > +			for (i = 0; i < HASH_ALGO__LAST; i++) {
> > > > +				if (!strcmp(args[0].from, hash_algo_name[i])) {
> > > > +					opt->hash = i;
> > > > +					break;
> > > > +				}
> > > > +			}
> > > > +			res = tpm_is_tpm2(TPM_ANY_NUM);
> > > 
> > > While looking at this, I wanted to verify that chips are still added to
> > > the tail of the tpm_chip_list.  Unfortunately, commit "afb5abc tpm:
> > > two-phase chip management functions" reverted David Howell's commit
> > > "770ab65 TPM: Add new TPMs to the tail of the list to prevent
> > > inadvertent change of dev".
> > > 
> > > > +			if (res < 0)
> > > > +				return res;
> > > > +			if (i == HASH_ALGO__LAST ||
> > > > +			    (!res && i != HASH_ALGO_SHA1))
> > > > +				return -EINVAL;
> > > > +			break;
> > > 
> > > If the first TPM registered is a TPM 1.2, then changing the default TPM
> > > 2.0 hash algorithm will fail.
> > 
> > Now that we are going fix this issue in 4.3 and 4.4 do you find this
> > patch otherwise acceptable?
> > 
> > PS. In other options that we don't support in TPM2 I'm planning to
> > submit a fix that they will return -EINVAL (like pcrinfo).
> 
> I don't have a problem failing the request, but I do suggest adding some
> sort of error message.  Different systems might behavior differently
> without any explanation.

Something like the pr_info("trusted_key: TPM 1.x supports only sha1")?

> Mimi

/Jarkko

------------------------------------------------------------------------------
Jarkko Sakkinen Nov. 4, 2015, 1:13 p.m. UTC | #6
On Tue, Nov 03, 2015 at 06:12:17PM +0200, Jarkko Sakkinen wrote:
> On Tue, Nov 03, 2015 at 10:39:11AM -0500, Mimi Zohar wrote:
> > On Tue, 2015-11-03 at 09:39 +0200, Jarkko Sakkinen wrote:
> > > On Mon, Nov 02, 2015 at 07:16:49AM -0500, Mimi Zohar wrote:
> > > > On Fri, 2015-10-30 at 13:35 +0200, Jarkko Sakkinen wrote:
> > > > 
> > > > > @@ -787,6 +791,20 @@ static int getoptions(char *c, struct trusted_key_payload *pay,
> > > > >  				return -EINVAL;
> > > > >  			opt->pcrlock = lock;
> > > > >  			break;
> > > > > +		case Opt_hash:
> > > > > +			for (i = 0; i < HASH_ALGO__LAST; i++) {
> > > > > +				if (!strcmp(args[0].from, hash_algo_name[i])) {
> > > > > +					opt->hash = i;
> > > > > +					break;
> > > > > +				}
> > > > > +			}
> > > > > +			res = tpm_is_tpm2(TPM_ANY_NUM);
> > > > 
> > > > While looking at this, I wanted to verify that chips are still added to
> > > > the tail of the tpm_chip_list.  Unfortunately, commit "afb5abc tpm:
> > > > two-phase chip management functions" reverted David Howell's commit
> > > > "770ab65 TPM: Add new TPMs to the tail of the list to prevent
> > > > inadvertent change of dev".
> > > > 
> > > > > +			if (res < 0)
> > > > > +				return res;
> > > > > +			if (i == HASH_ALGO__LAST ||
> > > > > +			    (!res && i != HASH_ALGO_SHA1))
> > > > > +				return -EINVAL;
> > > > > +			break;
> > > > 
> > > > If the first TPM registered is a TPM 1.2, then changing the default TPM
> > > > 2.0 hash algorithm will fail.
> > > 
> > > Now that we are going fix this issue in 4.3 and 4.4 do you find this
> > > patch otherwise acceptable?
> > > 
> > > PS. In other options that we don't support in TPM2 I'm planning to
> > > submit a fix that they will return -EINVAL (like pcrinfo).
> > 
> > I don't have a problem failing the request, but I do suggest adding some
> > sort of error message.  Different systems might behavior differently
> > without any explanation.
> 
> Something like the pr_info("trusted_key: TPM 1.x supports only sha1")?

I've started to think that maybe it was a bad idea to break this into
patch set as the changes are small and they make sense only together.
What do you think? Should quash everything into single patch?

> > Mimi

/Jarkko

------------------------------------------------------------------------------

Patch
diff mbox

diff --git a/Documentation/security/keys-trusted-encrypted.txt b/Documentation/security/keys-trusted-encrypted.txt
index e105ae9..fd2565b 100644
--- a/Documentation/security/keys-trusted-encrypted.txt
+++ b/Documentation/security/keys-trusted-encrypted.txt
@@ -38,6 +38,9 @@  Usage:
        pcrlock=	  pcr number to be extended to "lock" blob
        migratable= 0|1 indicating permission to reseal to new PCR values,
                    default 1 (resealing allowed)
+       hash=      hash algorithm name as a string. For TPM 1.x the only
+                  allowed value is sha1. For TPM 2.x the allowed values
+		  are sha1, sha256, sha384, sha512 and sm3-256.
 
 "keyctl print" returns an ascii hex copy of the sealed key, which is in standard
 TPM_STORED_DATA format.  The key length for new keys are always in bytes.
diff --git a/include/keys/trusted-type.h b/include/keys/trusted-type.h
index f91ecd9..a6a1008 100644
--- a/include/keys/trusted-type.h
+++ b/include/keys/trusted-type.h
@@ -36,6 +36,7 @@  struct trusted_key_options {
 	uint32_t pcrinfo_len;
 	unsigned char pcrinfo[MAX_PCRINFO_SIZE];
 	int pcrlock;
+	uint32_t hash;
 };
 
 extern struct key_type key_type_trusted;
diff --git a/security/keys/Kconfig b/security/keys/Kconfig
index 72483b8..fe4d74e 100644
--- a/security/keys/Kconfig
+++ b/security/keys/Kconfig
@@ -54,6 +54,7 @@  config TRUSTED_KEYS
 	select CRYPTO
 	select CRYPTO_HMAC
 	select CRYPTO_SHA1
+	select CRYPTO_HASH_INFO
 	help
 	  This option provides support for creating, sealing, and unsealing
 	  keys in the kernel. Trusted keys are random number symmetric keys,
diff --git a/security/keys/trusted.c b/security/keys/trusted.c
index d3633cf..7a87bcd 100644
--- a/security/keys/trusted.c
+++ b/security/keys/trusted.c
@@ -11,6 +11,7 @@ 
  * See Documentation/security/keys-trusted-encrypted.txt
  */
 
+#include <crypto/hash_info.h>
 #include <linux/uaccess.h>
 #include <linux/module.h>
 #include <linux/init.h>
@@ -710,7 +711,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_hash,
 };
 
 static const match_table_t key_tokens = {
@@ -723,6 +725,7 @@  static const match_table_t key_tokens = {
 	{Opt_pcrinfo, "pcrinfo=%s"},
 	{Opt_pcrlock, "pcrlock=%s"},
 	{Opt_migratable, "migratable=%s"},
+	{Opt_hash, "hash=%s"},
 	{Opt_err, NULL}
 };
 
@@ -736,6 +739,7 @@  static int getoptions(char *c, struct trusted_key_payload *pay,
 	int res;
 	unsigned long handle;
 	unsigned long lock;
+	int i;
 
 	while ((p = strsep(&c, " \t"))) {
 		if (*p == '\0' || *p == ' ' || *p == '\t')
@@ -787,6 +791,20 @@  static int getoptions(char *c, struct trusted_key_payload *pay,
 				return -EINVAL;
 			opt->pcrlock = lock;
 			break;
+		case Opt_hash:
+			for (i = 0; i < HASH_ALGO__LAST; i++) {
+				if (!strcmp(args[0].from, hash_algo_name[i])) {
+					opt->hash = i;
+					break;
+				}
+			}
+			res = tpm_is_tpm2(TPM_ANY_NUM);
+			if (res < 0)
+				return res;
+			if (i == HASH_ALGO__LAST ||
+			    (!res && i != HASH_ALGO_SHA1))
+				return -EINVAL;
+			break;
 		default:
 			return -EINVAL;
 		}