[PATCH 0/4] Basic trusted keys support for TPM 2.0

[PATCH 0/4] Basic trusted keys support for TPM 2.0

[Jarkko Sakkinen]

Basic trusted keys support, which means basic sealing with an
authentication value by using SHA256. After we get the groundwork in
place the functionality will be refined with algorithmic agility and
policy based sealing.

Jarkko Sakkinen (4):
  tpm: introduce struct tpm_buf
  trusted: move struct trusted_key_options to trusted-type.h
  tpm: seal/unseal for TPM 2.0
  keys, trusted: seal/unseal with TPM 2.0 chips

 drivers/char/tpm/tpm-interface.c |  75 ++++++
 drivers/char/tpm/tpm.h           |  78 ++++++
 drivers/char/tpm/tpm2-cmd.c      | 495 +++++++++++++++++++++++----------------
 include/keys/trusted-type.h      |  15 +-
 include/linux/tpm.h              |  26 ++
 include/linux/tpm_command.h      |   1 -
 security/keys/trusted.c          |  18 +-
 security/keys/trusted.h          |  18 +-
 8 files changed, 504 insertions(+), 222 deletions(-)

-- 
2.5.0

[PATCH 1/4] tpm: introduce struct tpm_buf

[Jarkko Sakkinen]

This patch introduces struct tpm_buf that provides a string buffer for
constructing TPM commands. This allows to construct variable sized TPM
commands. This feature is needed for sealing and unsealing trusted keys
because the input data is of variable size.

The commands in tpm2-cmd.c have been updated to use struct tpm_buf.

The code is based in the string buffer code in security/trusted/trusted.h.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm.h      |  64 +++++++++
 drivers/char/tpm/tpm2-cmd.c | 317 +++++++++++++++-----------------------------
 2 files changed, 173 insertions(+), 208 deletions(-)

diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index f8319a0..f04afb7 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -382,6 +382,70 @@ struct tpm_cmd_t {
 	tpm_cmd_params	params;
 } __packed;
 
+/* A string buffer type for constructing TPM commands. This is based on the
+ * string buffer code in security/keys/trusted.h.
+ */
+
+#define TPM_BUF_SIZE 512
+
+struct tpm_buf {
+	u8 data[TPM_BUF_SIZE];
+} __aligned(8);
+
+static inline void tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
+{
+	struct tpm_input_header *head = (struct tpm_input_header *) buf->data;
+
+	head->tag = cpu_to_be16(tag);
+	head->length = cpu_to_be32(sizeof(*head));
+	head->ordinal = cpu_to_be32(ordinal);
+}
+
+static inline u32 tpm_buf_length(struct tpm_buf *buf)
+{
+	struct tpm_input_header *head = (struct tpm_input_header *) buf->data;
+
+	return be32_to_cpu(head->length);
+}
+
+static inline u16 tpm_buf_tag(struct tpm_buf *buf)
+{
+	struct tpm_input_header *head = (struct tpm_input_header *) buf->data;
+
+	return be16_to_cpu(head->tag);
+}
+
+static inline void tpm_buf_append(struct tpm_buf *buf,
+				  const unsigned char *data,
+				  unsigned int len)
+{
+	struct tpm_input_header *head = (struct tpm_input_header *) buf->data;
+
+	BUG_ON((len + tpm_buf_length(buf)) > TPM_BUF_SIZE);
+
+	memcpy(&buf->data[tpm_buf_length(buf)], data, len);
+	head->length = cpu_to_be32(tpm_buf_length(buf) + len);
+}
+
+static inline void tpm_buf_append_u8(struct tpm_buf *buf, const u8 value)
+{
+	tpm_buf_append(buf, &value, 1);
+}
+
+static inline void tpm_buf_append_u16(struct tpm_buf *buf, const u16 value)
+{
+	__be16 value2 = cpu_to_be16(value);
+
+	tpm_buf_append(buf, (u8 *) &value2, 2);
+}
+
+static inline void tpm_buf_append_u32(struct tpm_buf *buf, const u32 value)
+{
+	__be32 value2 = cpu_to_be32(value);
+
+	tpm_buf_append(buf, (u8 *) &value2, 4);
+}
+
 extern struct class *tpm_class;
 extern dev_t tpm_devt;
 extern const struct file_operations tpm_fops;
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 011909a..0d55674 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -17,14 +17,6 @@
 
 #include "tpm.h"
 
-struct tpm2_startup_in {
-	__be16	startup_type;
-} __packed;
-
-struct tpm2_self_test_in {
-	u8	full_test;
-} __packed;
-
 struct tpm2_pcr_read_in {
 	__be32	pcr_selects_cnt;
 	__be16	hash_alg;
@@ -43,17 +35,7 @@ struct tpm2_pcr_read_out {
 	u8	digest[TPM_DIGEST_SIZE];
 } __packed;
 
-struct tpm2_null_auth_area {
-	__be32			handle;
-	__be16			nonce_size;
-	u8			attributes;
-	__be16			auth_size;
-} __packed;
-
 struct tpm2_pcr_extend_in {
-	__be32				pcr_idx;
-	__be32				auth_area_size;
-	struct tpm2_null_auth_area	auth_area;
 	__be32				digest_cnt;
 	__be16				hash_alg;
 	u8				digest[TPM_DIGEST_SIZE];
@@ -73,32 +55,11 @@ struct tpm2_get_tpm_pt_out {
 	__be32	value;
 } __packed;
 
-struct tpm2_get_random_in {
-	__be16	size;
-} __packed;
-
 struct tpm2_get_random_out {
 	__be16	size;
 	u8	buffer[TPM_MAX_RNG_DATA];
 } __packed;
 
-union tpm2_cmd_params {
-	struct	tpm2_startup_in		startup_in;
-	struct	tpm2_self_test_in	selftest_in;
-	struct	tpm2_pcr_read_in	pcrread_in;
-	struct	tpm2_pcr_read_out	pcrread_out;
-	struct	tpm2_pcr_extend_in	pcrextend_in;
-	struct	tpm2_get_tpm_pt_in	get_tpm_pt_in;
-	struct	tpm2_get_tpm_pt_out	get_tpm_pt_out;
-	struct	tpm2_get_random_in	getrandom_in;
-	struct	tpm2_get_random_out	getrandom_out;
-};
-
-struct tpm2_cmd {
-	tpm_cmd_header		header;
-	union tpm2_cmd_params	params;
-} __packed;
-
 /*
  * Array with one entry per ordinal defining the maximum amount
  * of time the chip could take to return the result. The values
@@ -221,15 +182,24 @@ static const u8 tpm2_ordinal_duration[TPM2_CC_LAST - TPM2_CC_FIRST + 1] = {
 	TPM_UNDEFINED		/* 18f */
 };
 
-#define TPM2_PCR_READ_IN_SIZE \
-	(sizeof(struct tpm_input_header) + \
-	 sizeof(struct tpm2_pcr_read_in))
+static void tpm2_buf_append_auth(struct tpm_buf *buf, u32 session_handle,
+				 const u8 *nonce, u16 nonce_len,
+				 u8 session_attributes,
+				 const u8 *hmac, u16 hmac_len)
+{
+	tpm_buf_append_u32(buf, 9 + nonce_len + hmac_len);
+	tpm_buf_append_u32(buf, session_handle);
+	tpm_buf_append_u16(buf, nonce_len);
 
-static const struct tpm_input_header tpm2_pcrread_header = {
-	.tag = cpu_to_be16(TPM2_ST_NO_SESSIONS),
-	.length = cpu_to_be32(TPM2_PCR_READ_IN_SIZE),
-	.ordinal = cpu_to_be32(TPM2_CC_PCR_READ)
-};
+	if (nonce && nonce_len)
+		tpm_buf_append(buf, nonce, nonce_len);
+
+	tpm_buf_append_u8(buf, session_attributes);
+	tpm_buf_append_u16(buf, hmac_len);
+
+	if (hmac && hmac_len)
+		tpm_buf_append(buf, hmac, hmac_len);
+}
 
 /**
  * tpm2_pcr_read() - read a PCR value
@@ -243,42 +213,36 @@ static const struct tpm_input_header tpm2_pcrread_header = {
  */
 int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
 {
+	struct tpm_buf buf;
+	struct tpm2_pcr_read_in in;
+	struct tpm2_pcr_read_out *out;
 	int rc;
-	struct tpm2_cmd cmd;
-	u8 *buf;
 
 	if (pcr_idx >= TPM2_PLATFORM_PCR)
 		return -EINVAL;
 
-	cmd.header.in = tpm2_pcrread_header;
-	cmd.params.pcrread_in.pcr_selects_cnt = cpu_to_be32(1);
-	cmd.params.pcrread_in.hash_alg = cpu_to_be16(TPM2_ALG_SHA1);
-	cmd.params.pcrread_in.pcr_select_size = TPM2_PCR_SELECT_MIN;
+	tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_PCR_READ);
+
+	in.pcr_selects_cnt = cpu_to_be32(1);
+	in.hash_alg = cpu_to_be16(TPM2_ALG_SHA1);
+	in.pcr_select_size = TPM2_PCR_SELECT_MIN;
+	in.pcr_select[0] = 0x00;
+	in.pcr_select[1] = 0x00;
+	in.pcr_select[2] = 0x00;
+	in.pcr_select[pcr_idx >> 3] = 1 << (pcr_idx & 0x7);
 
-	memset(cmd.params.pcrread_in.pcr_select, 0,
-	       sizeof(cmd.params.pcrread_in.pcr_select));
-	cmd.params.pcrread_in.pcr_select[pcr_idx >> 3] = 1 << (pcr_idx & 0x7);
+	tpm_buf_append(&buf, (u8 *) &in, sizeof(in));
 
-	rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd),
+	rc = tpm_transmit_cmd(chip, buf.data, TPM_BUF_SIZE,
 			      "attempting to read a pcr value");
-	if (rc == 0) {
-		buf = cmd.params.pcrread_out.digest;
-		memcpy(res_buf, buf, TPM_DIGEST_SIZE);
-	}
+	if (rc)
+		return rc;
 
-	return rc;
+	out = (struct tpm2_pcr_read_out *) &buf.data[TPM_HEADER_SIZE];
+	memcpy(res_buf, out->digest, TPM_DIGEST_SIZE);
+	return 0;
 }
 
-#define TPM2_GET_PCREXTEND_IN_SIZE \
-	(sizeof(struct tpm_input_header) + \
-	 sizeof(struct tpm2_pcr_extend_in))
-
-static const struct tpm_input_header tpm2_pcrextend_header = {
-	.tag = cpu_to_be16(TPM2_ST_SESSIONS),
-	.length = cpu_to_be32(TPM2_GET_PCREXTEND_IN_SIZE),
-	.ordinal = cpu_to_be32(TPM2_CC_PCR_EXTEND)
-};
-
 /**
  * tpm2_pcr_extend() - extend a PCR value
  * @chip:	TPM chip to use.
@@ -291,76 +255,63 @@ static const struct tpm_input_header tpm2_pcrextend_header = {
  */
 int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash)
 {
-	struct tpm2_cmd cmd;
-	int rc;
-
-	cmd.header.in = tpm2_pcrextend_header;
-	cmd.params.pcrextend_in.pcr_idx = cpu_to_be32(pcr_idx);
-	cmd.params.pcrextend_in.auth_area_size =
-		cpu_to_be32(sizeof(struct tpm2_null_auth_area));
-	cmd.params.pcrextend_in.auth_area.handle =
-		cpu_to_be32(TPM2_RS_PW);
-	cmd.params.pcrextend_in.auth_area.nonce_size = 0;
-	cmd.params.pcrextend_in.auth_area.attributes = 0;
-	cmd.params.pcrextend_in.auth_area.auth_size = 0;
-	cmd.params.pcrextend_in.digest_cnt = cpu_to_be32(1);
-	cmd.params.pcrextend_in.hash_alg = cpu_to_be16(TPM2_ALG_SHA1);
-	memcpy(cmd.params.pcrextend_in.digest, hash, TPM_DIGEST_SIZE);
-
-	rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd),
-			      "attempting extend a PCR value");
+	struct tpm_buf buf;
+	struct tpm2_pcr_extend_in in;
 
-	return rc;
-}
+	tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_PCR_EXTEND);
+	tpm_buf_append_u32(&buf, pcr_idx);
+	tpm2_buf_append_auth(&buf, TPM2_RS_PW, NULL /* nonce */, 0,
+			     0 /* session_attributes */, NULL /* hmac */, 0);
 
-#define TPM2_GETRANDOM_IN_SIZE \
-	(sizeof(struct tpm_input_header) + \
-	 sizeof(struct tpm2_get_random_in))
+	in.digest_cnt = cpu_to_be32(1);
+	in.hash_alg = cpu_to_be16(TPM2_ALG_SHA1);
+	memcpy(in.digest, hash, TPM_DIGEST_SIZE);
+	tpm_buf_append(&buf, (u8 *) &in, sizeof(in));
 
-static const struct tpm_input_header tpm2_getrandom_header = {
-	.tag = cpu_to_be16(TPM2_ST_NO_SESSIONS),
-	.length = cpu_to_be32(TPM2_GETRANDOM_IN_SIZE),
-	.ordinal = cpu_to_be32(TPM2_CC_GET_RANDOM)
-};
+	return tpm_transmit_cmd(chip, buf.data, TPM_BUF_SIZE,
+				"attempting extend a PCR value");
+}
 
 /**
  * tpm2_get_random() - get random bytes from the TPM RNG
  * @chip: TPM chip to use
- * @out: destination buffer for the random bytes
+ * @res_buf: destination buffer for the random bytes
  * @max: the max number of bytes to write to @out
  *
  * 0 is returned when the operation is successful. If a negative number is
  * returned it remarks a POSIX error code. If a positive number is returned
  * it remarks a TPM error.
  */
-int tpm2_get_random(struct tpm_chip *chip, u8 *out, size_t max)
+int tpm2_get_random(struct tpm_chip *chip, u8 *res_buf, size_t max)
 {
-	struct tpm2_cmd cmd;
+	struct tpm_buf buf;
+	struct tpm2_get_random_out *out;
 	u32 recd;
 	u32 num_bytes;
 	int err;
 	int total = 0;
 	int retries = 5;
-	u8 *dest = out;
+	u8 *dest = res_buf;
 
-	num_bytes = min_t(u32, max, sizeof(cmd.params.getrandom_out.buffer));
+	num_bytes = min_t(u32, max, TPM_MAX_RNG_DATA);
 
-	if (!out || !num_bytes ||
-	    max > sizeof(cmd.params.getrandom_out.buffer))
+	if (!res_buf || !num_bytes || max > TPM_MAX_RNG_DATA)
 		return -EINVAL;
 
 	do {
-		cmd.header.in = tpm2_getrandom_header;
-		cmd.params.getrandom_in.size = cpu_to_be16(num_bytes);
 
-		err = tpm_transmit_cmd(chip, &cmd, sizeof(cmd),
+		tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_GET_RANDOM);
+		tpm_buf_append_u16(&buf, num_bytes);
+
+		err = tpm_transmit_cmd(chip, buf.data, TPM_BUF_SIZE,
 				       "attempting get random");
 		if (err)
 			break;
 
-		recd = min_t(u32, be16_to_cpu(cmd.params.getrandom_out.size),
-			     num_bytes);
-		memcpy(dest, cmd.params.getrandom_out.buffer, recd);
+		out = (struct tpm2_get_random_out *) &buf.data[TPM_HEADER_SIZE];
+
+		recd = min_t(u32, be16_to_cpu(out->size), num_bytes);
+		memcpy(dest, out->buffer, recd);
 
 		dest += recd;
 		total += recd;
@@ -370,16 +321,6 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *out, size_t max)
 	return total ? total : -EIO;
 }
 
-#define TPM2_GET_TPM_PT_IN_SIZE \
-	(sizeof(struct tpm_input_header) + \
-	 sizeof(struct tpm2_get_tpm_pt_in))
-
-static const struct tpm_input_header tpm2_get_tpm_pt_header = {
-	.tag = cpu_to_be16(TPM2_ST_NO_SESSIONS),
-	.length = cpu_to_be32(TPM2_GET_TPM_PT_IN_SIZE),
-	.ordinal = cpu_to_be32(TPM2_CC_GET_CAPABILITY)
-};
-
 /**
  * tpm2_get_tpm_pt() - get value of a TPM_CAP_TPM_PROPERTIES type property
  * @chip:		TPM chip to use.
@@ -391,33 +332,31 @@ static const struct tpm_input_header tpm2_get_tpm_pt_header = {
  * returned it remarks a POSIX error code. If a positive number is returned
  * it remarks a TPM error.
  */
-ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id,  u32 *value,
+ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id, u32 *value,
 			const char *desc)
 {
-	struct tpm2_cmd cmd;
+	struct tpm_buf buf;
+	struct tpm2_get_tpm_pt_in in;
+	struct tpm2_get_tpm_pt_out *out;
 	int rc;
 
-	cmd.header.in = tpm2_get_tpm_pt_header;
-	cmd.params.get_tpm_pt_in.cap_id = cpu_to_be32(TPM2_CAP_TPM_PROPERTIES);
-	cmd.params.get_tpm_pt_in.property_id = cpu_to_be32(property_id);
-	cmd.params.get_tpm_pt_in.property_cnt = cpu_to_be32(1);
+	tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_GET_CAPABILITY);
 
-	rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), desc);
-	if (!rc)
-		*value = cmd.params.get_tpm_pt_out.value;
+	in.cap_id = cpu_to_be32(TPM2_CAP_TPM_PROPERTIES);
+	in.property_id = cpu_to_be32(property_id);
+	in.property_cnt = cpu_to_be32(1);
 
-	return rc;
-}
+	tpm_buf_append(&buf, (u8 *) &in, sizeof(in));
 
-#define TPM2_STARTUP_IN_SIZE \
-	(sizeof(struct tpm_input_header) + \
-	 sizeof(struct tpm2_startup_in))
+	rc = tpm_transmit_cmd(chip, buf.data, TPM_BUF_SIZE, desc);
+	if (!rc) {
+		out = (struct tpm2_get_tpm_pt_out *)
+			&buf.data[TPM_HEADER_SIZE];
+		*value = be32_to_cpu(out->value);
+	}
 
-static const struct tpm_input_header tpm2_startup_header = {
-	.tag = cpu_to_be16(TPM2_ST_NO_SESSIONS),
-	.length = cpu_to_be32(TPM2_STARTUP_IN_SIZE),
-	.ordinal = cpu_to_be32(TPM2_CC_STARTUP)
-};
+	return rc;
+}
 
 /**
  * tpm2_startup() - send startup command to the TPM chip
@@ -431,26 +370,18 @@ static const struct tpm_input_header tpm2_startup_header = {
  */
 int tpm2_startup(struct tpm_chip *chip, u16 startup_type)
 {
-	struct tpm2_cmd cmd;
+	struct tpm_buf buf;
+	int rc;
 
-	cmd.header.in = tpm2_startup_header;
+	tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_STARTUP);
+	tpm_buf_append_u16(&buf, startup_type);
+	rc = tpm_transmit_cmd(chip, buf.data, TPM_BUF_SIZE,
+			      "attempting to start the TPM");
 
-	cmd.params.startup_in.startup_type = cpu_to_be16(startup_type);
-	return tpm_transmit_cmd(chip, &cmd, sizeof(cmd),
-				"attempting to start the TPM");
+	return rc;
 }
 EXPORT_SYMBOL_GPL(tpm2_startup);
 
-#define TPM2_SHUTDOWN_IN_SIZE \
-	(sizeof(struct tpm_input_header) + \
-	 sizeof(struct tpm2_startup_in))
-
-static const struct tpm_input_header tpm2_shutdown_header = {
-	.tag = cpu_to_be16(TPM2_ST_NO_SESSIONS),
-	.length = cpu_to_be32(TPM2_SHUTDOWN_IN_SIZE),
-	.ordinal = cpu_to_be32(TPM2_CC_SHUTDOWN)
-};
-
 /**
  * tpm2_shutdown() - send shutdown command to the TPM chip
  * @chip:		TPM chip to use.
@@ -459,20 +390,11 @@ static const struct tpm_input_header tpm2_shutdown_header = {
  */
 void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type)
 {
-	struct tpm2_cmd cmd;
-	int rc;
-
-	cmd.header.in = tpm2_shutdown_header;
-	cmd.params.startup_in.startup_type = cpu_to_be16(shutdown_type);
+	struct tpm_buf buf;
 
-	rc = tpm_transmit_cmd(chip, &cmd, sizeof(cmd), "stopping the TPM");
-
-	/* In places where shutdown command is sent there's no much we can do
-	 * except print the error code on a system failure.
-	 */
-	if (rc < 0)
-		dev_warn(chip->pdev, "transmit returned %d while stopping the TPM",
-			 rc);
+	tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_SHUTDOWN);
+	tpm_buf_append_u16(&buf, shutdown_type);
+	tpm_transmit_cmd(chip, buf.data, TPM_BUF_SIZE, "stopping the TPM");
 }
 EXPORT_SYMBOL_GPL(tpm2_shutdown);
 
@@ -503,16 +425,6 @@ unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal)
 }
 EXPORT_SYMBOL_GPL(tpm2_calc_ordinal_duration);
 
-#define TPM2_SELF_TEST_IN_SIZE \
-	(sizeof(struct tpm_input_header) + \
-	 sizeof(struct tpm2_self_test_in))
-
-static const struct tpm_input_header tpm2_selftest_header = {
-	.tag = cpu_to_be16(TPM2_ST_NO_SESSIONS),
-	.length = cpu_to_be32(TPM2_SELF_TEST_IN_SIZE),
-	.ordinal = cpu_to_be32(TPM2_CC_SELF_TEST)
-};
-
 /**
  * tpm2_continue_selftest() - start a self test
  * @chip: TPM chip to use
@@ -525,13 +437,13 @@ static const struct tpm_input_header tpm2_selftest_header = {
  */
 static int tpm2_start_selftest(struct tpm_chip *chip, bool full)
 {
+	struct tpm_buf buf;
 	int rc;
-	struct tpm2_cmd cmd;
 
-	cmd.header.in = tpm2_selftest_header;
-	cmd.params.selftest_in.full_test = full;
+	tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_SELF_TEST);
+	tpm_buf_append_u8(&buf, full);
 
