[PATCH 0/2] Add session isolation and context saving to the space manager

[PATCH 0/2] Add session isolation and context saving to the space manager

[James Bottomley]

As requested, the first patch adds isolation and the second does
context switching.  I've also removed the flush emulation which changed
transient object accounting from lazy to strict.  Note that session
tracking has to be strict because the TPM needs to manage these closely
to avoid running out of global session numbers.

James

---

James Bottomley (2):
  tpm2: add session handle isolation to tpm spaces
  tpm2: context save and restore space managed sessions

 drivers/char/tpm/tpm-chip.c   |   6 +
 drivers/char/tpm/tpm.h        |   3 +
 drivers/char/tpm/tpm2-space.c | 377 ++++++++++++++++++++++++++++++++++--------
 drivers/char/tpm/tpms-dev.c   |   8 +
 4 files changed, 325 insertions(+), 69 deletions(-)

-- 
2.6.6

[PATCH 1/2] tpm2: add session handle isolation to tpm spaces

[James Bottomley]

sessions should be isolated during each instance of a tpm space.  This
means that spaces shouldn't be able to see each other's sessions and
also when a space is closed, all the sessions belonging to it should
be flushed.

This is implemented by adding a session_tbl to the space to track the
created session handles.  Sessions can be flushed either by not
setting the continueSession attribute in the session table or by an
explicit flush.  In the first case we have to mark the session as
being ready to flush and explicitly forget it if the command completes
successfully and in the second case we have to intercept the flush
instruction and clear the session from our table.

Finally, when the device handling the space is closed, we have to send
explicit flushes to all the remaining sessions belonging to the space
to ensure they are cleared out.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/char/tpm/tpm.h        |   2 +
 drivers/char/tpm/tpm2-space.c | 178 ++++++++++++++++++++++++++++++++++++++++--
 drivers/char/tpm/tpms-dev.c   |   1 +
 3 files changed, 173 insertions(+), 8 deletions(-)

diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 3346c48..265b7f5 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -158,6 +158,7 @@ enum tpm2_cc_attrs {
 struct tpm_space {
 	u32 context_tbl[14];
 	u8 *context_buf;
+	u32 session_tbl[6];
 };
 
 enum tpm_chip_flags {
@@ -584,4 +585,5 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space,
 		       u32 cc, u8 *buf, size_t bufsiz);
 int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space,
 		      u32 cc, u8 *buf, size_t bufsiz);
+void tpm2_flush_space(struct tpm_chip *chip, struct tpm_space *space);
 #endif
diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
index 3708e70..49048af 100644
--- a/drivers/char/tpm/tpm2-space.c
+++ b/drivers/char/tpm/tpm2-space.c
@@ -25,15 +25,83 @@ enum tpm2_handle_types {
 	TPM2_HT_TRANSIENT	= 0x80000000,
 };
 
-static void tpm2_flush_space(struct tpm_chip *chip)
+#define TPM2_HT_TAG_FOR_FLUSH	0xF0000000
+
+static int tpm2_session_find(struct tpm_space *space, u32 handle)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(space->session_tbl); i++)
+		if (handle == space->session_tbl[i])
+			break;
+	if (i == ARRAY_SIZE(space->session_tbl))
+		return -1;
+	return i;
+}
+
+static int tpm2_session_add(struct tpm_chip *chip,
+			    struct tpm_space *space, u32 handle)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(space->session_tbl); i++)
+		if (space->session_tbl[i] == 0)
+			break;
+	if (i == ARRAY_SIZE(space->session_tbl)) {
+		dev_err(&chip->dev, "out of session slots\n");
+		tpm2_flush_context_cmd(chip, handle, TPM_TRANSMIT_UNLOCKED);
+		return -ENOMEM;
+	}
+
+	space->session_tbl[i] = handle;
+
+	return 0;
+}
+
+/* if a space is active, emulate some commands */
+static int tpm2_intercept(struct tpm_chip *chip, struct tpm_space *space,
+			  u32 cc, u8 *buf, size_t bufsiz)
+{
+	int j;
+	u32 handle, handle_type;
+
+	if (!space)
+		return 0;
+
+	if (cc != TPM2_CC_FLUSH_CONTEXT)
+		return 0;
+	handle = get_unaligned_be32((__be32 *)&buf[10]);
+	handle_type = (handle & 0xFF000000);
+
+	if (handle_type != TPM2_HT_HMAC_SESSION &&
+	    handle_type != TPM2_HT_POLICY_SESSION)
+		/* let the TPM figure out and return the error */
+		return 0;
+
+	j = tpm2_session_find(space, handle);
+	if (j < 0)
+		return -EINVAL;
+
+	space->session_tbl[j] |= TPM2_HT_TAG_FOR_FLUSH;
+
+	return 0;
+}
+
+void tpm2_flush_space(struct tpm_chip *chip, struct tpm_space *space)
 {
-	struct tpm_space *space = &chip->work_space;
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(space->context_tbl); i++)
 		if (space->context_tbl[i] && ~space->context_tbl[i])
 			tpm2_flush_context_cmd(chip, space->context_tbl[i],
 					       TPM_TRANSMIT_UNLOCKED);
+
+	for (i = 0; i < ARRAY_SIZE(space->session_tbl); i++) {
+		space->session_tbl[i] &= ~TPM2_HT_TAG_FOR_FLUSH;
+		if (space->session_tbl[i])
+			tpm2_flush_context_cmd(chip, space->session_tbl[i],
+					       TPM_TRANSMIT_UNLOCKED);
+	}
 }
 
 struct tpm2_context {
@@ -94,10 +162,82 @@ static int tpm2_load_space(struct tpm_chip *chip)
 
 out_err:
 	tpm_buf_destroy(&buf);
-	tpm2_flush_space(chip);
+	tpm2_flush_space(chip, space);
 	return rc;
 }
 
