[PATCH 0/2] TPM 2.0 trusted key features for v4.5

[PATCH 0/2] TPM 2.0 trusted key features for v4.5

[Jarkko Sakkinen]

These are the remaining features to enable trusted keys for TPM 2.0 that very
not finished by the v4.4 merge window. These patches enable authorization
policy based sealing (like using PCRs together with a password for example or
something more complicated) with a user selected hash algorithm.

Jarkko Sakkinen (2):
  keys, trusted: select hash algorithm for TPM2 chips
  keys, trusted: seal with a policy

 Documentation/security/keys-trusted-encrypted.txt | 31 ++++++----
 crypto/hash_info.c                                |  2 +
 drivers/char/tpm/tpm.h                            | 10 +++-
 drivers/char/tpm/tpm2-cmd.c                       | 60 ++++++++++++++++---
 include/crypto/hash_info.h                        |  3 +
 include/keys/trusted-type.h                       |  4 ++
 include/uapi/linux/hash_info.h                    |  1 +
 security/keys/Kconfig                             |  1 +
 security/keys/trusted.c                           | 73 ++++++++++++++++++++++-
 9 files changed, 161 insertions(+), 24 deletions(-)

-- 
2.5.0

[PATCH 1/2] keys, trusted: select hash algorithm for TPM2 chips

[Jarkko Sakkinen]

Added 'hash=' option for selecting the hash algorithm for add_key()
syscall and documentation for it.

Added entry for sm3-256 to the following tables in order to support
TPM_ALG_SM3_256:

* hash_algo_name
* hash_digest_size

Includes support for the following hash algorithms:

* sha1
* sha256
* sha384
* sha512
* sm3-256

v2:

* Added missing select CRYPTO_HASH_INFO to security/keys/Kconfig

v3:

* Squashed patches into a single patch as the commits did not make
  alone any sense.
* Added a klog message when TPM 1.x is used for sealing and other than
  SHA-1 is used as the hash algorithm.
* Got rid of TPM2_HASH_COUNT and moved into ARRAY_SIZE(tpm2_hash_map).

v4:

* Added missing select CRYPTO_HASH_INFO to drivers/char/tpm/Kconfig

v5:

* Minor clean ups.
* Removed dev_dbg() from tpm2-cmd.c in order to get rid of
  CRYPTO_HASH_INFO dep.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Reviewed-by: James Morris <james.l.morris@oracle.com>
---
 Documentation/security/keys-trusted-encrypted.txt |  3 ++
 crypto/hash_info.c                                |  2 ++
 drivers/char/tpm/tpm.h                            | 10 +++++--
 drivers/char/tpm/tpm2-cmd.c                       | 36 +++++++++++++++++++++--
 include/crypto/hash_info.h                        |  3 ++
 include/keys/trusted-type.h                       |  1 +
 include/uapi/linux/hash_info.h                    |  1 +
 security/keys/Kconfig                             |  1 +
 security/keys/trusted.c                           | 27 ++++++++++++++++-
 9 files changed, 77 insertions(+), 7 deletions(-)

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/crypto/hash_info.c b/crypto/hash_info.c
index 3e7ff46..7b1e0b1 100644
--- a/crypto/hash_info.c
+++ b/crypto/hash_info.c
@@ -31,6 +31,7 @@ const char *const hash_algo_name[HASH_ALGO__LAST] = {
 	[HASH_ALGO_TGR_128]	= "tgr128",
 	[HASH_ALGO_TGR_160]	= "tgr160",
 	[HASH_ALGO_TGR_192]	= "tgr192",
+	[HASH_ALGO_SM3_256]	= "sm3-256",
 };
 EXPORT_SYMBOL_GPL(hash_algo_name);
 
@@ -52,5 +53,6 @@ const int hash_digest_size[HASH_ALGO__LAST] = {
 	[HASH_ALGO_TGR_128]	= TGR128_DIGEST_SIZE,
 	[HASH_ALGO_TGR_160]	= TGR160_DIGEST_SIZE,
 	[HASH_ALGO_TGR_192]	= TGR192_DIGEST_SIZE,
+	[HASH_ALGO_SM3_256]	= SM3256_DIGEST_SIZE,
 };
 EXPORT_SYMBOL_GPL(hash_digest_size);
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index a4257a3..cdd49cd 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -83,16 +83,20 @@ enum tpm2_structures {
 };
 
 enum tpm2_return_codes {
-	TPM2_RC_INITIALIZE	= 0x0100,
-	TPM2_RC_TESTING		= 0x090A,
+	TPM2_RC_HASH		= 0x0083, /* RC_FMT1 */
+	TPM2_RC_INITIALIZE	= 0x0100, /* RC_VER1 */
 	TPM2_RC_DISABLED	= 0x0120,
+	TPM2_RC_TESTING		= 0x090A, /* RC_WARN */
 };
 
 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 c121304..d9d0822 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -16,6 +16,7 @@
  */
 
 #include "tpm.h"
+#include <crypto/hash_info.h>
 #include <keys/trusted-type.h>
 
 enum tpm2_object_attributes {
@@ -104,6 +105,19 @@ struct tpm2_cmd {
 	union tpm2_cmd_params	params;
 } __packed;
 
+struct tpm2_hash {
+	unsigned int crypto_id;
+	unsigned int tpm_id;
+};
+
+static struct tpm2_hash tpm2_hash_map[] = {
+	{HASH_ALGO_SHA1, TPM2_ALG_SHA1},
+	{HASH_ALGO_SHA256, TPM2_ALG_SHA256},
+	{HASH_ALGO_SHA384, TPM2_ALG_SHA384},
+	{HASH_ALGO_SHA512, TPM2_ALG_SHA512},
+	{HASH_ALGO_SM3_256, TPM2_ALG_SM3_256},
+};
+
 /*
  * Array with one entry per ordinal defining the maximum amount
  * of time the chip could take to return the result. The values
@@ -429,8 +443,20 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
 {
 	unsigned int blob_len;
 	struct tpm_buf buf;
+	u32 hash;
+	int i;
 	int rc;
 
+	for (i = 0; i < ARRAY_SIZE(tpm2_hash_map); i++) {
+		if (options->hash == tpm2_hash_map[i].crypto_id) {
+			hash = tpm2_hash_map[i].tpm_id;
+			break;
+		}
+	}
+
+	if (i == ARRAY_SIZE(tpm2_hash_map))
+		return -EINVAL;
+
 	rc = tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_CREATE);
 	if (rc)
 		return rc;
@@ -455,7 +481,7 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
 	tpm_buf_append_u16(&buf, 14);
 
 	tpm_buf_append_u16(&buf, TPM2_ALG_KEYEDHASH);
-	tpm_buf_append_u16(&buf, TPM2_ALG_SHA256);
+	tpm_buf_append_u16(&buf, hash);
 	tpm_buf_append_u32(&buf, TPM2_ATTR_USER_WITH_AUTH);
 	tpm_buf_append_u16(&buf, 0); /* policy digest size */
 	tpm_buf_append_u16(&buf, TPM2_ALG_NULL);
@@ -488,8 +514,12 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
 out:
 	tpm_buf_destroy(&buf);
 
-	if (rc > 0)
-		rc = -EPERM;
+	if (rc > 0) {
+		if ((rc & TPM2_RC_HASH) == TPM2_RC_HASH)
+			rc = -EINVAL;
+		else
+			rc = -EPERM;
+	}
 
 	return rc;
 }