-	rc = tpm_transmit_cmd(chip, &cmd, TPM2_SELF_TEST_IN_SIZE,
+	rc = tpm_transmit_cmd(chip, buf.data, TPM_BUF_SIZE,
 			      "continue selftest");
 
 	/* At least some prototype chips seem to give RC_TESTING error
@@ -562,11 +474,10 @@ int tpm2_do_selftest(struct tpm_chip *chip)
 	unsigned int loops;
 	unsigned int delay_msec = 100;
 	unsigned long duration;
-	struct tpm2_cmd cmd;
+	u8 dig[TPM_DIGEST_SIZE];
 	int i;
 
 	duration = tpm2_calc_ordinal_duration(chip, TPM2_CC_SELF_TEST);
-
 	loops = jiffies_to_msecs(duration) / delay_msec;
 
 	rc = tpm2_start_selftest(chip, true);
@@ -574,21 +485,8 @@ int tpm2_do_selftest(struct tpm_chip *chip)
 		return rc;
 
 	for (i = 0; i < loops; i++) {
-		/* Attempt to read a PCR value */
-		cmd.header.in = tpm2_pcrread_header;
-		cmd.params.pcrread_in.pcr_selects_cnt = cpu_to_be32(1);
-		cmd.params.pcrread_in.hash_alg = cpu_to_be16(TPM2_ALG_SHA1);
-		cmd.params.pcrread_in.pcr_select_size = TPM2_PCR_SELECT_MIN;
-		cmd.params.pcrread_in.pcr_select[0] = 0x01;
-		cmd.params.pcrread_in.pcr_select[1] = 0x00;
-		cmd.params.pcrread_in.pcr_select[2] = 0x00;
-
-		rc = tpm_transmit_cmd(chip, (u8 *) &cmd, sizeof(cmd), NULL);
-		if (rc < 0)
-			break;
-
-		rc = be32_to_cpu(cmd.header.out.return_code);
-		if (rc != TPM2_RC_TESTING)
+		rc = tpm2_pcr_read(chip, 1, dig);
+		if (rc < 0 || rc != TPM2_RC_TESTING)
 			break;
 
 		msleep(delay_msec);
@@ -624,21 +522,24 @@ EXPORT_SYMBOL_GPL(tpm2_gen_interrupt);
  */
 int tpm2_probe(struct tpm_chip *chip)
 {
-	struct tpm2_cmd cmd;
+	struct tpm_buf buf;
+	struct tpm2_get_tpm_pt_in in;
 	int rc;
 
-	cmd.header.in = tpm2_get_tpm_pt_header;
-	cmd.params.get_tpm_pt_in.cap_id = cpu_to_be32(TPM2_CAP_TPM_PROPERTIES);
-	cmd.params.get_tpm_pt_in.property_id = cpu_to_be32(0x100);
-	cmd.params.get_tpm_pt_in.property_cnt = cpu_to_be32(1);
+	in.cap_id = cpu_to_be32(TPM2_CAP_TPM_PROPERTIES);
+	in.property_id = cpu_to_be32(0x100);
+	in.property_cnt = cpu_to_be32(1);
+
+	tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_GET_CAPABILITY);
+	tpm_buf_append(&buf, (u8 *) &in, sizeof(in));
 
-	rc = tpm_transmit(chip, (const char *) &cmd, sizeof(cmd));
+	rc = tpm_transmit(chip, buf.data, TPM_BUF_SIZE);
 	if (rc <  0)
 		return rc;
 	else if (rc < TPM_HEADER_SIZE)
 		return -EFAULT;
 
-	if (be16_to_cpu(cmd.header.out.tag) == TPM2_ST_NO_SESSIONS)
+	if (tpm_buf_tag(&buf) == TPM2_ST_NO_SESSIONS)
 		chip->flags |= TPM_CHIP_FLAG_TPM2;
 
 	return 0;
-- 
2.5.0

[PATCH 2/4] trusted: move struct trusted_key_options to trusted-type.h

[Jarkko Sakkinen]

Moved struct trusted_key_options to trustes-type.h so that the fields
can be accessed from drivers/char/tpm.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 include/keys/trusted-type.h | 13 +++++++++++++
 security/keys/trusted.h     | 11 -----------
 2 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/include/keys/trusted-type.h b/include/keys/trusted-type.h
index 56f82e5..9a2c626 100644
--- a/include/keys/trusted-type.h
+++ b/include/keys/trusted-type.h
@@ -12,10 +12,12 @@
 
 #include <linux/key.h>
 #include <linux/rcupdate.h>
+#include <linux/tpm.h>
 
 #define MIN_KEY_SIZE			32
 #define MAX_KEY_SIZE			128
 #define MAX_BLOB_SIZE			320
+#define MAX_PCRINFO_SIZE		64
 
 struct trusted_key_payload {
 	struct rcu_head rcu;
@@ -26,6 +28,17 @@ struct trusted_key_payload {
 	unsigned char blob[MAX_BLOB_SIZE];
 };
 
+struct trusted_key_options {
+	uint16_t keytype;
+	uint32_t keyhandle;
+	unsigned char keyauth[TPM_DIGEST_SIZE];
+	unsigned char blobauth[TPM_DIGEST_SIZE];
+	uint32_t pcrinfo_len;
+	unsigned char pcrinfo[MAX_PCRINFO_SIZE];
+	int pcrlock;
+};
+
+
 extern struct key_type key_type_trusted;
 
 #endif /* _KEYS_TRUSTED_TYPE_H */
diff --git a/security/keys/trusted.h b/security/keys/trusted.h
index 3249fbd..ff001a5 100644
--- a/security/keys/trusted.h
+++ b/security/keys/trusted.h
@@ -2,7 +2,6 @@
 #define __TRUSTED_KEY_H
 
 /* implementation specific TPM constants */
-#define MAX_PCRINFO_SIZE		64
 #define MAX_BUF_SIZE			512
 #define TPM_GETRANDOM_SIZE		14
 #define TPM_OSAP_SIZE			36
@@ -36,16 +35,6 @@ enum {
 	SRK_keytype = 4
 };
 
-struct trusted_key_options {
-	uint16_t keytype;
-	uint32_t keyhandle;
-	unsigned char keyauth[SHA1_DIGEST_SIZE];
-	unsigned char blobauth[SHA1_DIGEST_SIZE];
-	uint32_t pcrinfo_len;
-	unsigned char pcrinfo[MAX_PCRINFO_SIZE];
-	int pcrlock;
-};
-
 #define TPM_DEBUG 0
 
 #if TPM_DEBUG
-- 
2.5.0

[PATCH 3/4] tpm: seal/unseal for TPM 2.0

[Jarkko Sakkinen]

Added tpm_trusted_seal() and tpm_trusted_unseal() API for sealing
trusted keys.

This patch implements basic sealing and unsealing functionality for
TPM 2.0:

* Seal with a parent key using a 20 byte auth value.
* Unseal with a parent key using a 20 byte auth value.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm-interface.c |  75 ++++++++++++++++
 drivers/char/tpm/tpm.h           |  14 +++
 drivers/char/tpm/tpm2-cmd.c      | 184 +++++++++++++++++++++++++++++++++++++++
 include/keys/trusted-type.h      |   2 +-
 include/linux/tpm.h              |  26 ++++++
 5 files changed, 300 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index e85d341..6dd4c74 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -666,6 +666,29 @@ int tpm_pcr_read_dev(struct tpm_chip *chip, int pcr_idx, u8 *res_buf)
 }
 
 /**
+ * tpm_is_tpm2 - is the chip a TPM2 chip?
+ * @chip_num:	tpm idx # or ANY
+ *
+ * Returns 1 if the chip is a TPM2 chip.
+ */
+int tpm_is_tpm2(u32 chip_num)
+{
+	struct tpm_chip *chip;
+	int rc;
+
+	chip = tpm_chip_find_get(chip_num);
+	if (chip == NULL)
+		return -ENODEV;
+
+	rc = (chip->flags & TPM_CHIP_FLAG_TPM2) != 0;
+
+	tpm_chip_put(chip);
+
+	return rc;
+}
+EXPORT_SYMBOL_GPL(tpm_is_tpm2);
+
+/**
  * tpm_pcr_read - read a pcr value
  * @chip_num:	tpm idx # or ANY
  * @pcr_idx:	pcr idx to retrieve
@@ -1021,6 +1044,58 @@ int tpm_get_random(u32 chip_num, u8 *out, size_t max)
 }
 EXPORT_SYMBOL_GPL(tpm_get_random);
 
+/**
+ * tpm_seal_trusted() - seal a trusted key
+ * @chip_num: A specific chip number for the request or TPM_ANY_NUM
+ * @options: authentication values and other options
+ * @payload: the key data in clear and encrypted form
+ *
+ * Returns < 0 on error and 0 on success. At the moment, only TPM 2.0 chips
+ * are supported.
+ */
+int tpm_seal_trusted(u32 chip_num, struct trusted_key_payload *payload,
+		     struct trusted_key_options *options)
+{
+	struct tpm_chip *chip;
+	int rc;
+
+	chip = tpm_chip_find_get(chip_num);
+	if (chip == NULL || !(chip->flags & TPM_CHIP_FLAG_TPM2))
+		return -ENODEV;
+
+	rc = tpm2_seal_trusted(chip, payload, options);
+
+	tpm_chip_put(chip);
+	return rc;
+}
+EXPORT_SYMBOL_GPL(tpm_seal_trusted);
+
+/**
+ * tpm_unseal_trusted() - unseal a trusted key
+ * @chip_num: A specific chip number for the request or TPM_ANY_NUM
+ * @options: authentication values and other options
+ * @payload: the key data in clear and encrypted form
+ *
+ * Returns < 0 on error and 0 on success. At the moment, only TPM 2.0 chips
+ * are supported.
+ */
+int tpm_unseal_trusted(u32 chip_num, struct trusted_key_payload *payload,
+		       struct trusted_key_options *options)
+{
+	struct tpm_chip *chip;
+	int rc;
+
+	chip = tpm_chip_find_get(chip_num);
+	if (chip == NULL || !(chip->flags & TPM_CHIP_FLAG_TPM2))
+		return -ENODEV;
+
+	rc = tpm2_unseal_trusted(chip, payload, options);
+
+	tpm_chip_put(chip);
+	return rc;
+}
+EXPORT_SYMBOL_GPL(tpm_unseal_trusted);
+
 static int __init tpm_init(void)
 {
 	int rc;
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index f04afb7..2d79939 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -88,6 +88,9 @@ enum tpm2_return_codes {
 
 enum tpm2_algorithms {
 	TPM2_ALG_SHA1		= 0x0004,
+	TPM2_ALG_KEYEDHASH	= 0x0008,
+	TPM2_ALG_SHA256		= 0x000B,
+	TPM2_ALG_NULL		= 0x0010
 };
 
 enum tpm2_command_codes {
@@ -95,6 +98,10 @@ enum tpm2_command_codes {
 	TPM2_CC_SELF_TEST	= 0x0143,
 	TPM2_CC_STARTUP		= 0x0144,
 	TPM2_CC_SHUTDOWN	= 0x0145,
+	TPM2_CC_CREATE		= 0x0153,
+	TPM2_CC_LOAD		= 0x0157,
+	TPM2_CC_UNSEAL		= 0x015E,
+	TPM2_CC_FLUSH_CONTEXT	= 0x0165,
 	TPM2_CC_GET_CAPABILITY	= 0x017A,
 	TPM2_CC_GET_RANDOM	= 0x017B,
 	TPM2_CC_PCR_READ	= 0x017E,
@@ -492,6 +499,13 @@ static inline void tpm_remove_ppi(struct tpm_chip *chip)
 int tpm2_pcr_read(struct tpm_chip *chip, int pcr_idx, u8 *res_buf);
 int tpm2_pcr_extend(struct tpm_chip *chip, int pcr_idx, const u8 *hash);
 int tpm2_get_random(struct tpm_chip *chip, u8 *out, size_t max);
+int tpm2_seal_trusted(struct tpm_chip *chip,
+		      struct trusted_key_payload *payload,
+		      struct trusted_key_options *options);
+int tpm2_unseal_trusted(struct tpm_chip *chip,
+			struct trusted_key_payload *payload,
+			struct trusted_key_options *options);
+
 ssize_t tpm2_get_tpm_pt(struct tpm_chip *chip, u32 property_id,
 			u32 *value, const char *desc);
 
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 0d55674..0986c96 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -16,6 +16,11 @@
  */
 
 #include "tpm.h"
+#include <keys/trusted-type.h>
+
+enum tpm2_object_attributes {
+	TPM2_ATTR_USER_WITH_AUTH	= BIT(6),
+};
 
 struct tpm2_pcr_read_in {
 	__be32	pcr_selects_cnt;
@@ -322,6 +327,185 @@ int tpm2_get_random(struct tpm_chip *chip, u8 *res_buf, size_t max)
 }
 
 /**
+ * tpm2_seal_trusted() - seal a trusted key
+ * @chip_num: A specific chip number for the request or TPM_ANY_NUM
+ * @options: authentication values and other options
+ * @payload: the key data in clear and encrypted form
+ *
+ * Returns < 0 on error and 0 on success.
+ */
+int tpm2_seal_trusted(struct tpm_chip *chip,
+		      struct trusted_key_payload *payload,
+		      struct trusted_key_options *options)
+{
+	unsigned int blob_len;
+	struct tpm_buf buf;
+	int rc;
+
+	tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_CREATE);
+	tpm_buf_append_u32(&buf, options->keyhandle);
+	tpm2_buf_append_auth(&buf, TPM2_RS_PW,
+			     NULL /* nonce */, 0,
+			     0 /* session_attributes */,
+			     options->keyauth /* hmac */,
+			     TPM_DIGEST_SIZE);
+
+	/* sensitive */
+	tpm_buf_append_u16(&buf, 4 + TPM_DIGEST_SIZE + payload->key_len);
+
+	tpm_buf_append_u16(&buf, TPM_DIGEST_SIZE);
+	tpm_buf_append(&buf, options->blobauth, TPM_DIGEST_SIZE);
+	tpm_buf_append_u16(&buf, payload->key_len);
+	tpm_buf_append(&buf, payload->key, payload->key_len);
+
+	/* public */
+	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_u32(&buf, TPM2_ATTR_USER_WITH_AUTH);
+	tpm_buf_append_u16(&buf, 0); /* policy digest size */
+	tpm_buf_append_u16(&buf, TPM2_ALG_NULL);
+	tpm_buf_append_u16(&buf, 0);
+
+	/* outside info */
+	tpm_buf_append_u16(&buf, 0);
+
+	/* creation PCR */
+	tpm_buf_append_u32(&buf, 0);
+
+	rc = tpm_transmit_cmd(chip, buf.data, TPM_BUF_SIZE, "sealing data");
+	if (rc)
+		goto out;
+
+	blob_len = be32_to_cpup((__be32 *) &buf.data[TPM_HEADER_SIZE]);
+	if (blob_len > MAX_BLOB_SIZE) {
+		rc = -E2BIG;
+		goto out;
+	}
+
+	memcpy(payload->blob, &buf.data[TPM_HEADER_SIZE + 4], blob_len);
+	payload->blob_len = blob_len;
+
+out:
+	if (rc > 0)
+		rc = -EPERM;
+
+	return rc;
+}
+
+static int tpm2_load(struct tpm_chip *chip,
+		     struct trusted_key_payload *payload,
+		     struct trusted_key_options *options,
+		     u32 *blob_handle)
+{
+	struct tpm_buf buf;
+	unsigned int private_len;
+	unsigned int public_len;
+	unsigned int blob_len;
+	int rc;
+
+	private_len = be16_to_cpup((__be16 *) &payload->blob[0]);
+	if (private_len > (payload->blob_len - 2))
+		return -E2BIG;
+
+	public_len = be16_to_cpup((__be16 *) &payload->blob[2 + private_len]);
+	blob_len = private_len + public_len + 4;
+	if (blob_len > payload->blob_len)
+		return -E2BIG;
+
+	tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_LOAD);
+	tpm_buf_append_u32(&buf, options->keyhandle);
+	tpm2_buf_append_auth(&buf, TPM2_RS_PW,
+			     NULL /* nonce */, 0,
+			     0 /* session_attributes */,
+			     options->keyauth /* hmac */,
+			     TPM_DIGEST_SIZE);
+
+	tpm_buf_append(&buf, payload->blob, payload->blob_len);
+
+	rc = tpm_transmit_cmd(chip, buf.data, TPM_BUF_SIZE, "loading blob");
+	if (!rc)
+		*blob_handle = be32_to_cpup(
+			(__be32 *) &buf.data[TPM_HEADER_SIZE]);
+
+	if (rc > 0)
+		rc = -EPERM;
+
+	return rc;
+}
+
+static void tpm2_flush_context(struct tpm_chip *chip, u32 handle)
+{
+	struct tpm_buf buf;
+	int rc;
+
+	tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS, TPM2_CC_FLUSH_CONTEXT);
+	tpm_buf_append_u32(&buf, handle);
+
+	rc = tpm_transmit_cmd(chip, buf.data, TPM_BUF_SIZE, "flushing context");
+	if (rc)
+		dev_warn(chip->pdev, "0x%08x was not flushed\n", handle);
+}
+
+static int tpm2_unseal(struct tpm_chip *chip,
+		       struct trusted_key_payload *payload,
+		       struct trusted_key_options *options,
+		       u32 blob_handle)
+{
+	struct tpm_buf buf;
+	int rc;
+
+	tpm_buf_init(&buf, TPM2_ST_SESSIONS, TPM2_CC_UNSEAL);
+	tpm_buf_append_u32(&buf, blob_handle);
+	tpm2_buf_append_auth(&buf, TPM2_RS_PW,
+			     NULL /* nonce */, 0,
+			     0 /* session_attributes */,
+			     options->blobauth /* hmac */,
+			     TPM_DIGEST_SIZE);
+
+	rc = tpm_transmit_cmd(chip, buf.data, TPM_BUF_SIZE, "unsealing");
+	if (rc > 0)
+		rc = -EPERM;
+
+	if (!rc) {
+		payload->key_len = be16_to_cpup(
+			(__be16 *) &buf.data[TPM_HEADER_SIZE + 4]);
+
+		memcpy(payload->key, &buf.data[TPM_HEADER_SIZE + 6],
+		       payload->key_len);
+	}
+
+	return rc;
+}
+
+/**
+ * tpm_unseal_trusted() - unseal a trusted key
+ * @chip_num: A specific chip number for the request or TPM_ANY_NUM
+ * @options: authentication values and other options
+ * @payload: the key data in clear and encrypted form
+ *
+ * Returns < 0 on error and 0 on success.
+ */
+int tpm2_unseal_trusted(struct tpm_chip *chip,
+			struct trusted_key_payload *payload,
+			struct trusted_key_options *options)
+{
+	u32 blob_handle;
+	int rc;
+
+	rc = tpm2_load(chip, payload, options, &blob_handle);
+	if (rc)
+		return rc;
+
+	rc = tpm2_unseal(chip, payload, options, blob_handle);
+
+	tpm2_flush_context(chip, blob_handle);
+
+	return rc;
+}
+
+/**
  * tpm2_get_tpm_pt() - get value of a TPM_CAP_TPM_PROPERTIES type property
  * @chip:		TPM chip to use.
  * @property_id:	property ID.
diff --git a/include/keys/trusted-type.h b/include/keys/trusted-type.h
index 9a2c626..0224b21 100644
--- a/include/keys/trusted-type.h
+++ b/include/keys/trusted-type.h
@@ -16,7 +16,7 @@
 
 #define MIN_KEY_SIZE			32
 #define MAX_KEY_SIZE			128
-#define MAX_BLOB_SIZE			320
+#define MAX_BLOB_SIZE			512
 #define MAX_PCRINFO_SIZE		64
 
 struct trusted_key_payload {
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index 8350c53..706e63e 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -30,6 +30,8 @@
 #define	TPM_ANY_NUM 0xFFFF
 
 struct tpm_chip;
+struct trusted_key_payload;
+struct trusted_key_options;
 
 struct tpm_class_ops {
 	const u8 req_complete_mask;
@@ -46,11 +48,22 @@ struct tpm_class_ops {
 
 #if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)
 
+extern int tpm_is_tpm2(u32 chip_num);
 extern int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf);
 extern int tpm_pcr_extend(u32 chip_num, int pcr_idx, const u8 *hash);
 extern int tpm_send(u32 chip_num, void *cmd, size_t buflen);
 extern int tpm_get_random(u32 chip_num, u8 *data, size_t max);
+extern int tpm_seal_trusted(u32 chip_num,
+			    struct trusted_key_payload *payload,
+			    struct trusted_key_options *options);
+extern int tpm_unseal_trusted(u32 chip_num,
+			      struct trusted_key_payload *payload,
+			      struct trusted_key_options *options);
 #else
+static inline int tpm_is_tpm2(u32 chip_num)
+{
+	return -ENODEV;
+}
 static inline int tpm_pcr_read(u32 chip_num, int pcr_idx, u8 *res_buf) {
 	return -ENODEV;
 }
@@ -63,5 +76,18 @@ static inline int tpm_send(u32 chip_num, void *cmd, size_t buflen) {
 static inline int tpm_get_random(u32 chip_num, u8 *data, size_t max) {
 	return -ENODEV;
 }
+
+static inline int tpm_seal_trusted(u32 chip_num,
+				   struct trusted_key_payload *payload,
+				   struct trusted_key_options *options)
+{
+	return -ENODEV;
+}
+static inline int tpm_unseal_trusted(u32 chip_num,
+				     struct trusted_key_payload *payload,
+				     struct trusted_key_options *options)
+{
+	return -ENODEV;
+}
 #endif
 #endif
-- 
2.5.0

Re: [PATCH 3/4] tpm: seal/unseal for TPM 2.0

[Jason Gunthorpe]

On Fri, Oct 02, 2015 at 11:38:17AM +0300, Jarkko Sakkinen wrote:
> Added tpm_trusted_seal() and tpm_trusted_unseal() API for sealing
> trusted keys.
> 
> This patch implements basic sealing and unsealing functionality for
> TPM 2.0:

We really need to stop using chip id's as a handle - the caller should
be using a pointer, it is just a horrible API, and the TPM_ANY_NUM
business is awful too.. TPM's are stateful devices!

Is it feasible to introduce new APIs with a saner scheme?

The api layering also seems really weird to me. At a minimum the
tpm_seal_trusted should be called within key_seal, but really, should
key_seal be migrated into the TPM core? I'm not sure it makes alot of
sense to have a tpm_seal_trusted which uses the high level key structs
when other tpm functions are all low level RPC wrappers...

Jason

Re: [PATCH 3/4] tpm: seal/unseal for TPM 2.0

[Jarkko Sakkinen]

On Tue, Oct 13, 2015 at 11:34:42AM -0600, Jason Gunthorpe wrote:
> On Fri, Oct 02, 2015 at 11:38:17AM +0300, Jarkko Sakkinen wrote:
> > Added tpm_trusted_seal() and tpm_trusted_unseal() API for sealing
> > trusted keys.
> > 
> > This patch implements basic sealing and unsealing functionality for
> > TPM 2.0:
> 
> We really need to stop using chip id's as a handle - the caller should
> be using a pointer, it is just a horrible API, and the TPM_ANY_NUM
> business is awful too.. TPM's are stateful devices!

Eventually this needs to be refactored out. I don't see it in the scope
of these patches or as high priority ATM.

> Is it feasible to introduce new APIs with a saner scheme?
> 
> The api layering also seems really weird to me. At a minimum the
> tpm_seal_trusted should be called within key_seal, but really, should
> key_seal be migrated into the TPM core? I'm not sure it makes alot of
> sense to have a tpm_seal_trusted which uses the high level key structs
> when other tpm functions are all low level RPC wrappers...

I think tpm_seal() inside trusted.c is not a very good API. It takes the
ad hoc version of the structs given to key_seal from stack. I don't see
a problem here.

My viewpoint has been that key_seal/unseal in trusted.c should be
refactored out and TPM1 implementations seal/unseal should be moved to
the TPM subsystem. There's so little amount of in-kernel low-level TPM
code that IMHO it makes sense to keep in one place (as are all the other
TPM utility functions).

I can work on the TPM1 migration when we have the basic TPM2 stuff in
place.

> Jason

/Jakrkko

[PATCH 4/4] keys, trusted: seal/unseal with TPM 2.0 chips

[Jarkko Sakkinen]

Call tpm_seal_trusted() and tpm_unseal_trusted() for TPM 2.0 chips.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm2-cmd.c |  2 +-
 include/linux/tpm_command.h |  1 -
 security/keys/trusted.c     | 18 ++++++++++++++----
 security/keys/trusted.h     |  7 +++++++
 4 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 0986c96..0fba698 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -422,7 +422,7 @@ static int tpm2_load(struct tpm_chip *chip,
 			     options->keyauth /* hmac */,
 			     TPM_DIGEST_SIZE);
 
-	tpm_buf_append(&buf, payload->blob, payload->blob_len);
+	tpm_buf_append(&buf, payload->blob, blob_len);
 
 	rc = tpm_transmit_cmd(chip, buf.data, TPM_BUF_SIZE, "loading blob");
 	if (!rc)
diff --git a/include/linux/tpm_command.h b/include/linux/tpm_command.h
index 727512e..d7b0f82 100644
--- a/include/linux/tpm_command.h
+++ b/include/linux/tpm_command.h
@@ -22,7 +22,6 @@
 #define TPM_ORD_UNSEAL                  24
 
 /* Other constants */
-#define SRKHANDLE                       0x40000000
 #define TPM_NONCE_SIZE                  20
 
 #endif
diff --git a/security/keys/trusted.c b/security/keys/trusted.c
index c0594cb..f6557b1 100644
--- a/security/keys/trusted.c
+++ b/security/keys/trusted.c
@@ -601,7 +601,7 @@ static int tpm_unseal(struct tpm_buf *tb,
 	}
 
 	ordinal = htonl(TPM_ORD_UNSEAL);
-	keyhndl = htonl(SRKHANDLE);
+	keyhndl = htonl(TPM1_SRKHANDLE);
 	ret = tpm_get_random(TPM_ANY_NUM, nonceodd, TPM_NONCE_SIZE);
 	if (ret != TPM_NONCE_SIZE) {
 		pr_info("trusted_key: tpm_get_random failed (%d)\n", ret);
@@ -867,7 +867,11 @@ static struct trusted_key_options *trusted_options_alloc(void)
 	if (options) {
 		/* set any non-zero defaults */
 		options->keytype = SRK_keytype;
-		options->keyhandle = SRKHANDLE;
+
+		if (tpm_is_tpm2(TPM_ANY_NUM))
+			options->keyhandle = TPM2_SRKHANDLE;
+		else
+			options->keyhandle = TPM1_SRKHANDLE;
 	}
 	return options;
 }
@@ -937,7 +941,10 @@ static int trusted_instantiate(struct key *key,
 
 	switch (key_cmd) {
 	case Opt_load:
-		ret = key_unseal(payload, options);
+		if (tpm_is_tpm2(TPM_ANY_NUM))
+			ret = tpm_unseal_trusted(TPM_ANY_NUM, payload, options);
+		else
+			ret = key_unseal(payload, options);
 		dump_payload(payload);
 		dump_options(options);
 		if (ret < 0)
@@ -950,7 +957,10 @@ static int trusted_instantiate(struct key *key,
 			pr_info("trusted_key: key_create failed (%d)\n", ret);
 			goto out;
 		}
-		ret = key_seal(payload, options);
+		if (tpm_is_tpm2(TPM_ANY_NUM))
+			ret = tpm_seal_trusted(TPM_ANY_NUM, payload, options);
+		else
+			ret = key_seal(payload, options);
 		if (ret < 0)
 			pr_info("trusted_key: key_seal failed (%d)\n", ret);
 		break;
diff --git a/security/keys/trusted.h b/security/keys/trusted.h
index ff001a5..fc32c47 100644
--- a/security/keys/trusted.h
+++ b/security/keys/trusted.h
@@ -12,6 +12,13 @@
 #define TPM_RETURN_OFFSET		6
 #define TPM_DATA_OFFSET			10
 
+/* Transient object handles start from 0x80000000 in TPM 2.0, which makes it
+ * a sane default.
+ */
+
+#define TPM1_SRKHANDLE	0x40000000
+#define TPM2_SRKHANDLE	0x80000000
+
 #define LOAD32(buffer, offset)	(ntohl(*(uint32_t *)&buffer[offset]))
 #define LOAD32N(buffer, offset)	(*(uint32_t *)&buffer[offset])
 #define LOAD16(buffer, offset)	(ntohs(*(uint16_t *)&buffer[offset]))
-- 
2.5.0

RE: [tpmdd-devel] [PATCH 4/4] keys, trusted: seal/unseal with TPM 2.0 chips

[Fuchs, Andreas]

Hi Jarkko,

[snip]

diff --git a/security/keys/trusted.h b/security/keys/trusted.h
index ff001a5..fc32c47 100644
--- a/security/keys/trusted.h
+++ b/security/keys/trusted.h
@@ -12,6 +12,13 @@
 #define TPM_RETURN_OFFSET              6
 #define TPM_DATA_OFFSET                        10

+/* Transient object handles start from 0x80000000 in TPM 2.0, which makes it
+ * a sane default.
+ */
+
+#define TPM1_SRKHANDLE 0x40000000
+#define TPM2_SRKHANDLE 0x80000000
+
 #define LOAD32(buffer, offset) (ntohl(*(uint32_t *)&buffer[offset]))
 #define LOAD32N(buffer, offset)        (*(uint32_t *)&buffer[offset])
 #define LOAD16(buffer, offset) (ntohs(*(uint16_t *)&buffer[offset]))

This TPM2_SRKHANDLE is unfortunately wrong.

Transient handles are assigned and returned by the TPM following the commands TPM2_CreatePrimary, TPM2_LoadObject and TPM2_ContextLoad. You can only use transient handles as returned by the TPM in order to refer to the corresponding object created inside the TPM via these commands. They can never assumed to be constant. The fact that TPMs return 0x80000000 for the first loaded Object and 0x80000001 for the second is merely a coincidence... ;-)

TPM2 also has no (single) SRK anymore. You have to create your own SRK / Storage Primary Keys via TPM2_CreatePrimary and use the transient handle returned from there. This however requires SH-authorization, usually via Policy IMHO, so not easy to manage. So IMHO, this might be something for the future but for the moment relying on a persistent key would be better...

For persistent SRKs it should become a convention to have those around. Those handles start with 0x81000000 and the SRKs (or Storage primary Keys) shall live within 0x81000000 to 0x8100FFFF (see http://www.trustedcomputinggroup.org/resources/registry_of_reserved_tpm_20_handles_and_localities)

I'd recommend to rely on the existence of a handle inside this range with an empty auth-value. So maybe install a persistent SRK to 0x81000000 via TPM2_EvictControl and then use this from within the kernel for anything following.
P.S. You should check for the key's TPMA_OBJECT to have fixedTPM SET. I don't know if there is an actual test for owner-generated SRK testing. I'll ask around though...

Note: you can query for handles in this range via TPM2_GetCapability(TPM_CAP_HANDLES, 0x81000000) and then look for fitting keys.


Feel free to discuss other approaches.

Cheers,
Andreas

Re: [tpmdd-devel] [PATCH 4/4] keys, trusted: seal/unseal with TPM 2.0 chips

[Jarkko Sakkinen]

On Sat, Oct 03, 2015 at 10:00:59AM +0000, Fuchs, Andreas wrote:
> Hi Jarkko,
> 
> [snip]
> 
> diff --git a/security/keys/trusted.h b/security/keys/trusted.h
> index ff001a5..fc32c47 100644
> --- a/security/keys/trusted.h
> +++ b/security/keys/trusted.h
> @@ -12,6 +12,13 @@
>  #define TPM_RETURN_OFFSET              6
>  #define TPM_DATA_OFFSET                        10
> 
> +/* Transient object handles start from 0x80000000 in TPM 2.0, which makes it
> + * a sane default.
> + */
> +
> +#define TPM1_SRKHANDLE 0x40000000
> +#define TPM2_SRKHANDLE 0x80000000
> +
>  #define LOAD32(buffer, offset) (ntohl(*(uint32_t *)&buffer[offset]))
>  #define LOAD32N(buffer, offset)        (*(uint32_t *)&buffer[offset])
>  #define LOAD16(buffer, offset) (ntohs(*(uint16_t *)&buffer[offset]))
> 
> This TPM2_SRKHANDLE is unfortunately wrong.
> 
> Transient handles are assigned and returned by the TPM following the
> commands TPM2_CreatePrimary, TPM2_LoadObject and TPM2_ContextLoad. You
> can only use transient handles as returned by the TPM in order to
> refer to the corresponding object created inside the TPM via these
> commands. They can never assumed to be constant. The fact that TPMs
> return 0x80000000 for the first loaded Object and 0x80000001 for the
> second is merely a coincidence... ;-)
> 
> TPM2 also has no (single) SRK anymore. You have to create your own SRK
> / Storage Primary Keys via TPM2_CreatePrimary and use the transient
> handle returned from there. This however requires SH-authorization,
> usually via Policy IMHO, so not easy to manage. So IMHO, this might be
> something for the future but for the moment relying on a persistent
> key would be better...
> 
> For persistent SRKs it should become a convention to have those
> around. Those handles start with 0x81000000 and the SRKs (or Storage
> primary Keys) shall live within 0x81000000 to 0x8100FFFF (see
> http://www.trustedcomputinggroup.org/resources/registry_of_reserved_tpm_20_handles_and_localities)
> 
> I'd recommend to rely on the existence of a handle inside this range
> with an empty auth-value. So maybe install a persistent SRK to
> 0x81000000 via TPM2_EvictControl and then use this from within the
> kernel for anything following.
> P.S. You should check for the key's TPMA_OBJECT to have fixedTPM SET.
> I don't know if there is an actual test for owner-generated SRK
> testing. I'll ask around though...
> 
> Note: you can query for handles in this range via
> TPM2_GetCapability(TPM_CAP_HANDLES, 0x81000000) and then look for
> fitting keys.
> 
> 
> Feel free to discuss other approaches.

I'm fully aware of all what you said. My take was to use 0x800000000 as
a default value if you don't the handle ID explicitly in 'description'
parameter of the add_key() syscall.

> Cheers,
> Andreas

/Jarkko

Re: [tpmdd-devel] [PATCH 4/4] keys, trusted: seal/unseal with TPM 2.0 chips

[Jarkko Sakkinen]

On Sat, Oct 03, 2015 at 01:26:55PM +0300, Jarkko Sakkinen wrote:
> On Sat, Oct 03, 2015 at 10:00:59AM +0000, Fuchs, Andreas wrote:
> > Hi Jarkko,
> > 
> > [snip]
> > 
> > diff --git a/security/keys/trusted.h b/security/keys/trusted.h
> > index ff001a5..fc32c47 100644
> > --- a/security/keys/trusted.h
> > +++ b/security/keys/trusted.h
> > @@ -12,6 +12,13 @@
> >  #define TPM_RETURN_OFFSET              6
> >  #define TPM_DATA_OFFSET                        10
> > 
> > +/* Transient object handles start from 0x80000000 in TPM 2.0, which makes it
> > + * a sane default.
> > + */
> > +
> > +#define TPM1_SRKHANDLE 0x40000000
> > +#define TPM2_SRKHANDLE 0x80000000
> > +
> >  #define LOAD32(buffer, offset) (ntohl(*(uint32_t *)&buffer[offset]))
> >  #define LOAD32N(buffer, offset)        (*(uint32_t *)&buffer[offset])
> >  #define LOAD16(buffer, offset) (ntohs(*(uint16_t *)&buffer[offset]))
> > 
> > This TPM2_SRKHANDLE is unfortunately wrong.
> > 
> > Transient handles are assigned and returned by the TPM following the
> > commands TPM2_CreatePrimary, TPM2_LoadObject and TPM2_ContextLoad. You
> > can only use transient handles as returned by the TPM in order to
> > refer to the corresponding object created inside the TPM via these
> > commands. They can never assumed to be constant. The fact that TPMs
> > return 0x80000000 for the first loaded Object and 0x80000001 for the
> > second is merely a coincidence... ;-)
> > 
> > TPM2 also has no (single) SRK anymore. You have to create your own SRK
> > / Storage Primary Keys via TPM2_CreatePrimary and use the transient
> > handle returned from there. This however requires SH-authorization,
> > usually via Policy IMHO, so not easy to manage. So IMHO, this might be
> > something for the future but for the moment relying on a persistent
> > key would be better...
> > 
> > For persistent SRKs it should become a convention to have those
> > around. Those handles start with 0x81000000 and the SRKs (or Storage
> > primary Keys) shall live within 0x81000000 to 0x8100FFFF (see
> > http://www.trustedcomputinggroup.org/resources/registry_of_reserved_tpm_20_handles_and_localities)
> > 
> > I'd recommend to rely on the existence of a handle inside this range
> > with an empty auth-value. So maybe install a persistent SRK to
> > 0x81000000 via TPM2_EvictControl and then use this from within the
> > kernel for anything following.
> > P.S. You should check for the key's TPMA_OBJECT to have fixedTPM SET.
> > I don't know if there is an actual test for owner-generated SRK
> > testing. I'll ask around though...
> > 
> > Note: you can query for handles in this range via
> > TPM2_GetCapability(TPM_CAP_HANDLES, 0x81000000) and then look for
> > fitting keys.
> > 
> > 
> > Feel free to discuss other approaches.
> 
> I'm fully aware of all what you said. My take was to use 0x800000000 as
> a default value if you don't the handle ID explicitly in 'description'
> parameter of the add_key() syscall.

I don't know how much you've done user space code that uses TPM2 chip.
I'm not saying I've written a lot of it but here's my experience.

In Haswell/PTT you can have 3 transient handles at a time. How you use
them is as follows:

1. Load/create your data to TPM filling transient handles starting from
   0x80000000.
2. Do your sealing/unsealing/whatever.
3. Flush transient handles.

For single handle use cases transient handle is in practice always
0x80000000 so it's very convenient to have that as the default value.

I think you are looking at this too much from specification point of
view. I've chosen the approach that is most convenient to use even
though the handle does not have exactly the same semantics as with TPM
1.x.

> > Cheers,
> > Andreas
> 
> /Jarkko

/Jarkko

RE: [tpmdd-devel] [PATCH 4/4] keys, trusted: seal/unseal with TPM 2.0 chips

[Fuchs, Andreas]

Hi Jarkko,

thanks for the clearification...

However, I'd recommend against doing so.
Furthermore, if there is a resource-manager running in userspace, applications only get virtual handles and TPM might be empty actually...

If that's what you're aiming for, I'd recommend passing the pointer to a context-saved-blob and have the kernel load the key this way. That ensures no problems with resource-manager and handle-mixups.

Cheers,
Andreas
________________________________________
From: Jarkko Sakkinen [jarkko.sakkinen@linux.intel.com]
Sent: Saturday, October 03, 2015 12:26
To: Fuchs, Andreas
Cc: tpmdd-devel@lists.sourceforge.net; linux-kernel@vger.kernel.org; David Howells; gregkh@linuxfoundation.org; open list:KEYS-TRUSTED; open list:KEYS-TRUSTED; James Morris; David Safford; akpm@linux-foundation.org; Serge E. Hallyn
Subject: Re: [tpmdd-devel] [PATCH 4/4] keys,    trusted: seal/unseal with TPM 2.0 chips

On Sat, Oct 03, 2015 at 10:00:59AM +0000, Fuchs, Andreas wrote:
> Hi Jarkko,
>
> [snip]
>
> diff --git a/security/keys/trusted.h b/security/keys/trusted.h
> index ff001a5..fc32c47 100644
> --- a/security/keys/trusted.h
> +++ b/security/keys/trusted.h
> @@ -12,6 +12,13 @@
>  #define TPM_RETURN_OFFSET              6
>  #define TPM_DATA_OFFSET                        10
>
> +/* Transient object handles start from 0x80000000 in TPM 2.0, which makes it
> + * a sane default.
> + */
> +
> +#define TPM1_SRKHANDLE 0x40000000
> +#define TPM2_SRKHANDLE 0x80000000
> +
>  #define LOAD32(buffer, offset) (ntohl(*(uint32_t *)&buffer[offset]))
>  #define LOAD32N(buffer, offset)        (*(uint32_t *)&buffer[offset])
>  #define LOAD16(buffer, offset) (ntohs(*(uint16_t *)&buffer[offset]))
>
> This TPM2_SRKHANDLE is unfortunately wrong.
>
> Transient handles are assigned and returned by the TPM following the
> commands TPM2_CreatePrimary, TPM2_LoadObject and TPM2_ContextLoad. You
> can only use transient handles as returned by the TPM in order to
> refer to the corresponding object created inside the TPM via these
> commands. They can never assumed to be constant. The fact that TPMs
> return 0x80000000 for the first loaded Object and 0x80000001 for the
> second is merely a coincidence... ;-)
>
> TPM2 also has no (single) SRK anymore. You have to create your own SRK
> / Storage Primary Keys via TPM2_CreatePrimary and use the transient
> handle returned from there. This however requires SH-authorization,
> usually via Policy IMHO, so not easy to manage. So IMHO, this might be
> something for the future but for the moment relying on a persistent
> key would be better...
>
> For persistent SRKs it should become a convention to have those
> around. Those handles start with 0x81000000 and the SRKs (or Storage
> primary Keys) shall live within 0x81000000 to 0x8100FFFF (see
> http://www.trustedcomputinggroup.org/resources/registry_of_reserved_tpm_20_handles_and_localities)
>
> I'd recommend to rely on the existence of a handle inside this range
> with an empty auth-value. So maybe install a persistent SRK to
> 0x81000000 via TPM2_EvictControl and then use this from within the
> kernel for anything following.
> P.S. You should check for the key's TPMA_OBJECT to have fixedTPM SET.
> I don't know if there is an actual test for owner-generated SRK
> testing. I'll ask around though...
>
> Note: you can query for handles in this range via
> TPM2_GetCapability(TPM_CAP_HANDLES, 0x81000000) and then look for
> fitting keys.
>
>
> Feel free to discuss other approaches.

I'm fully aware of all what you said. My take was to use 0x800000000 as
a default value if you don't the handle ID explicitly in 'description'
parameter of the add_key() syscall.

> Cheers,
> Andreas

/Jarkko

Re: [tpmdd-devel] [PATCH 4/4] keys, trusted: seal/unseal with TPM 2.0 chips

[Jarkko Sakkinen]

On Sun, Oct 04, 2015 at 06:57:42PM +0000, Fuchs, Andreas wrote:
> Hi Jarkko,
> 
> thanks for the clearification...
> 
> However, I'd recommend against doing so.
>
> Furthermore, if there is a resource-manager running in userspace,
> applications only get virtual handles and TPM might be empty
> actually...
> 
> If that's what you're aiming for, I'd recommend passing the pointer to
> a context-saved-blob and have the kernel load the key this way. That
> ensures no problems with resource-manager and handle-mixups.

TPM 1.x interface has the same race if you do not use the default value
for the 'keyhandle' option.

In practice a processs in TCB (or root) would do all the keyctl magic so
I do not see huge issue here. It can be orchestrated by the
OS/distribution. From my point of view you are over-engineering in wrong
place.

It would be easy to add a way to provide the sealing key as blob later
on if the simple approach chosen would not be sufficient. I'm confident
that for 99% of all real-world use cases the interface provided by the
patch set is sufficient.

> Cheers,
> Andreas

/Jarkko

> From: Jarkko Sakkinen [jarkko.sakkinen@linux.intel.com]
> Sent: Saturday, October 03, 2015 12:26
> To: Fuchs, Andreas
> Cc: tpmdd-devel@lists.sourceforge.net; linux-kernel@vger.kernel.org; David Howells; gregkh@linuxfoundation.org; open list:KEYS-TRUSTED; open list:KEYS-TRUSTED; James Morris; David Safford; akpm@linux-foundation.org; Serge E. Hallyn
> Subject: Re: [tpmdd-devel] [PATCH 4/4] keys,    trusted: seal/unseal with TPM 2.0 chips
> 
> On Sat, Oct 03, 2015 at 10:00:59AM +0000, Fuchs, Andreas wrote:
> > Hi Jarkko,
> >
> > [snip]
> >
> > diff --git a/security/keys/trusted.h b/security/keys/trusted.h
> > index ff001a5..fc32c47 100644
> > --- a/security/keys/trusted.h
> > +++ b/security/keys/trusted.h
> > @@ -12,6 +12,13 @@
> >  #define TPM_RETURN_OFFSET              6
> >  #define TPM_DATA_OFFSET                        10
> >
> > +/* Transient object handles start from 0x80000000 in TPM 2.0, which makes it
> > + * a sane default.
> > + */
> > +
> > +#define TPM1_SRKHANDLE 0x40000000
> > +#define TPM2_SRKHANDLE 0x80000000
> > +
> >  #define LOAD32(buffer, offset) (ntohl(*(uint32_t *)&buffer[offset]))
> >  #define LOAD32N(buffer, offset)        (*(uint32_t *)&buffer[offset])
> >  #define LOAD16(buffer, offset) (ntohs(*(uint16_t *)&buffer[offset]))
> >
> > This TPM2_SRKHANDLE is unfortunately wrong.
> >
> > Transient handles are assigned and returned by the TPM following the
> > commands TPM2_CreatePrimary, TPM2_LoadObject and TPM2_ContextLoad. You
> > can only use transient handles as returned by the TPM in order to
> > refer to the corresponding object created inside the TPM via these
> > commands. They can never assumed to be constant. The fact that TPMs
> > return 0x80000000 for the first loaded Object and 0x80000001 for the
> > second is merely a coincidence... ;-)
> >
> > TPM2 also has no (single) SRK anymore. You have to create your own SRK
> > / Storage Primary Keys via TPM2_CreatePrimary and use the transient
> > handle returned from there. This however requires SH-authorization,
> > usually via Policy IMHO, so not easy to manage. So IMHO, this might be
> > something for the future but for the moment relying on a persistent
> > key would be better...
> >
> > For persistent SRKs it should become a convention to have those
> > around. Those handles start with 0x81000000 and the SRKs (or Storage
> > primary Keys) shall live within 0x81000000 to 0x8100FFFF (see
> > http://www.trustedcomputinggroup.org/resources/registry_of_reserved_tpm_20_handles_and_localities)
> >
> > I'd recommend to rely on the existence of a handle inside this range
> > with an empty auth-value. So maybe install a persistent SRK to
> > 0x81000000 via TPM2_EvictControl and then use this from within the
> > kernel for anything following.
> > P.S. You should check for the key's TPMA_OBJECT to have fixedTPM SET.
> > I don't know if there is an actual test for owner-generated SRK
> > testing. I'll ask around though...
> >
> > Note: you can query for handles in this range via
> > TPM2_GetCapability(TPM_CAP_HANDLES, 0x81000000) and then look for
> > fitting keys.
> >
> >
> > Feel free to discuss other approaches.
> 
> I'm fully aware of all what you said. My take was to use 0x800000000 as
> a default value if you don't the handle ID explicitly in 'description'
> parameter of the add_key() syscall.
> 
> > Cheers,
> > Andreas
> 
> /Jarkko

RE: [tpmdd-devel] [PATCH 4/4] keys, trusted: seal/unseal with TPM 2.0 chips

[Fuchs, Andreas]

Hi Jarkko,

/dev/tpm0 is single-open only. So if root wants to do anything there, you'd have to stop the tss-daemon first.
Or do you see "keyctl magic" only happening in initrd ?

I have to admit that I have no experience with the 1.2 trusted key features. Never used them. Is there software available that uses non-SRKs on 1.2 ? How did they solve the problem of a running TrouSerS-tcsd ?
The main difference for 2.0 is the non-existence of the SRK. That's where I see the need for rethinking...

Cheers,
Andreas
________________________________________
From: Jarkko Sakkinen [jarkko.sakkinen@linux.intel.com]
Sent: Monday, October 05, 2015 10:37
To: Fuchs, Andreas
Cc: tpmdd-devel@lists.sourceforge.net; linux-kernel@vger.kernel.org; David Howells; gregkh@linuxfoundation.org; open list:KEYS-TRUSTED; open list:KEYS-TRUSTED; James Morris; David Safford; akpm@linux-foundation.org; Serge E. Hallyn
Subject: Re: [tpmdd-devel] [PATCH 4/4] keys,    trusted: seal/unseal with TPM 2.0 chips

On Sun, Oct 04, 2015 at 06:57:42PM +0000, Fuchs, Andreas wrote:
> Hi Jarkko,
>
> thanks for the clearification...
>
> However, I'd recommend against doing so.
>
> Furthermore, if there is a resource-manager running in userspace,
> applications only get virtual handles and TPM might be empty
> actually...
>
> If that's what you're aiming for, I'd recommend passing the pointer to
> a context-saved-blob and have the kernel load the key this way. That
> ensures no problems with resource-manager and handle-mixups.

TPM 1.x interface has the same race if you do not use the default value
for the 'keyhandle' option.

In practice a processs in TCB (or root) would do all the keyctl magic so
I do not see huge issue here. It can be orchestrated by the
OS/distribution. From my point of view you are over-engineering in wrong
place.

It would be easy to add a way to provide the sealing key as blob later
on if the simple approach chosen would not be sufficient. I'm confident
that for 99% of all real-world use cases the interface provided by the
patch set is sufficient.

> Cheers,
> Andreas

/Jarkko

> From: Jarkko Sakkinen [jarkko.sakkinen@linux.intel.com]
> Sent: Saturday, October 03, 2015 12:26
> To: Fuchs, Andreas
> Cc: tpmdd-devel@lists.sourceforge.net; linux-kernel@vger.kernel.org; David Howells; gregkh@linuxfoundation.org; open list:KEYS-TRUSTED; open list:KEYS-TRUSTED; James Morris; David Safford; akpm@linux-foundation.org; Serge E. Hallyn
> Subject: Re: [tpmdd-devel] [PATCH 4/4] keys,    trusted: seal/unseal with TPM 2.0 chips
>
> On Sat, Oct 03, 2015 at 10:00:59AM +0000, Fuchs, Andreas wrote:
> > Hi Jarkko,
> >
> > [snip]
> >
> > diff --git a/security/keys/trusted.h b/security/keys/trusted.h
> > index ff001a5..fc32c47 100644
> > --- a/security/keys/trusted.h
> > +++ b/security/keys/trusted.h
> > @@ -12,6 +12,13 @@
> >  #define TPM_RETURN_OFFSET              6
> >  #define TPM_DATA_OFFSET                        10
> >
> > +/* Transient object handles start from 0x80000000 in TPM 2.0, which makes it
> > + * a sane default.
> > + */
> > +
> > +#define TPM1_SRKHANDLE 0x40000000
> > +#define TPM2_SRKHANDLE 0x80000000
> > +
> >  #define LOAD32(buffer, offset) (ntohl(*(uint32_t *)&buffer[offset]))
> >  #define LOAD32N(buffer, offset)        (*(uint32_t *)&buffer[offset])
> >  #define LOAD16(buffer, offset) (ntohs(*(uint16_t *)&buffer[offset]))
> >
> > This TPM2_SRKHANDLE is unfortunately wrong.
> >
> > Transient handles are assigned and returned by the TPM following the
> > commands TPM2_CreatePrimary, TPM2_LoadObject and TPM2_ContextLoad. You
> > can only use transient handles as returned by the TPM in order to
> > refer to the corresponding object created inside the TPM via these
> > commands. They can never assumed to be constant. The fact that TPMs
> > return 0x80000000 for the first loaded Object and 0x80000001 for the
> > second is merely a coincidence... ;-)
> >
> > TPM2 also has no (single) SRK anymore. You have to create your own SRK
> > / Storage Primary Keys via TPM2_CreatePrimary and use the transient
> > handle returned from there. This however requires SH-authorization,
> > usually via Policy IMHO, so not easy to manage. So IMHO, this might be
> > something for the future but for the moment relying on a persistent
> > key would be better...
> >
> > For persistent SRKs it should become a convention to have those
> > around. Those handles start with 0x81000000 and the SRKs (or Storage
> > primary Keys) shall live within 0x81000000 to 0x8100FFFF (see
> > http://www.trustedcomputinggroup.org/resources/registry_of_reserved_tpm_20_handles_and_localities)
> >
> > I'd recommend to rely on the existence of a handle inside this range
> > with an empty auth-value. So maybe install a persistent SRK to
> > 0x81000000 via TPM2_EvictControl and then use this from within the
> > kernel for anything following.
> > P.S. You should check for the key's TPMA_OBJECT to have fixedTPM SET.
> > I don't know if there is an actual test for owner-generated SRK
> > testing. I'll ask around though...
> >
> > Note: you can query for handles in this range via
> > TPM2_GetCapability(TPM_CAP_HANDLES, 0x81000000) and then look for
> > fitting keys.
> >
> >
> > Feel free to discuss other approaches.
>
> I'm fully aware of all what you said. My take was to use 0x800000000 as
> a default value if you don't the handle ID explicitly in 'description'
> parameter of the add_key() syscall.
>
> > Cheers,
> > Andreas
>
> /Jarkko

Re: [tpmdd-devel] [PATCH 4/4] keys, trusted: seal/unseal with TPM 2.0 chips

[Jarkko Sakkinen]

On Mon, Oct 05, 2015 at 09:00:48AM +0000, Fuchs, Andreas wrote:
> Hi Jarkko,
> 
> /dev/tpm0 is single-open only. So if root wants to do anything there,
> you'd have to stop the tss-daemon first.
> Or do you see "keyctl magic" only happening in initrd ?
> 
> I have to admit that I have no experience with the 1.2 trusted key
> features. Never used them. Is there software available that uses
> non-SRKs on 1.2 ? How did they solve the problem of a running
> TrouSerS-tcsd ?
> The main difference for 2.0 is the non-existence of the SRK. That's
> where I see the need for rethinking...

I see where you are getting and it is a valid queston.

In the long run solution would be to have an access broker in the kernel
so that you can open a separated context for transient handles if you
need one and have all the eviction/loading magic in the kernel. However,
this requires a lot of work, and for example for small single user
embedded systems you would anyway want to disable it by using some
compilation option (lets say CONFIG_TCG_TPM2_ACCESS_BROKER).

Your proposal would only partly fix the issue because you need
additional TPM2_Load in trusted.c in order to load the key before
executing TPM2_Unseal operation.

I'm not saying the solution is perfect but it works for notable coverage
of systems (especially in the IoT side) and it doesn't prevent to
implement access broker later on.

I would be interested to get feedback from ChromeOS developers on this
as they are using TPM quite extensively for user data encryption and
various other use cases.

> Cheers,
> Andreas

/Jarkko

> ________________________________________
> From: Jarkko Sakkinen [jarkko.sakkinen@linux.intel.com]
> Sent: Monday, October 05, 2015 10:37
> To: Fuchs, Andreas
> Cc: tpmdd-devel@lists.sourceforge.net; linux-kernel@vger.kernel.org; David Howells; gregkh@linuxfoundation.org; open list:KEYS-TRUSTED; open list:KEYS-TRUSTED; James Morris; David Safford; akpm@linux-foundation.org; Serge E. Hallyn
> Subject: Re: [tpmdd-devel] [PATCH 4/4] keys,    trusted: seal/unseal with TPM 2.0 chips
> 
> On Sun, Oct 04, 2015 at 06:57:42PM +0000, Fuchs, Andreas wrote:
> > Hi Jarkko,
> >
> > thanks for the clearification...
> >
> > However, I'd recommend against doing so.
> >
> > Furthermore, if there is a resource-manager running in userspace,
> > applications only get virtual handles and TPM might be empty
> > actually...
> >
> > If that's what you're aiming for, I'd recommend passing the pointer to
> > a context-saved-blob and have the kernel load the key this way. That
> > ensures no problems with resource-manager and handle-mixups.
> 
> TPM 1.x interface has the same race if you do not use the default value
> for the 'keyhandle' option.
> 
> In practice a processs in TCB (or root) would do all the keyctl magic so
> I do not see huge issue here. It can be orchestrated by the
> OS/distribution. From my point of view you are over-engineering in wrong
> place.
> 
> It would be easy to add a way to provide the sealing key as blob later
> on if the simple approach chosen would not be sufficient. I'm confident
> that for 99% of all real-world use cases the interface provided by the
> patch set is sufficient.
> 
> > Cheers,
> > Andreas
> 
> /Jarkko
> 
> > From: Jarkko Sakkinen [jarkko.sakkinen@linux.intel.com]
> > Sent: Saturday, October 03, 2015 12:26
> > To: Fuchs, Andreas
> > Cc: tpmdd-devel@lists.sourceforge.net; linux-kernel@vger.kernel.org; David Howells; gregkh@linuxfoundation.org; open list:KEYS-TRUSTED; open list:KEYS-TRUSTED; James Morris; David Safford; akpm@linux-foundation.org; Serge E. Hallyn
> > Subject: Re: [tpmdd-devel] [PATCH 4/4] keys,    trusted: seal/unseal with TPM 2.0 chips
> >
> > On Sat, Oct 03, 2015 at 10:00:59AM +0000, Fuchs, Andreas wrote:
> > > Hi Jarkko,
> > >
> > > [snip]
> > >
> > > diff --git a/security/keys/trusted.h b/security/keys/trusted.h
> > > index ff001a5..fc32c47 100644
> > > --- a/security/keys/trusted.h
> > > +++ b/security/keys/trusted.h
> > > @@ -12,6 +12,13 @@
> > >  #define TPM_RETURN_OFFSET              6
> > >  #define TPM_DATA_OFFSET                        10
> > >
> > > +/* Transient object handles start from 0x80000000 in TPM 2.0, which makes it
> > > + * a sane default.
> > > + */
> > > +
> > > +#define TPM1_SRKHANDLE 0x40000000
> > > +#define TPM2_SRKHANDLE 0x80000000
> > > +
> > >  #define LOAD32(buffer, offset) (ntohl(*(uint32_t *)&buffer[offset]))
> > >  #define LOAD32N(buffer, offset)        (*(uint32_t *)&buffer[offset])
> > >  #define LOAD16(buffer, offset) (ntohs(*(uint16_t *)&buffer[offset]))
> > >
> > > This TPM2_SRKHANDLE is unfortunately wrong.
> > >
> > > Transient handles are assigned and returned by the TPM following the
> > > commands TPM2_CreatePrimary, TPM2_LoadObject and TPM2_ContextLoad. You
> > > can only use transient handles as returned by the TPM in order to
> > > refer to the corresponding object created inside the TPM via these
> > > commands. They can never assumed to be constant. The fact that TPMs
> > > return 0x80000000 for the first loaded Object and 0x80000001 for the
> > > second is merely a coincidence... ;-)
> > >
> > > TPM2 also has no (single) SRK anymore. You have to create your own SRK
> > > / Storage Primary Keys via TPM2_CreatePrimary and use the transient
> > > handle returned from there. This however requires SH-authorization,
> > > usually via Policy IMHO, so not easy to manage. So IMHO, this might be
> > > something for the future but for the moment relying on a persistent
> > > key would be better...
> > >
> > > For persistent SRKs it should become a convention to have those
> > > around. Those handles start with 0x81000000 and the SRKs (or Storage
> > > primary Keys) shall live within 0x81000000 to 0x8100FFFF (see
> > > http://www.trustedcomputinggroup.org/resources/registry_of_reserved_tpm_20_handles_and_localities)
> > >
> > > I'd recommend to rely on the existence of a handle inside this range
> > > with an empty auth-value. So maybe install a persistent SRK to
> > > 0x81000000 via TPM2_EvictControl and then use this from within the
> > > kernel for anything following.
> > > P.S. You should check for the key's TPMA_OBJECT to have fixedTPM SET.
> > > I don't know if there is an actual test for owner-generated SRK
> > > testing. I'll ask around though...
> > >
> > > Note: you can query for handles in this range via
> > > TPM2_GetCapability(TPM_CAP_HANDLES, 0x81000000) and then look for
> > > fitting keys.
> > >
> > >
> > > Feel free to discuss other approaches.
> >
> > I'm fully aware of all what you said. My take was to use 0x800000000 as
> > a default value if you don't the handle ID explicitly in 'description'
> > parameter of the add_key() syscall.
> >
> > > Cheers,
> > > Andreas
> >
> > /Jarkko
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [tpmdd-devel] [PATCH 4/4] keys, trusted: seal/unseal with TPM 2.0 chips

[Fuchs, Andreas]

That's why I propose to give the context-save-blob into the kernel. It does not require any TPM2_Load of the key-chain or TPM2_CreatePrimary prior to key usage.

BTW, in the current TSS2-model context-save-blobs are the preferred way for "moving/copying" loaded objects between two applications or threads. The TSS2 crew did not see any value in having a "libdrm-like" flink() call. Since you have to transfer the handle anyways, transferring those few bytes of blob are actually just as easy and management inside the daemon becomes way simple without flink()ing... ;-)

Regarding the in-kernel "minimal resource manager": AFAIK there is already a tpm-mutex inside the kernel. We could use that mutex and then have the algorithm:

- get_mutex(tpm)
- If TPM_has_no_empty_slots:
    - tmpBlob0 = Context_Save(some_handle)
    - context_Flush(some_handle)
- if TPM_has_one_empts_slot: /* also true if "no_empty_slots" above */
    - tmpBlob1 = Context_Save(another_handle)
    - context_Flush(another_handle)

- keyhandle = Context_Load(keyBlob)
- datahandle = Context_Load(dataBlob)
- unseal(datahanle, keyhandle)
- Context_Flush(keyhandle)
- Context_Flush(datahandle)

- if tmpBlob0:
    - h1 = Context_Load(tmpBlob0)
    - h2 = Context_Load(tmpBlob0)
    - assert((h1 == some_handle && h2 == another_handle) || (h2 == some_handle && h1 == another_handle))
    - Context_Flush(another_handle) /* This is needed to get the original handles guaranteed */
- if tmpBlob1:
    - assert(another_handle == Context_Load(tmpBlob1))

- release_mutex(tpm)

We don't need anything more fancy than this. And it should even guarantee that the old values are still present after mutex_release, so (opposed to a full-blown resource-manager) we do not need to keep track and rewrite virtual handles inside the user-space commands.

IMHO, this should be lightweight enough even for the most embedded of applications, since the 2*2k blobs are only allocated on demand...

ChromeOS with 1.2 uses a forked patched TrouSerS, right ?
I don't know about 2.0 though...

Cheers,
Andreas

________________________________________
From: Jarkko Sakkinen [jarkko.sakkinen@linux.intel.com]
Sent: Monday, October 05, 2015 13:56
To: Fuchs, Andreas
Cc: tpmdd-devel@lists.sourceforge.net; linux-kernel@vger.kernel.org; David Howells; gregkh@linuxfoundation.org; open list:KEYS-TRUSTED; open list:KEYS-TRUSTED; James Morris; David Safford; akpm@linux-foundation.org; Serge E. Hallyn
Subject: Re: [tpmdd-devel] [PATCH 4/4] keys,    trusted: seal/unseal with TPM 2.0 chips

On Mon, Oct 05, 2015 at 09:00:48AM +0000, Fuchs, Andreas wrote:
> Hi Jarkko,
>
> /dev/tpm0 is single-open only. So if root wants to do anything there,
> you'd have to stop the tss-daemon first.
> Or do you see "keyctl magic" only happening in initrd ?
>
> I have to admit that I have no experience with the 1.2 trusted key
> features. Never used them. Is there software available that uses
> non-SRKs on 1.2 ? How did they solve the problem of a running
> TrouSerS-tcsd ?
> The main difference for 2.0 is the non-existence of the SRK. That's
> where I see the need for rethinking...

I see where you are getting and it is a valid queston.

In the long run solution would be to have an access broker in the kernel
so that you can open a separated context for transient handles if you
need one and have all the eviction/loading magic in the kernel. However,
this requires a lot of work, and for example for small single user
embedded systems you would anyway want to disable it by using some
compilation option (lets say CONFIG_TCG_TPM2_ACCESS_BROKER).

Your proposal would only partly fix the issue because you need
additional TPM2_Load in trusted.c in order to load the key before
executing TPM2_Unseal operation.

I'm not saying the solution is perfect but it works for notable coverage
of systems (especially in the IoT side) and it doesn't prevent to
implement access broker later on.

I would be interested to get feedback from ChromeOS developers on this
as they are using TPM quite extensively for user data encryption and
various other use cases.

> Cheers,
> Andreas

/Jarkko

> ________________________________________
> From: Jarkko Sakkinen [jarkko.sakkinen@linux.intel.com]
> Sent: Monday, October 05, 2015 10:37
> To: Fuchs, Andreas
> Cc: tpmdd-devel@lists.sourceforge.net; linux-kernel@vger.kernel.org; David Howells; gregkh@linuxfoundation.org; open list:KEYS-TRUSTED; open list:KEYS-TRUSTED; James Morris; David Safford; akpm@linux-foundation.org; Serge E. Hallyn
> Subject: Re: [tpmdd-devel] [PATCH 4/4] keys,    trusted: seal/unseal with TPM 2.0 chips
>
> On Sun, Oct 04, 2015 at 06:57:42PM +0000, Fuchs, Andreas wrote:
> > Hi Jarkko,
> >
> > thanks for the clearification...
> >
> > However, I'd recommend against doing so.
> >
> > Furthermore, if there is a resource-manager running in userspace,
> > applications only get virtual handles and TPM might be empty
> > actually...
> >
> > If that's what you're aiming for, I'd recommend passing the pointer to
> > a context-saved-blob and have the kernel load the key this way. That
> > ensures no problems with resource-manager and handle-mixups.
>
> TPM 1.x interface has the same race if you do not use the default value
> for the 'keyhandle' option.
>
> In practice a processs in TCB (or root) would do all the keyctl magic so
> I do not see huge issue here. It can be orchestrated by the
> OS/distribution. From my point of view you are over-engineering in wrong
> place.
>
> It would be easy to add a way to provide the sealing key as blob later
> on if the simple approach chosen would not be sufficient. I'm confident
> that for 99% of all real-world use cases the interface provided by the
> patch set is sufficient.
>
> > Cheers,
> > Andreas
>
> /Jarkko
>
> > From: Jarkko Sakkinen [jarkko.sakkinen@linux.intel.com]
> > Sent: Saturday, October 03, 2015 12:26
> > To: Fuchs, Andreas
> > Cc: tpmdd-devel@lists.sourceforge.net; linux-kernel@vger.kernel.org; David Howells; gregkh@linuxfoundation.org; open list:KEYS-TRUSTED; open list:KEYS-TRUSTED; James Morris; David Safford; akpm@linux-foundation.org; Serge E. Hallyn
> > Subject: Re: [tpmdd-devel] [PATCH 4/4] keys,    trusted: seal/unseal with TPM 2.0 chips
> >
> > On Sat, Oct 03, 2015 at 10:00:59AM +0000, Fuchs, Andreas wrote:
> > > Hi Jarkko,
> > >
> > > [snip]
> > >
> > > diff --git a/security/keys/trusted.h b/security/keys/trusted.h
> > > index ff001a5..fc32c47 100644
> > > --- a/security/keys/trusted.h
> > > +++ b/security/keys/trusted.h
> > > @@ -12,6 +12,13 @@
> > >  #define TPM_RETURN_OFFSET              6
> > >  #define TPM_DATA_OFFSET                        10
> > >
> > > +/* Transient object handles start from 0x80000000 in TPM 2.0, which makes it
> > > + * a sane default.
> > > + */
> > > +
> > > +#define TPM1_SRKHANDLE 0x40000000
> > > +#define TPM2_SRKHANDLE 0x80000000
> > > +
> > >  #define LOAD32(buffer, offset) (ntohl(*(uint32_t *)&buffer[offset]))
> > >  #define LOAD32N(buffer, offset)        (*(uint32_t *)&buffer[offset])
> > >  #define LOAD16(buffer, offset) (ntohs(*(uint16_t *)&buffer[offset]))
> > >
> > > This TPM2_SRKHANDLE is unfortunately wrong.
> > >
> > > Transient handles are assigned and returned by the TPM following the
> > > commands TPM2_CreatePrimary, TPM2_LoadObject and TPM2_ContextLoad. You
> > > can only use transient handles as returned by the TPM in order to
> > > refer to the corresponding object created inside the TPM via these
> > > commands. They can never assumed to be constant. The fact that TPMs
> > > return 0x80000000 for the first loaded Object and 0x80000001 for the
> > > second is merely a coincidence... ;-)
> > >
> > > TPM2 also has no (single) SRK anymore. You have to create your own SRK
> > > / Storage Primary Keys via TPM2_CreatePrimary and use the transient
> > > handle returned from there. This however requires SH-authorization,
> > > usually via Policy IMHO, so not easy to manage. So IMHO, this might be
> > > something for the future but for the moment relying on a persistent
> > > key would be better...
> > >
> > > For persistent SRKs it should become a convention to have those
> > > around. Those handles start with 0x81000000 and the SRKs (or Storage
> > > primary Keys) shall live within 0x81000000 to 0x8100FFFF (see
> > > http://www.trustedcomputinggroup.org/resources/registry_of_reserved_tpm_20_handles_and_localities)
> > >
> > > I'd recommend to rely on the existence of a handle inside this range
> > > with an empty auth-value. So maybe install a persistent SRK to
> > > 0x81000000 via TPM2_EvictControl and then use this from within the
> > > kernel for anything following.
> > > P.S. You should check for the key's TPMA_OBJECT to have fixedTPM SET.
> > > I don't know if there is an actual test for owner-generated SRK
> > > testing. I'll ask around though...
> > >
> > > Note: you can query for handles in this range via
> > > TPM2_GetCapability(TPM_CAP_HANDLES, 0x81000000) and then look for
> > > fitting keys.
> > >
> > >
> > > Feel free to discuss other approaches.
> >
> > I'm fully aware of all what you said. My take was to use 0x800000000 as
> > a default value if you don't the handle ID explicitly in 'description'
> > parameter of the add_key() syscall.
> >
> > > Cheers,
> > > Andreas
> >
> > /Jarkko
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [tpmdd-devel] [PATCH 4/4] keys, trusted: seal/unseal with TPM 2.0 chips

[Jarkko Sakkinen]

I don't mean to be impolite but could line up your replies properly
and avoid top-posting. I'd recommend 72 chars per line. Thanks.

On Mon, Oct 05, 2015 at 12:20:47PM +0000, Fuchs, Andreas wrote:
> That's why I propose to give the context-save-blob into the kernel. It
> does not require any TPM2_Load of the key-chain or TPM2_CreatePrimary
> prior to key usage.
> 
> BTW, in the current TSS2-model context-save-blobs are the preferred
> way for "moving/copying" loaded objects between two applications or
> threads. The TSS2 crew did not see any value in having a "libdrm-like"
> flink() call. Since you have to transfer the handle anyways,
> transferring those few bytes of blob are actually just as easy and
> management inside the daemon becomes way simple without flink()ing...
> ;-)
> 
> Regarding the in-kernel "minimal resource manager": AFAIK there is
> already a tpm-mutex inside the kernel. We could use that mutex and
> then have the algorithm:
>
> [SNIP]

I don't care about one purpose hacks. Second, I don't care about pseudo
code (at least not for "too big things"). It has tendency to mask
unexpected details. If you want to propose something, please go through
the patch process.

> We don't need anything more fancy than this. And it should even
> guarantee that the old values are still present after mutex_release,
> so (opposed to a full-blown resource-manager) we do not need to keep
> track and rewrite virtual handles inside the user-space commands.
> 
> IMHO, this should be lightweight enough even for the most embedded of
> applications, since the 2*2k blobs are only allocated on demand...

It's still unnecessary functionality and increases the kernel image size
and every hack requires maintenance. It would probably end up needing
compilation flag as there exists efforts like:

https://tiny.wiki.kernel.org/

My simple and stupid solution does not *prevent* adding better
synchronization. I would go with that and implement access broker
properly and not for just one use case later on.

> ChromeOS with 1.2 uses a forked patched TrouSerS, right ?
> I don't know about 2.0 though...

Yup, patched to use UDP sockets.

> Cheers,
> Andreas

/Jarkko

RE: [tpmdd-devel] [PATCH 4/4] keys, trusted: seal/unseal with TPM 2.0 chips

[Fuchs, Andreas]

> > Regarding the in-kernel "minimal resource manager": AFAIK there is
> > already a tpm-mutex inside the kernel. We could use that mutex and
> > then have the algorithm:
> >
> > [SNIP]
> 
> I don't care about one purpose hacks. Second, I don't care about pseudo
> code (at least not for "too big things"). It has tendency to mask
> unexpected details. If you want to propose something, please go through
> the patch process.
> 
> > We don't need anything more fancy than this. And it should even
> > guarantee that the old values are still present after mutex_release,
> > so (opposed to a full-blown resource-manager) we do not need to keep
> > track and rewrite virtual handles inside the user-space commands.
> >
> > IMHO, this should be lightweight enough even for the most embedded of
> > applications, since the 2*2k blobs are only allocated on demand...
> 
> It's still unnecessary functionality and increases the kernel image size
> and every hack requires maintenance. It would probably end up needing
> compilation flag as there exists efforts like:
> 
> https://tiny.wiki.kernel.org/
> 
> My simple and stupid solution does not *prevent* adding better
> synchronization. I would go with that and implement access broker
> properly and not for just one use case later on.

Unfortunately, I'm not able to write up some code for this myself atm.
Other priorities unfortunately.

I was just pointing out, that the proposed patch will not fit in with
the current approach in TSS2.0, before this user-facing kernel API is
set in stone and _corrected_ new syscalls need to be added later.

Also, the pseudo-code proposal should be a proper minimal access broker
that should solve most accesses to TPM transient objects down the road. 
Session-brokering is a different beast of course.

Cheers,
Andreas

Re: [tpmdd-devel] [PATCH 4/4] keys, trusted: seal/unseal with TPM 2.0 chips

[Jarkko Sakkinen]

On Mon, Oct 05, 2015 at 01:36:18PM +0000, Fuchs, Andreas wrote:
> > It's still unnecessary functionality and increases the kernel image size
> > and every hack requires maintenance. It would probably end up needing
> > compilation flag as there exists efforts like:
> > 
> > https://tiny.wiki.kernel.org/
> > 
> > My simple and stupid solution does not *prevent* adding better
> > synchronization. I would go with that and implement access broker
> > properly and not for just one use case later on.
> 
> Unfortunately, I'm not able to write up some code for this myself atm.
> Other priorities unfortunately.
> 
> I was just pointing out, that the proposed patch will not fit in with
> the current approach in TSS2.0, before this user-facing kernel API is
> set in stone and _corrected_ new syscalls need to be added later.

Why you would want new system calls? Do you know how hard it is to get
new system calls accepted? It's usually nearly impossible to get new
system calls in. You are going wrong direction there.

I do not see why couldn't survive in TSS 2.0 implementation for a while
without in-kernel access broker even if the world isn't perfect and
improve from that when the support becomes available. I'm not frankly
following your rationale here.

On the other hand I see use for the kernel images without access broker
in small embdedded devices.

I CC'd to Will Arthur as he has been working with TSS 2.0 for along
time just in case.

> Also, the pseudo-code proposal should be a proper minimal access broker
> that should solve most accesses to TPM transient objects down the road.
> Session-brokering is a different beast of course.

I don't mean to be rude but pseudo code doesn't matter much. We know
what is required from an access broker in terms of TPM 2.0 commands and
locking. Only working code matters at this point.

I still don't see why you couldn't add access broker later on. The patch
set does not make the API worse than it is right now.

> Cheers,
> Andreas

/Jarkko

RE: [tpmdd-devel] [PATCH 4/4] keys, trusted: seal/unseal with TPM 2.0 chips

[Fuchs, Andreas]

> > I was just pointing out, that the proposed patch will not fit in with
> > the current approach in TSS2.0, before this user-facing kernel API is
> > set in stone and _corrected_ new syscalls need to be added later.
> 
> Why you would want new system calls? Do you know how hard it is to get
> new system calls accepted? It's usually nearly impossible to get new
> system calls in. You are going wrong direction there.
> 
> I do not see why couldn't survive in TSS 2.0 implementation for a while
> without in-kernel access broker even if the world isn't perfect and
> improve from that when the support becomes available. I'm not frankly
> following your rationale here.
> 
> On the other hand I see use for the kernel images without access broker
> in small embdedded devices.
> 
> I CC'd to Will Arthur as he has been working with TSS 2.0 for along
> time just in case.
> 
> > Also, the pseudo-code proposal should be a proper minimal access broker
> > that should solve most accesses to TPM transient objects down the road.
> > Session-brokering is a different beast of course.
> 
> I don't mean to be rude but pseudo code doesn't matter much. We know
> what is required from an access broker in terms of TPM 2.0 commands and
> locking. Only working code matters at this point.
> 
> I still don't see why you couldn't add access broker later on. The patch
> set does not make the API worse than it is right now.

OK, I guess we got stuck in the follow-up discussions and missed the points. 


My 1st point is:

TPM1.2's 0x40000000 SRK handle was a well-known, singleton, always-present
key, that could be relied upon.

TPM2.0's 0x80000000 is a temporary, TPM-assigned, context-specific handle,
that cannot be relied upon.

Therefore, I think your patch should not use it.

Instead, I'd recommend using the closest equivalent to an SRK that TPM2.0
has to offer, which is within the range 0x81000000 to 0x8100FFFF.
(see http://www.trustedcomputinggroup.org/resources/registry_of_reserved_tpm_20_handles_and_localities)
You might want to use TPM2_GetCapability() to find the correct one.

Also User-Space could reference any of these handles in the 
0x81000000-0x81FFFFFF range. This would be fine.



My 2nd point is:

It is IMHO a bad idea to allow user-space to provide transient handles as
parameter to the TPM, because TSS2.0 will virtualize handles and /dev/tpm0
is single-access only.
Instead I'd recommend passing context-saved-blobs to the kernel.

Then you brought up the valid point that this requires kernel-space resource
broker and I provided some sketch-idea in pseudo-code for discussion of
general approach. I did not know that the access broker was solved already.



Conclusion

I would just like to prevent having an API that expects (and defaults to)
transient handles be set in stone for the kernel, since it will not meet
reality... ;-)



@Will: Hope you're doing fine... Maybe we can discuss the TSS-side of things
tomorrow...

Cheers,
Andreas

Re: [tpmdd-devel] [PATCH 4/4] keys, trusted: seal/unseal with TPM 2.0 chips

[Jarkko Sakkinen]

On Mon, Oct 05, 2015 at 02:13:15PM +0000, Fuchs, Andreas wrote:
> > > I was just pointing out, that the proposed patch will not fit in with
> > > the current approach in TSS2.0, before this user-facing kernel API is
> > > set in stone and _corrected_ new syscalls need to be added later.
> > 
> > Why you would want new system calls? Do you know how hard it is to get
> > new system calls accepted? It's usually nearly impossible to get new
> > system calls in. You are going wrong direction there.
> > 
> > I do not see why couldn't survive in TSS 2.0 implementation for a while
> > without in-kernel access broker even if the world isn't perfect and
> > improve from that when the support becomes available. I'm not frankly
> > following your rationale here.
> > 
> > On the other hand I see use for the kernel images without access broker
> > in small embdedded devices.
> > 
> > I CC'd to Will Arthur as he has been working with TSS 2.0 for along
> > time just in case.
> > 
> > > Also, the pseudo-code proposal should be a proper minimal access broker
> > > that should solve most accesses to TPM transient objects down the road.
> > > Session-brokering is a different beast of course.
> > 
> > I don't mean to be rude but pseudo code doesn't matter much. We know
> > what is required from an access broker in terms of TPM 2.0 commands and
> > locking. Only working code matters at this point.
> > 
> > I still don't see why you couldn't add access broker later on. The patch
> > set does not make the API worse than it is right now.
> 
> OK, I guess we got stuck in the follow-up discussions and missed the points. 

Yup, don't get me wrong here. I like this discussion and am willing to
listen to reasonable arguments.

> My 1st point is:
> 
> TPM1.2's 0x40000000 SRK handle was a well-known, singleton, always-present
> key, that could be relied upon.
> 
> TPM2.0's 0x80000000 is a temporary, TPM-assigned, context-specific handle,
> that cannot be relied upon.
> 
> Therefore, I think your patch should not use it.
> 
> Instead, I'd recommend using the closest equivalent to an SRK that TPM2.0
> has to offer, which is within the range 0x81000000 to 0x8100FFFF.
> (see http://www.trustedcomputinggroup.org/resources/registry_of_reserved_tpm_20_handles_and_localities)
> You might want to use TPM2_GetCapability() to find the correct one.
> 
> Also User-Space could reference any of these handles in the 
> 0x81000000-0x81FFFFFF range. This would be fine.

Alright. How about requiring keyhandle as explicit option for TPM 2.0?
Would that be a more reasonable solution in your opinion? That would
acceptable for me.

> My 2nd point is:
> 
> It is IMHO a bad idea to allow user-space to provide transient handles as
> parameter to the TPM, because TSS2.0 will virtualize handles and /dev/tpm0
> is single-access only.
> Instead I'd recommend passing context-saved-blobs to the kernel.
> 
> Then you brought up the valid point that this requires kernel-space resource
> broker and I provided some sketch-idea in pseudo-code for discussion of
> general approach. I did not know that the access broker was solved already.

Yeah. I'm not against implementing the broker and how I've been thinking
implementing it is not too far away what you just suggested.

I'm not just seeing why that couldn't be done as a separate patch set
later on.

> Conclusion
> 
> I would just like to prevent having an API that expects (and defaults to)
> transient handles be set in stone for the kernel, since it will not meet
> reality... ;-)
> 
> 
> 
> @Will: Hope you're doing fine... Maybe we can discuss the TSS-side of things
> tomorrow...
> 
> Cheers,
> Andreas--

/Jarkko

RE: [tpmdd-devel] [PATCH 4/4] keys, trusted: seal/unseal with TPM 2.0 chips

[Arthur, Will C]

I'm having trouble following the arguments here.

Can someone summarize the two (or more) different approaches being proposed?

Will Arthur
Intel Corporation
Server Security Firmware
803-216-2101

-----Original Message-----
From: Jarkko Sakkinen [mailto:jarkko.sakkinen@linux.intel.com] 
Sent: Monday, October 5, 2015 10:28 AM
To: Fuchs, Andreas <andreas.fuchs@sit.fraunhofer.de>
Cc: tpmdd-devel@lists.sourceforge.net; linux-kernel@vger.kernel.org; David Howells <dhowells@redhat.com>; gregkh@linuxfoundation.org; open list:KEYS-TRUSTED <linux-security-module@vger.kernel.org>; open list:KEYS-TRUSTED <keyrings@vger.kernel.org>; James Morris <james.l.morris@oracle.com>; David Safford <safford@us.ibm.com>; akpm@linux-foundation.org; Serge E. Hallyn <serge@hallyn.com>; josh@joshtripplet.org; Maliszewski, Richard L <richard.l.maliszewski@intel.com>; Wiseman, Monty <monty.wiseman@intel.com>; Arthur, Will C <will.c.arthur@intel.com>
Subject: Re: [tpmdd-devel] [PATCH 4/4] keys, trusted: seal/unseal with TPM 2.0 chips

On Mon, Oct 05, 2015 at 02:13:15PM +0000, Fuchs, Andreas wrote:
> > > I was just pointing out, that the proposed patch will not fit in 
> > > with the current approach in TSS2.0, before this user-facing 
> > > kernel API is set in stone and _corrected_ new syscalls need to be added later.
> > 
> > Why you would want new system calls? Do you know how hard it is to 
> > get new system calls accepted? It's usually nearly impossible to get 
> > new system calls in. You are going wrong direction there.
> > 
> > I do not see why couldn't survive in TSS 2.0 implementation for a 
> > while without in-kernel access broker even if the world isn't 
> > perfect and improve from that when the support becomes available. 
> > I'm not frankly following your rationale here.
> > 
> > On the other hand I see use for the kernel images without access 
> > broker in small embdedded devices.
> > 
> > I CC'd to Will Arthur as he has been working with TSS 2.0 for along 
> > time just in case.
> > 
> > > Also, the pseudo-code proposal should be a proper minimal access 
> > > broker that should solve most accesses to TPM transient objects down the road.
> > > Session-brokering is a different beast of course.
> > 
> > I don't mean to be rude but pseudo code doesn't matter much. We know 
> > what is required from an access broker in terms of TPM 2.0 commands 
> > and locking. Only working code matters at this point.
> > 
> > I still don't see why you couldn't add access broker later on. The 
> > patch set does not make the API worse than it is right now.
> 
> OK, I guess we got stuck in the follow-up discussions and missed the points. 

Yup, don't get me wrong here. I like this discussion and am willing to listen to reasonable arguments.

> My 1st point is:
> 
> TPM1.2's 0x40000000 SRK handle was a well-known, singleton, 
> always-present key, that could be relied upon.
> 
> TPM2.0's 0x80000000 is a temporary, TPM-assigned, context-specific 
> handle, that cannot be relied upon.
> 
> Therefore, I think your patch should not use it.
> 
> Instead, I'd recommend using the closest equivalent to an SRK that 
> TPM2.0 has to offer, which is within the range 0x81000000 to 0x8100FFFF.
> (see 
> http://www.trustedcomputinggroup.org/resources/registry_of_reserved_tp
> m_20_handles_and_localities) You might want to use 
> TPM2_GetCapability() to find the correct one.
> 
> Also User-Space could reference any of these handles in the 
> 0x81000000-0x81FFFFFF range. This would be fine.

Alright. How about requiring keyhandle as explicit option for TPM 2.0?
Would that be a more reasonable solution in your opinion? That would acceptable for me.

> My 2nd point is:
> 
> It is IMHO a bad idea to allow user-space to provide transient handles 
> as parameter to the TPM, because TSS2.0 will virtualize handles and 
> /dev/tpm0 is single-access only.
> Instead I'd recommend passing context-saved-blobs to the kernel.
> 
> Then you brought up the valid point that this requires kernel-space 
> resource broker and I provided some sketch-idea in pseudo-code for 
> discussion of general approach. I did not know that the access broker was solved already.

Yeah. I'm not against implementing the broker and how I've been thinking implementing it is not too far away what you just suggested.

I'm not just seeing why that couldn't be done as a separate patch set later on.

> Conclusion
> 
> I would just like to prevent having an API that expects (and defaults 
> to) transient handles be set in stone for the kernel, since it will 
> not meet reality... ;-)
> 
> 
> 
> @Will: Hope you're doing fine... Maybe we can discuss the TSS-side of 
> things tomorrow...
> 
> Cheers,
> Andreas--