+static void tpm2_unmap_sessions(struct tpm_chip *chip, u32 rc)
+{
+	struct tpm_space *space = &chip->work_space;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(space->session_tbl); i++) {
+		if ((space->session_tbl[i] & TPM2_HT_TAG_FOR_FLUSH) !=
+		    TPM2_HT_TAG_FOR_FLUSH)
+			continue;
+		if (rc == TPM2_RC_SUCCESS)
+			space->session_tbl[i] = 0;
+		else
+			/* for unsuccessful command, keep session */
+			space->session_tbl[i] &= ~TPM2_HT_TAG_FOR_FLUSH;
+	}
+}
+
+static int tpm2_map_sessions(struct tpm_chip *chip, u8 *buf, size_t len,
+			     size_t start)
+{
+	struct tpm_space *space = &chip->work_space;
+	u32 size = be32_to_cpup((__be32 *)&buf[start]);
+	int i;
+
+	/* skip over authorizationSize */
+	start += 4;
+
+	if (size > len - start) {
+		dev_err(&chip->dev, "Invalid authorization header size %u\n",
+			size);
+		return -EINVAL;
+	}
+
+	for (i = start; i < start+size; ) {
+		u16 skip;
+		u8 attr;
+		int j;
+		u32 handle, handle_type;
+
+		/* TPMI_SH_AUTH_SESSION */
+		handle = get_unaligned_be32((__be32 *)&buf[i]);
+		handle_type = handle & 0xFF000000;
+		i += 4;
+		/* TPM2B_DIGEST */
+		skip = get_unaligned_be16((__be16 *)&buf[i]);
+		i += skip + sizeof(skip);
+		/* TPMA_SESSION */
+		attr = buf[i++];
+		/* TPM2B_AUTH */
+		skip = get_unaligned_be16((__be16 *)&buf[i]);
+		i += skip + sizeof(skip);
+
+		if (handle_type != TPM2_HT_HMAC_SESSION &&
+		    handle_type != TPM2_HT_POLICY_SESSION)
+			continue;
+
+		j = tpm2_session_find(space, handle);
+		if (j < 0)
+			return -EINVAL;
+		if ((attr & 1) == 0)
+			/* session is flushed by the command */
+			space->session_tbl[j] |= TPM2_HT_TAG_FOR_FLUSH;
+	}
+
+	if (i != start+size) {
+		dev_err(&chip->dev, "Authorization session overflow\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int tpm2_map_command(struct tpm_chip *chip, u32 cc, u8 *cmd, size_t len)
 {
 	struct tpm_space *space = &chip->work_space;
@@ -105,6 +245,7 @@ static int tpm2_map_command(struct tpm_chip *chip, u32 cc, u8 *cmd, size_t len)
 	u32 vhandle;
 	u32 phandle;
 	u32 attrs;
+	u16 tag = get_unaligned_be16((__be16 *)cmd);
 	int i;
 	int j;
 	int rc;
@@ -132,11 +273,14 @@ static int tpm2_map_command(struct tpm_chip *chip, u32 cc, u8 *cmd, size_t len)
 		*((__be32 *)&cmd[TPM_HEADER_SIZE + 4 * i]) =
 			cpu_to_be32(phandle);
 	}
+	if (tag == TPM2_ST_SESSIONS)
+		tpm2_map_sessions(chip, cmd, len,
+				  TPM_HEADER_SIZE + 4*nr_handles);
 
 	return 0;
 
 out_err:
-	tpm2_flush_space(chip);
+	tpm2_flush_space(chip, space);
 	return rc;
 }
 
@@ -150,8 +294,14 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space,
 
 	memcpy(&chip->work_space.context_tbl, &space->context_tbl,
 	       sizeof(space->context_tbl));
+	memcpy(&chip->work_space.session_tbl, &space->session_tbl,
+	       sizeof(space->session_tbl));
 	memcpy(chip->work_space.context_buf, space->context_buf, PAGE_SIZE);
 
+	rc = tpm2_intercept(chip, space, cc, buf, bufsiz);
+	if (rc)
+		return rc;
+
 	rc = tpm2_load_space(chip);
 	if (rc)
 		return rc;
@@ -166,13 +316,17 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space,
 static int tpm2_map_response(struct tpm_chip *chip, u32 cc, u8 *rsp, size_t len)
 {
 	struct tpm_space *space = &chip->work_space;
-	u32 phandle;
+	u32 phandle, phandle_type;
 	u32 vhandle;
 	u32 attrs;
 	u32 return_code = get_unaligned_be32((__be32 *)&rsp[6]);
+	u16 tag = get_unaligned_be16((__be16 *)rsp);
 	int i;
 	int rc;
 
+	if (tag == TPM2_ST_SESSIONS)
+		tpm2_unmap_sessions(chip, return_code);
+
 	if (return_code != TPM2_RC_SUCCESS)
 		return 0;
 
@@ -188,9 +342,15 @@ static int tpm2_map_response(struct tpm_chip *chip, u32 cc, u8 *rsp, size_t len)
 		return 0;
 
 	phandle = be32_to_cpup((__be32 *)&rsp[TPM_HEADER_SIZE]);
-	if ((phandle & 0xFF000000) != TPM2_HT_TRANSIENT)
+	phandle_type = (phandle & 0xFF000000);
+	if (phandle_type != TPM2_HT_TRANSIENT &&
+	    phandle_type != TPM2_HT_HMAC_SESSION &&
+	    phandle_type != TPM2_HT_POLICY_SESSION)
 		return 0;
 
+	if (phandle_type != TPM2_HT_TRANSIENT)
+		return tpm2_session_add(chip, space, phandle);
+
 	/* Garbage collect a dead context. */
 	for (i = 0; i < ARRAY_SIZE(space->context_tbl); i++) {
 		if (space->context_tbl[i] == phandle) {
@@ -217,7 +377,7 @@ static int tpm2_map_response(struct tpm_chip *chip, u32 cc, u8 *rsp, size_t len)
 	return 0;
 
 out_err:
-	tpm2_flush_space(chip);
+	tpm2_flush_space(chip, space);
 	return rc;
 }
 
@@ -277,7 +437,7 @@ static int tpm2_save_space(struct tpm_chip *chip)
 	return 0;
 out_err:
 	tpm_buf_destroy(&buf);
-	tpm2_flush_space(chip);
+	tpm2_flush_space(chip, space);
 	return rc;
 }
 
@@ -299,6 +459,8 @@ int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space,
 
 	memcpy(&space->context_tbl, &chip->work_space.context_tbl,
 	       sizeof(space->context_tbl));
+	memcpy(&space->session_tbl, &chip->work_space.session_tbl,
+	       sizeof(space->session_tbl));
 	memcpy(space->context_buf, chip->work_space.context_buf, PAGE_SIZE);
 
 	return 0;
diff --git a/drivers/char/tpm/tpms-dev.c b/drivers/char/tpm/tpms-dev.c
index 6bb687f..d6e3491 100644
--- a/drivers/char/tpm/tpms-dev.c
+++ b/drivers/char/tpm/tpms-dev.c
@@ -36,6 +36,7 @@ static int tpms_release(struct inode *inode, struct file *file)
 	struct file_priv *fpriv = file->private_data;
 	struct tpms_priv *priv = container_of(fpriv, struct tpms_priv, priv);
 
+	tpm2_flush_space(fpriv->chip, &priv->space);
 	tpm_common_release(file, fpriv);
 	kfree(priv->space.context_buf);
 	kfree(priv);
-- 
2.6.6

[PATCH 2/2] tpm2: context save and restore space managed sessions

[James Bottomley]

Now that sessions are isolated, we can introduce a session_buf in the
tpm2 space to save and restore them.  This allows us to have many more
sessions active simultaneously (up to TPM_PT_MAX_SESSIONS).  As part
of this, we must intercept and manually remove contexts for flushed
sessions.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/char/tpm/tpm-chip.c   |   6 ++
 drivers/char/tpm/tpm.h        |   1 +
 drivers/char/tpm/tpm2-space.c | 223 ++++++++++++++++++++++++++++--------------
 drivers/char/tpm/tpms-dev.c   |   7 ++
 4 files changed, 164 insertions(+), 73 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 96ea93e..a625884 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -130,6 +130,7 @@ static void tpm_dev_release(struct device *dev)
 
 	kfree(chip->log.bios_event_log);
 	kfree(chip->work_space.context_buf);
+	kfree(chip->work_space.session_buf);
 	kfree(chip);
 }
 
@@ -223,6 +224,11 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
 		rc = -ENOMEM;
 		goto out;
 	}
+	chip->work_space.session_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
+	if (!chip->work_space.session_buf) {
+		rc = -ENOMEM;
+		goto out;
+	}
 
 	return chip;
 
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 265b7f5..9923daa 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -159,6 +159,7 @@ struct tpm_space {
 	u32 context_tbl[14];
 	u8 *context_buf;
 	u32 session_tbl[6];
+	u8 *session_buf;
 };
 
 enum tpm_chip_flags {
diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
index 49048af..04c9431 100644
--- a/drivers/char/tpm/tpm2-space.c
+++ b/drivers/char/tpm/tpm2-space.c
@@ -27,6 +27,91 @@ enum tpm2_handle_types {
 
 #define TPM2_HT_TAG_FOR_FLUSH	0xF0000000
 
+struct tpm2_context {
+	__be64 sequence;
+	__be32 saved_handle;
+	__be32 hierarchy;
+	__be16 blob_size;
+} __packed;
+
+static int tpm2_context_save(struct tpm_chip *chip, u8 *area,
+			     int *offset, u32 handle)
+{
+	struct tpm_buf buf;
+	u32 s;
+	int rc;
+
+	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS,
+			  TPM2_CC_CONTEXT_SAVE);
+	if (rc)
+		return rc;
+
+	tpm_buf_append_u32(&buf, handle);
+
+	rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE,
+			      TPM_HEADER_SIZE, TPM_TRANSMIT_UNLOCKED,
+			      NULL);
+	if ((rc & TPM2_RC_HANDLE) == TPM2_RC_HANDLE) {
+		/* no handle to save */
+		rc = 1;
+		goto out;
+	} else if (rc) {
+		dev_warn(&chip->dev, "%s: saving failed with %d\n",
+			 __func__, rc);
+		rc = -EFAULT;
+		goto out;
+	}
+
+	s = tpm_buf_length(&buf) - TPM_HEADER_SIZE;
+	if ((*offset + s) > PAGE_SIZE) {
+		dev_warn(&chip->dev, "out of context storage\n");
+		rc = -ENOMEM;
+		goto out;
+	}
+
+	memcpy(&area[*offset], &buf.data[TPM_HEADER_SIZE], s);
+	*offset += s;
+
+ out:
+	tpm_buf_destroy(&buf);
+	return rc;
+}
+
+static int tpm2_context_load(struct tpm_chip *chip, u8 *area,
+			     int *offset, u32 *handle)
+{
+	struct tpm_buf buf;
+	struct tpm2_context *ctx;
+	int rc;
+	u32 s;
+
+	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS,
+			  TPM2_CC_CONTEXT_LOAD);
+	if (rc)
+		return rc;
+
+	ctx = (struct tpm2_context *)&area[*offset];
+	s = sizeof(*ctx) + be16_to_cpu(ctx->blob_size);
+	tpm_buf_append(&buf, (const void *)ctx, s);
+
+	rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE,
+			      TPM_HEADER_SIZE + 4,
+			      TPM_TRANSMIT_UNLOCKED, NULL);
+	if (rc) {
+		dev_warn(&chip->dev, "context loading failed with %d\n", rc);
+		rc = -EFAULT;
+		goto out;
+	}
+	*handle = get_unaligned_be32((__be32 *)&buf.data[TPM_HEADER_SIZE]);
+
+	*offset += s;
+
+ out:
+	tpm_buf_destroy(&buf);
+
+	return rc;
+}
+
 static int tpm2_session_find(struct tpm_space *space, u32 handle)
 {
 	int i;
@@ -58,11 +143,35 @@ static int tpm2_session_add(struct tpm_chip *chip,
 	return 0;
 }
 
+static int tpm2_session_forget(struct tpm_space *space, u32 handle)
+{
+	int i, j;
+	struct tpm2_context *ctx;
+
+	for (i = 0, j = 0; i < ARRAY_SIZE(space->session_tbl); i++) {
+		if (space->session_tbl[i] == 0)
+			continue;
+
+		ctx = (struct tpm2_context *)&space->session_buf[j];
+		j += sizeof(*ctx) + get_unaligned_be16(&ctx->blob_size);
+
+		if (space->session_tbl[i] != handle)
+			continue;
+
+		/* forget the session context */
+		memcpy(ctx, &space->session_buf[j], PAGE_SIZE - j);
+		space->session_tbl[i] = 0;
+		break;
+	}
+	if (i == ARRAY_SIZE(space->session_tbl))
+		return -EINVAL;
+	return 0;
+}
+
 /* if a space is active, emulate some commands */
-static int tpm2_intercept(struct tpm_chip *chip, struct tpm_space *space,
-			  u32 cc, u8 *buf, size_t bufsiz)
+static int tpm2_intercept(struct tpm_chip *chip, u32 cc, u8 *buf, size_t bufsiz)
 {
-	int j;
+	struct tpm_space *space = &chip->work_space;
 	u32 handle, handle_type;
 
 	if (!space)
@@ -78,13 +187,7 @@ static int tpm2_intercept(struct tpm_chip *chip, struct tpm_space *space,
 		/* let the TPM figure out and return the error */
 		return 0;
 
-	j = tpm2_session_find(space, handle);
-	if (j < 0)
-		return -EINVAL;
-
-	space->session_tbl[j] |= TPM2_HT_TAG_FOR_FLUSH;
-
-	return 0;
+	return tpm2_session_forget(space, handle);
 }
 
 void tpm2_flush_space(struct tpm_chip *chip, struct tpm_space *space)
@@ -104,22 +207,12 @@ void tpm2_flush_space(struct tpm_chip *chip, struct tpm_space *space)
 	}
 }
 