diff --git a/include/crypto/hash_info.h b/include/crypto/hash_info.h
index e1e5a3e..56f217d 100644
--- a/include/crypto/hash_info.h
+++ b/include/crypto/hash_info.h
@@ -34,6 +34,9 @@
 #define TGR160_DIGEST_SIZE 20
 #define TGR192_DIGEST_SIZE 24
 
+/* not defined in include/crypto/ */
+#define SM3256_DIGEST_SIZE 32
+
 extern const char *const hash_algo_name[HASH_ALGO__LAST];
 extern const int hash_digest_size[HASH_ALGO__LAST];
 
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/include/uapi/linux/hash_info.h b/include/uapi/linux/hash_info.h
index ca18c45..ebf8fd8 100644
--- a/include/uapi/linux/hash_info.h
+++ b/include/uapi/linux/hash_info.h
@@ -31,6 +31,7 @@ enum hash_algo {
 	HASH_ALGO_TGR_128,
 	HASH_ALGO_TGR_160,
 	HASH_ALGO_TGR_192,
+	HASH_ALGO_SM3_256,
 	HASH_ALGO__LAST
 };
 
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 903dace..b5b0a55 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,14 @@ static int getoptions(char *c, struct trusted_key_payload *pay,
 	int res;
 	unsigned long handle;
 	unsigned long lock;
+	int i;
+	int tpm2;
+
+	tpm2 = tpm_is_tpm2(TPM_ANY_NUM);
+	if (tpm2 < 0)
+		return tpm2;
+
+	opt->hash = tpm2 ? HASH_ALGO_SHA256 : HASH_ALGO_SHA1;
 
 	while ((p = strsep(&c, " \t"))) {
 		if (*p == '\0' || *p == ' ' || *p == '\t')
@@ -787,6 +798,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;
+				}
+			}
+			if (i == HASH_ALGO__LAST)
+				return -EINVAL;
+			if  (!tpm2 && i != HASH_ALGO_SHA1) {
+				pr_info("trusted_key: TPM 1.x only supports SHA-1.\n");
+				return -EINVAL;
+			}
+			break;
 		default:
 			return -EINVAL;
 		}
-- 
2.5.0

[PATCH 2/2] keys, trusted: seal with a policy

[Jarkko Sakkinen]

Support for sealing with a authorization policy.

Two new options for trusted keys:

* 'policydigest=': provide an auth policy digest for sealing.
* 'policyhandle=': provide a policy session handle for unsealing.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 Documentation/security/keys-trusted-encrypted.txt | 34 ++++++++++-------
 drivers/char/tpm/tpm2-cmd.c                       | 24 ++++++++++--
 include/keys/trusted-type.h                       |  3 ++
 security/keys/trusted.c                           | 46 ++++++++++++++++++++++-
 4 files changed, 87 insertions(+), 20 deletions(-)

diff --git a/Documentation/security/keys-trusted-encrypted.txt b/Documentation/security/keys-trusted-encrypted.txt
index fd2565b..324ddf5 100644
--- a/Documentation/security/keys-trusted-encrypted.txt
+++ b/Documentation/security/keys-trusted-encrypted.txt
@@ -27,20 +27,26 @@ Usage:
     keyctl print keyid
 
     options:
-       keyhandle= ascii hex value of sealing key default 0x40000000 (SRK)
-       keyauth=	  ascii hex auth for sealing key default 0x00...i
-		  (40 ascii zeros)
-       blobauth=  ascii hex auth for sealed data default 0x00...
-		  (40 ascii zeros)
-       blobauth=  ascii hex auth for sealed data default 0x00...
-		  (40 ascii zeros)
-       pcrinfo=	  ascii hex of PCR_INFO or PCR_INFO_LONG (no default)
-       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.
+       keyhandle=    ascii hex value of sealing key default 0x40000000 (SRK)
+       keyauth=	     ascii hex auth for sealing key default 0x00...i
+                     (40 ascii zeros)
+       blobauth=     ascii hex auth for sealed data default 0x00...
+                     (40 ascii zeros)
+       blobauth=     ascii hex auth for sealed data default 0x00...
+                     (40 ascii zeros)
+       pcrinfo=	     ascii hex of PCR_INFO or PCR_INFO_LONG (no default)
+       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.
+       policydigest= digest for the authorization policy. must be calculated
+                     with the same hash algorithm as specified by the 'hash='
+                     option.
+       policyhandle= handle to an authorization policy session that defines the
+                     same policy and with the same hash algorithm as was used to
+                     seal the key.
 
 "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/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index d9d0822..45a6340 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -478,12 +478,26 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
 	tpm_buf_append_u8(&buf, payload->migratable);
 
 	/* public */
-	tpm_buf_append_u16(&buf, 14);
+	if (options->policydigest)
+		tpm_buf_append_u16(&buf, 14 + options->digest_len);
+	else
+		tpm_buf_append_u16(&buf, 14);
 
 	tpm_buf_append_u16(&buf, TPM2_ALG_KEYEDHASH);
 	tpm_buf_append_u16(&buf, hash);
-	tpm_buf_append_u32(&buf, TPM2_ATTR_USER_WITH_AUTH);
-	tpm_buf_append_u16(&buf, 0); /* policy digest size */
+
+	/* policy */
+	if (options->policydigest) {
+		tpm_buf_append_u32(&buf, 0);
+		tpm_buf_append_u16(&buf, options->digest_len);
+		tpm_buf_append(&buf, options->policydigest,
+			       options->digest_len);
+	} else {
+		tpm_buf_append_u32(&buf, TPM2_ATTR_USER_WITH_AUTH);
+		tpm_buf_append_u16(&buf, 0);
+	}
+
+	/* public parameters */
 	tpm_buf_append_u16(&buf, TPM2_ALG_NULL);
 	tpm_buf_append_u16(&buf, 0);
 