/Jarkko

RE: [tpmdd-devel] [PATCH 4/4] keys, trusted: seal/unseal with TPM 2.0 chips

[Fuchs, Andreas]

> > OK, I guess we got stuck in the follow-up discussions and missed the points.
> 
> Yup, don't get me wrong here. I like this discussion and am willing to
> listen to reasonable arguments.

We could not agree more. I'm always up for a good discussion... ;-)

> > My 1st point is:
> >
> > TPM1.2's 0x40000000 SRK handle was a well-known, singleton, always-present
> > key, that could be relied upon.
> >
> > TPM2.0's 0x80000000 is a temporary, TPM-assigned, context-specific handle,
> > that cannot be relied upon.
> >
> > Therefore, I think your patch should not use it.
> >
> > Instead, I'd recommend using the closest equivalent to an SRK that TPM2.0
> > has to offer, which is within the range 0x81000000 to 0x8100FFFF.
> > (see http://www.trustedcomputinggroup.org/resources/registry_of_reserved_tpm_20_handles_and_localities)
> > You might want to use TPM2_GetCapability() to find the correct one.
> >
> > Also User-Space could reference any of these handles in the
> > 0x81000000-0x81FFFFFF range. This would be fine.
> 
> Alright. How about requiring keyhandle as explicit option for TPM 2.0?
> Would that be a more reasonable solution in your opinion? That would
> acceptable for me.

You mean getting rid of the default behaviour ?
That sound reasonable to me as well. A later patch could add the possibility
to use the TPM2_GetCapability() thing at a later stage then...

> > My 2nd point is:
> >
> > It is IMHO a bad idea to allow user-space to provide transient handles as
> > parameter to the TPM, because TSS2.0 will virtualize handles and /dev/tpm0
> > is single-access only.
> > Instead I'd recommend passing context-saved-blobs to the kernel.
> >
> > Then you brought up the valid point that this requires kernel-space resource
> > broker and I provided some sketch-idea in pseudo-code for discussion of
> > general approach. I did not know that the access broker was solved already.
> 
> Yeah. I'm not against implementing the broker and how I've been thinking
> implementing it is not too far away what you just suggested.
> 
> I'm not just seeing why that couldn't be done as a separate patch set
> later on.

I should have been more clear then. I agree that it can be added later on.
Or rather I think it should be added at some later point...

I was just trying to point out that the concept is not too difficult, since
kernel-space (minimal) resource-manager makes a lot of people go "oh god,
never ever, way too big, way too complicated", which IMHO it is not.
Until then, I think that the call should just return -EBUSY when you cannot
load the sealed data if no slots are available ?

I looked at Patch 3/4 and it seems you default to -EPERM on TPM2_Create()-
and TPM2_Load()-failures ?
You might want to test against rc == TPM_RC_OBJECT_MEMORY and return -EBUSY
in those cases. Would you agree ?
(P.S. I can cross-post there if that's prefered ?)

Cheers,
Andreas

Re: [tpmdd-devel] [PATCH 4/4] keys, trusted: seal/unseal with TPM 2.0 chips

[Jarkko Sakkinen]

On Tue, Oct 06, 2015 at 06:22:29AM +0000, Fuchs, Andreas wrote:
> > > OK, I guess we got stuck in the follow-up discussions and missed the points.
> > 
> > Yup, don't get me wrong here. I like this discussion and am willing to
> > listen to reasonable arguments.
> 
> We could not agree more. I'm always up for a good discussion... ;-)
> 
> > > My 1st point is:
> > >
> > > TPM1.2's 0x40000000 SRK handle was a well-known, singleton, always-present
> > > key, that could be relied upon.
> > >
> > > TPM2.0's 0x80000000 is a temporary, TPM-assigned, context-specific handle,
> > > that cannot be relied upon.
> > >
> > > Therefore, I think your patch should not use it.
> > >
> > > Instead, I'd recommend using the closest equivalent to an SRK that TPM2.0
> > > has to offer, which is within the range 0x81000000 to 0x8100FFFF.
> > > (see http://www.trustedcomputinggroup.org/resources/registry_of_reserved_tpm_20_handles_and_localities)
> > > You might want to use TPM2_GetCapability() to find the correct one.
> > >
> > > Also User-Space could reference any of these handles in the
> > > 0x81000000-0x81FFFFFF range. This would be fine.
> > 
> > Alright. How about requiring keyhandle as explicit option for TPM 2.0?
> > Would that be a more reasonable solution in your opinion? That would
> > acceptable for me.
> 
> You mean getting rid of the default behaviour ?
> That sound reasonable to me as well. A later patch could add the possibility
> to use the TPM2_GetCapability() thing at a later stage then...
> 
> > > My 2nd point is:
> > >
> > > It is IMHO a bad idea to allow user-space to provide transient handles as
> > > parameter to the TPM, because TSS2.0 will virtualize handles and /dev/tpm0
> > > is single-access only.
> > > Instead I'd recommend passing context-saved-blobs to the kernel.
> > >
> > > Then you brought up the valid point that this requires kernel-space resource
> > > broker and I provided some sketch-idea in pseudo-code for discussion of
> > > general approach. I did not know that the access broker was solved already.
> > 
> > Yeah. I'm not against implementing the broker and how I've been thinking
> > implementing it is not too far away what you just suggested.
> > 
> > I'm not just seeing why that couldn't be done as a separate patch set
> > later on.
> 
> I should have been more clear then. I agree that it can be added later on.
> Or rather I think it should be added at some later point...
> 
> I was just trying to point out that the concept is not too difficult, since
> kernel-space (minimal) resource-manager makes a lot of people go "oh god,
> never ever, way too big, way too complicated", which IMHO it is not.
> Until then, I think that the call should just return -EBUSY when you cannot
> load the sealed data if no slots are available ?

Well this is kind of argument where there is no argument. I already had
plans how to do access broker back in 2014 that are more or less along
the lines of the pseudo code you sent. The problem is the lack of active
maintainers in the subsystem. That's why I get easily frustated
discussing about access broker in the first place :)

I would have implemented access broker months and months ago if I didn't
have so much code in the queue for this subsystem. There can be literally
months delay without any feedback. Right now I have four different
patches or patch sets in the queue:

- PPI support (yes you cannot enable TPM chips at the moment from Linux)
- Two bug fixes (CRB command buffer alignment, dTPM2 ACPI hot plugging)
- Basic trusted keys

I wouldn't blame any particular person about the situation but things
cannot continue like this. I've been thinking if I could acquire
co-maintainership of the subsystem for TPM 2 parts (don't really have
time or expertise for TPM 1.x parts).

I could post my architecture (never really written it except in my head
but should not take too long time) to my blog at jsakkine.blogspot.com
if you are interested discussing more.

> I looked at Patch 3/4 and it seems you default to -EPERM on TPM2_Create()-
> and TPM2_Load()-failures ?
> You might want to test against rc == TPM_RC_OBJECT_MEMORY and return -EBUSY
> in those cases. Would you agree ?
> (P.S. I can cross-post there if that's prefered ?)

Have to check the return values. I posted this patch set already in
early July. You are the first reviewer in three months for this patch
set.

I think the reason was that for TPM 1.x returned -EPERM in all error
scenarios and I didn't want to endanger behaviour of command-line tools
such as 'keyctl'. I would keep it that way unless you can guarantee that
command-line tools will continue work correctly if I change it to
-EBUSY.

Anyway, I will recheck this part of the patch set but likely are not
going to do any changes because I don't want to break the user space.

I will consider revising the patch set with keyhandle required as an
explicit option.

> Cheers,
> Andreas

/Jarkko

RE: [tpmdd-devel] [PATCH 4/4] keys, trusted: seal/unseal with TPM 2.0 chips

[Fuchs, Andreas]

> > I was just trying to point out that the concept is not too difficult, since
> > kernel-space (minimal) resource-manager makes a lot of people go "oh god,
> > never ever, way too big, way too complicated", which IMHO it is not.
> > Until then, I think that the call should just return -EBUSY when you cannot
> > load the sealed data if no slots are available ?
> 
> Well this is kind of argument where there is no argument. I already had
> plans how to do access broker back in 2014 that are more or less along
> the lines of the pseudo code you sent. The problem is the lack of active
> maintainers in the subsystem. That's why I get easily frustated
> discussing about access broker in the first place :)
> 
> I would have implemented access broker months and months ago if I didn't
> have so much code in the queue for this subsystem. There can be literally
> months delay without any feedback. Right now I have four different
> patches or patch sets in the queue:
> 
> - PPI support (yes you cannot enable TPM chips at the moment from Linux)
> - Two bug fixes (CRB command buffer alignment, dTPM2 ACPI hot plugging)
> - Basic trusted keys
> 
> I wouldn't blame any particular person about the situation but things
> cannot continue like this. I've been thinking if I could acquire
> co-maintainership of the subsystem for TPM 2 parts (don't really have
> time or expertise for TPM 1.x parts).

I think I know this situation. You have all my sympathies... ;-)

> I could post my architecture (never really written it except in my head
> but should not take too long time) to my blog at jsakkine.blogspot.com
> if you are interested discussing more.

Well, I came in to tpmdd-devel rather recently and only with a small time budget
to spend, but I'd be highly interested to learn about your thoughts.

As you can tell, I've been involved on the userspace side of things and
therefore already bent my head around some different architectures for
different scenarios. Also your input might help us in the specification of
userspace side as well.

So please go ahead and write it up, if you can spare the time.
Or let's get on the phone some time.

> > I looked at Patch 3/4 and it seems you default to -EPERM on TPM2_Create()-
> > and TPM2_Load()-failures ?
> > You might want to test against rc == TPM_RC_OBJECT_MEMORY and return -EBUSY
> > in those cases. Would you agree ?
> > (P.S. I can cross-post there if that's prefered ?)
> 
> Have to check the return values. I posted this patch set already in
> early July. You are the first reviewer in three months for this patch
> set.
> 
> I think the reason was that for TPM 1.x returned -EPERM in all error
> scenarios and I didn't want to endanger behaviour of command-line tools
> such as 'keyctl'. I would keep it that way unless you can guarantee that
> command-line tools will continue work correctly if I change it to
> -EBUSY.
> 
> Anyway, I will recheck this part of the patch set but likely are not
> going to do any changes because I don't want to break the user space.
> 
> I will consider revising the patch set with keyhandle required as an
> explicit option.

Hmm... Will the old keyctl work without modification with the 2.0 patches
anyways ?
The different keyHandle values and missing default keyHandle will yield
"differences" anyways, I'd say.
IMHO, we should get it as correct as possible given that TPM 2.0 is still
very young.

Is adding "additional" ReturnCodes considered ABI-incompatible breaking
anyways ?

Cheers,
Andreas

Re: [tpmdd-devel] [PATCH 4/4] keys, trusted: seal/unseal with TPM 2.0 chips

[Jarkko Sakkinen]

On Tue, Oct 06, 2015 at 01:16:02PM +0000, Fuchs, Andreas wrote:
> > > I was just trying to point out that the concept is not too difficult, since
> > > kernel-space (minimal) resource-manager makes a lot of people go "oh god,
> > > never ever, way too big, way too complicated", which IMHO it is not.
> > > Until then, I think that the call should just return -EBUSY when you cannot
> > > load the sealed data if no slots are available ?
> > 
> > Well this is kind of argument where there is no argument. I already had
> > plans how to do access broker back in 2014 that are more or less along
> > the lines of the pseudo code you sent. The problem is the lack of active
> > maintainers in the subsystem. That's why I get easily frustated
> > discussing about access broker in the first place :)
> > 
> > I would have implemented access broker months and months ago if I didn't
> > have so much code in the queue for this subsystem. There can be literally
> > months delay without any feedback. Right now I have four different
> > patches or patch sets in the queue:
> > 
> > - PPI support (yes you cannot enable TPM chips at the moment from Linux)
> > - Two bug fixes (CRB command buffer alignment, dTPM2 ACPI hot plugging)
> > - Basic trusted keys
> > 
> > I wouldn't blame any particular person about the situation but things
> > cannot continue like this. I've been thinking if I could acquire
> > co-maintainership of the subsystem for TPM 2 parts (don't really have
> > time or expertise for TPM 1.x parts).
> 
> I think I know this situation. You have all my sympathies... ;-)
> 
> > I could post my architecture (never really written it except in my head
> > but should not take too long time) to my blog at jsakkine.blogspot.com
> > if you are interested discussing more.
> 
> Well, I came in to tpmdd-devel rather recently and only with a small time budget
> to spend, but I'd be highly interested to learn about your thoughts.
> 
> As you can tell, I've been involved on the userspace side of things and
> therefore already bent my head around some different architectures for
> different scenarios. Also your input might help us in the specification of
> userspace side as well.
> 
> So please go ahead and write it up, if you can spare the time.
> Or let's get on the phone some time.
> 
> > > I looked at Patch 3/4 and it seems you default to -EPERM on TPM2_Create()-
> > > and TPM2_Load()-failures ?
> > > You might want to test against rc == TPM_RC_OBJECT_MEMORY and return -EBUSY
> > > in those cases. Would you agree ?
> > > (P.S. I can cross-post there if that's prefered ?)
> > 
> > Have to check the return values. I posted this patch set already in
> > early July. You are the first reviewer in three months for this patch
> > set.
> > 
> > I think the reason was that for TPM 1.x returned -EPERM in all error
> > scenarios and I didn't want to endanger behaviour of command-line tools
> > such as 'keyctl'. I would keep it that way unless you can guarantee that
> > command-line tools will continue work correctly if I change it to
> > -EBUSY.
> > 
> > Anyway, I will recheck this part of the patch set but likely are not
> > going to do any changes because I don't want to break the user space.
> > 
> > I will consider revising the patch set with keyhandle required as an
> > explicit option.
> 
> Hmm... Will the old keyctl work without modification with the 2.0 patches
> anyways ?

Yes it does and it should. I've been using keyctl utility to test my
patch set.

> The different keyHandle values and missing default keyHandle will yield
> "differences" anyways, I'd say.
> IMHO, we should get it as correct as possible given that TPM 2.0 is still
> very young.
> 
> Is adding "additional" ReturnCodes considered ABI-incompatible breaking
> anyways ?

Yes they are if they make the user space utiltiy malfunction.

> Cheers,
> Andreas

/Jarkko

RE: [tpmdd-devel] [PATCH 4/4] keys, trusted: seal/unseal with TPM 2.0 chips

[Fuchs, Andreas]

> > > > I looked at Patch 3/4 and it seems you default to -EPERM on TPM2_Create()-
> > > > and TPM2_Load()-failures ?
> > > > You might want to test against rc == TPM_RC_OBJECT_MEMORY and return -EBUSY
> > > > in those cases. Would you agree ?
> > > > (P.S. I can cross-post there if that's prefered ?)
> > >
> > > Have to check the return values. I posted this patch set already in
> > > early July. You are the first reviewer in three months for this patch
> > > set.
> > >
> > > I think the reason was that for TPM 1.x returned -EPERM in all error
> > > scenarios and I didn't want to endanger behaviour of command-line tools
> > > such as 'keyctl'. I would keep it that way unless you can guarantee that
> > > command-line tools will continue work correctly if I change it to
> > > -EBUSY.
> > >
> > > Anyway, I will recheck this part of the patch set but likely are not
> > > going to do any changes because I don't want to break the user space.
> > >
> > > I will consider revising the patch set with keyhandle required as an
> > > explicit option.
> >
> > Hmm... Will the old keyctl work without modification with the 2.0 patches
> > anyways ?
> 
> Yes it does and it should. I've been using keyctl utility to test my
> patch set.
> 
> > The different keyHandle values and missing default keyHandle will yield
> > "differences" anyways, I'd say.
> > IMHO, we should get it as correct as possible given that TPM 2.0 is still
> > very young.
> >
> > Is adding "additional" ReturnCodes considered ABI-incompatible breaking
> > anyways ?
> 
> Yes they are if they make the user space utiltiy malfunction.

AFAICT, keyctl just perror()s. Which is what I would have hoped.
So it guess it should work with -EBUSY.
Example-Trace of calls for key_adding:
http://anonscm.debian.org/cgit/collab-maint/keyutils.git/tree/keyutils.c#n43
http://anonscm.debian.org/cgit/collab-maint/keyutils.git/tree/keyctl.c#n379
http://anonscm.debian.org/cgit/collab-maint/keyutils.git/tree/keyctl.c#n131

Wish I could test it myself.
I understand, if you don't want to test my thoughts on this.
I just cannot perform the tests myself right now... :-(

Cheers,
Andreas

Re: [tpmdd-devel] [PATCH 4/4] keys, trusted: seal/unseal with TPM 2.0 chips

[Jarkko Sakkinen]

On Wed, Oct 07, 2015 at 10:04:40AM +0000, Fuchs, Andreas wrote:
> > > > > I looked at Patch 3/4 and it seems you default to -EPERM on TPM2_Create()-
> > > > > and TPM2_Load()-failures ?
> > > > > You might want to test against rc == TPM_RC_OBJECT_MEMORY and return -EBUSY
> > > > > in those cases. Would you agree ?
> > > > > (P.S. I can cross-post there if that's prefered ?)
> > > >
> > > > Have to check the return values. I posted this patch set already in
> > > > early July. You are the first reviewer in three months for this patch
> > > > set.
> > > >
> > > > I think the reason was that for TPM 1.x returned -EPERM in all error
> > > > scenarios and I didn't want to endanger behaviour of command-line tools
> > > > such as 'keyctl'. I would keep it that way unless you can guarantee that
> > > > command-line tools will continue work correctly if I change it to
> > > > -EBUSY.
> > > >
> > > > Anyway, I will recheck this part of the patch set but likely are not
> > > > going to do any changes because I don't want to break the user space.
> > > >
> > > > I will consider revising the patch set with keyhandle required as an
> > > > explicit option.
> > >
> > > Hmm... Will the old keyctl work without modification with the 2.0 patches
> > > anyways ?
> > 
> > Yes it does and it should. I've been using keyctl utility to test my
> > patch set.
> > 
> > > The different keyHandle values and missing default keyHandle will yield
> > > "differences" anyways, I'd say.
> > > IMHO, we should get it as correct as possible given that TPM 2.0 is still
> > > very young.
> > >
> > > Is adding "additional" ReturnCodes considered ABI-incompatible breaking
> > > anyways ?
> > 
> > Yes they are if they make the user space utiltiy malfunction.
> 
> AFAICT, keyctl just perror()s. Which is what I would have hoped.
> So it guess it should work with -EBUSY.
> Example-Trace of calls for key_adding:
> http://anonscm.debian.org/cgit/collab-maint/keyutils.git/tree/keyutils.c#n43
> http://anonscm.debian.org/cgit/collab-maint/keyutils.git/tree/keyctl.c#n379
> http://anonscm.debian.org/cgit/collab-maint/keyutils.git/tree/keyctl.c#n131
> 
> Wish I could test it myself.
> I understand, if you don't want to test my thoughts on this.
> I just cannot perform the tests myself right now... :-(

I would submit this change as a separate patch later anyway and not
include it into this patch set. If it doesn't do harm it can be added
later on. This patch set has been now in queue for three months so I
only make modifications that are absolutely necessary.

Changing keyhandle as mandatory option seems like such changes. This
doesn't.

> Cheers,
> Andreas--

/Jarkko

RE: [tpmdd-devel] [PATCH 4/4] keys, trusted: seal/unseal with TPM 2.0 chips

[Fuchs, Andreas]

> > > > > > I looked at Patch 3/4 and it seems you default to -EPERM on TPM2_Create()-
> > > > > > and TPM2_Load()-failures ?
> > > > > > You might want to test against rc == TPM_RC_OBJECT_MEMORY and return -EBUSY
> > > > > > in those cases. Would you agree ?
> > > > > > (P.S. I can cross-post there if that's prefered ?)
> > > > >
> > > > > Have to check the return values. I posted this patch set already in
> > > > > early July. You are the first reviewer in three months for this patch
> > > > > set.
> > > > >
> > > > > I think the reason was that for TPM 1.x returned -EPERM in all error
> > > > > scenarios and I didn't want to endanger behaviour of command-line tools
> > > > > such as 'keyctl'. I would keep it that way unless you can guarantee that
> > > > > command-line tools will continue work correctly if I change it to
> > > > > -EBUSY.
> > > > >
> > > > > Anyway, I will recheck this part of the patch set but likely are not
> > > > > going to do any changes because I don't want to break the user space.
> > > > >
> > > > > I will consider revising the patch set with keyhandle required as an
> > > > > explicit option.
> > > >
> > > > Hmm... Will the old keyctl work without modification with the 2.0 patches
> > > > anyways ?
> > >
> > > Yes it does and it should. I've been using keyctl utility to test my
> > > patch set.
> > >
> > > > The different keyHandle values and missing default keyHandle will yield
> > > > "differences" anyways, I'd say.
> > > > IMHO, we should get it as correct as possible given that TPM 2.0 is still
> > > > very young.
> > > >
> > > > Is adding "additional" ReturnCodes considered ABI-incompatible breaking
> > > > anyways ?
> > >
> > > Yes they are if they make the user space utiltiy malfunction.
> >
> > AFAICT, keyctl just perror()s. Which is what I would have hoped.
> > So it guess it should work with -EBUSY.
> > Example-Trace of calls for key_adding:
> > http://anonscm.debian.org/cgit/collab-maint/keyutils.git/tree/keyutils.c#n43
> > http://anonscm.debian.org/cgit/collab-maint/keyutils.git/tree/keyctl.c#n379
> > http://anonscm.debian.org/cgit/collab-maint/keyutils.git/tree/keyctl.c#n131
> >
> > Wish I could test it myself.
> > I understand, if you don't want to test my thoughts on this.
> > I just cannot perform the tests myself right now... :-(
> 
> I would submit this change as a separate patch later anyway and not
> include it into this patch set. If it doesn't do harm it can be added
> later on. This patch set has been now in queue for three months so I
> only make modifications that are absolutely necessary.
> 
> Changing keyhandle as mandatory option seems like such changes. This
> doesn't.

Fine with me.

P.S. do you have a git repo with all your queued and future patches at HEAD ?

Cheers,
Andreas

Re: [tpmdd-devel] [PATCH 4/4] keys, trusted: seal/unseal with TPM 2.0 chips

[Jarkko Sakkinen]

On Wed, 2015-10-07 at 10:32 +0000, Fuchs, Andreas wrote:
> > > > > > > I looked at Patch 3/4 and it seems you default to -EPERM
> > > > > > > on TPM2_Create()-
> > > > > > > and TPM2_Load()-failures ?
> > > > > > > You might want to test against rc == TPM_RC_OBJECT_MEMORY
> > > > > > > and return -EBUSY
> > > > > > > in those cases. Would you agree ?
> > > > > > > (P.S. I can cross-post there if that's prefered ?)
> > > > > > 
> > > > > > Have to check the return values. I posted this patch set
> > > > > > already in
> > > > > > early July. You are the first reviewer in three months for
> > > > > > this patch
> > > > > > set.
> > > > > > 
> > > > > > I think the reason was that for TPM 1.x returned -EPERM in
> > > > > > all error
> > > > > > scenarios and I didn't want to endanger behaviour of
> > > > > > command-line tools
> > > > > > such as 'keyctl'. I would keep it that way unless you can
> > > > > > guarantee that
> > > > > > command-line tools will continue work correctly if I change
> > > > > > it to
> > > > > > -EBUSY.
> > > > > > 
> > > > > > Anyway, I will recheck this part of the patch set but
> > > > > > likely are not
> > > > > > going to do any changes because I don't want to break the
> > > > > > user space.
> > > > > > 
> > > > > > I will consider revising the patch set with keyhandle
> > > > > > required as an
> > > > > > explicit option.
> > > > > 
> > > > > Hmm... Will the old keyctl work without modification with the
> > > > > 2.0 patches
> > > > > anyways ?
> > > > 
> > > > Yes it does and it should. I've been using keyctl utility to
> > > > test my
> > > > patch set.
> > > > 
> > > > > The different keyHandle values and missing default keyHandle
> > > > > will yield
> > > > > "differences" anyways, I'd say.
> > > > > IMHO, we should get it as correct as possible given that TPM
> > > > > 2.0 is still
> > > > > very young.
> > > > > 
> > > > > Is adding "additional" ReturnCodes considered ABI
> > > > > -incompatible breaking
> > > > > anyways ?
> > > > 
> > > > Yes they are if they make the user space utiltiy malfunction.
> > > 
> > > AFAICT, keyctl just perror()s. Which is what I would have hoped.
> > > So it guess it should work with -EBUSY.
> > > Example-Trace of calls for key_adding:
> > > http://anonscm.debian.org/cgit/collab-maint/keyutils.git/tree/key
> > > utils.c#n43
> > > http://anonscm.debian.org/cgit/collab-maint/keyutils.git/tree/key
> > > ctl.c#n379
> > > http://anonscm.debian.org/cgit/collab-maint/keyutils.git/tree/key
> > > ctl.c#n131
> > > 
> > > Wish I could test it myself.
> > > I understand, if you don't want to test my thoughts on this.
> > > I just cannot perform the tests myself right now... :-(
> > 
> > I would submit this change as a separate patch later anyway and not
> > include it into this patch set. If it doesn't do harm it can be
> > added
> > later on. This patch set has been now in queue for three months so
> > I
> > only make modifications that are absolutely necessary.
> > 
> > Changing keyhandle as mandatory option seems like such changes.
> > This
> > doesn't.
> 
> Fine with me.
> 
> P.S. do you have a git repo with all your queued and future patches
> at HEAD ?

In separate branches:

https://github.com/jsakkine/linux-tpm2/branches

> Cheers,
> Andreas

/Jarkko