-struct tpm2_context {
-	__be64 sequence;
-	__be32 saved_handle;
-	__be32 hierarchy;
-	__be16 blob_size;
-} __packed;
-
 static int tpm2_load_space(struct tpm_chip *chip)
 {
 	struct tpm_space *space = &chip->work_space;
-	struct tpm2_context *ctx;
-	struct tpm_buf buf;
 	int i;
 	int j;
 	int rc;
-	u32 s;
 
 	for (i = 0, j = 0; i < ARRAY_SIZE(space->context_tbl); i++) {
 		if (!space->context_tbl[i])
@@ -131,37 +224,33 @@ static int tpm2_load_space(struct tpm_chip *chip)
 			return -EFAULT;
 		}
 
-		rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS,
-				 TPM2_CC_CONTEXT_LOAD);
+		rc = tpm2_context_load(chip, space->context_buf,
+				       &j, &space->context_tbl[i]);
 		if (rc)
-			return rc;
-
-		ctx = (struct tpm2_context *)&space->context_buf[j];
-		s = sizeof(*ctx) + be16_to_cpu(ctx->blob_size);
-		tpm_buf_append(&buf, &space->context_buf[j], s);
-
-		rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE,
-				      TPM_HEADER_SIZE + 4,
-				      TPM_TRANSMIT_UNLOCKED, NULL);
-		if (rc) {
-			dev_warn(&chip->dev, "%s: loading failed with %d\n",
-				 __func__, rc);
-			rc = -EFAULT;
 			goto out_err;
-		}
 
-		space->context_tbl[i] =
-			be32_to_cpup((__be32 *)&buf.data[TPM_HEADER_SIZE]);
+	}
 
-		j += s;
+	for (i = 0, j = 0; i < ARRAY_SIZE(space->session_tbl); i++) {
+		u32 handle;
 
-		tpm_buf_destroy(&buf);
+		if (!space->session_tbl[i])
+			continue;
+
+		rc = tpm2_context_load(chip, space->session_buf,
+				       &j, &handle);
+		if (rc)
+			goto out_err;
+		if (handle != (space->session_tbl[i] & ~TPM2_HT_TAG_FOR_FLUSH)) {
+			dev_warn(&chip->dev, "session restored to wrong handle\n");
+			rc = -EFAULT;
+			goto out_err;
+		}
 	}
 
 	return 0;
 
 out_err:
-	tpm_buf_destroy(&buf);
 	tpm2_flush_space(chip, space);
 	return rc;
 }
@@ -297,8 +386,9 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space,
 	memcpy(&chip->work_space.session_tbl, &space->session_tbl,
 	       sizeof(space->session_tbl));
 	memcpy(chip->work_space.context_buf, space->context_buf, PAGE_SIZE);
+	memcpy(chip->work_space.session_buf, space->session_buf, PAGE_SIZE);
 
-	rc = tpm2_intercept(chip, space, cc, buf, bufsiz);
+	rc = tpm2_intercept(chip, cc, buf, bufsiz);
 	if (rc)
 		return rc;
 
@@ -384,59 +474,45 @@ static int tpm2_map_response(struct tpm_chip *chip, u32 cc, u8 *rsp, size_t len)
 static int tpm2_save_space(struct tpm_chip *chip)
 {
 	struct tpm_space *space = &chip->work_space;
-	struct tpm_buf buf;
 	int i;
 	int j;
 	int rc;
-	u32 s;
 
 	for (i = 0, j = 0; i < ARRAY_SIZE(space->context_tbl); i++) {
 		if (!(space->context_tbl[i] && ~space->context_tbl[i]))
 			continue;
 
-		rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS,
-				  TPM2_CC_CONTEXT_SAVE);
-		if (rc)
-			return rc;
-
-		tpm_buf_append_u32(&buf, space->context_tbl[i]);
-
-		rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE,
-				      TPM_HEADER_SIZE, TPM_TRANSMIT_UNLOCKED,
-				      NULL);
-		if ((rc & TPM2_RC_HANDLE) == TPM2_RC_HANDLE) {
+		rc = tpm2_context_save(chip, space->context_buf, &j,
+				       space->context_tbl[i]);
+		if (rc < 0)
+			goto out_err;
+		if (rc > 0) {
 			space->context_tbl[i] = 0;
 			continue;
-		} else if (rc) {
-			dev_warn(&chip->dev, "%s: saving failed with %d\n",
-				 __func__, rc);
-			rc = -EFAULT;
-			goto out_err;
 		}
 
-		s = tpm_buf_length(&buf) - TPM_HEADER_SIZE;
-		if ((j + s) > PAGE_SIZE) {
-			dev_warn(&chip->dev, "%s: out of backing storage\n",
-				 __func__);
-			rc = -ENOMEM;
-			goto out_err;
-		}
-
-		memcpy(&space->context_buf[j], &buf.data[TPM_HEADER_SIZE], s);
-
 		tpm2_flush_context_cmd(chip, space->context_tbl[i],
 				       TPM_TRANSMIT_UNLOCKED);
 
 		space->context_tbl[i] = ~0;
+	}
 
-		j += s;
+	for (i = 0, j = 0; i < ARRAY_SIZE(space->session_tbl); i++) {
+		if (!space->session_tbl[i])
+			continue;
 
-		tpm_buf_destroy(&buf);
+		rc = tpm2_context_save(chip, space->session_buf, &j,
+				       space->session_tbl[i]);
+		if (rc < 0)
+			goto out_err;
+		if (rc > 0) {
+			space->context_tbl[i] = 0;
+			continue;
+		}
 	}
 
 	return 0;
 out_err:
-	tpm_buf_destroy(&buf);
 	tpm2_flush_space(chip, space);
 	return rc;
 }
@@ -462,6 +538,7 @@ int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space,
 	memcpy(&space->session_tbl, &chip->work_space.session_tbl,
 	       sizeof(space->session_tbl));
 	memcpy(space->context_buf, chip->work_space.context_buf, PAGE_SIZE);
+	memcpy(space->session_buf, chip->work_space.session_buf, PAGE_SIZE);
 
 	return 0;
 }
diff --git a/drivers/char/tpm/tpms-dev.c b/drivers/char/tpm/tpms-dev.c
index d6e3491..12b6e34 100644
--- a/drivers/char/tpm/tpms-dev.c
+++ b/drivers/char/tpm/tpms-dev.c
@@ -25,6 +25,12 @@ static int tpms_open(struct inode *inode, struct file *file)
 		kfree(priv);
 		return -ENOMEM;
 	}
+	priv->space.session_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
+	if (priv->space.session_buf == NULL) {
+		kfree(priv->space.context_buf);
+		kfree(priv);
+		return -ENOMEM;
+	}
 
 	tpm_common_open(file, chip, &priv->priv);
 
@@ -39,6 +45,7 @@ static int tpms_release(struct inode *inode, struct file *file)
 	tpm2_flush_space(fpriv->chip, &priv->space);
 	tpm_common_release(file, fpriv);
 	kfree(priv->space.context_buf);
+	kfree(priv->space.session_buf);
 	kfree(priv);
 
 	return 0;
-- 
2.6.6

Re: [tpmdd-devel] [PATCH 1/2] tpm2: add session handle isolation to tpm spaces

[Jarkko Sakkinen]

On Wed, Jan 18, 2017 at 10:09:46AM -0500, James Bottomley wrote:
> sessions should be isolated during each instance of a tpm space.  This
> means that spaces shouldn't be able to see each other's sessions and
> also when a space is closed, all the sessions belonging to it should
> be flushed.
> 
> This is implemented by adding a session_tbl to the space to track the
> created session handles.  Sessions can be flushed either by not
> setting the continueSession attribute in the session table or by an
> explicit flush.  In the first case we have to mark the session as
> being ready to flush and explicitly forget it if the command completes
> successfully and in the second case we have to intercept the flush
> instruction and clear the session from our table.

You could do this without these nasty corner cases by arbage collecting
when a command emits a new session handle.

When a session handle is created check if any of the spaces contain it
and remove from the array. No special cases needed.

This will render the need to do any kind of interception whatsoever
unneeded.

/Jarkko

Re: [tpmdd-devel] [PATCH 2/2] tpm2: context save and restore space managed sessions

[Jarkko Sakkinen]

On Wed, Jan 18, 2017 at 10:10:42AM -0500, James Bottomley wrote:
> Now that sessions are isolated, we can introduce a session_buf in the
> tpm2 space to save and restore them.  This allows us to have many more
> sessions active simultaneously (up to TPM_PT_MAX_SESSIONS).  As part
> of this, we must intercept and manually remove contexts for flushed
> sessions.

Again I don't understand the interception part. Like with transient
objects I just catch TPM_RC_HANDLE error and forget them in the save
part.

PS. Do you mind if I take part of the patch that encapsulates a
single context save as of my patch that implements transient object
swapping? It merely moves the code in there to a different location.
Would just make the patch set cleaner. I would do this for v4 of the
patch set.

/Jarkko