@@ -613,7 +627,9 @@ static int tpm2_unseal(struct tpm_chip *chip,
 		return rc;
 
 	tpm_buf_append_u32(&buf, blob_handle);
-	tpm2_buf_append_auth(&buf, TPM2_RS_PW,
+	tpm2_buf_append_auth(&buf,
+			     options->policyhandle ?
+			     options->policyhandle : TPM2_RS_PW,
 			     NULL /* nonce */, 0,
 			     0 /* session_attributes */,
 			     options->blobauth /* hmac */,
diff --git a/include/keys/trusted-type.h b/include/keys/trusted-type.h
index a6a1008..2c3f9f7 100644
--- a/include/keys/trusted-type.h
+++ b/include/keys/trusted-type.h
@@ -37,6 +37,9 @@ struct trusted_key_options {
 	unsigned char pcrinfo[MAX_PCRINFO_SIZE];
 	int pcrlock;
 	uint32_t hash;
+	uint32_t digest_len;
+	unsigned char *policydigest;
+	uint32_t policyhandle;
 };
 
 extern struct key_type key_type_trusted;
diff --git a/security/keys/trusted.c b/security/keys/trusted.c
index b5b0a55..b726a83 100644
--- a/security/keys/trusted.c
+++ b/security/keys/trusted.c
@@ -713,6 +713,8 @@ enum {
 	Opt_keyhandle, Opt_keyauth, Opt_blobauth,
 	Opt_pcrinfo, Opt_pcrlock, Opt_migratable,
 	Opt_hash,
+	Opt_policydigest,
+	Opt_policyhandle,
 };
 
 static const match_table_t key_tokens = {
@@ -726,6 +728,8 @@ static const match_table_t key_tokens = {
 	{Opt_pcrlock, "pcrlock=%s"},
 	{Opt_migratable, "migratable=%s"},
 	{Opt_hash, "hash=%s"},
+	{Opt_policydigest, "policydigest=%s"},
+	{Opt_policyhandle, "policyhandle=%s"},
 	{Opt_err, NULL}
 };
 
@@ -739,6 +743,7 @@ static int getoptions(char *c, struct trusted_key_payload *pay,
 	int res;
 	unsigned long handle;
 	unsigned long lock;
+	unsigned int policydigest_len;
 	int i;
 	int tpm2;
 
@@ -747,6 +752,8 @@ static int getoptions(char *c, struct trusted_key_payload *pay,
 		return tpm2;
 
 	opt->hash = tpm2 ? HASH_ALGO_SHA256 : HASH_ALGO_SHA1;
+	opt->digest_len = hash_digest_size[opt->hash];
+	policydigest_len = opt->digest_len;
 
 	while ((p = strsep(&c, " \t"))) {
 		if (*p == '\0' || *p == ' ' || *p == '\t')
@@ -802,6 +809,8 @@ static int getoptions(char *c, struct trusted_key_payload *pay,
 			for (i = 0; i < HASH_ALGO__LAST; i++) {
 				if (!strcmp(args[0].from, hash_algo_name[i])) {
 					opt->hash = i;
+					opt->digest_len =
+						hash_digest_size[opt->hash];
 					break;
 				}
 			}
@@ -812,10 +821,37 @@ static int getoptions(char *c, struct trusted_key_payload *pay,
 				return -EINVAL;
 			}
 			break;
+		case Opt_policydigest:
+			if (!tpm2 ||
+			    strlen(args[0].from) != (2 * opt->digest_len))
+				return -EINVAL;
+			kfree(opt->policydigest);
+			opt->policydigest = kzalloc(opt->digest_len,
+						    GFP_KERNEL);
+			if (!opt->policydigest)
+				return -ENOMEM;
+			res = hex2bin(opt->policydigest, args[0].from,
+				      opt->digest_len);
+			if (res < 0)
+				return -EINVAL;
+			policydigest_len = opt->digest_len;
+			break;
+		case Opt_policyhandle:
+			if (!tpm2)
+				return -EINVAL;
+			res = kstrtoul(args[0].from, 16, &handle);
+			if (res < 0)
+				return -EINVAL;
+			opt->policyhandle = handle;
+			break;
 		default:
 			return -EINVAL;
 		}
 	}
+
+	if (opt->policydigest && policydigest_len != opt->digest_len)
+		return -EINVAL;
+
 	return 0;
 }
 
@@ -904,6 +940,12 @@ static struct trusted_key_options *trusted_options_alloc(void)
 	return options;
 }
 
+static void trusted_options_free(struct trusted_key_options *options)
+{
+	kfree(options->policydigest);
+	kfree(options);
+}
+
 static struct trusted_key_payload *trusted_payload_alloc(struct key *key)
 {
 	struct trusted_key_payload *p = NULL;
@@ -1010,7 +1052,7 @@ static int trusted_instantiate(struct key *key,
 		ret = pcrlock(options->pcrlock);
 out:
 	kfree(datablob);
-	kfree(options);
+	trusted_options_free(options);
 	if (!ret)
 		rcu_assign_keypointer(key, payload);
 	else
@@ -1098,7 +1140,7 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
 	call_rcu(&p->rcu, trusted_rcu_free);
 out:
 	kfree(datablob);
-	kfree(new_o);
+	trusted_options_free(new_o);
 	return ret;
 }
 
-- 
2.5.0

Re: [PATCH 2/2] keys, trusted: seal with a policy

[James Morris]

On Tue, 17 Nov 2015, Jarkko Sakkinen wrote:

>  			}
>  			break;
> +		case Opt_policydigest:
> +			if (!tpm2 ||
> +			    strlen(args[0].from) != (2 * opt->digest_len))
> +				return -EINVAL;
> +			kfree(opt->policydigest);
> +			opt->policydigest = kzalloc(opt->digest_len,
> +						    GFP_KERNEL);

Is it correct to kfree opt->policydigest here before allocating it?


> +			if (!opt->policydigest)
> +				return -ENOMEM;
> +			res = hex2bin(opt->policydigest, args[0].from,
> +				      opt->digest_len);
> +			if (res < 0)
> +				return -EINVAL;

Do you need to kfree it here on error?

-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 2/2] keys, trusted: seal with a policy

[Jarkko Sakkinen]

On Wed, Nov 18, 2015 at 11:21:01AM +1100, James Morris wrote:
> On Tue, 17 Nov 2015, Jarkko Sakkinen wrote:
> 
> >  			}
> >  			break;
> > +		case Opt_policydigest:
> > +			if (!tpm2 ||
> > +			    strlen(args[0].from) != (2 * opt->digest_len))
> > +				return -EINVAL;
> > +			kfree(opt->policydigest);
> > +			opt->policydigest = kzalloc(opt->digest_len,
> > +						    GFP_KERNEL);
> 
> Is it correct to kfree opt->policydigest here before allocating it?

I think so. The same option might be encountered multiple times.

I don't have the check for nulliy because opt is kzalloc'd and
checkpatch.pl complained that

WARNING: kfree(NULL) is safe and this check is probably not required
#20: FILE: security/keys/trusted.c:829:
+			if (opt->policydigest)
+				kfree(opt->policydigest);

> > +			if (!opt->policydigest)
> > +				return -ENOMEM;
> > +			res = hex2bin(opt->policydigest, args[0].from,
> > +				      opt->digest_len);
> > +			if (res < 0)
> > +				return -EINVAL;
> 
> Do you need to kfree it here on error?

trusted_options_free() will kfree it.

> -- 
> James Morris
> <jmorris@namei.org>

/Jarkko

RE: [tpmdd-devel] [PATCH 2/2] keys, trusted: seal with a policy

[Fuchs, Andreas]

> ________________________________________
> From: Jarkko Sakkinen [jarkko.sakkinen@linux.intel.com]
> Sent: Tuesday, November 17, 2015 17:27
> 
> Support for sealing with a authorization policy.
> 
> Two new options for trusted keys:
> 
> * 'policydigest=': provide an auth policy digest for sealing.
> * 'policyhandle=': provide a policy session handle for unsealing.

Hi Jarkko,

just out of curiosity; when testing this, how did you calculate the blobauth parameter ?
Since its calculation requires the cpHash for the unseal()-command...
If you "predict" the cpHash in userSpace, this would mean that userspace needs to know the
kernels way of constructing the unseal()-command to the TPM, which in turn would make
this part of the ABI and require documentation before upstreaming, imho.

Cheers,
Andreas

Re: [PATCH 2/2] keys, trusted: seal with a policy

[James Morris]

On Wed, 18 Nov 2015, Jarkko Sakkinen wrote:

> On Wed, Nov 18, 2015 at 11:21:01AM +1100, James Morris wrote:
> > On Tue, 17 Nov 2015, Jarkko Sakkinen wrote:
> > 
> > >  			}
> > >  			break;
> > > +		case Opt_policydigest:
> > > +			if (!tpm2 ||
> > > +			    strlen(args[0].from) != (2 * opt->digest_len))
> > > +				return -EINVAL;
> > > +			kfree(opt->policydigest);
> > > +			opt->policydigest = kzalloc(opt->digest_len,
> > > +						    GFP_KERNEL);
> > 
> > Is it correct to kfree opt->policydigest here before allocating it?
> 
> I think so. The same option might be encountered multiple times.

This would surely signify an error?


-- 
James Morris
<jmorris@namei.org>

Re: [tpmdd-devel] [PATCH 2/2] keys, trusted: seal with a policy

[Jarkko Sakkinen]

On Thu, Nov 19, 2015 at 10:59:57AM +0000, Fuchs, Andreas wrote:
> > ________________________________________
> > From: Jarkko Sakkinen [jarkko.sakkinen@linux.intel.com]
> > Sent: Tuesday, November 17, 2015 17:27
> > 
> > Support for sealing with a authorization policy.
> > 
> > Two new options for trusted keys:
> > 
> > * 'policydigest=': provide an auth policy digest for sealing.
> > * 'policyhandle=': provide a policy session handle for unsealing.
> 
> Hi Jarkko,
> 
> just out of curiosity; when testing this, how did you calculate the blobauth parameter ?
> Since its calculation requires the cpHash for the unseal()-command...
> If you "predict" the cpHash in userSpace, this would mean that userspace needs to know the
> kernels way of constructing the unseal()-command to the TPM, which in turn would make
> this part of the ABI and require documentation before upstreaming, imho.

Is this a comment about the patch? Have you actually read the source
code or where is this coming from? Please read the source code.

> Cheers,
> Andreas--

/Jarkko

Re: [PATCH 2/2] keys, trusted: seal with a policy

[Jarkko Sakkinen]

On Tue, Nov 17, 2015 at 06:27:22PM +0200, Jarkko Sakkinen wrote:
> Support for sealing with a authorization policy.
> 
> Two new options for trusted keys:
> 
> * 'policydigest=': provide an auth policy digest for sealing.
> * 'policyhandle=': provide a policy session handle for unsealing.

I think it is good to say a word about how to test this since the user
space supports is still lagging a bit (there's no way to do a "sticky"
handle in TSS2 yet).

I have my own low-level test scripts over here:

https://github.com/jsakkine/tpm2-scripts

Trivial example:

KEYHANDLE=$(sudo ./tpm2-root-key)
POLICYDIGEST=$(sudo ./tpm2-pcr-policy --pcr 16 --name-alg=sha256 --bank=sha1 --trial)
POLICYHANDLE=$(sudo ./tpm2-pcr-policy --pcr 16 --name-alg=sha256 --bank=sha1)

KEYID=$(keyctl add trusted kmk "new 32 keyhandle=$KEYHANDLE hash=sha256 policydigest=$POLICYDIGEST" @u)
keyctl pipe $KEYID
keyctl clear @u
keyctl add trusted kmk "load `cat blob.hex` keyhandle=$KEYHANDLE  policyhandle=0x03000000" @u
keyctl clear @u

sudo ./tpm2-flush $KEYHANDLE

/Jarkko

Re: [PATCH 2/2] keys, trusted: seal with a policy

[Jarkko Sakkinen]

On Tue, Nov 17, 2015 at 06:27:22PM +0200, Jarkko Sakkinen wrote:
> Support for sealing with a authorization policy.
> 
> Two new options for trusted keys:
> 
> * 'policydigest=': provide an auth policy digest for sealing.
> * 'policyhandle=': provide a policy session handle for unsealing.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

This patch has been now peer tested by Colin Ian King. There's still one
thing that I'm thinking before daring to put this into pull request.
Should the option names reflect that they associate to the blob and not
to the root key?

My *guess* would be that it's not very common use case to seal primary
keys with policies (PCRs and so forth) and therefore I think these
names are fine.

[1] In TPM 2.0 there is no fixed root key in the chip. Instead tit
contains random seeds from which you can derive primary keys by using
the TPM2_CreatePrimary command. That's why we require for example
keyhandle as an explicit option when used with a TPM 2.0 chip.

/Jarkko

> ---
>  Documentation/security/keys-trusted-encrypted.txt | 34 ++++++++++-------
>  drivers/char/tpm/tpm2-cmd.c                       | 24 ++++++++++--
>  include/keys/trusted-type.h                       |  3 ++
>  security/keys/trusted.c                           | 46 ++++++++++++++++++++++-
>  4 files changed, 87 insertions(+), 20 deletions(-)
> 
> diff --git a/Documentation/security/keys-trusted-encrypted.txt b/Documentation/security/keys-trusted-encrypted.txt
> index fd2565b..324ddf5 100644
> --- a/Documentation/security/keys-trusted-encrypted.txt
> +++ b/Documentation/security/keys-trusted-encrypted.txt
> @@ -27,20 +27,26 @@ Usage:
>      keyctl print keyid
>  
>      options:
> -       keyhandle= ascii hex value of sealing key default 0x40000000 (SRK)
> -       keyauth=	  ascii hex auth for sealing key default 0x00...i
> -		  (40 ascii zeros)
> -       blobauth=  ascii hex auth for sealed data default 0x00...
> -		  (40 ascii zeros)
> -       blobauth=  ascii hex auth for sealed data default 0x00...
> -		  (40 ascii zeros)
> -       pcrinfo=	  ascii hex of PCR_INFO or PCR_INFO_LONG (no default)
> -       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.
> +       keyhandle=    ascii hex value of sealing key default 0x40000000 (SRK)
> +       keyauth=	     ascii hex auth for sealing key default 0x00...i
> +                     (40 ascii zeros)
> +       blobauth=     ascii hex auth for sealed data default 0x00...
> +                     (40 ascii zeros)
> +       blobauth=     ascii hex auth for sealed data default 0x00...
> +                     (40 ascii zeros)
> +       pcrinfo=	     ascii hex of PCR_INFO or PCR_INFO_LONG (no default)
> +       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.
> +       policydigest= digest for the authorization policy. must be calculated
> +                     with the same hash algorithm as specified by the 'hash='
> +                     option.
> +       policyhandle= handle to an authorization policy session that defines the
> +                     same policy and with the same hash algorithm as was used to
> +                     seal the key.
>  
>  "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/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index d9d0822..45a6340 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -478,12 +478,26 @@ int tpm2_seal_trusted(struct tpm_chip *chip,
>  	tpm_buf_append_u8(&buf, payload->migratable);
>  
>  	/* public */
> -	tpm_buf_append_u16(&buf, 14);
> +	if (options->policydigest)
> +		tpm_buf_append_u16(&buf, 14 + options->digest_len);
> +	else
> +		tpm_buf_append_u16(&buf, 14);
>  
>  	tpm_buf_append_u16(&buf, TPM2_ALG_KEYEDHASH);
>  	tpm_buf_append_u16(&buf, hash);
> -	tpm_buf_append_u32(&buf, TPM2_ATTR_USER_WITH_AUTH);
> -	tpm_buf_append_u16(&buf, 0); /* policy digest size */
> +
> +	/* policy */
> +	if (options->policydigest) {
> +		tpm_buf_append_u32(&buf, 0);
> +		tpm_buf_append_u16(&buf, options->digest_len);
> +		tpm_buf_append(&buf, options->policydigest,
> +			       options->digest_len);
> +	} else {
> +		tpm_buf_append_u32(&buf, TPM2_ATTR_USER_WITH_AUTH);
> +		tpm_buf_append_u16(&buf, 0);
> +	}
> +
> +	/* public parameters */
>  	tpm_buf_append_u16(&buf, TPM2_ALG_NULL);
>  	tpm_buf_append_u16(&buf, 0);
>  
> @@ -613,7 +627,9 @@ static int tpm2_unseal(struct tpm_chip *chip,
>  		return rc;
>  
>  	tpm_buf_append_u32(&buf, blob_handle);
> -	tpm2_buf_append_auth(&buf, TPM2_RS_PW,
> +	tpm2_buf_append_auth(&buf,
> +			     options->policyhandle ?
> +			     options->policyhandle : TPM2_RS_PW,
>  			     NULL /* nonce */, 0,
>  			     0 /* session_attributes */,
>  			     options->blobauth /* hmac */,
> diff --git a/include/keys/trusted-type.h b/include/keys/trusted-type.h
> index a6a1008..2c3f9f7 100644
> --- a/include/keys/trusted-type.h
> +++ b/include/keys/trusted-type.h
> @@ -37,6 +37,9 @@ struct trusted_key_options {
>  	unsigned char pcrinfo[MAX_PCRINFO_SIZE];
>  	int pcrlock;
>  	uint32_t hash;
> +	uint32_t digest_len;
> +	unsigned char *policydigest;
> +	uint32_t policyhandle;
>  };
>  
>  extern struct key_type key_type_trusted;
> diff --git a/security/keys/trusted.c b/security/keys/trusted.c
> index b5b0a55..b726a83 100644
> --- a/security/keys/trusted.c
> +++ b/security/keys/trusted.c
> @@ -713,6 +713,8 @@ enum {
>  	Opt_keyhandle, Opt_keyauth, Opt_blobauth,
>  	Opt_pcrinfo, Opt_pcrlock, Opt_migratable,
>  	Opt_hash,
> +	Opt_policydigest,
> +	Opt_policyhandle,
>  };
>  
>  static const match_table_t key_tokens = {
> @@ -726,6 +728,8 @@ static const match_table_t key_tokens = {
>  	{Opt_pcrlock, "pcrlock=%s"},
>  	{Opt_migratable, "migratable=%s"},
>  	{Opt_hash, "hash=%s"},
> +	{Opt_policydigest, "policydigest=%s"},
> +	{Opt_policyhandle, "policyhandle=%s"},
>  	{Opt_err, NULL}
>  };
>  
> @@ -739,6 +743,7 @@ static int getoptions(char *c, struct trusted_key_payload *pay,
>  	int res;
>  	unsigned long handle;
>  	unsigned long lock;
> +	unsigned int policydigest_len;
>  	int i;
>  	int tpm2;
>  
> @@ -747,6 +752,8 @@ static int getoptions(char *c, struct trusted_key_payload *pay,
>  		return tpm2;
>  
>  	opt->hash = tpm2 ? HASH_ALGO_SHA256 : HASH_ALGO_SHA1;
> +	opt->digest_len = hash_digest_size[opt->hash];
> +	policydigest_len = opt->digest_len;
>  
>  	while ((p = strsep(&c, " \t"))) {
>  		if (*p == '\0' || *p == ' ' || *p == '\t')
> @@ -802,6 +809,8 @@ static int getoptions(char *c, struct trusted_key_payload *pay,
>  			for (i = 0; i < HASH_ALGO__LAST; i++) {
>  				if (!strcmp(args[0].from, hash_algo_name[i])) {
>  					opt->hash = i;
> +					opt->digest_len =
> +						hash_digest_size[opt->hash];
>  					break;
>  				}
>  			}
> @@ -812,10 +821,37 @@ static int getoptions(char *c, struct trusted_key_payload *pay,
>  				return -EINVAL;
>  			}
>  			break;
> +		case Opt_policydigest:
> +			if (!tpm2 ||
> +			    strlen(args[0].from) != (2 * opt->digest_len))
> +				return -EINVAL;
> +			kfree(opt->policydigest);
> +			opt->policydigest = kzalloc(opt->digest_len,
> +						    GFP_KERNEL);
> +			if (!opt->policydigest)
> +				return -ENOMEM;
> +			res = hex2bin(opt->policydigest, args[0].from,
> +				      opt->digest_len);
> +			if (res < 0)
> +				return -EINVAL;
> +			policydigest_len = opt->digest_len;
> +			break;
> +		case Opt_policyhandle:
> +			if (!tpm2)
> +				return -EINVAL;
> +			res = kstrtoul(args[0].from, 16, &handle);
> +			if (res < 0)
> +				return -EINVAL;
> +			opt->policyhandle = handle;
> +			break;
>  		default:
>  			return -EINVAL;
>  		}
>  	}
> +
> +	if (opt->policydigest && policydigest_len != opt->digest_len)
> +		return -EINVAL;
> +
>  	return 0;
>  }
>  
> @@ -904,6 +940,12 @@ static struct trusted_key_options *trusted_options_alloc(void)
>  	return options;
>  }
>  
> +static void trusted_options_free(struct trusted_key_options *options)
> +{
> +	kfree(options->policydigest);
> +	kfree(options);
> +}
> +
>  static struct trusted_key_payload *trusted_payload_alloc(struct key *key)
>  {
>  	struct trusted_key_payload *p = NULL;
> @@ -1010,7 +1052,7 @@ static int trusted_instantiate(struct key *key,
>  		ret = pcrlock(options->pcrlock);
>  out:
>  	kfree(datablob);
> -	kfree(options);
> +	trusted_options_free(options);
>  	if (!ret)
>  		rcu_assign_keypointer(key, payload);
>  	else
> @@ -1098,7 +1140,7 @@ static int trusted_update(struct key *key, struct key_preparsed_payload *prep)
>  	call_rcu(&p->rcu, trusted_rcu_free);
>  out:
>  	kfree(datablob);
> -	kfree(new_o);
> +	trusted_options_free(new_o);
>  	return ret;
>  }
>  
> -- 
> 2.5.0
> 

Re: [PATCH 2/2] keys, trusted: seal with a policy

[Jarkko Sakkinen]

On Fri, Nov 20, 2015 at 01:34:35PM +1100, James Morris wrote:
> On Wed, 18 Nov 2015, Jarkko Sakkinen wrote:
> 
> > On Wed, Nov 18, 2015 at 11:21:01AM +1100, James Morris wrote:
> > > On Tue, 17 Nov 2015, Jarkko Sakkinen wrote:
> > > 
> > > >  			}
> > > >  			break;
> > > > +		case Opt_policydigest:
> > > > +			if (!tpm2 ||
> > > > +			    strlen(args[0].from) != (2 * opt->digest_len))
> > > > +				return -EINVAL;
> > > > +			kfree(opt->policydigest);
> > > > +			opt->policydigest = kzalloc(opt->digest_len,
> > > > +						    GFP_KERNEL);
> > > 
> > > Is it correct to kfree opt->policydigest here before allocating it?
> > 
> > I think so. The same option might be encountered multiple times.
> 
> This would surely signify an error?

I'm following the semantics of other options. That's why I implemented
it that way for example:

keyctl add trusted kmk "new 32 keyhandle=0x80000000 keyhandle=0x80000000"

is perfectly OK. I just thought that it'd be more odd if this option
behaved in a different way...

> -- 
> James Morris
> <jmorris@namei.org>

/Jarkko

Re: [PATCH 2/2] keys, trusted: seal with a policy

[James Morris]

On Mon, 7 Dec 2015, Jarkko Sakkinen wrote:

> On Fri, Nov 20, 2015 at 01:34:35PM +1100, James Morris wrote:
> > On Wed, 18 Nov 2015, Jarkko Sakkinen wrote:
> > 
> > > On Wed, Nov 18, 2015 at 11:21:01AM +1100, James Morris wrote:
> > > > On Tue, 17 Nov 2015, Jarkko Sakkinen wrote:
> > > > 
> > > > >  			}
> > > > >  			break;
> > > > > +		case Opt_policydigest:
> > > > > +			if (!tpm2 ||
> > > > > +			    strlen(args[0].from) != (2 * opt->digest_len))
> > > > > +				return -EINVAL;
> > > > > +			kfree(opt->policydigest);
> > > > > +			opt->policydigest = kzalloc(opt->digest_len,
> > > > > +						    GFP_KERNEL);
> > > > 
> > > > Is it correct to kfree opt->policydigest here before allocating it?
> > > 
> > > I think so. The same option might be encountered multiple times.
> > 
> > This would surely signify an error?
> 
> I'm following the semantics of other options. That's why I implemented
> it that way for example:
> 
> keyctl add trusted kmk "new 32 keyhandle=0x80000000 keyhandle=0x80000000"
> 
> is perfectly OK. I just thought that it'd be more odd if this option
> behaved in a different way...

It seems broken to me -- if you're messing up keyctl commands you might 
want to know about it, but we should remain consistent.


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 2/2] keys, trusted: seal with a policy

[Jarkko Sakkinen]

On Tue, Dec 08, 2015 at 09:35:05AM +1100, James Morris wrote:
> On Mon, 7 Dec 2015, Jarkko Sakkinen wrote:
> 
> > On Fri, Nov 20, 2015 at 01:34:35PM +1100, James Morris wrote:
> > > On Wed, 18 Nov 2015, Jarkko Sakkinen wrote:
> > > 
> > > > On Wed, Nov 18, 2015 at 11:21:01AM +1100, James Morris wrote:
> > > > > On Tue, 17 Nov 2015, Jarkko Sakkinen wrote:
> > > > > 
> > > > > >  			}
> > > > > >  			break;
> > > > > > +		case Opt_policydigest:
> > > > > > +			if (!tpm2 ||
> > > > > > +			    strlen(args[0].from) != (2 * opt->digest_len))
> > > > > > +				return -EINVAL;
> > > > > > +			kfree(opt->policydigest);
> > > > > > +			opt->policydigest = kzalloc(opt->digest_len,
> > > > > > +						    GFP_KERNEL);
> > > > > 
> > > > > Is it correct to kfree opt->policydigest here before allocating it?
> > > > 
> > > > I think so. The same option might be encountered multiple times.
> > > 
> > > This would surely signify an error?
> > 
> > I'm following the semantics of other options. That's why I implemented
> > it that way for example:
> > 
> > keyctl add trusted kmk "new 32 keyhandle=0x80000000 keyhandle=0x80000000"
> > 
> > is perfectly OK. I just thought that it'd be more odd if this option
> > behaved in a different way...
> 
> It seems broken to me -- if you're messing up keyctl commands you might 
> want to know about it, but we should remain consistent.

So should I return error if policyhandle/digest appears a second time? I
agree that it'd be better to return -EINVAL.

The existing behavior is such that any option can appear multiple times
and I chose to be consistent with that.

> -- 
> James Morris
> <jmorris@namei.org>

/Jarkko

Re: [PATCH 2/2] keys, trusted: seal with a policy

[Jarkko Sakkinen]

On Tue, Dec 08, 2015 at 01:01:02PM +0200, Jarkko Sakkinen wrote:
> On Tue, Dec 08, 2015 at 09:35:05AM +1100, James Morris wrote:
> > On Mon, 7 Dec 2015, Jarkko Sakkinen wrote:
> > 
> > > On Fri, Nov 20, 2015 at 01:34:35PM +1100, James Morris wrote:
> > > > On Wed, 18 Nov 2015, Jarkko Sakkinen wrote:
> > > > 
> > > > > On Wed, Nov 18, 2015 at 11:21:01AM +1100, James Morris wrote:
> > > > > > On Tue, 17 Nov 2015, Jarkko Sakkinen wrote:
> > > > > > 
> > > > > > >  			}
> > > > > > >  			break;
> > > > > > > +		case Opt_policydigest:
> > > > > > > +			if (!tpm2 ||
> > > > > > > +			    strlen(args[0].from) != (2 * opt->digest_len))
> > > > > > > +				return -EINVAL;
> > > > > > > +			kfree(opt->policydigest);
> > > > > > > +			opt->policydigest = kzalloc(opt->digest_len,
> > > > > > > +						    GFP_KERNEL);
> > > > > > 
> > > > > > Is it correct to kfree opt->policydigest here before allocating it?
> > > > > 
> > > > > I think so. The same option might be encountered multiple times.
> > > > 
> > > > This would surely signify an error?
> > > 
> > > I'm following the semantics of other options. That's why I implemented
> > > it that way for example:
> > > 
> > > keyctl add trusted kmk "new 32 keyhandle=0x80000000 keyhandle=0x80000000"
> > > 
> > > is perfectly OK. I just thought that it'd be more odd if this option
> > > behaved in a different way...
> > 
> > It seems broken to me -- if you're messing up keyctl commands you might 
> > want to know about it, but we should remain consistent.
> 
> So should I return error if policyhandle/digest appears a second time? I
> agree that it'd be better to return -EINVAL.
> 
> The existing behavior is such that any option can appear multiple times
> and I chose to be consistent with that.

Mimi, David?

/Jarkko

Re: [PATCH 2/2] keys, trusted: seal with a policy

[Mimi Zohar]

On Tue, 2015-12-08 at 22:24 +0200, Jarkko Sakkinen wrote:
> On Tue, Dec 08, 2015 at 01:01:02PM +0200, Jarkko Sakkinen wrote:
> > On Tue, Dec 08, 2015 at 09:35:05AM +1100, James Morris wrote:
> > > On Mon, 7 Dec 2015, Jarkko Sakkinen wrote:
> > > 
> > > > On Fri, Nov 20, 2015 at 01:34:35PM +1100, James Morris wrote:
> > > > > On Wed, 18 Nov 2015, Jarkko Sakkinen wrote:
> > > > > 
> > > > > > On Wed, Nov 18, 2015 at 11:21:01AM +1100, James Morris wrote:
> > > > > > > On Tue, 17 Nov 2015, Jarkko Sakkinen wrote:
> > > > > > > 
> > > > > > > >  			}
> > > > > > > >  			break;
> > > > > > > > +		case Opt_policydigest:
> > > > > > > > +			if (!tpm2 ||
> > > > > > > > +			    strlen(args[0].from) != (2 * opt->digest_len))
> > > > > > > > +				return -EINVAL;
> > > > > > > > +			kfree(opt->policydigest);
> > > > > > > > +			opt->policydigest = kzalloc(opt->digest_len,
> > > > > > > > +						    GFP_KERNEL);