> 
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> ---
>  drivers/char/tpm/tpm-chip.c   |   6 ++
>  drivers/char/tpm/tpm.h        |   1 +
>  drivers/char/tpm/tpm2-space.c | 223 ++++++++++++++++++++++++++++--------------
>  drivers/char/tpm/tpms-dev.c   |   7 ++
>  4 files changed, 164 insertions(+), 73 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 96ea93e..a625884 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -130,6 +130,7 @@ static void tpm_dev_release(struct device *dev)
>  
>  	kfree(chip->log.bios_event_log);
>  	kfree(chip->work_space.context_buf);
> +	kfree(chip->work_space.session_buf);
>  	kfree(chip);
>  }
>  
> @@ -223,6 +224,11 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
>  		rc = -ENOMEM;
>  		goto out;
>  	}
> +	chip->work_space.session_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
> +	if (!chip->work_space.session_buf) {
> +		rc = -ENOMEM;
> +		goto out;
> +	}
>  
>  	return chip;
>  
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 265b7f5..9923daa 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -159,6 +159,7 @@ struct tpm_space {
>  	u32 context_tbl[14];
>  	u8 *context_buf;
>  	u32 session_tbl[6];
> +	u8 *session_buf;
>  };
>  
>  enum tpm_chip_flags {
> diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
> index 49048af..04c9431 100644
> --- a/drivers/char/tpm/tpm2-space.c
> +++ b/drivers/char/tpm/tpm2-space.c
> @@ -27,6 +27,91 @@ enum tpm2_handle_types {
>  
>  #define TPM2_HT_TAG_FOR_FLUSH	0xF0000000
>  
> +struct tpm2_context {
> +	__be64 sequence;
> +	__be32 saved_handle;
> +	__be32 hierarchy;
> +	__be16 blob_size;
> +} __packed;
> +
> +static int tpm2_context_save(struct tpm_chip *chip, u8 *area,
> +			     int *offset, u32 handle)
> +{
> +	struct tpm_buf buf;
> +	u32 s;
> +	int rc;
> +
> +	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS,
> +			  TPM2_CC_CONTEXT_SAVE);
> +	if (rc)
> +		return rc;
> +
> +	tpm_buf_append_u32(&buf, handle);
> +
> +	rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE,
> +			      TPM_HEADER_SIZE, TPM_TRANSMIT_UNLOCKED,
> +			      NULL);
> +	if ((rc & TPM2_RC_HANDLE) == TPM2_RC_HANDLE) {
> +		/* no handle to save */
> +		rc = 1;
> +		goto out;
> +	} else if (rc) {
> +		dev_warn(&chip->dev, "%s: saving failed with %d\n",
> +			 __func__, rc);
> +		rc = -EFAULT;
> +		goto out;
> +	}
> +
> +	s = tpm_buf_length(&buf) - TPM_HEADER_SIZE;
> +	if ((*offset + s) > PAGE_SIZE) {
> +		dev_warn(&chip->dev, "out of context storage\n");
> +		rc = -ENOMEM;
> +		goto out;
> +	}
> +
> +	memcpy(&area[*offset], &buf.data[TPM_HEADER_SIZE], s);
> +	*offset += s;
> +
> + out:
> +	tpm_buf_destroy(&buf);
> +	return rc;
> +}
> +
> +static int tpm2_context_load(struct tpm_chip *chip, u8 *area,
> +			     int *offset, u32 *handle)
> +{
> +	struct tpm_buf buf;
> +	struct tpm2_context *ctx;
> +	int rc;
> +	u32 s;
> +
> +	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS,
> +			  TPM2_CC_CONTEXT_LOAD);
> +	if (rc)
> +		return rc;
> +
> +	ctx = (struct tpm2_context *)&area[*offset];
> +	s = sizeof(*ctx) + be16_to_cpu(ctx->blob_size);
> +	tpm_buf_append(&buf, (const void *)ctx, s);
> +
> +	rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE,
> +			      TPM_HEADER_SIZE + 4,
> +			      TPM_TRANSMIT_UNLOCKED, NULL);
> +	if (rc) {
> +		dev_warn(&chip->dev, "context loading failed with %d\n", rc);
> +		rc = -EFAULT;
> +		goto out;
> +	}
> +	*handle = get_unaligned_be32((__be32 *)&buf.data[TPM_HEADER_SIZE]);
> +
> +	*offset += s;
> +
> + out:
> +	tpm_buf_destroy(&buf);
> +
> +	return rc;
> +}
> +
>  static int tpm2_session_find(struct tpm_space *space, u32 handle)
>  {
>  	int i;
> @@ -58,11 +143,35 @@ static int tpm2_session_add(struct tpm_chip *chip,
>  	return 0;
>  }
>  
> +static int tpm2_session_forget(struct tpm_space *space, u32 handle)
> +{
> +	int i, j;
> +	struct tpm2_context *ctx;
> +
> +	for (i = 0, j = 0; i < ARRAY_SIZE(space->session_tbl); i++) {
> +		if (space->session_tbl[i] == 0)
> +			continue;
> +
> +		ctx = (struct tpm2_context *)&space->session_buf[j];
> +		j += sizeof(*ctx) + get_unaligned_be16(&ctx->blob_size);
> +
> +		if (space->session_tbl[i] != handle)
> +			continue;
> +
> +		/* forget the session context */
> +		memcpy(ctx, &space->session_buf[j], PAGE_SIZE - j);
> +		space->session_tbl[i] = 0;
> +		break;
> +	}
> +	if (i == ARRAY_SIZE(space->session_tbl))
> +		return -EINVAL;
> +	return 0;
> +}
> +
>  /* if a space is active, emulate some commands */
> -static int tpm2_intercept(struct tpm_chip *chip, struct tpm_space *space,
> -			  u32 cc, u8 *buf, size_t bufsiz)
> +static int tpm2_intercept(struct tpm_chip *chip, u32 cc, u8 *buf, size_t bufsiz)
>  {
> -	int j;
> +	struct tpm_space *space = &chip->work_space;
>  	u32 handle, handle_type;
>  
>  	if (!space)
> @@ -78,13 +187,7 @@ static int tpm2_intercept(struct tpm_chip *chip, struct tpm_space *space,
>  		/* let the TPM figure out and return the error */
>  		return 0;
>  
> -	j = tpm2_session_find(space, handle);
> -	if (j < 0)
> -		return -EINVAL;
> -
> -	space->session_tbl[j] |= TPM2_HT_TAG_FOR_FLUSH;
> -
> -	return 0;
> +	return tpm2_session_forget(space, handle);
>  }
>  
>  void tpm2_flush_space(struct tpm_chip *chip, struct tpm_space *space)
> @@ -104,22 +207,12 @@ void tpm2_flush_space(struct tpm_chip *chip, struct tpm_space *space)
>  	}
>  }
>  
> -struct tpm2_context {
> -	__be64 sequence;
> -	__be32 saved_handle;
> -	__be32 hierarchy;
> -	__be16 blob_size;
> -} __packed;
> -
>  static int tpm2_load_space(struct tpm_chip *chip)
>  {
>  	struct tpm_space *space = &chip->work_space;
> -	struct tpm2_context *ctx;
> -	struct tpm_buf buf;
>  	int i;
>  	int j;
>  	int rc;
> -	u32 s;
>  
>  	for (i = 0, j = 0; i < ARRAY_SIZE(space->context_tbl); i++) {
>  		if (!space->context_tbl[i])
> @@ -131,37 +224,33 @@ static int tpm2_load_space(struct tpm_chip *chip)
>  			return -EFAULT;
>  		}
>  
> -		rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS,
> -				 TPM2_CC_CONTEXT_LOAD);
> +		rc = tpm2_context_load(chip, space->context_buf,
> +				       &j, &space->context_tbl[i]);
>  		if (rc)
> -			return rc;
> -
> -		ctx = (struct tpm2_context *)&space->context_buf[j];
> -		s = sizeof(*ctx) + be16_to_cpu(ctx->blob_size);
> -		tpm_buf_append(&buf, &space->context_buf[j], s);
> -
> -		rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE,
> -				      TPM_HEADER_SIZE + 4,
> -				      TPM_TRANSMIT_UNLOCKED, NULL);
> -		if (rc) {
> -			dev_warn(&chip->dev, "%s: loading failed with %d\n",
> -				 __func__, rc);
> -			rc = -EFAULT;
>  			goto out_err;
> -		}
>  
> -		space->context_tbl[i] =
> -			be32_to_cpup((__be32 *)&buf.data[TPM_HEADER_SIZE]);
> +	}
>  
> -		j += s;
> +	for (i = 0, j = 0; i < ARRAY_SIZE(space->session_tbl); i++) {
> +		u32 handle;
>  
> -		tpm_buf_destroy(&buf);
> +		if (!space->session_tbl[i])
> +			continue;
> +
> +		rc = tpm2_context_load(chip, space->session_buf,
> +				       &j, &handle);
> +		if (rc)
> +			goto out_err;
> +		if (handle != (space->session_tbl[i] & ~TPM2_HT_TAG_FOR_FLUSH)) {
> +			dev_warn(&chip->dev, "session restored to wrong handle\n");
> +			rc = -EFAULT;
> +			goto out_err;
> +		}
>  	}
>  
>  	return 0;
>  
>  out_err:
> -	tpm_buf_destroy(&buf);
>  	tpm2_flush_space(chip, space);
>  	return rc;
>  }
> @@ -297,8 +386,9 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space,
>  	memcpy(&chip->work_space.session_tbl, &space->session_tbl,
>  	       sizeof(space->session_tbl));
>  	memcpy(chip->work_space.context_buf, space->context_buf, PAGE_SIZE);
> +	memcpy(chip->work_space.session_buf, space->session_buf, PAGE_SIZE);
>  
> -	rc = tpm2_intercept(chip, space, cc, buf, bufsiz);
> +	rc = tpm2_intercept(chip, cc, buf, bufsiz);
>  	if (rc)
>  		return rc;
>  
> @@ -384,59 +474,45 @@ static int tpm2_map_response(struct tpm_chip *chip, u32 cc, u8 *rsp, size_t len)
>  static int tpm2_save_space(struct tpm_chip *chip)
>  {
>  	struct tpm_space *space = &chip->work_space;
> -	struct tpm_buf buf;
>  	int i;
>  	int j;
>  	int rc;
> -	u32 s;
>  
>  	for (i = 0, j = 0; i < ARRAY_SIZE(space->context_tbl); i++) {
>  		if (!(space->context_tbl[i] && ~space->context_tbl[i]))
>  			continue;
>  
> -		rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS,
> -				  TPM2_CC_CONTEXT_SAVE);
> -		if (rc)
> -			return rc;
> -
> -		tpm_buf_append_u32(&buf, space->context_tbl[i]);
> -
> -		rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE,
> -				      TPM_HEADER_SIZE, TPM_TRANSMIT_UNLOCKED,
> -				      NULL);
> -		if ((rc & TPM2_RC_HANDLE) == TPM2_RC_HANDLE) {
> +		rc = tpm2_context_save(chip, space->context_buf, &j,
> +				       space->context_tbl[i]);
> +		if (rc < 0)
> +			goto out_err;
> +		if (rc > 0) {
>  			space->context_tbl[i] = 0;
>  			continue;
> -		} else if (rc) {
> -			dev_warn(&chip->dev, "%s: saving failed with %d\n",
> -				 __func__, rc);
> -			rc = -EFAULT;
> -			goto out_err;
>  		}
>  
> -		s = tpm_buf_length(&buf) - TPM_HEADER_SIZE;
> -		if ((j + s) > PAGE_SIZE) {
> -			dev_warn(&chip->dev, "%s: out of backing storage\n",
> -				 __func__);
> -			rc = -ENOMEM;
> -			goto out_err;
> -		}
> -
> -		memcpy(&space->context_buf[j], &buf.data[TPM_HEADER_SIZE], s);
> -
>  		tpm2_flush_context_cmd(chip, space->context_tbl[i],
>  				       TPM_TRANSMIT_UNLOCKED);
>  
>  		space->context_tbl[i] = ~0;
> +	}
>  
> -		j += s;
> +	for (i = 0, j = 0; i < ARRAY_SIZE(space->session_tbl); i++) {
> +		if (!space->session_tbl[i])
> +			continue;
>  
> -		tpm_buf_destroy(&buf);
> +		rc = tpm2_context_save(chip, space->session_buf, &j,
> +				       space->session_tbl[i]);
> +		if (rc < 0)
> +			goto out_err;
> +		if (rc > 0) {
> +			space->context_tbl[i] = 0;
> +			continue;
> +		}
>  	}
>  
>  	return 0;
>  out_err:
> -	tpm_buf_destroy(&buf);
>  	tpm2_flush_space(chip, space);
>  	return rc;
>  }
> @@ -462,6 +538,7 @@ int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space,
>  	memcpy(&space->session_tbl, &chip->work_space.session_tbl,
>  	       sizeof(space->session_tbl));
>  	memcpy(space->context_buf, chip->work_space.context_buf, PAGE_SIZE);
> +	memcpy(space->session_buf, chip->work_space.session_buf, PAGE_SIZE);
>  
>  	return 0;
>  }
> diff --git a/drivers/char/tpm/tpms-dev.c b/drivers/char/tpm/tpms-dev.c
> index d6e3491..12b6e34 100644
> --- a/drivers/char/tpm/tpms-dev.c
> +++ b/drivers/char/tpm/tpms-dev.c
> @@ -25,6 +25,12 @@ static int tpms_open(struct inode *inode, struct file *file)
>  		kfree(priv);
>  		return -ENOMEM;
>  	}
> +	priv->space.session_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
> +	if (priv->space.session_buf == NULL) {
> +		kfree(priv->space.context_buf);
> +		kfree(priv);
> +		return -ENOMEM;
> +	}
>  
>  	tpm_common_open(file, chip, &priv->priv);
>  
> @@ -39,6 +45,7 @@ static int tpms_release(struct inode *inode, struct file *file)
>  	tpm2_flush_space(fpriv->chip, &priv->space);
>  	tpm_common_release(file, fpriv);
>  	kfree(priv->space.context_buf);
> +	kfree(priv->space.session_buf);
>  	kfree(priv);
>  
>  	return 0;
> -- 
> 2.6.6
> 
> 
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, SlashDot.org! http://sdm.link/slashdot
> _______________________________________________
> tpmdd-devel mailing list
> tpmdd-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Re: [tpmdd-devel] [PATCH 1/2] tpm2: add session handle isolation to tpm spaces

[James Bottomley]

On Thu, 2017-01-19 at 13:58 +0200, Jarkko Sakkinen wrote:
> On Wed, Jan 18, 2017 at 10:09:46AM -0500, James Bottomley wrote:
> > sessions should be isolated during each instance of a tpm space. 
> >  This means that spaces shouldn't be able to see each other's 
> > sessions and also when a space is closed, all the sessions 
> > belonging to it should be flushed.
> > 
> > This is implemented by adding a session_tbl to the space to track 
> > the created session handles.  Sessions can be flushed either by not
> > setting the continueSession attribute in the session table or by an
> > explicit flush.  In the first case we have to mark the session as
> > being ready to flush and explicitly forget it if the command 
> > completes successfully and in the second case we have to intercept 
> > the flush instruction and clear the session from our table.
> 
> You could do this without these nasty corner cases by arbage 
> collecting when a command emits a new session handle.

I could for this patch set.  However, the global session accounting RFC
requires strict accounting, because it needs to know exactly when to
retry a command that failed because we were out of sessions and because
we don't want to needlessly evict a session if there was one available
which we didn't see because of lazy accounting.  It would be a lot of
churn to do it lazily in this patch set and then switch to strict in
that one, so I chose to account sessions strictly always.

> When a session handle is created check if any of the spaces contain 
> it and remove from the array. No special cases needed.
> 
> This will render the need to do any kind of interception whatsoever
> unneeded.