You're allocating the exact amount of storage needed.  There's no reason
to use kzalloc here or elsewhere in the patch.

> > > > > > > 
> > > > > > > Is it correct to kfree opt->policydigest here before allocating it?
> > > > > > 
> > > > > > I think so. The same option might be encountered multiple times.
> > > > > 
> > > > > This would surely signify an error?
> > > > 
> > > > I'm following the semantics of other options. That's why I implemented
> > > > it that way for example:
> > > > 
> > > > keyctl add trusted kmk "new 32 keyhandle=0x80000000 keyhandle=0x80000000"
> > > > 
> > > > is perfectly OK. I just thought that it'd be more odd if this option
> > > > behaved in a different way...
> > > 
> > > It seems broken to me -- if you're messing up keyctl commands you might 
> > > want to know about it, but we should remain consistent.
> > 
> > So should I return error if policyhandle/digest appears a second time? I
> > agree that it'd be better to return -EINVAL.
> > 
> > The existing behavior is such that any option can appear multiple times
> > and I chose to be consistent with that.
> 
> Mimi, David?

I don't have a problem with changing the existing behavior to allow the
options to be specified only once.

BTW, you might want to fail the getoptions() parsing earlier, rather
than waiting until after the while loop to test "policydigest_len !=
opt->digest_len".   In both Opt_hash and Opt_policydigest you can check
to see if the other option has already been specified.