It can be done either way for these two patches, but I think, for the
reason above, it should begin life as it will go on (i.e. strict
accounting).  It's not that much extra code to do it and it's easier to
follow the flow of the three patches.

James

Re: [tpmdd-devel] [PATCH 2/2] tpm2: context save and restore space managed sessions

[James Bottomley]

On Thu, 2017-01-19 at 14:04 +0200, Jarkko Sakkinen wrote:
> On Wed, Jan 18, 2017 at 10:10:42AM -0500, James Bottomley wrote:
> > Now that sessions are isolated, we can introduce a session_buf in 
> > the tpm2 space to save and restore them.  This allows us to have 
> > many more sessions active simultaneously (up to 
> > TPM_PT_MAX_SESSIONS).  As part of this, we must intercept and 
> > manually remove contexts for flushed sessions.
> 
> Again I don't understand the interception part. Like with transient
> objects I just catch TPM_RC_HANDLE error and forget them in the save
> part.

it's for the global session tracking patch (see other email for
details)

> PS. Do you mind if I take part of the patch that encapsulates a
> single context save as of my patch that implements transient object
> swapping? It merely moves the code in there to a different location.
> Would just make the patch set cleaner. I would do this for v4 of the
> patch set.

Sure.  Rebase should be able to do this easily for me.

James

Re: [tpmdd-devel] [PATCH 1/2] tpm2: add session handle isolation to tpm spaces

[Jarkko Sakkinen]

On Thu, Jan 19, 2017 at 07:11:23AM -0500, James Bottomley wrote:
> On Thu, 2017-01-19 at 13:58 +0200, Jarkko Sakkinen wrote:
> > On Wed, Jan 18, 2017 at 10:09:46AM -0500, James Bottomley wrote:
> > > sessions should be isolated during each instance of a tpm space. 
> > >  This means that spaces shouldn't be able to see each other's 
> > > sessions and also when a space is closed, all the sessions 
> > > belonging to it should be flushed.
> > > 
> > > This is implemented by adding a session_tbl to the space to track 
> > > the created session handles.  Sessions can be flushed either by not
> > > setting the continueSession attribute in the session table or by an
> > > explicit flush.  In the first case we have to mark the session as
> > > being ready to flush and explicitly forget it if the command 
> > > completes successfully and in the second case we have to intercept 
> > > the flush instruction and clear the session from our table.
> > 
> > You could do this without these nasty corner cases by arbage 
> > collecting when a command emits a new session handle.
> 
> I could for this patch set.  However, the global session accounting RFC
> requires strict accounting, because it needs to know exactly when to
> retry a command that failed because we were out of sessions and because
> we don't want to needlessly evict a session if there was one available
> which we didn't see because of lazy accounting.  It would be a lot of
> churn to do it lazily in this patch set and then switch to strict in
> that one, so I chose to account sessions strictly always.

Lazy is kind of ambiguous word so I'll have to check that we have same
definition for it in this context.

I'm talking about not trying to detect if something gets deleted. When
something gets created you would go through the global list of sessions
and check if it is used. If so, it must be that the session was deleted
at some point.

Your argument is that in this scheme sometimes there might be a session
marked as "reserved" but it is in fact free. This might lead to useless
eviction. Am I correct?

My argument is that the lazy scheme is more generic (does not require
special cases). As a subsystem maintainer I tend to be more fond of that
kind of solutions. Having special cases raises questios like (for
example):

1. What if standard gets added something that does not fall into the
   current set of special cases? You never know.
2. What about vendor specific commands? The lazy scheme is compatible
   with them. The standard does not put any kind of constraints for
   vendor specific commands.

You could solve the problem you are stating by getting the full the
list of alive sessions with CAP_HANDLES and mark dangling sessions
as free.

PS. I've started to think that maybe also with sessions it is better
to have just one change that implements full eviction like we have
for transient objects after seeing your breakdown. I'm sorry about
putting you extra trouble doing the isolation only patch. It's better
to do this right once...

/Jarkko

Re: [tpmdd-devel] [PATCH 1/2] tpm2: add session handle isolation to tpm spaces

[James Bottomley]

On Fri, 2017-01-20 at 15:23 +0200, Jarkko Sakkinen wrote:
> On Thu, Jan 19, 2017 at 07:11:23AM -0500, James Bottomley wrote:
> > On Thu, 2017-01-19 at 13:58 +0200, Jarkko Sakkinen wrote:
> > > On Wed, Jan 18, 2017 at 10:09:46AM -0500, James Bottomley wrote:
> > > > sessions should be isolated during each instance of a tpm 
> > > > space.  This means that spaces shouldn't be able to see each 
> > > > other's sessions and also when a space is closed, all the 
> > > > sessions belonging to it should be flushed.
> > > > 
> > > > This is implemented by adding a session_tbl to the space to 
> > > > track the created session handles.  Sessions can be flushed 
> > > > either by not setting the continueSession attribute in the 
> > > > session table or by an explicit flush.  In the first case we 
> > > > have to mark the session as being ready to flush and explicitly 
> > > > forget it if the command completes successfully and in the 
> > > > second case we have to intercept the flush instruction and
> > > > clear the session from our table.
> > > 
> > > You could do this without these nasty corner cases by arbage 
> > > collecting when a command emits a new session handle.
> > 
> > I could for this patch set.  However, the global session accounting 
> > RFC requires strict accounting, because it needs to know exactly 
> > when to retry a command that failed because we were out of sessions 
> > and because we don't want to needlessly evict a session if there 
> > was one available which we didn't see because of lazy accounting. 
> >  It would be a lot of churn to do it lazily in this patch set and 
> > then switch to strict in that one, so I chose to account sessions
> > strictly always.
> 
> Lazy is kind of ambiguous word so I'll have to check that we have 
> same definition for it in this context.
> 
> I'm talking about not trying to detect if something gets deleted. 
> When something gets created you would go through the global list of
> sessions and check if it is used. If so, it must be that the session 
> was deleted at some point.

That's my terminology too.  We're talking about lazy and strict
tracking of session flushing.

> Your argument is that in this scheme sometimes there might be a
> session marked as "reserved" but it is in fact free. This might lead
> to useless eviction. Am I correct?

Yes, but not just that, it will also lead to over long waits because we
can no longer wake the waiters the moment a session becomes free.

> My argument is that the lazy scheme is more generic (does not require
> special cases). As a subsystem maintainer I tend to be more fond of 
> that kind of solutions. Having special cases raises questios like 
> (for example):
> 
> 1. What if standard gets added something that does not fall into the
>    current set of special cases? You never know.
> 2. What about vendor specific commands? The lazy scheme is compatible
>    with them. The standard does not put any kind of constraints for
>    vendor specific commands.

We rely on the assertion in the Manual that sessions are only returned
in the handle area (as we do for objects).  We also rely on the
guarantee that they're only destroyed by flush or continueSession being
0 in the session attributes.