Mimi

Re: [PATCH 2/2] keys, trusted: seal with a policy

[Jarkko Sakkinen]

On Tue, Dec 08, 2015 at 06:56:17PM -0500, Mimi Zohar wrote:
> On Tue, 2015-12-08 at 22:24 +0200, Jarkko Sakkinen wrote:
> > On Tue, Dec 08, 2015 at 01:01:02PM +0200, Jarkko Sakkinen wrote:
> > > On Tue, Dec 08, 2015 at 09:35:05AM +1100, James Morris wrote:
> > > > On Mon, 7 Dec 2015, Jarkko Sakkinen wrote:
> > > > 
> > > > > On Fri, Nov 20, 2015 at 01:34:35PM +1100, James Morris wrote:
> > > > > > On Wed, 18 Nov 2015, Jarkko Sakkinen wrote:
> > > > > > 
> > > > > > > On Wed, Nov 18, 2015 at 11:21:01AM +1100, James Morris wrote:
> > > > > > > > On Tue, 17 Nov 2015, Jarkko Sakkinen wrote:
> > > > > > > > 
> > > > > > > > >  			}
> > > > > > > > >  			break;
> > > > > > > > > +		case Opt_policydigest:
> > > > > > > > > +			if (!tpm2 ||
> > > > > > > > > +			    strlen(args[0].from) != (2 * opt->digest_len))
> > > > > > > > > +				return -EINVAL;
> > > > > > > > > +			kfree(opt->policydigest);
> > > > > > > > > +			opt->policydigest = kzalloc(opt->digest_len,
> > > > > > > > > +						    GFP_KERNEL);
> 
> You're allocating the exact amount of storage needed.  There's no reason
> to use kzalloc here or elsewhere in the patch.