If some mad vendor introduces a command that creates an object and
doesn't return it in the handle area, we'll get screwed for both
transient objects and sessions.  Sessions also could have issues if
some mad vendor creates a command that flushes them outside of the
above description.  That's why the standard has all these caveats about
handle and session creation.  In theory the vendors are not allowed to
violate them in their own commands ...

> You could solve the problem you are stating by getting the full the
> list of alive sessions with CAP_HANDLES and mark dangling sessions
> as free.

That's a command which produces a huge output ... I'd have to do it at
the end of every input command to get the list of current handles ... I
don't really think it's a better solution.

Let me describe the failure case with strict destruction accounting:
supposing a mad vendor does introduce a command that flushes a session
outside the standards prescribed way.  What happens is that it gets re
-used before the TPM exhausts handles, and tpm2_session_chip_add will
simply to replace what it currently has.  The only consequence is a
single missed wakeup.  So even in the face of vendor failure, this
scheme will work almost all of the time and it will always work better
than a lazy scheme because the failure case gives us properties
identical to the lazy case.  To make this case iron, we should get a
failure on context save, so I can use that failure to drop the handle
and I think the strict scheme will then always perform better than the
lazy scheme (let's call it strict with lazy backup) will that suffice?

> PS. I've started to think that maybe also with sessions it is better
> to have just one change that implements full eviction like we have
> for transient objects after seeing your breakdown. I'm sorry about
> putting you extra trouble doing the isolation only patch. It's better
> to do this right once...

Well, I can put them back together again, but you could just apply them
together as two patches ... they are now bisectable.

James


> /Jarkko
> 

Re: [tpmdd-devel] [PATCH 1/2] tpm2: add session handle isolation to tpm spaces

[Jarkko Sakkinen]

On Fri, Jan 20, 2017 at 09:39:13AM -0500, James Bottomley wrote:
> On Fri, 2017-01-20 at 15:23 +0200, Jarkko Sakkinen wrote:
> > On Thu, Jan 19, 2017 at 07:11:23AM -0500, James Bottomley wrote:
> > > On Thu, 2017-01-19 at 13:58 +0200, Jarkko Sakkinen wrote:
> > > > On Wed, Jan 18, 2017 at 10:09:46AM -0500, James Bottomley wrote:
> > > > > sessions should be isolated during each instance of a tpm 
> > > > > space.  This means that spaces shouldn't be able to see each 
> > > > > other's sessions and also when a space is closed, all the 
> > > > > sessions belonging to it should be flushed.
> > > > > 
> > > > > This is implemented by adding a session_tbl to the space to 
> > > > > track the created session handles.  Sessions can be flushed 
> > > > > either by not setting the continueSession attribute in the 
> > > > > session table or by an explicit flush.  In the first case we 
> > > > > have to mark the session as being ready to flush and explicitly 
> > > > > forget it if the command completes successfully and in the 
> > > > > second case we have to intercept the flush instruction and
> > > > > clear the session from our table.
> > > > 
> > > > You could do this without these nasty corner cases by arbage 
> > > > collecting when a command emits a new session handle.
> > > 
> > > I could for this patch set.  However, the global session accounting 
> > > RFC requires strict accounting, because it needs to know exactly 
> > > when to retry a command that failed because we were out of sessions 
> > > and because we don't want to needlessly evict a session if there 
> > > was one available which we didn't see because of lazy accounting. 
> > >  It would be a lot of churn to do it lazily in this patch set and 
> > > then switch to strict in that one, so I chose to account sessions
> > > strictly always.
> > 
> > Lazy is kind of ambiguous word so I'll have to check that we have 
> > same definition for it in this context.
> > 
> > I'm talking about not trying to detect if something gets deleted. 
> > When something gets created you would go through the global list of
> > sessions and check if it is used. If so, it must be that the session 
> > was deleted at some point.
> 
> That's my terminology too.  We're talking about lazy and strict
> tracking of session flushing.
> 
> > Your argument is that in this scheme sometimes there might be a
> > session marked as "reserved" but it is in fact free. This might lead
> > to useless eviction. Am I correct?
> 
> Yes, but not just that, it will also lead to over long waits because we
> can no longer wake the waiters the moment a session becomes free.
> 
> > My argument is that the lazy scheme is more generic (does not require
> > special cases). As a subsystem maintainer I tend to be more fond of 
> > that kind of solutions. Having special cases raises questios like 
> > (for example):
> > 
> > 1. What if standard gets added something that does not fall into the
> >    current set of special cases? You never know.
> > 2. What about vendor specific commands? The lazy scheme is compatible
> >    with them. The standard does not put any kind of constraints for
> >    vendor specific commands.
> 
> We rely on the assertion in the Manual that sessions are only returned
> in the handle area (as we do for objects).  We also rely on the
> guarantee that they're only destroyed by flush or continueSession being
> 0 in the session attributes.
> 
> If some mad vendor introduces a command that creates an object and
> doesn't return it in the handle area, we'll get screwed for both
> transient objects and sessions.  Sessions also could have issues if
> some mad vendor creates a command that flushes them outside of the
> above description.  That's why the standard has all these caveats about
> handle and session creation.  In theory the vendors are not allowed to
> violate them in their own commands ...
> 
> > You could solve the problem you are stating by getting the full the
> > list of alive sessions with CAP_HANDLES and mark dangling sessions
> > as free.
> 
> That's a command which produces a huge output ... I'd have to do it at
> the end of every input command to get the list of current handles ... I
> don't really think it's a better solution.
> 
> Let me describe the failure case with strict destruction accounting:
> supposing a mad vendor does introduce a command that flushes a session
> outside the standards prescribed way.  What happens is that it gets re
> -used before the TPM exhausts handles, and tpm2_session_chip_add will
> simply to replace what it currently has.  The only consequence is a
> single missed wakeup.  So even in the face of vendor failure, this
> scheme will work almost all of the time and it will always work better
> than a lazy scheme because the failure case gives us properties
> identical to the lazy case.  To make this case iron, we should get a
> failure on context save, so I can use that failure to drop the handle
> and I think the strict scheme will then always perform better than the
> lazy scheme (let's call it strict with lazy backup) will that suffice?
> 
> > PS. I've started to think that maybe also with sessions it is better
> > to have just one change that implements full eviction like we have
> > for transient objects after seeing your breakdown. I'm sorry about
> > putting you extra trouble doing the isolation only patch. It's better
> > to do this right once...
> 
> Well, I can put them back together again, but you could just apply them
> together as two patches ... they are now bisectable.

Sure forgot this last comment. It's really irrelevant. I'll reply
properly later on.

> James

/Jarkko

Re: [tpmdd-devel] [PATCH 1/2] tpm2: add session handle isolation to tpm spaces

[Jarkko Sakkinen]

On Fri, Jan 20, 2017 at 09:27:25AM -0500, Ken Goldman wrote:
> On 1/20/2017 8:23 AM, Jarkko Sakkinen wrote:
> >
> > I'm talking about not trying to detect if something gets deleted. When
> > something gets created you would go through the global list of sessions
> > and check if it is used. If so, it must be that the session was deleted
> > at some point.
> 
> Are you saying that, when a process flushes (or continue = false) a 
> session, this code won't actually flush the context?  You'll wait until 
> another startauthsession creates a handle, and then delete other 
> occurrences of it?

I just wouldn't get care if a session gets deleted. You can detect it
postmortem when something gets created with the same handle.

/Jarkko