Yup. I'll change this.

> > > > > > > > 
> > > > > > > > Is it correct to kfree opt->policydigest here before allocating it?
> > > > > > > 
> > > > > > > I think so. The same option might be encountered multiple times.
> > > > > > 
> > > > > > This would surely signify an error?
> > > > > 
> > > > > I'm following the semantics of other options. That's why I implemented
> > > > > it that way for example:
> > > > > 
> > > > > keyctl add trusted kmk "new 32 keyhandle=0x80000000 keyhandle=0x80000000"
> > > > > 
> > > > > is perfectly OK. I just thought that it'd be more odd if this option
> > > > > behaved in a different way...
> > > > 
> > > > It seems broken to me -- if you're messing up keyctl commands you might 
> > > > want to know about it, but we should remain consistent.
> > > 
> > > So should I return error if policyhandle/digest appears a second time? I
> > > agree that it'd be better to return -EINVAL.
> > > 
> > > The existing behavior is such that any option can appear multiple times
> > > and I chose to be consistent with that.
> > 
> > Mimi, David?
> 
> I don't have a problem with changing the existing behavior to allow the
> options to be specified only once.

I don't think this patch is right place to change the behavior as it
should be done for other options too.

> BTW, you might want to fail the getoptions() parsing earlier, rather
> than waiting until after the while loop to test "policydigest_len !=
> opt->digest_len".   In both Opt_hash and Opt_policydigest you can check
> to see if the other option has already been specified.

Agreed.

> Mimi

/Jarkko

Re: [PATCH 2/2] keys, trusted: seal with a policy

[Mimi Zohar]

On Wed, 2015-12-09 at 16:24 +0200, Jarkko Sakkinen wrote:
> On Tue, Dec 08, 2015 at 06:56:17PM -0500, Mimi Zohar wrote:
> > On Tue, 2015-12-08 at 22:24 +0200, Jarkko Sakkinen wrote:
> > > On Tue, Dec 08, 2015 at 01:01:02PM +0200, Jarkko Sakkinen wrote:
> > > > On Tue, Dec 08, 2015 at 09:35:05AM +1100, James Morris wrote:
> > > > > On Mon, 7 Dec 2015, Jarkko Sakkinen wrote:
> > > > > 
> > > > > > On Fri, Nov 20, 2015 at 01:34:35PM +1100, James Morris wrote:
> > > > > > > On Wed, 18 Nov 2015, Jarkko Sakkinen wrote:
> > > > > > > 
> > > > > > > > On Wed, Nov 18, 2015 at 11:21:01AM +1100, James Morris wrote:
> > > > > > > > > On Tue, 17 Nov 2015, Jarkko Sakkinen wrote:
> > > > > > > > > 
> > > > > > > > > >  			}
> > > > > > > > > >  			break;
> > > > > > > > > > +		case Opt_policydigest:
> > > > > > > > > > +			if (!tpm2 ||
> > > > > > > > > > +			    strlen(args[0].from) != (2 * opt->digest_len))
> > > > > > > > > > +				return -EINVAL;
> > > > > > > > > > +			kfree(opt->policydigest);
> > > > > > > > > > +			opt->policydigest = kzalloc(opt->digest_len,
> > > > > > > > > > +						    GFP_KERNEL);
> > 
> > You're allocating the exact amount of storage needed.  There's no reason
> > to use kzalloc here or elsewhere in the patch.
> 
> Yup. I'll change this.
> 
> > > > > > > > > 
> > > > > > > > > Is it correct to kfree opt->policydigest here before allocating it?
> > > > > > > > 
> > > > > > > > I think so. The same option might be encountered multiple times.
> > > > > > > 
> > > > > > > This would surely signify an error?
> > > > > > 
> > > > > > I'm following the semantics of other options. That's why I implemented
> > > > > > it that way for example:
> > > > > > 
> > > > > > keyctl add trusted kmk "new 32 keyhandle=0x80000000 keyhandle=0x80000000"
> > > > > > 
> > > > > > is perfectly OK. I just thought that it'd be more odd if this option
> > > > > > behaved in a different way...
> > > > > 
> > > > > It seems broken to me -- if you're messing up keyctl commands you might 
> > > > > want to know about it, but we should remain consistent.
> > > > 
> > > > So should I return error if policyhandle/digest appears a second time? I
> > > > agree that it'd be better to return -EINVAL.
> > > > 
> > > > The existing behavior is such that any option can appear multiple times
> > > > and I chose to be consistent with that.
> > > 
> > > Mimi, David?
> > 
> > I don't have a problem with changing the existing behavior to allow the
> > options to be specified only once.
> 
> I don't think this patch is right place to change the behavior as it
> should be done for other options too.

I think the easiest way of checking if a token has already been seen
would be to define
 a flag and use test_and_set_bit(token, <flag>) after the following code
snippet.

          while ((p = strsep(&c, " \t"))) {
                if (*p == '\0' || *p == ' ' || *p == '\t')
                        continue;
                token = match_token(p, key_tokens, args);

Having a separate patch is probably a good idea.

Mimi