[PATCH 0/2] Add session handling to tpm spaces

[PATCH 0/2] Add session handling to tpm spaces

[James Bottomley]

These two patches are easily separated.  The first does session
handling.  This incarnation uses TPM_RC_REFERENCE_H0 errors from
context save or load to signal when a session has been flushed (so it
no longer does tracking or flush emulation).  The second patch does
management for session exhaustion and is essentially unchanged from the
prior RFC.

James

---


James Bottomley (2):
  tpm2: add session handle context saving and restoring to the space code
  tpm2-space: add handling for global session exhaustion

 drivers/char/tpm/tpm-chip.c   |   7 +
 drivers/char/tpm/tpm.h        |  42 +++++-
 drivers/char/tpm/tpm2-cmd.c   |  15 +++
 drivers/char/tpm/tpm2-space.c | 294 ++++++++++++++++++++++++++++++++++++++++--
 drivers/char/tpm/tpms-dev.c   |  25 +++-
 5 files changed, 369 insertions(+), 14 deletions(-)

-- 
2.6.6

[PATCH 1/2] tpm2: add session handle context saving and restoring to the space code

[James Bottomley]

sessions are different from transient objects in that their handles
may not be virtualized (because they're used for some hmac
calculations).  Additionally when a session is context saved, a
vestigial memory remains in the TPM and if it is also flushed, that
will be lost and the session context will refuse to load next time, so
the code is updated to flush only transient objects after a context
save.  Add a separate array (chip->session_tbl) to save and restore
sessions by handle.  Use the failure of a context save or load to
signal that the session has been flushed from the TPM and we can
remove its memory from chip->session_tbl.

Sessions are also isolated during each instance of a tpm space.  This
means that spaces shouldn't be able to see each other's sessions and
is enforced by ensuring that a space user may only refer to sessions
handles that are present in their own chip->session_tbl.  Finally when
a space is closed, all the sessions belonging to it should be flushed
so the handles may be re-used by other spaces.

Signed-off-by: James Bottomley <James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org>
---
 drivers/char/tpm/tpm-chip.c   |  6 +++
 drivers/char/tpm/tpm.h        |  3 ++
 drivers/char/tpm/tpm2-space.c | 99 +++++++++++++++++++++++++++++++++++++++----
 drivers/char/tpm/tpms-dev.c   |  8 ++++
 4 files changed, 107 insertions(+), 9 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index ed4f887..6282ad0 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 c48255e..b77fc60 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -159,6 +159,8 @@ enum tpm2_cc_attrs {
 struct tpm_space {
 	u32 context_tbl[3];
 	u8 *context_buf;
+	u32 session_tbl[6];
+	u8 *session_buf;
 };
 
 enum tpm_chip_flags {
@@ -588,4 +590,5 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space, u32 cc,
 		       u8 *cmd);
 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 7fd2fc5..ba4310a 100644
--- a/drivers/char/tpm/tpm2-space.c
+++ b/drivers/char/tpm/tpm2-space.c
@@ -59,11 +59,16 @@ static int tpm2_load_context(struct tpm_chip *chip, u8 *buf,
 
 	rc = tpm_transmit_cmd(chip, NULL, tbuf.data, PAGE_SIZE, 4,
 			      TPM_TRANSMIT_UNLOCKED, NULL);
+
 	if (rc < 0) {
 		dev_warn(&chip->dev, "%s: failed with a system error %d\n",
 			 __func__, rc);
 		tpm_buf_destroy(&tbuf);
 		return -EFAULT;
+	} else if ((rc & TPM2_RC_HANDLE) == TPM2_RC_HANDLE ||
+		   rc == TPM2_RC_REFERENCE_H0) {
+		rc = -ENOENT;
+		tpm_buf_destroy(&tbuf);
 	} else if (rc > 0) {
 		dev_warn(&chip->dev, "%s: failed with a TPM error 0x%04X\n",
 			 __func__, rc);
@@ -116,21 +121,44 @@ static int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 *buf,
 	}
 
 	memcpy(&buf[*offset], &tbuf.data[TPM_HEADER_SIZE], body_size);
-	tpm2_flush_context_cmd(chip, handle, TPM_TRANSMIT_UNLOCKED);
 	*offset += body_size;
 	tpm_buf_destroy(&tbuf);
 	return 0;
 }
 
-static void tpm2_flush_space(struct tpm_chip *chip)
+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;
+}
+
+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++) {
+		if (space->session_tbl[i])
+			tpm2_flush_context_cmd(chip, space->session_tbl[i],
+					       TPM_TRANSMIT_UNLOCKED);
+	}
 }
 
 static int tpm2_load_space(struct tpm_chip *chip)
@@ -156,6 +184,28 @@ static int tpm2_load_space(struct tpm_chip *chip)
 			return rc;
 	}
 
+	for (i = 0, offset = 0; i < ARRAY_SIZE(space->session_tbl); i++) {
+		u32 handle;
+
+		if (!space->session_tbl[i])
+			continue;
+
+		rc = tpm2_load_context(chip, space->session_buf,
+				       &offset, &handle);
+		if (rc == -ENOENT) {
+			/* load failed, just forget session */
+			space->session_tbl[i] = 0;
+		} else if (rc) {
+			tpm2_flush_space(chip, space);
+			return rc;
+		}
+		if (handle != space->session_tbl[i]) {
+			dev_warn(&chip->dev, "session restored to wrong handle\n");
+			tpm2_flush_space(chip, space);
+			return -EFAULT;
+		}
+	}
+
 	return 0;
 }
 
@@ -215,17 +265,20 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space, u32 cc,
 
 	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);
+	memcpy(chip->work_space.session_buf, space->session_buf, PAGE_SIZE);
 
 	rc = tpm2_load_space(chip);
 	if (rc) {
-		tpm2_flush_space(chip);
+		tpm2_flush_space(chip, &chip->work_space);
 		return rc;
 	}
 
 	rc = tpm2_map_command(chip, cc, cmd);
 	if (rc) {
-		tpm2_flush_space(chip);
+		tpm2_flush_space(chip, &chip->work_space);
 		return rc;
 	}
 
@@ -235,7 +288,7 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space, u32 cc,
 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]);
@@ -254,9 +307,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) {
@@ -302,9 +361,28 @@ static int tpm2_save_space(struct tpm_chip *chip)
 		} else if (rc)
 			return rc;
 
+		tpm2_flush_context_cmd(chip, space->context_tbl[i],
+				       TPM_TRANSMIT_UNLOCKED);
 		space->context_tbl[i] = ~0;
 	}
 
+	for (i = 0, offset = 0; i < ARRAY_SIZE(space->session_tbl); i++) {
+		if (!space->session_tbl[i])
+			continue;
+
+		rc = tpm2_save_context(chip, space->session_tbl[i],
+				       space->session_buf, PAGE_SIZE,
+				       &offset);
+
+		if (rc == -ENOENT) {
+			/* handle error saving session, just forget it */
+			space->session_tbl[i] = 0;
+		} else if (rc < 0) {
+			tpm2_flush_space(chip, space);
+			return rc;
+		}
+	}
+
 	return 0;
 }
 
@@ -318,19 +396,22 @@ int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space,
 
 	rc = tpm2_map_response(chip, cc, buf, bufsiz);
 	if (rc) {
-		tpm2_flush_space(chip);
+		tpm2_flush_space(chip, space);
 		return rc;
 	}
 
 	rc = tpm2_save_space(chip);
 	if (rc) {
-		tpm2_flush_space(chip);
+		tpm2_flush_space(chip, space);
 		return rc;
 	}
 
 	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);
+	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 2ac2537..6efead3 100644
--- a/drivers/char/tpm/tpms-dev.c
+++ b/drivers/char/tpm/tpms-dev.c
@@ -27,6 +27,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);
 
@@ -38,8 +44,10 @@ 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->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

[PATCH 1/2] tpm2: add session handle context saving and restoring to the space code

[James Bottomley]

sessions are different from transient objects in that their handles
may not be virtualized (because they're used for some hmac
calculations).  Additionally when a session is context saved, a
vestigial memory remains in the TPM and if it is also flushed, that
will be lost and the session context will refuse to load next time, so
the code is updated to flush only transient objects after a context
save.  Add a separate array (chip->session_tbl) to save and restore
sessions by handle.  Use the failure of a context save or load to
signal that the session has been flushed from the TPM and we can
remove its memory from chip->session_tbl.

Sessions are also isolated during each instance of a tpm space.  This
means that spaces shouldn't be able to see each other's sessions and
is enforced by ensuring that a space user may only refer to sessions
handles that are present in their own chip->session_tbl.  Finally when
a space is closed, all the sessions belonging to it should be flushed
so the handles may be re-used by other spaces.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/char/tpm/tpm-chip.c   |  6 +++
 drivers/char/tpm/tpm.h        |  3 ++
 drivers/char/tpm/tpm2-space.c | 99 +++++++++++++++++++++++++++++++++++++++----
 drivers/char/tpm/tpms-dev.c   |  8 ++++
 4 files changed, 107 insertions(+), 9 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index ed4f887..6282ad0 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 c48255e..b77fc60 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -159,6 +159,8 @@ enum tpm2_cc_attrs {
 struct tpm_space {
 	u32 context_tbl[3];
 	u8 *context_buf;
+	u32 session_tbl[6];
+	u8 *session_buf;
 };
 
 enum tpm_chip_flags {
@@ -588,4 +590,5 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space, u32 cc,
 		       u8 *cmd);
 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 7fd2fc5..ba4310a 100644
--- a/drivers/char/tpm/tpm2-space.c
+++ b/drivers/char/tpm/tpm2-space.c
@@ -59,11 +59,16 @@ static int tpm2_load_context(struct tpm_chip *chip, u8 *buf,
 
 	rc = tpm_transmit_cmd(chip, NULL, tbuf.data, PAGE_SIZE, 4,
 			      TPM_TRANSMIT_UNLOCKED, NULL);
+
 	if (rc < 0) {
 		dev_warn(&chip->dev, "%s: failed with a system error %d\n",
 			 __func__, rc);
 		tpm_buf_destroy(&tbuf);
 		return -EFAULT;
+	} else if ((rc & TPM2_RC_HANDLE) == TPM2_RC_HANDLE ||
+		   rc == TPM2_RC_REFERENCE_H0) {
+		rc = -ENOENT;
+		tpm_buf_destroy(&tbuf);
 	} else if (rc > 0) {
 		dev_warn(&chip->dev, "%s: failed with a TPM error 0x%04X\n",
 			 __func__, rc);
@@ -116,21 +121,44 @@ static int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 *buf,
 	}
 
 	memcpy(&buf[*offset], &tbuf.data[TPM_HEADER_SIZE], body_size);
-	tpm2_flush_context_cmd(chip, handle, TPM_TRANSMIT_UNLOCKED);
 	*offset += body_size;
 	tpm_buf_destroy(&tbuf);
 	return 0;
 }
 
-static void tpm2_flush_space(struct tpm_chip *chip)
+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;
+}
+
+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++) {
+		if (space->session_tbl[i])
+			tpm2_flush_context_cmd(chip, space->session_tbl[i],
+					       TPM_TRANSMIT_UNLOCKED);
+	}
 }
 
 static int tpm2_load_space(struct tpm_chip *chip)
@@ -156,6 +184,28 @@ static int tpm2_load_space(struct tpm_chip *chip)
 			return rc;
 	}
 
+	for (i = 0, offset = 0; i < ARRAY_SIZE(space->session_tbl); i++) {
+		u32 handle;
+
+		if (!space->session_tbl[i])
+			continue;
+
+		rc = tpm2_load_context(chip, space->session_buf,
+				       &offset, &handle);
+		if (rc == -ENOENT) {
+			/* load failed, just forget session */
+			space->session_tbl[i] = 0;
+		} else if (rc) {
+			tpm2_flush_space(chip, space);
+			return rc;
+		}
+		if (handle != space->session_tbl[i]) {
+			dev_warn(&chip->dev, "session restored to wrong handle\n");
+			tpm2_flush_space(chip, space);
+			return -EFAULT;
+		}
+	}
+
 	return 0;
 }
 
@@ -215,17 +265,20 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space, u32 cc,
 
 	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);
+	memcpy(chip->work_space.session_buf, space->session_buf, PAGE_SIZE);
 
 	rc = tpm2_load_space(chip);
 	if (rc) {
-		tpm2_flush_space(chip);
+		tpm2_flush_space(chip, &chip->work_space);
 		return rc;
 	}
 
 	rc = tpm2_map_command(chip, cc, cmd);
 	if (rc) {
-		tpm2_flush_space(chip);
+		tpm2_flush_space(chip, &chip->work_space);
 		return rc;
 	}
 
@@ -235,7 +288,7 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space, u32 cc,
 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]);
@@ -254,9 +307,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) {
@@ -302,9 +361,28 @@ static int tpm2_save_space(struct tpm_chip *chip)
 		} else if (rc)
 			return rc;
 
+		tpm2_flush_context_cmd(chip, space->context_tbl[i],
+				       TPM_TRANSMIT_UNLOCKED);
 		space->context_tbl[i] = ~0;
 	}
 
+	for (i = 0, offset = 0; i < ARRAY_SIZE(space->session_tbl); i++) {
+		if (!space->session_tbl[i])
+			continue;
+
+		rc = tpm2_save_context(chip, space->session_tbl[i],
+				       space->session_buf, PAGE_SIZE,
+				       &offset);
+
+		if (rc == -ENOENT) {
+			/* handle error saving session, just forget it */
+			space->session_tbl[i] = 0;
+		} else if (rc < 0) {
+			tpm2_flush_space(chip, space);
+			return rc;
+		}
+	}
+
 	return 0;
 }
 
@@ -318,19 +396,22 @@ int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space,
 
 	rc = tpm2_map_response(chip, cc, buf, bufsiz);
 	if (rc) {
-		tpm2_flush_space(chip);
+		tpm2_flush_space(chip, space);
 		return rc;
 	}
 
 	rc = tpm2_save_space(chip);
 	if (rc) {
-		tpm2_flush_space(chip);
+		tpm2_flush_space(chip, space);
 		return rc;
 	}
 
 	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);
+	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 2ac2537..6efead3 100644
--- a/drivers/char/tpm/tpms-dev.c
+++ b/drivers/char/tpm/tpms-dev.c
@@ -27,6 +27,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);
 
@@ -38,8 +44,10 @@ 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->space.session_buf);
 	kfree(priv);
 
 	return 0;
-- 
2.6.6

Re: [PATCH 1/2] tpm2: add session handle context saving and restoring to the space code

[Jarkko Sakkinen]

On Mon, Jan 23, 2017 at 09:37:11PM -0800, James Bottomley wrote:
> sessions are different from transient objects in that their handles
> may not be virtualized (because they're used for some hmac
> calculations).  Additionally when a session is context saved, a
> vestigial memory remains in the TPM and if it is also flushed, that
> will be lost and the session context will refuse to load next time, so
> the code is updated to flush only transient objects after a context
> save.  Add a separate array (chip->session_tbl) to save and restore
> sessions by handle.  Use the failure of a context save or load to
> signal that the session has been flushed from the TPM and we can
> remove its memory from chip->session_tbl.
> 
> Sessions are also isolated during each instance of a tpm space.  This
> means that spaces shouldn't be able to see each other's sessions and
> is enforced by ensuring that a space user may only refer to sessions
> handles that are present in their own chip->session_tbl.  Finally when
> a space is closed, all the sessions belonging to it should be flushed
> so the handles may be re-used by other spaces.
> 
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>

I'm wondering if you ever need more than two sessions at once? If we
would limit the number of sessions to that you could probably simplify a
lot.

> ---
>  drivers/char/tpm/tpm-chip.c   |  6 +++
>  drivers/char/tpm/tpm.h        |  3 ++
>  drivers/char/tpm/tpm2-space.c | 99 +++++++++++++++++++++++++++++++++++++++----
>  drivers/char/tpm/tpms-dev.c   |  8 ++++
>  4 files changed, 107 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index ed4f887..6282ad0 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 c48255e..b77fc60 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -159,6 +159,8 @@ enum tpm2_cc_attrs {
>  struct tpm_space {
>  	u32 context_tbl[3];
>  	u8 *context_buf;
> +	u32 session_tbl[6];
> +	u8 *session_buf;
>  };
>  
>  enum tpm_chip_flags {
> @@ -588,4 +590,5 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space, u32 cc,
>  		       u8 *cmd);
>  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);

Why the extra parameter?

>  #endif
> diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
> index 7fd2fc5..ba4310a 100644
> --- a/drivers/char/tpm/tpm2-space.c
> +++ b/drivers/char/tpm/tpm2-space.c
> @@ -59,11 +59,16 @@ static int tpm2_load_context(struct tpm_chip *chip, u8 *buf,
>  
>  	rc = tpm_transmit_cmd(chip, NULL, tbuf.data, PAGE_SIZE, 4,
>  			      TPM_TRANSMIT_UNLOCKED, NULL);
> +

cruft

>  	if (rc < 0) {
>  		dev_warn(&chip->dev, "%s: failed with a system error %d\n",
>  			 __func__, rc);
>  		tpm_buf_destroy(&tbuf);
>  		return -EFAULT;
> +	} else if ((rc & TPM2_RC_HANDLE) == TPM2_RC_HANDLE ||
> +		   rc == TPM2_RC_REFERENCE_H0) {
> +		rc = -ENOENT;
> +		tpm_buf_destroy(&tbuf);
>  	} else if (rc > 0) {
>  		dev_warn(&chip->dev, "%s: failed with a TPM error 0x%04X\n",
>  			 __func__, rc);
> @@ -116,21 +121,44 @@ static int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 *buf,
>  	}
>  
>  	memcpy(&buf[*offset], &tbuf.data[TPM_HEADER_SIZE], body_size);
> -	tpm2_flush_context_cmd(chip, handle, TPM_TRANSMIT_UNLOCKED);
>  	*offset += body_size;
>  	tpm_buf_destroy(&tbuf);
>  	return 0;
>  }
>  
> -static void tpm2_flush_space(struct tpm_chip *chip)
> +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;
> +}
> +
> +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++) {
> +		if (space->session_tbl[i])
> +			tpm2_flush_context_cmd(chip, space->session_tbl[i],
> +					       TPM_TRANSMIT_UNLOCKED);
> +	}
>  }
>  
>  static int tpm2_load_space(struct tpm_chip *chip)
> @@ -156,6 +184,28 @@ static int tpm2_load_space(struct tpm_chip *chip)
>  			return rc;
>  	}
>  
> +	for (i = 0, offset = 0; i < ARRAY_SIZE(space->session_tbl); i++) {
> +		u32 handle;
> +
> +		if (!space->session_tbl[i])
> +			continue;
> +
> +		rc = tpm2_load_context(chip, space->session_buf,
> +				       &offset, &handle);
> +		if (rc == -ENOENT) {
> +			/* load failed, just forget session */
> +			space->session_tbl[i] = 0;
> +		} else if (rc) {
> +			tpm2_flush_space(chip, space);
> +			return rc;
> +		}
> +		if (handle != space->session_tbl[i]) {
> +			dev_warn(&chip->dev, "session restored to wrong handle\n");
> +			tpm2_flush_space(chip, space);
> +			return -EFAULT;
> +		}
> +	}
> +
>  	return 0;
>  }
>  
> @@ -215,17 +265,20 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space, u32 cc,
>  
>  	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);
> +	memcpy(chip->work_space.session_buf, space->session_buf, PAGE_SIZE);

For transient objects the rollback is straight forward and totally
predictable. Given that with sessions you always keep some information
in the TPM the rollback would be a bit more complicated.

Now your code seems to just keep the previous session_buf, doesn't it?
Does that always work or not?

PS. I have a high-level idea of attack vectors that are prevented by
having meta-data for session inside the TPM but can you point me to
the correct place in the TPM 2.0 specification that discusses about
this?

/Jarkko

Re: [PATCH 1/2] tpm2: add session handle context saving and restoring to the space code

[James Bottomley]

On Thu, 2017-01-26 at 14:51 +0200, Jarkko Sakkinen wrote:
> On Mon, Jan 23, 2017 at 09:37:11PM -0800, James Bottomley wrote:
> > sessions are different from transient objects in that their handles
> > may not be virtualized (because they're used for some hmac
> > calculations).  Additionally when a session is context saved, a
> > vestigial memory remains in the TPM and if it is also flushed, that
> > will be lost and the session context will refuse to load next time, 
> > so the code is updated to flush only transient objects after a 
> > context save.  Add a separate array (chip->session_tbl) to save and 
> > restore sessions by handle.  Use the failure of a context save or 
> > load to signal that the session has been flushed from the TPM and 
> > we can remove its memory from chip->session_tbl.
> > 
> > Sessions are also isolated during each instance of a tpm space. 
> >  This means that spaces shouldn't be able to see each other's 
> > sessions and is enforced by ensuring that a space user may only 
> > refer to sessions handles that are present in their own chip
> > ->session_tbl.  Finally when a space is closed, all the sessions 
> > belonging to it should be flushed so the handles may be re-used by
> > other spaces.
> > 
> > Signed-off-by: James Bottomley <
> > James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org>
> 
> I'm wondering if you ever need more than two sessions at once? If we
> would limit the number of sessions to that you could probably 
> simplify a lot.

Three seems to be the agreed maximum: hmac authority, parameter
encryption and command audit.

I'll fix up the rest

James


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH 1/2] tpm2: add session handle context saving and restoring to the space code

[James Bottomley]

On Thu, 2017-01-26 at 14:51 +0200, Jarkko Sakkinen wrote:
> On Mon, Jan 23, 2017 at 09:37:11PM -0800, James Bottomley wrote:
> > sessions are different from transient objects in that their handles
> > may not be virtualized (because they're used for some hmac
> > calculations).  Additionally when a session is context saved, a
> > vestigial memory remains in the TPM and if it is also flushed, that
> > will be lost and the session context will refuse to load next time, 
> > so the code is updated to flush only transient objects after a 
> > context save.  Add a separate array (chip->session_tbl) to save and 
> > restore sessions by handle.  Use the failure of a context save or 
> > load to signal that the session has been flushed from the TPM and 
> > we can remove its memory from chip->session_tbl.
> > 
> > Sessions are also isolated during each instance of a tpm space. 
> >  This means that spaces shouldn't be able to see each other's 
> > sessions and is enforced by ensuring that a space user may only 
> > refer to sessions handles that are present in their own chip
> > ->session_tbl.  Finally when a space is closed, all the sessions 
> > belonging to it should be flushed so the handles may be re-used by
> > other spaces.
> > 
> > Signed-off-by: James Bottomley <
> > James.Bottomley@HansenPartnership.com>
> 
> I'm wondering if you ever need more than two sessions at once? If we
> would limit the number of sessions to that you could probably 
> simplify a lot.

Three seems to be the agreed maximum: hmac authority, parameter
encryption and command audit.

I'll fix up the rest

James

Re: [PATCH 1/2] tpm2: add session handle context saving and restoring to the space code

[Jarkko Sakkinen]

On Thu, Jan 26, 2017 at 07:18:49AM -0800, James Bottomley wrote:
> On Thu, 2017-01-26 at 14:51 +0200, Jarkko Sakkinen wrote:
> > On Mon, Jan 23, 2017 at 09:37:11PM -0800, James Bottomley wrote:
> > > sessions are different from transient objects in that their handles
> > > may not be virtualized (because they're used for some hmac
> > > calculations).  Additionally when a session is context saved, a
> > > vestigial memory remains in the TPM and if it is also flushed, that
> > > will be lost and the session context will refuse to load next time, 
> > > so the code is updated to flush only transient objects after a 
> > > context save.  Add a separate array (chip->session_tbl) to save and 
> > > restore sessions by handle.  Use the failure of a context save or 
> > > load to signal that the session has been flushed from the TPM and 
> > > we can remove its memory from chip->session_tbl.
> > > 
> > > Sessions are also isolated during each instance of a tpm space. 
> > >  This means that spaces shouldn't be able to see each other's 
> > > sessions and is enforced by ensuring that a space user may only 
> > > refer to sessions handles that are present in their own chip
> > > ->session_tbl.  Finally when a space is closed, all the sessions 
> > > belonging to it should be flushed so the handles may be re-used by
> > > other spaces.
> > > 
> > > Signed-off-by: James Bottomley <
> > > James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org>
> > 
> > I'm wondering if you ever need more than two sessions at once? If we
> > would limit the number of sessions to that you could probably 
> > simplify a lot.
> 
> Three seems to be the agreed maximum: hmac authority, parameter
> encryption and command audit.
> 
> I'll fix up the rest
> 
> James

Right. I've also set the limit for trasient objects to three.

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH 1/2] tpm2: add session handle context saving and restoring to the space code

[Jarkko Sakkinen]

On Thu, Jan 26, 2017 at 07:18:49AM -0800, James Bottomley wrote:
> On Thu, 2017-01-26 at 14:51 +0200, Jarkko Sakkinen wrote:
> > On Mon, Jan 23, 2017 at 09:37:11PM -0800, James Bottomley wrote:
> > > sessions are different from transient objects in that their handles
> > > may not be virtualized (because they're used for some hmac
> > > calculations).  Additionally when a session is context saved, a
> > > vestigial memory remains in the TPM and if it is also flushed, that
> > > will be lost and the session context will refuse to load next time, 
> > > so the code is updated to flush only transient objects after a 
> > > context save.  Add a separate array (chip->session_tbl) to save and 
> > > restore sessions by handle.  Use the failure of a context save or 
> > > load to signal that the session has been flushed from the TPM and 
> > > we can remove its memory from chip->session_tbl.
> > > 
> > > Sessions are also isolated during each instance of a tpm space. 
> > >  This means that spaces shouldn't be able to see each other's 
> > > sessions and is enforced by ensuring that a space user may only 
> > > refer to sessions handles that are present in their own chip
> > > ->session_tbl.  Finally when a space is closed, all the sessions 
> > > belonging to it should be flushed so the handles may be re-used by
> > > other spaces.
> > > 
> > > Signed-off-by: James Bottomley <
> > > James.Bottomley@HansenPartnership.com>
> > 
> > I'm wondering if you ever need more than two sessions at once? If we
> > would limit the number of sessions to that you could probably 
> > simplify a lot.
> 
> Three seems to be the agreed maximum: hmac authority, parameter
> encryption and command audit.
> 
> I'll fix up the rest
> 
> James

Right. I've also set the limit for trasient objects to three.

/Jarkko

Re: [PATCH 1/2] tpm2: add session handle context saving and restoring to the space code

[Ken Goldman]

On 1/26/2017 10:18 AM, James Bottomley wrote:
>>
>> I'm wondering if you ever need more than two sessions at once? If we
>> would limit the number of sessions to that you could probably
>> simplify a lot.
>
> Three seems to be the agreed maximum: hmac authority, parameter
> encryption and command audit.

3 is recommended.

There is also the possibility that the encrypt (response) and decrypt 
(command) parameters could use different keys, and thus need their own 
session.

FYI: It is possible to have one session that does HMAC, parameter
encryption for command and response, and audit.





------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH 1/2] tpm2: add session handle context saving and restoring to the space code

[James Bottomley]

On Thu, 2017-01-26 at 14:51 +0200, Jarkko Sakkinen wrote:
[...]
> > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> > index c48255e..b77fc60 100644
> > --- a/drivers/char/tpm/tpm.h
> > +++ b/drivers/char/tpm/tpm.h
> > @@ -159,6 +159,8 @@ enum tpm2_cc_attrs {
> >  struct tpm_space {
> >  	u32 context_tbl[3];
> >  	u8 *context_buf;
> > +	u32 session_tbl[6];
> > +	u8 *session_buf;
> >  };
> >  
> >  enum tpm_chip_flags {
> > @@ -588,4 +590,5 @@ int tpm2_prepare_space(struct tpm_chip *chip,
> > struct tpm_space *space, u32 cc,
> >  		       u8 *cmd);
> >  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);
> 
> Why the extra parameter?

Because it was called from tpms-dev in release to flush all the
sessions still active.  At that point the work space is gone, so we
have to call it with the real space.  However, I realised that callsite
didn't hold the tpm_mutex like it should and that we don't need to
flush the contexts because they'll be guaranteed empty, so I added a
new function tpm2_kill_space() that does this.

> >  #endif
> > diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2
> > -space.c
> > index 7fd2fc5..ba4310a 100644
> > --- a/drivers/char/tpm/tpm2-space.c
> > +++ b/drivers/char/tpm/tpm2-space.c
> > @@ -59,11 +59,16 @@ static int tpm2_load_context(struct tpm_chip
> > *chip, u8 *buf,
> >  
> >  	rc = tpm_transmit_cmd(chip, NULL, tbuf.data, PAGE_SIZE, 4,
> >  			      TPM_TRANSMIT_UNLOCKED, NULL);
> > +
> 
> cruft

Removed.

[...] 
> > @@ -215,17 +265,20 @@ int tpm2_prepare_space(struct tpm_chip *chip,
> > struct tpm_space *space, u32 cc,
> >  
> >  	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);
> > +	memcpy(chip->work_space.session_buf, space->session_buf,
> > PAGE_SIZE);
> 
> For transient objects the rollback is straight forward and totally
> predictable. Given that with sessions you always keep some 
> information in the TPM the rollback would be a bit more complicated.

There is basically no rollback unless you really want to understand
what the commands did.  If we get a fault on the context save or load
that isn't one we understand, like TPM_RC_HANDLE meaning the session
won't load or TPM_RC_REFERENCE_H0 meaning the session no longer exists,
then I think the only option is flushing everything.

I'd like us to agree on a hard failure model: if we get some
unexplained error during our context loads or saves, we should clear
out the entire space (which would allow us to use pointers instead of
copyring) but we don't.  So we follow the soft failure.  However,
sessions will mostly fail to load after this with RPM_RC_HANDLE, so we
get the equivalent of soft failure for transients and hard failure for
sessions.

> Now your code seems to just keep the previous session_buf, doesn't
> it? Does that always work or not?

Yes, after tpm_flush_space, the session memory is gone and all the
sessions will refuse to load with RC_HANDLE, so we end up effectively
clearing them out.  It seemed better to do it this way than to try to
special case all the session stuff.

> PS. I have a high-level idea of attack vectors that are prevented by
> having meta-data for session inside the TPM but can you point me to
> the correct place in the TPM 2.0 specification that discusses about
> this?

The problem is replay.  If I'm snooping your TPM commands and I capture
your session context, if we don't have replay detection, I can re-load
your session HMAC and replay your command because the session has the
authorization or the encryption.  The TPM designers thought the only
way to avoid replay was to use a counter which was part of the session
context which increments every time a session is saved.  On loading you
check that the counters match and fail if they don't.  The only way to
implement this is to keep a memory of the counter in the TPM, hence the
annoying vestigial sessions.

It's note 2 of the architecture Part 1 guide, chapter "15.4 Session
Handles (MSO=02_16 and 03_16 )"

James

> 
> /Jarkko
> 


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH 1/2] tpm2: add session handle context saving and restoring to the space code

[James Bottomley]

On Thu, 2017-01-26 at 14:51 +0200, Jarkko Sakkinen wrote:
[...]
> > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> > index c48255e..b77fc60 100644
> > --- a/drivers/char/tpm/tpm.h
> > +++ b/drivers/char/tpm/tpm.h
> > @@ -159,6 +159,8 @@ enum tpm2_cc_attrs {
> >  struct tpm_space {
> >  	u32 context_tbl[3];
> >  	u8 *context_buf;
> > +	u32 session_tbl[6];
> > +	u8 *session_buf;
> >  };
> >  
> >  enum tpm_chip_flags {
> > @@ -588,4 +590,5 @@ int tpm2_prepare_space(struct tpm_chip *chip,
> > struct tpm_space *space, u32 cc,
> >  		       u8 *cmd);
> >  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);
> 
> Why the extra parameter?

Because it was called from tpms-dev in release to flush all the
sessions still active.  At that point the work space is gone, so we
have to call it with the real space.  However, I realised that callsite
didn't hold the tpm_mutex like it should and that we don't need to
flush the contexts because they'll be guaranteed empty, so I added a
new function tpm2_kill_space() that does this.

> >  #endif
> > diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2
> > -space.c
> > index 7fd2fc5..ba4310a 100644
> > --- a/drivers/char/tpm/tpm2-space.c
> > +++ b/drivers/char/tpm/tpm2-space.c
> > @@ -59,11 +59,16 @@ static int tpm2_load_context(struct tpm_chip
> > *chip, u8 *buf,
> >  
> >  	rc = tpm_transmit_cmd(chip, NULL, tbuf.data, PAGE_SIZE, 4,
> >  			      TPM_TRANSMIT_UNLOCKED, NULL);
> > +
> 
> cruft

Removed.

[...] 
> > @@ -215,17 +265,20 @@ int tpm2_prepare_space(struct tpm_chip *chip,
> > struct tpm_space *space, u32 cc,
> >  
> >  	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);
> > +	memcpy(chip->work_space.session_buf, space->session_buf,
> > PAGE_SIZE);
> 
> For transient objects the rollback is straight forward and totally
> predictable. Given that with sessions you always keep some 
> information in the TPM the rollback would be a bit more complicated.

There is basically no rollback unless you really want to understand
what the commands did.  If we get a fault on the context save or load
that isn't one we understand, like TPM_RC_HANDLE meaning the session
won't load or TPM_RC_REFERENCE_H0 meaning the session no longer exists,
then I think the only option is flushing everything.

I'd like us to agree on a hard failure model: if we get some
unexplained error during our context loads or saves, we should clear
out the entire space (which would allow us to use pointers instead of
copyring) but we don't.  So we follow the soft failure.  However,
sessions will mostly fail to load after this with RPM_RC_HANDLE, so we
get the equivalent of soft failure for transients and hard failure for
sessions.

> Now your code seems to just keep the previous session_buf, doesn't
> it? Does that always work or not?

Yes, after tpm_flush_space, the session memory is gone and all the
sessions will refuse to load with RC_HANDLE, so we end up effectively
clearing them out.  It seemed better to do it this way than to try to
special case all the session stuff.

> PS. I have a high-level idea of attack vectors that are prevented by
> having meta-data for session inside the TPM but can you point me to
> the correct place in the TPM 2.0 specification that discusses about
> this?

The problem is replay.  If I'm snooping your TPM commands and I capture
your session context, if we don't have replay detection, I can re-load
your session HMAC and replay your command because the session has the
authorization or the encryption.  The TPM designers thought the only
way to avoid replay was to use a counter which was part of the session
context which increments every time a session is saved.  On loading you
check that the counters match and fail if they don't.  The only way to
implement this is to keep a memory of the counter in the TPM, hence the
annoying vestigial sessions.

It's note 2 of the architecture Part 1 guide, chapter "15.4 Session
Handles (MSO=02_16 and 03_16 )"

James

> 
> /Jarkko
> 

Re: [PATCH 1/2] tpm2: add session handle context saving and restoring to the space code

[Jarkko Sakkinen]

On Thu, Jan 26, 2017 at 08:26:19AM -0800, James Bottomley wrote:
> On Thu, 2017-01-26 at 14:51 +0200, Jarkko Sakkinen wrote:
> [...]
> > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> > > index c48255e..b77fc60 100644
> > > --- a/drivers/char/tpm/tpm.h
> > > +++ b/drivers/char/tpm/tpm.h
> > > @@ -159,6 +159,8 @@ enum tpm2_cc_attrs {
> > >  struct tpm_space {
> > >  	u32 context_tbl[3];
> > >  	u8 *context_buf;
> > > +	u32 session_tbl[6];
> > > +	u8 *session_buf;
> > >  };
> > >  
> > >  enum tpm_chip_flags {
> > > @@ -588,4 +590,5 @@ int tpm2_prepare_space(struct tpm_chip *chip,
> > > struct tpm_space *space, u32 cc,
> > >  		       u8 *cmd);
> > >  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);
> > 
> > Why the extra parameter?
> 
> Because it was called from tpms-dev in release to flush all the
> sessions still active.  At that point the work space is gone, so we
> have to call it with the real space.  However, I realised that callsite
> didn't hold the tpm_mutex like it should and that we don't need to
> flush the contexts because they'll be guaranteed empty, so I added a
> new function tpm2_kill_space() that does this.
> 
> > >  #endif
> > > diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2
> > > -space.c
> > > index 7fd2fc5..ba4310a 100644
> > > --- a/drivers/char/tpm/tpm2-space.c
> > > +++ b/drivers/char/tpm/tpm2-space.c
> > > @@ -59,11 +59,16 @@ static int tpm2_load_context(struct tpm_chip
> > > *chip, u8 *buf,
> > >  
> > >  	rc = tpm_transmit_cmd(chip, NULL, tbuf.data, PAGE_SIZE, 4,
> > >  			      TPM_TRANSMIT_UNLOCKED, NULL);
> > > +
> > 
> > cruft
> 
> Removed.
> 
> [...] 
> > > @@ -215,17 +265,20 @@ int tpm2_prepare_space(struct tpm_chip *chip,
> > > struct tpm_space *space, u32 cc,
> > >  
> > >  	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);
> > > +	memcpy(chip->work_space.session_buf, space->session_buf,
> > > PAGE_SIZE);
> > 
> > For transient objects the rollback is straight forward and totally
> > predictable. Given that with sessions you always keep some 
> > information in the TPM the rollback would be a bit more complicated.
> 
> There is basically no rollback unless you really want to understand
> what the commands did.  If we get a fault on the context save or load
> that isn't one we understand, like TPM_RC_HANDLE meaning the session
> won't load or TPM_RC_REFERENCE_H0 meaning the session no longer exists,
> then I think the only option is flushing everything.
> 
> I'd like us to agree on a hard failure model: if we get some
> unexplained error during our context loads or saves, we should clear
> out the entire space (which would allow us to use pointers instead of
> copyring) but we don't.  So we follow the soft failure.  However,
> sessions will mostly fail to load after this with RPM_RC_HANDLE, so we
> get the equivalent of soft failure for transients and hard failure for
> sessions.
> 
> > Now your code seems to just keep the previous session_buf, doesn't
> > it? Does that always work or not?
> 
> Yes, after tpm_flush_space, the session memory is gone and all the
> sessions will refuse to load with RC_HANDLE, so we end up effectively
> clearing them out.  It seemed better to do it this way than to try to
> special case all the session stuff.

Maybe it would make sense to have a comment in code to state this?
Otherwise, I'm fine with this semantics.

> > PS. I have a high-level idea of attack vectors that are prevented by
> > having meta-data for session inside the TPM but can you point me to
> > the correct place in the TPM 2.0 specification that discusses about
> > this?
> 
> The problem is replay.  If I'm snooping your TPM commands and I capture
> your session context, if we don't have replay detection, I can re-load
> your session HMAC and replay your command because the session has the
> authorization or the encryption.  The TPM designers thought the only
> way to avoid replay was to use a counter which was part of the session
> context which increments every time a session is saved.  On loading you
> check that the counters match and fail if they don't.  The only way to
> implement this is to keep a memory of the counter in the TPM, hence the
> annoying vestigial sessions.
> 
> It's note 2 of the architecture Part 1 guide, chapter "15.4 Session
> Handles (MSO=02_16 and 03_16 )"
> 
> James

Thank you.

Once these are in shape I think we have something that could be put into
a release, don't you think?

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH 1/2] tpm2: add session handle context saving and restoring to the space code

[Jarkko Sakkinen]

On Thu, Jan 26, 2017 at 08:26:19AM -0800, James Bottomley wrote:
> On Thu, 2017-01-26 at 14:51 +0200, Jarkko Sakkinen wrote:
> [...]
> > > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> > > index c48255e..b77fc60 100644
> > > --- a/drivers/char/tpm/tpm.h
> > > +++ b/drivers/char/tpm/tpm.h
> > > @@ -159,6 +159,8 @@ enum tpm2_cc_attrs {
> > >  struct tpm_space {
> > >  	u32 context_tbl[3];
> > >  	u8 *context_buf;
> > > +	u32 session_tbl[6];
> > > +	u8 *session_buf;
> > >  };
> > >  
> > >  enum tpm_chip_flags {
> > > @@ -588,4 +590,5 @@ int tpm2_prepare_space(struct tpm_chip *chip,
> > > struct tpm_space *space, u32 cc,
> > >  		       u8 *cmd);
> > >  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);
> > 
> > Why the extra parameter?
> 
> Because it was called from tpms-dev in release to flush all the
> sessions still active.  At that point the work space is gone, so we
> have to call it with the real space.  However, I realised that callsite
> didn't hold the tpm_mutex like it should and that we don't need to
> flush the contexts because they'll be guaranteed empty, so I added a
> new function tpm2_kill_space() that does this.
> 
> > >  #endif
> > > diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2
> > > -space.c
> > > index 7fd2fc5..ba4310a 100644
> > > --- a/drivers/char/tpm/tpm2-space.c
> > > +++ b/drivers/char/tpm/tpm2-space.c
> > > @@ -59,11 +59,16 @@ static int tpm2_load_context(struct tpm_chip
> > > *chip, u8 *buf,
> > >  
> > >  	rc = tpm_transmit_cmd(chip, NULL, tbuf.data, PAGE_SIZE, 4,
> > >  			      TPM_TRANSMIT_UNLOCKED, NULL);
> > > +
> > 
> > cruft
> 
> Removed.
> 
> [...] 
> > > @@ -215,17 +265,20 @@ int tpm2_prepare_space(struct tpm_chip *chip,
> > > struct tpm_space *space, u32 cc,
> > >  
> > >  	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);
> > > +	memcpy(chip->work_space.session_buf, space->session_buf,
> > > PAGE_SIZE);
> > 
> > For transient objects the rollback is straight forward and totally
> > predictable. Given that with sessions you always keep some 
> > information in the TPM the rollback would be a bit more complicated.
> 
> There is basically no rollback unless you really want to understand
> what the commands did.  If we get a fault on the context save or load
> that isn't one we understand, like TPM_RC_HANDLE meaning the session
> won't load or TPM_RC_REFERENCE_H0 meaning the session no longer exists,
> then I think the only option is flushing everything.
> 
> I'd like us to agree on a hard failure model: if we get some
> unexplained error during our context loads or saves, we should clear
> out the entire space (which would allow us to use pointers instead of
> copyring) but we don't.  So we follow the soft failure.  However,
> sessions will mostly fail to load after this with RPM_RC_HANDLE, so we
> get the equivalent of soft failure for transients and hard failure for
> sessions.
> 
> > Now your code seems to just keep the previous session_buf, doesn't
> > it? Does that always work or not?
> 
> Yes, after tpm_flush_space, the session memory is gone and all the
> sessions will refuse to load with RC_HANDLE, so we end up effectively
> clearing them out.  It seemed better to do it this way than to try to
> special case all the session stuff.

Maybe it would make sense to have a comment in code to state this?
Otherwise, I'm fine with this semantics.

> > PS. I have a high-level idea of attack vectors that are prevented by
> > having meta-data for session inside the TPM but can you point me to
> > the correct place in the TPM 2.0 specification that discusses about
> > this?
> 
> The problem is replay.  If I'm snooping your TPM commands and I capture
> your session context, if we don't have replay detection, I can re-load
> your session HMAC and replay your command because the session has the
> authorization or the encryption.  The TPM designers thought the only
> way to avoid replay was to use a counter which was part of the session
> context which increments every time a session is saved.  On loading you
> check that the counters match and fail if they don't.  The only way to
> implement this is to keep a memory of the counter in the TPM, hence the
> annoying vestigial sessions.
> 
> It's note 2 of the architecture Part 1 guide, chapter "15.4 Session
> Handles (MSO=02_16 and 03_16 )"
> 
> James

Thank you.

Once these are in shape I think we have something that could be put into
a release, don't you think?

/Jarkko

[PATCH 2/2] tpm2-space: add handling for global session exhaustion

[James Bottomley]

In a TPM2, sessions can be globally exhausted once there are
TPM_PT_ACTIVE_SESSION_MAX of them (even if they're all context saved).
The Strategy for handling this is to keep a global count of all the
sessions along with their creation time.  Then if we see the TPM run
out of sessions (via the TPM_RC_SESSION_HANDLES) we first wait for one
to become free, but if it doesn't, we forcibly evict an existing one.
The eviction strategy waits until the current command is repeated to
evict the session which should guarantee there is an available slot.

On the force eviction case, we make sure that the victim session is at
least SESSION_TIMEOUT old (currently 2 seconds).  The wait queue for
session slots is a FIFO one, ensuring that once we run out of
sessions, everyone will get a session in a bounded time and once they
get one, they'll have SESSION_TIMEOUT to use it before it may be
subject to eviction.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/char/tpm/tpm-chip.c   |   1 +
 drivers/char/tpm/tpm.h        |  39 +++++++-
 drivers/char/tpm/tpm2-cmd.c   |  15 +++
 drivers/char/tpm/tpm2-space.c | 213 +++++++++++++++++++++++++++++++++++++++---
 drivers/char/tpm/tpms-dev.c   |  17 +++-
 5 files changed, 271 insertions(+), 14 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 6282ad0..150c6b8 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -164,6 +164,7 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
 
 	mutex_init(&chip->tpm_mutex);
 	init_rwsem(&chip->ops_sem);
+	init_waitqueue_head(&chip->session_wait);
 
 	chip->ops = ops;
 
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index b77fc60..9fd6101 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -95,7 +95,8 @@ enum tpm2_return_codes {
 	TPM2_RC_HANDLE		= 0x008B,
 	TPM2_RC_INITIALIZE	= 0x0100, /* RC_VER1 */
 	TPM2_RC_DISABLED	= 0x0120,
-	TPM2_RC_TESTING		= 0x090A, /* RC_WARN */
+	TPM2_RC_SESSION_HANDLES	= 0x0905, /* RC_WARN */
+	TPM2_RC_TESTING		= 0x090A,
 	TPM2_RC_REFERENCE_H0	= 0x0910,
 };
 
@@ -137,7 +138,8 @@ enum tpm2_capabilities {
 };
 
 enum tpm2_properties {
-	TPM_PT_TOTAL_COMMANDS	= 0x0129,
+	TPM_PT_TOTAL_COMMANDS		= 0x0129,
+	TPM_PT_ACTIVE_SESSIONS_MAX	= 0x0111,
 };
 
 enum tpm2_startup_types {
@@ -161,8 +163,24 @@ struct tpm_space {
 	u8 *context_buf;
 	u32 session_tbl[6];
 	u8 *session_buf;
+	u32 reserved_handle;
 };
 
+#define TPM2_HANDLE_FORCE_EVICT 0xFFFFFFFF
+
+static inline void tpm2_session_force_evict(struct tpm_space *space)
+{
+	/* if reserved handle is not empty, we already have a
+	 * session for eviction, so no need to force one
+	 */
+	if (space->reserved_handle == 0)
+		space->reserved_handle = TPM2_HANDLE_FORCE_EVICT;
+}
+static inline bool tpm2_is_session_force_evict(struct tpm_space *space)
+{
+	return space->reserved_handle == TPM2_HANDLE_FORCE_EVICT;
+}
+
 enum tpm_chip_flags {
 	TPM_CHIP_FLAG_TPM2		= BIT(1),
 	TPM_CHIP_FLAG_IRQ		= BIT(2),
@@ -175,6 +193,12 @@ struct tpm_chip_seqops {
 	const struct seq_operations *seqops;
 };
 
+struct tpm_sessions {
+	struct tpm_space *space;
+	u32 handle;
+	unsigned long created;
+};
+
 struct tpm_chip {
 	struct device dev;
 	struct device devs;
@@ -217,8 +241,12 @@ struct tpm_chip {
 #endif /* CONFIG_ACPI */
 
 	struct tpm_space work_space;
+	struct tpm_space *space;
 	u32 nr_commands;
 	u32 *cc_attrs_tbl;
+	struct tpm_sessions *sessions;
+	int max_sessions;
+	wait_queue_head_t session_wait;
 };
 
 #define to_tpm_chip(d) container_of(d, struct tpm_chip, dev)
@@ -571,6 +599,13 @@ 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);
 void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle,
 			    unsigned int flags);
+static inline void tpm2_session_clear_reserved(struct tpm_chip *chip,
+					       struct tpm_space *space)
+{
+	if (space->reserved_handle && !tpm2_is_session_force_evict(space))
+		tpm2_flush_context_cmd(chip, space->reserved_handle, 0);
+	space->reserved_handle = 0;
+}
 int tpm2_seal_trusted(struct tpm_chip *chip,
 		      struct trusted_key_payload *payload,
 		      struct trusted_key_options *options);
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 0178816..3a54479 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -1000,6 +1000,7 @@ int tpm2_auto_startup(struct tpm_chip *chip)
 	struct tpm_buf buf;
 	u32 nr_commands;
 	u32 *attrs;
+	u32 nr_sessions;
 	int rc;
 	int i;
 
@@ -1075,6 +1076,20 @@ int tpm2_auto_startup(struct tpm_chip *chip)
 
 	tpm_buf_destroy(&buf);
 
+	rc = tpm2_get_tpm_pt(chip, TPM_PT_ACTIVE_SESSIONS_MAX,
+			     &nr_sessions, NULL);
+	if (rc)
+		goto out;
+
+	if (nr_sessions > 256)
+		nr_sessions = 256;
+
+	chip->max_sessions = nr_sessions;
+	chip->sessions = devm_kzalloc(&chip->dev,
+				      nr_sessions * sizeof(*chip->sessions),
+				      GFP_KERNEL);
+	if (!chip->sessions)
+		rc = -ENOMEM;
 out:
 	if (rc > 0)
 		rc = -ENODEV;
diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
index ba4310a..7304280 100644
--- a/drivers/char/tpm/tpm2-space.c
+++ b/drivers/char/tpm/tpm2-space.c
@@ -32,6 +32,166 @@ struct tpm2_context {
 	__be16 blob_size;
 } __packed;
 
+static struct tpm_sessions *tpm2_session_chip_get(struct tpm_chip *chip)
+{
+	int i;
+
+	for (i = 0; i < chip->max_sessions; i++)
+		if (chip->sessions[i].space == NULL)
+			return &chip->sessions[i];
+
+	return NULL;
+}
+
+static struct tpm_sessions *tpm2_session_chip_find_oldest(struct tpm_chip *chip)
+{
+	struct tpm_sessions *sess = NULL;
+	int i;
+
+	for (i = 0; i < chip->max_sessions; i++) {
+		if (chip->sessions[i].space == NULL)
+			continue;
+
+		if (!sess || time_after(sess->created,
+					chip->sessions[i].created))
+			sess = &chip->sessions[i];
+	}
+
+	return sess;
+}
+
+static void tpm2_session_chip_add(struct tpm_chip *chip,
+				  struct tpm_space *space, u32 h)
+{
+	struct tpm_sessions *sess = tpm2_session_chip_get(chip);
+
+	sess->space = space;
+	sess->handle = h;
+	sess->created = jiffies;
+}
+
+static void tpm2_session_chip_remove(struct tpm_chip *chip, u32 h)
+{
+	int i;
+
+	for (i = 0; i < chip->max_sessions; i++)
+		if (chip->sessions[i].handle == h)
+			break;
+	if (i == chip->max_sessions) {
+		dev_warn(&chip->dev, "Missing session %08x", h);
+		return;
+	}
+
+	memset(&chip->sessions[i], 0, sizeof(chip->sessions[i]));
+	wake_up(&chip->session_wait);
+}
+
+static int tpm2_session_forget(struct tpm_chip *chip, 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;
+}
+
+static int tpm2_session_wait(struct tpm_chip *chip, struct tpm_space *space)
+{
+	int rc, failed;
+	struct tpm_sessions *sess;
+	const unsigned long min_timeout = msecs_to_jiffies(2000);
+	unsigned long timeout = min_timeout;
+	DEFINE_WAIT(wait);
+
+	for (failed = 0; ; ) {
+		prepare_to_wait(&chip->session_wait, &wait, TASK_INTERRUPTIBLE);
+
+		mutex_unlock(&chip->tpm_mutex);
+		rc = schedule_timeout_interruptible(timeout);
+		mutex_lock(&chip->tpm_mutex);
+
+		finish_wait(&chip->session_wait, &wait);
+
+		if (signal_pending(current))
+			/* got interrupted */
+			return -EINTR;
+
+		if (rc > 0 && !tpm2_is_session_force_evict(space))
+			/* got woken, so slot is free.  We don't
+			 * reserve the slot here because a) we can't
+			 * (no pending session in the TPM to evict)
+			 * and b) no-one is hogging sessions, so no
+			 * evidence of need.
+			 */
+			return 0;
+
+		/* timed out or victim required; select a victim
+		 * session to kill
+		 */
+		sess = tpm2_session_chip_find_oldest(chip);
+		if (sess == NULL) {
+			/* we get here when we can't create a session
+			 * but there are no listed active sessions
+			 * meaning they're all in various space
+			 * structures as victim sessions.  The wait
+			 * queue is a fair sequence, so we need to
+			 * wait a bit harder
+			 */
+			if (failed++ > 3)
+				break;
+			timeout *= 2;
+			dev_info(&chip->dev, "failed to get session, waiting for %us\n", jiffies_to_msecs(timeout)/1000);
+			continue;
+		}
+		/* is the victim old enough? */
+		timeout = jiffies - sess->created;
+		if (timeout > min_timeout)
+			break;
+		/* otherwise wait until the victim is old enough */
+		timeout = min_timeout - timeout;
+	}
+	if (sess == NULL)
+		/* still can't get a victim, give up */
+		return -EINVAL;
+
+	/* store the physical handle */
+	space->reserved_handle = sess->handle;
+	dev_info(&chip->dev, "Selecting handle %08x for eviction\n",
+		 space->reserved_handle);
+
+	/* cause a mapping failure if this session handle is
+	 * ever used in the victim space again
+	 */
+	tpm2_session_forget(chip, sess->space, sess->handle);
+	/* clear the session, but don't wake any other waiters */
+	memset(sess, 0, sizeof(*sess));
+	/* so now we have a saved physical handle but this handle is
+	 * still in the tpm.  After this we repeat the command, but
+	 * flush the handle once we obtain the tpm_mutex on the repeat
+	 * so, in theory, we should have a free handle to
+	 * re-execute
+	 */
+
+	return 0;
+}
+
 int tpm2_init_space(struct tpm_chip *chip, struct tpm_space *space)
 {
 	space->context_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
@@ -65,8 +225,7 @@ static int tpm2_load_context(struct tpm_chip *chip, u8 *buf,
 			 __func__, rc);
 		tpm_buf_destroy(&tbuf);
 		return -EFAULT;
-	} else if ((rc & TPM2_RC_HANDLE) == TPM2_RC_HANDLE ||
-		   rc == TPM2_RC_REFERENCE_H0) {
+	} else if (rc == TPM2_RC_REFERENCE_H0) {
 		rc = -ENOENT;
 		tpm_buf_destroy(&tbuf);
 	} else if (rc > 0) {
@@ -126,9 +285,9 @@ static int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 *buf,
 	return 0;
 }
 
-static int tpm2_session_add(struct tpm_chip *chip,
-			    struct tpm_space *space, u32 handle)
+static int tpm2_session_add(struct tpm_chip *chip, u32 handle)
 {
+	struct tpm_space *space = &chip->work_space;
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(space->session_tbl); i++)
@@ -141,6 +300,7 @@ static int tpm2_session_add(struct tpm_chip *chip,
 	}
 
 	space->session_tbl[i] = handle;
+	tpm2_session_chip_add(chip, chip->space, handle);
 
 	return 0;
 }
@@ -155,9 +315,18 @@ void tpm2_flush_space(struct tpm_chip *chip, struct tpm_space *space)
 					       TPM_TRANSMIT_UNLOCKED);
 
 	for (i = 0; i < ARRAY_SIZE(space->session_tbl); i++) {
-		if (space->session_tbl[i])
-			tpm2_flush_context_cmd(chip, space->session_tbl[i],
-					       TPM_TRANSMIT_UNLOCKED);
+		if (!space->session_tbl[i])
+			continue;
+
+		tpm2_session_chip_remove(chip, space->session_tbl[i]);
+		tpm2_flush_context_cmd(chip, space->session_tbl[i],
+				       TPM_TRANSMIT_UNLOCKED);
+	}
+	if (space->reserved_handle && !tpm2_is_session_force_evict(space)) {
+		tpm2_flush_context_cmd(chip, space->reserved_handle,
+				       TPM_TRANSMIT_UNLOCKED);
+		space->reserved_handle = 0;
+		/* subtlety here: if force evict is set, we don't clear it */
 	}
 }
 
@@ -194,6 +363,7 @@ static int tpm2_load_space(struct tpm_chip *chip)
 				       &offset, &handle);
 		if (rc == -ENOENT) {
 			/* load failed, just forget session */
+			tpm2_session_chip_remove(chip, space->session_tbl[i]);
 			space->session_tbl[i] = 0;
 		} else if (rc) {
 			tpm2_flush_space(chip, space);
@@ -269,6 +439,7 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space, u32 cc,
 	       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);
+	chip->space = space;
 
 	rc = tpm2_load_space(chip);
 	if (rc) {
@@ -282,16 +453,28 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space, u32 cc,
 		return rc;
 	}
 
+	if (space->reserved_handle && !tpm2_is_session_force_evict(space)) {
+		/* this is a trick to allow a previous command which
+		 * failed because it was out of handle space to
+		 * succeed.  The handle is still in the TPM, so now we
+		 * flush it under the tpm_mutex which should ensure we
+		 * can create a new one
+		 */
+		tpm2_flush_context_cmd(chip, space->reserved_handle,
+				       TPM_TRANSMIT_UNLOCKED);
+		space->reserved_handle = 0;
+	}
+
 	return 0;
 }
 
-static int tpm2_map_response(struct tpm_chip *chip, u32 cc, u8 *rsp, size_t len)
+static int tpm2_map_response(struct tpm_chip *chip, u32 cc, u8 *rsp, size_t len,
+			     u32 return_code)
 {
 	struct tpm_space *space = &chip->work_space;
 	u32 phandle, phandle_type;
 	u32 vhandle;
 	u32 attrs;
-	u32 return_code = get_unaligned_be32((__be32 *)&rsp[6]);
 	int i;
 
 	if (return_code != TPM2_RC_SUCCESS)
@@ -314,7 +497,7 @@ static int tpm2_map_response(struct tpm_chip *chip, u32 cc, u8 *rsp, size_t len)
 		return 0;
 
 	if (phandle_type != TPM2_HT_TRANSIENT)
-		return tpm2_session_add(chip, space, phandle);
+		return tpm2_session_add(chip, phandle);
 
 	/* Garbage collect a dead context. */
 	for (i = 0; i < ARRAY_SIZE(space->context_tbl); i++) {
@@ -376,6 +559,7 @@ static int tpm2_save_space(struct tpm_chip *chip)
 
 		if (rc == -ENOENT) {
 			/* handle error saving session, just forget it */
+			tpm2_session_chip_remove(chip, space->session_tbl[i]);
 			space->session_tbl[i] = 0;
 		} else if (rc < 0) {
 			tpm2_flush_space(chip, space);
@@ -390,11 +574,12 @@ int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space,
 		      u32 cc, u8 *buf, size_t bufsiz)
 {
 	int rc;
+	u32 return_code = get_unaligned_be32((__be32 *)&buf[6]);
 
 	if (!space)
 		return 0;
 
-	rc = tpm2_map_response(chip, cc, buf, bufsiz);
+	rc = tpm2_map_response(chip, cc, buf, bufsiz, return_code);
 	if (rc) {
 		tpm2_flush_space(chip, space);
 		return rc;
@@ -412,6 +597,12 @@ int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space,
 	       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);
+	chip->space = NULL;
+
+	if (return_code == TPM2_RC_SESSION_HANDLES) {
+		tpm2_session_wait(chip, space);
+		return -EAGAIN;
+	}
 
 	return 0;
 }
diff --git a/drivers/char/tpm/tpms-dev.c b/drivers/char/tpm/tpms-dev.c
index 6efead3..9766fb4 100644
--- a/drivers/char/tpm/tpms-dev.c
+++ b/drivers/char/tpm/tpms-dev.c
@@ -58,8 +58,23 @@ ssize_t tpms_write(struct file *file, const char __user *buf,
 {
 	struct file_priv *fpriv = file->private_data;
 	struct tpms_priv *priv = container_of(fpriv, struct tpms_priv, priv);
+	int count = 0;
+	const int max_count = 3; /* number of retries */
+	int rc;
+
+	for (count = 0; count < max_count; count++) {
+		rc = tpm_common_write(file, buf, size, off, &priv->space);
+		if (rc != -EAGAIN)
+			break;
+		if (count == max_count - 2)
+			/* second to last go around, force an eviction if
+			 * this go fails, so final go should succeed
+			 */
+			tpm2_session_force_evict(&priv->space);
+	}
+	tpm2_session_clear_reserved(fpriv->chip, &priv->space);
 
-	return tpm_common_write(file, buf, size, off, &priv->space);
+	return rc;
 }
 
 const struct file_operations tpms_fops = {
-- 
2.6.6

Re: [PATCH 2/2] tpm2-space: add handling for global session exhaustion

[Jarkko Sakkinen]

On Mon, Jan 23, 2017 at 09:38:33PM -0800, James Bottomley wrote:
> In a TPM2, sessions can be globally exhausted once there are
> TPM_PT_ACTIVE_SESSION_MAX of them (even if they're all context saved).
> The Strategy for handling this is to keep a global count of all the
> sessions along with their creation time.  Then if we see the TPM run
> out of sessions (via the TPM_RC_SESSION_HANDLES) we first wait for one
> to become free, but if it doesn't, we forcibly evict an existing one.
> The eviction strategy waits until the current command is repeated to
> evict the session which should guarantee there is an available slot.
> 
> On the force eviction case, we make sure that the victim session is at
> least SESSION_TIMEOUT old (currently 2 seconds).  The wait queue for
> session slots is a FIFO one, ensuring that once we run out of
> sessions, everyone will get a session in a bounded time and once they
> get one, they'll have SESSION_TIMEOUT to use it before it may be
> subject to eviction.
> 
> Signed-off-by: James Bottomley <James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org>

This is not a proper review yet. Just quick question: why do you need
a real time (i.e. created)? Maybe in the force eviction case it would
be enough to sleep lets say 500 ms and pick the victim with smallest
number? I.e. just have increasing u64 counter instead of real time.

That would simplify this patch a lot and does not prevent to refine
later on if workloads show need for more complex logic.

/Jarkko

> ---
>  drivers/char/tpm/tpm-chip.c   |   1 +
>  drivers/char/tpm/tpm.h        |  39 +++++++-
>  drivers/char/tpm/tpm2-cmd.c   |  15 +++
>  drivers/char/tpm/tpm2-space.c | 213 +++++++++++++++++++++++++++++++++++++++---
>  drivers/char/tpm/tpms-dev.c   |  17 +++-
>  5 files changed, 271 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 6282ad0..150c6b8 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -164,6 +164,7 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
>  
>  	mutex_init(&chip->tpm_mutex);
>  	init_rwsem(&chip->ops_sem);
> +	init_waitqueue_head(&chip->session_wait);
>  
>  	chip->ops = ops;
>  
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index b77fc60..9fd6101 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -95,7 +95,8 @@ enum tpm2_return_codes {
>  	TPM2_RC_HANDLE		= 0x008B,
>  	TPM2_RC_INITIALIZE	= 0x0100, /* RC_VER1 */
>  	TPM2_RC_DISABLED	= 0x0120,
> -	TPM2_RC_TESTING		= 0x090A, /* RC_WARN */
> +	TPM2_RC_SESSION_HANDLES	= 0x0905, /* RC_WARN */
> +	TPM2_RC_TESTING		= 0x090A,
>  	TPM2_RC_REFERENCE_H0	= 0x0910,
>  };
>  
> @@ -137,7 +138,8 @@ enum tpm2_capabilities {
>  };
>  
>  enum tpm2_properties {
> -	TPM_PT_TOTAL_COMMANDS	= 0x0129,
> +	TPM_PT_TOTAL_COMMANDS		= 0x0129,
> +	TPM_PT_ACTIVE_SESSIONS_MAX	= 0x0111,
>  };
>  
>  enum tpm2_startup_types {
> @@ -161,8 +163,24 @@ struct tpm_space {
>  	u8 *context_buf;
>  	u32 session_tbl[6];
>  	u8 *session_buf;
> +	u32 reserved_handle;
>  };
>  
> +#define TPM2_HANDLE_FORCE_EVICT 0xFFFFFFFF
> +
> +static inline void tpm2_session_force_evict(struct tpm_space *space)
> +{
> +	/* if reserved handle is not empty, we already have a
> +	 * session for eviction, so no need to force one
> +	 */
> +	if (space->reserved_handle == 0)
> +		space->reserved_handle = TPM2_HANDLE_FORCE_EVICT;
> +}
> +static inline bool tpm2_is_session_force_evict(struct tpm_space *space)
> +{
> +	return space->reserved_handle == TPM2_HANDLE_FORCE_EVICT;
> +}
> +
>  enum tpm_chip_flags {
>  	TPM_CHIP_FLAG_TPM2		= BIT(1),
>  	TPM_CHIP_FLAG_IRQ		= BIT(2),
> @@ -175,6 +193,12 @@ struct tpm_chip_seqops {
>  	const struct seq_operations *seqops;
>  };
>  
> +struct tpm_sessions {
> +	struct tpm_space *space;
> +	u32 handle;
> +	unsigned long created;
> +};
> +
>  struct tpm_chip {
>  	struct device dev;
>  	struct device devs;
> @@ -217,8 +241,12 @@ struct tpm_chip {
>  #endif /* CONFIG_ACPI */
>  
>  	struct tpm_space work_space;
> +	struct tpm_space *space;
>  	u32 nr_commands;
>  	u32 *cc_attrs_tbl;
> +	struct tpm_sessions *sessions;
> +	int max_sessions;
> +	wait_queue_head_t session_wait;
>  };
>  
>  #define to_tpm_chip(d) container_of(d, struct tpm_chip, dev)
> @@ -571,6 +599,13 @@ 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);
>  void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle,
>  			    unsigned int flags);
> +static inline void tpm2_session_clear_reserved(struct tpm_chip *chip,
> +					       struct tpm_space *space)
> +{
> +	if (space->reserved_handle && !tpm2_is_session_force_evict(space))
> +		tpm2_flush_context_cmd(chip, space->reserved_handle, 0);
> +	space->reserved_handle = 0;
> +}
>  int tpm2_seal_trusted(struct tpm_chip *chip,
>  		      struct trusted_key_payload *payload,
>  		      struct trusted_key_options *options);
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index 0178816..3a54479 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -1000,6 +1000,7 @@ int tpm2_auto_startup(struct tpm_chip *chip)
>  	struct tpm_buf buf;
>  	u32 nr_commands;
>  	u32 *attrs;
> +	u32 nr_sessions;
>  	int rc;
>  	int i;
>  
> @@ -1075,6 +1076,20 @@ int tpm2_auto_startup(struct tpm_chip *chip)
>  
>  	tpm_buf_destroy(&buf);
>  
> +	rc = tpm2_get_tpm_pt(chip, TPM_PT_ACTIVE_SESSIONS_MAX,
> +			     &nr_sessions, NULL);
> +	if (rc)
> +		goto out;
> +
> +	if (nr_sessions > 256)
> +		nr_sessions = 256;
> +
> +	chip->max_sessions = nr_sessions;
> +	chip->sessions = devm_kzalloc(&chip->dev,
> +				      nr_sessions * sizeof(*chip->sessions),
> +				      GFP_KERNEL);
> +	if (!chip->sessions)
> +		rc = -ENOMEM;
>  out:
>  	if (rc > 0)
>  		rc = -ENODEV;
> diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
> index ba4310a..7304280 100644
> --- a/drivers/char/tpm/tpm2-space.c
> +++ b/drivers/char/tpm/tpm2-space.c
> @@ -32,6 +32,166 @@ struct tpm2_context {
>  	__be16 blob_size;
>  } __packed;
>  
> +static struct tpm_sessions *tpm2_session_chip_get(struct tpm_chip *chip)
> +{
> +	int i;
> +
> +	for (i = 0; i < chip->max_sessions; i++)
> +		if (chip->sessions[i].space == NULL)
> +			return &chip->sessions[i];
> +
> +	return NULL;
> +}
> +
> +static struct tpm_sessions *tpm2_session_chip_find_oldest(struct tpm_chip *chip)
> +{
> +	struct tpm_sessions *sess = NULL;
> +	int i;
> +
> +	for (i = 0; i < chip->max_sessions; i++) {
> +		if (chip->sessions[i].space == NULL)
> +			continue;
> +
> +		if (!sess || time_after(sess->created,
> +					chip->sessions[i].created))
> +			sess = &chip->sessions[i];
> +	}
> +
> +	return sess;
> +}
> +
> +static void tpm2_session_chip_add(struct tpm_chip *chip,
> +				  struct tpm_space *space, u32 h)
> +{
> +	struct tpm_sessions *sess = tpm2_session_chip_get(chip);
> +
> +	sess->space = space;
> +	sess->handle = h;
> +	sess->created = jiffies;
> +}
> +
> +static void tpm2_session_chip_remove(struct tpm_chip *chip, u32 h)
> +{
> +	int i;
> +
> +	for (i = 0; i < chip->max_sessions; i++)
> +		if (chip->sessions[i].handle == h)
> +			break;
> +	if (i == chip->max_sessions) {
> +		dev_warn(&chip->dev, "Missing session %08x", h);
> +		return;
> +	}
> +
> +	memset(&chip->sessions[i], 0, sizeof(chip->sessions[i]));
> +	wake_up(&chip->session_wait);
> +}
> +
> +static int tpm2_session_forget(struct tpm_chip *chip, 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;
> +}
> +
> +static int tpm2_session_wait(struct tpm_chip *chip, struct tpm_space *space)
> +{
> +	int rc, failed;
> +	struct tpm_sessions *sess;
> +	const unsigned long min_timeout = msecs_to_jiffies(2000);
> +	unsigned long timeout = min_timeout;
> +	DEFINE_WAIT(wait);
> +
> +	for (failed = 0; ; ) {
> +		prepare_to_wait(&chip->session_wait, &wait, TASK_INTERRUPTIBLE);
> +
> +		mutex_unlock(&chip->tpm_mutex);
> +		rc = schedule_timeout_interruptible(timeout);
> +		mutex_lock(&chip->tpm_mutex);
> +
> +		finish_wait(&chip->session_wait, &wait);
> +
> +		if (signal_pending(current))
> +			/* got interrupted */
> +			return -EINTR;
> +
> +		if (rc > 0 && !tpm2_is_session_force_evict(space))
> +			/* got woken, so slot is free.  We don't
> +			 * reserve the slot here because a) we can't
> +			 * (no pending session in the TPM to evict)
> +			 * and b) no-one is hogging sessions, so no
> +			 * evidence of need.
> +			 */
> +			return 0;
> +
> +		/* timed out or victim required; select a victim
> +		 * session to kill
> +		 */
> +		sess = tpm2_session_chip_find_oldest(chip);
> +		if (sess == NULL) {
> +			/* we get here when we can't create a session
> +			 * but there are no listed active sessions
> +			 * meaning they're all in various space
> +			 * structures as victim sessions.  The wait
> +			 * queue is a fair sequence, so we need to
> +			 * wait a bit harder
> +			 */
> +			if (failed++ > 3)
> +				break;
> +			timeout *= 2;
> +			dev_info(&chip->dev, "failed to get session, waiting for %us\n", jiffies_to_msecs(timeout)/1000);
> +			continue;
> +		}
> +		/* is the victim old enough? */
> +		timeout = jiffies - sess->created;
> +		if (timeout > min_timeout)
> +			break;
> +		/* otherwise wait until the victim is old enough */
> +		timeout = min_timeout - timeout;
> +	}
> +	if (sess == NULL)
> +		/* still can't get a victim, give up */
> +		return -EINVAL;
> +
> +	/* store the physical handle */
> +	space->reserved_handle = sess->handle;
> +	dev_info(&chip->dev, "Selecting handle %08x for eviction\n",
> +		 space->reserved_handle);
> +
> +	/* cause a mapping failure if this session handle is
> +	 * ever used in the victim space again
> +	 */
> +	tpm2_session_forget(chip, sess->space, sess->handle);
> +	/* clear the session, but don't wake any other waiters */
> +	memset(sess, 0, sizeof(*sess));
> +	/* so now we have a saved physical handle but this handle is
> +	 * still in the tpm.  After this we repeat the command, but
> +	 * flush the handle once we obtain the tpm_mutex on the repeat
> +	 * so, in theory, we should have a free handle to
> +	 * re-execute
> +	 */
> +
> +	return 0;
> +}
> +
>  int tpm2_init_space(struct tpm_chip *chip, struct tpm_space *space)
>  {
>  	space->context_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
> @@ -65,8 +225,7 @@ static int tpm2_load_context(struct tpm_chip *chip, u8 *buf,
>  			 __func__, rc);
>  		tpm_buf_destroy(&tbuf);
>  		return -EFAULT;
> -	} else if ((rc & TPM2_RC_HANDLE) == TPM2_RC_HANDLE ||
> -		   rc == TPM2_RC_REFERENCE_H0) {
> +	} else if (rc == TPM2_RC_REFERENCE_H0) {
>  		rc = -ENOENT;
>  		tpm_buf_destroy(&tbuf);
>  	} else if (rc > 0) {
> @@ -126,9 +285,9 @@ static int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 *buf,
>  	return 0;
>  }
>  
> -static int tpm2_session_add(struct tpm_chip *chip,
> -			    struct tpm_space *space, u32 handle)
> +static int tpm2_session_add(struct tpm_chip *chip, u32 handle)
>  {
> +	struct tpm_space *space = &chip->work_space;
>  	int i;
>  
>  	for (i = 0; i < ARRAY_SIZE(space->session_tbl); i++)
> @@ -141,6 +300,7 @@ static int tpm2_session_add(struct tpm_chip *chip,
>  	}
>  
>  	space->session_tbl[i] = handle;
> +	tpm2_session_chip_add(chip, chip->space, handle);
>  
>  	return 0;
>  }
> @@ -155,9 +315,18 @@ void tpm2_flush_space(struct tpm_chip *chip, struct tpm_space *space)
>  					       TPM_TRANSMIT_UNLOCKED);
>  
>  	for (i = 0; i < ARRAY_SIZE(space->session_tbl); i++) {
> -		if (space->session_tbl[i])
> -			tpm2_flush_context_cmd(chip, space->session_tbl[i],
> -					       TPM_TRANSMIT_UNLOCKED);
> +		if (!space->session_tbl[i])
> +			continue;
> +
> +		tpm2_session_chip_remove(chip, space->session_tbl[i]);
> +		tpm2_flush_context_cmd(chip, space->session_tbl[i],
> +				       TPM_TRANSMIT_UNLOCKED);
> +	}
> +	if (space->reserved_handle && !tpm2_is_session_force_evict(space)) {
> +		tpm2_flush_context_cmd(chip, space->reserved_handle,
> +				       TPM_TRANSMIT_UNLOCKED);
> +		space->reserved_handle = 0;
> +		/* subtlety here: if force evict is set, we don't clear it */
>  	}
>  }
>  
> @@ -194,6 +363,7 @@ static int tpm2_load_space(struct tpm_chip *chip)
>  				       &offset, &handle);
>  		if (rc == -ENOENT) {
>  			/* load failed, just forget session */
> +			tpm2_session_chip_remove(chip, space->session_tbl[i]);
>  			space->session_tbl[i] = 0;
>  		} else if (rc) {
>  			tpm2_flush_space(chip, space);
> @@ -269,6 +439,7 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space, u32 cc,
>  	       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);
> +	chip->space = space;
>  
>  	rc = tpm2_load_space(chip);
>  	if (rc) {
> @@ -282,16 +453,28 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space, u32 cc,
>  		return rc;
>  	}
>  
> +	if (space->reserved_handle && !tpm2_is_session_force_evict(space)) {
> +		/* this is a trick to allow a previous command which
> +		 * failed because it was out of handle space to
> +		 * succeed.  The handle is still in the TPM, so now we
> +		 * flush it under the tpm_mutex which should ensure we
> +		 * can create a new one
> +		 */
> +		tpm2_flush_context_cmd(chip, space->reserved_handle,
> +				       TPM_TRANSMIT_UNLOCKED);
> +		space->reserved_handle = 0;
> +	}
> +
>  	return 0;
>  }
>  
> -static int tpm2_map_response(struct tpm_chip *chip, u32 cc, u8 *rsp, size_t len)
> +static int tpm2_map_response(struct tpm_chip *chip, u32 cc, u8 *rsp, size_t len,
> +			     u32 return_code)
>  {
>  	struct tpm_space *space = &chip->work_space;
>  	u32 phandle, phandle_type;
>  	u32 vhandle;
>  	u32 attrs;
> -	u32 return_code = get_unaligned_be32((__be32 *)&rsp[6]);
>  	int i;
>  
>  	if (return_code != TPM2_RC_SUCCESS)
> @@ -314,7 +497,7 @@ static int tpm2_map_response(struct tpm_chip *chip, u32 cc, u8 *rsp, size_t len)
>  		return 0;
>  
>  	if (phandle_type != TPM2_HT_TRANSIENT)
> -		return tpm2_session_add(chip, space, phandle);
> +		return tpm2_session_add(chip, phandle);
>  
>  	/* Garbage collect a dead context. */
>  	for (i = 0; i < ARRAY_SIZE(space->context_tbl); i++) {
> @@ -376,6 +559,7 @@ static int tpm2_save_space(struct tpm_chip *chip)
>  
>  		if (rc == -ENOENT) {
>  			/* handle error saving session, just forget it */
> +			tpm2_session_chip_remove(chip, space->session_tbl[i]);
>  			space->session_tbl[i] = 0;
>  		} else if (rc < 0) {
>  			tpm2_flush_space(chip, space);
> @@ -390,11 +574,12 @@ int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space,
>  		      u32 cc, u8 *buf, size_t bufsiz)
>  {
>  	int rc;
> +	u32 return_code = get_unaligned_be32((__be32 *)&buf[6]);
>  
>  	if (!space)
>  		return 0;
>  
> -	rc = tpm2_map_response(chip, cc, buf, bufsiz);
> +	rc = tpm2_map_response(chip, cc, buf, bufsiz, return_code);
>  	if (rc) {
>  		tpm2_flush_space(chip, space);
>  		return rc;
> @@ -412,6 +597,12 @@ int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space,
>  	       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);
> +	chip->space = NULL;
> +
> +	if (return_code == TPM2_RC_SESSION_HANDLES) {
> +		tpm2_session_wait(chip, space);
> +		return -EAGAIN;
> +	}
>  
>  	return 0;
>  }
> diff --git a/drivers/char/tpm/tpms-dev.c b/drivers/char/tpm/tpms-dev.c
> index 6efead3..9766fb4 100644
> --- a/drivers/char/tpm/tpms-dev.c
> +++ b/drivers/char/tpm/tpms-dev.c
> @@ -58,8 +58,23 @@ ssize_t tpms_write(struct file *file, const char __user *buf,
>  {
>  	struct file_priv *fpriv = file->private_data;
>  	struct tpms_priv *priv = container_of(fpriv, struct tpms_priv, priv);
> +	int count = 0;
> +	const int max_count = 3; /* number of retries */
> +	int rc;
> +
> +	for (count = 0; count < max_count; count++) {
> +		rc = tpm_common_write(file, buf, size, off, &priv->space);
> +		if (rc != -EAGAIN)
> +			break;
> +		if (count == max_count - 2)
> +			/* second to last go around, force an eviction if
> +			 * this go fails, so final go should succeed
> +			 */
> +			tpm2_session_force_evict(&priv->space);
> +	}
> +	tpm2_session_clear_reserved(fpriv->chip, &priv->space);
>  
> -	return tpm_common_write(file, buf, size, off, &priv->space);
> +	return rc;
>  }
>  
>  const struct file_operations tpms_fops = {
> -- 
> 2.6.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH 2/2] tpm2-space: add handling for global session exhaustion

[Jarkko Sakkinen]

On Mon, Jan 23, 2017 at 09:38:33PM -0800, James Bottomley wrote:
> In a TPM2, sessions can be globally exhausted once there are
> TPM_PT_ACTIVE_SESSION_MAX of them (even if they're all context saved).
> The Strategy for handling this is to keep a global count of all the
> sessions along with their creation time.  Then if we see the TPM run
> out of sessions (via the TPM_RC_SESSION_HANDLES) we first wait for one
> to become free, but if it doesn't, we forcibly evict an existing one.
> The eviction strategy waits until the current command is repeated to
> evict the session which should guarantee there is an available slot.
> 
> On the force eviction case, we make sure that the victim session is at
> least SESSION_TIMEOUT old (currently 2 seconds).  The wait queue for
> session slots is a FIFO one, ensuring that once we run out of
> sessions, everyone will get a session in a bounded time and once they
> get one, they'll have SESSION_TIMEOUT to use it before it may be
> subject to eviction.
> 
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>

This is not a proper review yet. Just quick question: why do you need
a real time (i.e. created)? Maybe in the force eviction case it would
be enough to sleep lets say 500 ms and pick the victim with smallest
number? I.e. just have increasing u64 counter instead of real time.

That would simplify this patch a lot and does not prevent to refine
later on if workloads show need for more complex logic.

/Jarkko

> ---
>  drivers/char/tpm/tpm-chip.c   |   1 +
>  drivers/char/tpm/tpm.h        |  39 +++++++-
>  drivers/char/tpm/tpm2-cmd.c   |  15 +++
>  drivers/char/tpm/tpm2-space.c | 213 +++++++++++++++++++++++++++++++++++++++---
>  drivers/char/tpm/tpms-dev.c   |  17 +++-
>  5 files changed, 271 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
> index 6282ad0..150c6b8 100644
> --- a/drivers/char/tpm/tpm-chip.c
> +++ b/drivers/char/tpm/tpm-chip.c
> @@ -164,6 +164,7 @@ struct tpm_chip *tpm_chip_alloc(struct device *pdev,
>  
>  	mutex_init(&chip->tpm_mutex);
>  	init_rwsem(&chip->ops_sem);
> +	init_waitqueue_head(&chip->session_wait);
>  
>  	chip->ops = ops;
>  
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index b77fc60..9fd6101 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -95,7 +95,8 @@ enum tpm2_return_codes {
>  	TPM2_RC_HANDLE		= 0x008B,
>  	TPM2_RC_INITIALIZE	= 0x0100, /* RC_VER1 */
>  	TPM2_RC_DISABLED	= 0x0120,
> -	TPM2_RC_TESTING		= 0x090A, /* RC_WARN */
> +	TPM2_RC_SESSION_HANDLES	= 0x0905, /* RC_WARN */
> +	TPM2_RC_TESTING		= 0x090A,
>  	TPM2_RC_REFERENCE_H0	= 0x0910,
>  };
>  
> @@ -137,7 +138,8 @@ enum tpm2_capabilities {
>  };
>  
>  enum tpm2_properties {
> -	TPM_PT_TOTAL_COMMANDS	= 0x0129,
> +	TPM_PT_TOTAL_COMMANDS		= 0x0129,
> +	TPM_PT_ACTIVE_SESSIONS_MAX	= 0x0111,
>  };
>  
>  enum tpm2_startup_types {
> @@ -161,8 +163,24 @@ struct tpm_space {
>  	u8 *context_buf;
>  	u32 session_tbl[6];
>  	u8 *session_buf;
> +	u32 reserved_handle;
>  };
>  
> +#define TPM2_HANDLE_FORCE_EVICT 0xFFFFFFFF
> +
> +static inline void tpm2_session_force_evict(struct tpm_space *space)
> +{
> +	/* if reserved handle is not empty, we already have a
> +	 * session for eviction, so no need to force one
> +	 */
> +	if (space->reserved_handle == 0)
> +		space->reserved_handle = TPM2_HANDLE_FORCE_EVICT;
> +}
> +static inline bool tpm2_is_session_force_evict(struct tpm_space *space)
> +{
> +	return space->reserved_handle == TPM2_HANDLE_FORCE_EVICT;
> +}
> +
>  enum tpm_chip_flags {
>  	TPM_CHIP_FLAG_TPM2		= BIT(1),
>  	TPM_CHIP_FLAG_IRQ		= BIT(2),
> @@ -175,6 +193,12 @@ struct tpm_chip_seqops {
>  	const struct seq_operations *seqops;
>  };
>  
> +struct tpm_sessions {
> +	struct tpm_space *space;
> +	u32 handle;
> +	unsigned long created;
> +};
> +
>  struct tpm_chip {
>  	struct device dev;
>  	struct device devs;
> @@ -217,8 +241,12 @@ struct tpm_chip {
>  #endif /* CONFIG_ACPI */
>  
>  	struct tpm_space work_space;
> +	struct tpm_space *space;
>  	u32 nr_commands;
>  	u32 *cc_attrs_tbl;
> +	struct tpm_sessions *sessions;
> +	int max_sessions;
> +	wait_queue_head_t session_wait;
>  };
>  
>  #define to_tpm_chip(d) container_of(d, struct tpm_chip, dev)
> @@ -571,6 +599,13 @@ 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);
>  void tpm2_flush_context_cmd(struct tpm_chip *chip, u32 handle,
>  			    unsigned int flags);
> +static inline void tpm2_session_clear_reserved(struct tpm_chip *chip,
> +					       struct tpm_space *space)
> +{
> +	if (space->reserved_handle && !tpm2_is_session_force_evict(space))
> +		tpm2_flush_context_cmd(chip, space->reserved_handle, 0);
> +	space->reserved_handle = 0;
> +}
>  int tpm2_seal_trusted(struct tpm_chip *chip,
>  		      struct trusted_key_payload *payload,
>  		      struct trusted_key_options *options);
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index 0178816..3a54479 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -1000,6 +1000,7 @@ int tpm2_auto_startup(struct tpm_chip *chip)
>  	struct tpm_buf buf;
>  	u32 nr_commands;
>  	u32 *attrs;
> +	u32 nr_sessions;
>  	int rc;
>  	int i;
>  
> @@ -1075,6 +1076,20 @@ int tpm2_auto_startup(struct tpm_chip *chip)
>  
>  	tpm_buf_destroy(&buf);
>  
> +	rc = tpm2_get_tpm_pt(chip, TPM_PT_ACTIVE_SESSIONS_MAX,
> +			     &nr_sessions, NULL);
> +	if (rc)
> +		goto out;
> +
> +	if (nr_sessions > 256)
> +		nr_sessions = 256;
> +
> +	chip->max_sessions = nr_sessions;
> +	chip->sessions = devm_kzalloc(&chip->dev,
> +				      nr_sessions * sizeof(*chip->sessions),
> +				      GFP_KERNEL);
> +	if (!chip->sessions)
> +		rc = -ENOMEM;
>  out:
>  	if (rc > 0)
>  		rc = -ENODEV;
> diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
> index ba4310a..7304280 100644
> --- a/drivers/char/tpm/tpm2-space.c
> +++ b/drivers/char/tpm/tpm2-space.c
> @@ -32,6 +32,166 @@ struct tpm2_context {
>  	__be16 blob_size;
>  } __packed;
>  
> +static struct tpm_sessions *tpm2_session_chip_get(struct tpm_chip *chip)
> +{
> +	int i;
> +
> +	for (i = 0; i < chip->max_sessions; i++)
> +		if (chip->sessions[i].space == NULL)
> +			return &chip->sessions[i];
> +
> +	return NULL;
> +}
> +
> +static struct tpm_sessions *tpm2_session_chip_find_oldest(struct tpm_chip *chip)
> +{
> +	struct tpm_sessions *sess = NULL;
> +	int i;
> +
> +	for (i = 0; i < chip->max_sessions; i++) {
> +		if (chip->sessions[i].space == NULL)
> +			continue;
> +
> +		if (!sess || time_after(sess->created,
> +					chip->sessions[i].created))
> +			sess = &chip->sessions[i];
> +	}
> +
> +	return sess;
> +}
> +
> +static void tpm2_session_chip_add(struct tpm_chip *chip,
> +				  struct tpm_space *space, u32 h)
> +{
> +	struct tpm_sessions *sess = tpm2_session_chip_get(chip);
> +
> +	sess->space = space;
> +	sess->handle = h;
> +	sess->created = jiffies;
> +}
> +
> +static void tpm2_session_chip_remove(struct tpm_chip *chip, u32 h)
> +{
> +	int i;
> +
> +	for (i = 0; i < chip->max_sessions; i++)
> +		if (chip->sessions[i].handle == h)
> +			break;
> +	if (i == chip->max_sessions) {
> +		dev_warn(&chip->dev, "Missing session %08x", h);
> +		return;
> +	}
> +
> +	memset(&chip->sessions[i], 0, sizeof(chip->sessions[i]));
> +	wake_up(&chip->session_wait);
> +}
> +
> +static int tpm2_session_forget(struct tpm_chip *chip, 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;
> +}
> +
> +static int tpm2_session_wait(struct tpm_chip *chip, struct tpm_space *space)
> +{
> +	int rc, failed;
> +	struct tpm_sessions *sess;
> +	const unsigned long min_timeout = msecs_to_jiffies(2000);
> +	unsigned long timeout = min_timeout;
> +	DEFINE_WAIT(wait);
> +
> +	for (failed = 0; ; ) {
> +		prepare_to_wait(&chip->session_wait, &wait, TASK_INTERRUPTIBLE);
> +
> +		mutex_unlock(&chip->tpm_mutex);
> +		rc = schedule_timeout_interruptible(timeout);
> +		mutex_lock(&chip->tpm_mutex);
> +
> +		finish_wait(&chip->session_wait, &wait);
> +
> +		if (signal_pending(current))
> +			/* got interrupted */
> +			return -EINTR;
> +
> +		if (rc > 0 && !tpm2_is_session_force_evict(space))
> +			/* got woken, so slot is free.  We don't
> +			 * reserve the slot here because a) we can't
> +			 * (no pending session in the TPM to evict)
> +			 * and b) no-one is hogging sessions, so no
> +			 * evidence of need.
> +			 */
> +			return 0;
> +
> +		/* timed out or victim required; select a victim
> +		 * session to kill
> +		 */
> +		sess = tpm2_session_chip_find_oldest(chip);
> +		if (sess == NULL) {
> +			/* we get here when we can't create a session
> +			 * but there are no listed active sessions
> +			 * meaning they're all in various space
> +			 * structures as victim sessions.  The wait
> +			 * queue is a fair sequence, so we need to
> +			 * wait a bit harder
> +			 */
> +			if (failed++ > 3)
> +				break;
> +			timeout *= 2;
> +			dev_info(&chip->dev, "failed to get session, waiting for %us\n", jiffies_to_msecs(timeout)/1000);
> +			continue;
> +		}
> +		/* is the victim old enough? */
> +		timeout = jiffies - sess->created;
> +		if (timeout > min_timeout)
> +			break;
> +		/* otherwise wait until the victim is old enough */
> +		timeout = min_timeout - timeout;
> +	}
> +	if (sess == NULL)
> +		/* still can't get a victim, give up */
> +		return -EINVAL;
> +
> +	/* store the physical handle */
> +	space->reserved_handle = sess->handle;
> +	dev_info(&chip->dev, "Selecting handle %08x for eviction\n",
> +		 space->reserved_handle);
> +
> +	/* cause a mapping failure if this session handle is
> +	 * ever used in the victim space again
> +	 */
> +	tpm2_session_forget(chip, sess->space, sess->handle);
> +	/* clear the session, but don't wake any other waiters */
> +	memset(sess, 0, sizeof(*sess));
> +	/* so now we have a saved physical handle but this handle is
> +	 * still in the tpm.  After this we repeat the command, but
> +	 * flush the handle once we obtain the tpm_mutex on the repeat
> +	 * so, in theory, we should have a free handle to
> +	 * re-execute
> +	 */
> +
> +	return 0;
> +}
> +
>  int tpm2_init_space(struct tpm_chip *chip, struct tpm_space *space)
>  {
>  	space->context_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
> @@ -65,8 +225,7 @@ static int tpm2_load_context(struct tpm_chip *chip, u8 *buf,
>  			 __func__, rc);
>  		tpm_buf_destroy(&tbuf);
>  		return -EFAULT;
> -	} else if ((rc & TPM2_RC_HANDLE) == TPM2_RC_HANDLE ||
> -		   rc == TPM2_RC_REFERENCE_H0) {
> +	} else if (rc == TPM2_RC_REFERENCE_H0) {
>  		rc = -ENOENT;
>  		tpm_buf_destroy(&tbuf);
>  	} else if (rc > 0) {
> @@ -126,9 +285,9 @@ static int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 *buf,
>  	return 0;
>  }
>  
> -static int tpm2_session_add(struct tpm_chip *chip,
> -			    struct tpm_space *space, u32 handle)
> +static int tpm2_session_add(struct tpm_chip *chip, u32 handle)
>  {
> +	struct tpm_space *space = &chip->work_space;
>  	int i;
>  
>  	for (i = 0; i < ARRAY_SIZE(space->session_tbl); i++)
> @@ -141,6 +300,7 @@ static int tpm2_session_add(struct tpm_chip *chip,
>  	}
>  
>  	space->session_tbl[i] = handle;
> +	tpm2_session_chip_add(chip, chip->space, handle);
>  
>  	return 0;
>  }
> @@ -155,9 +315,18 @@ void tpm2_flush_space(struct tpm_chip *chip, struct tpm_space *space)
>  					       TPM_TRANSMIT_UNLOCKED);
>  
>  	for (i = 0; i < ARRAY_SIZE(space->session_tbl); i++) {
> -		if (space->session_tbl[i])
> -			tpm2_flush_context_cmd(chip, space->session_tbl[i],
> -					       TPM_TRANSMIT_UNLOCKED);
> +		if (!space->session_tbl[i])
> +			continue;
> +
> +		tpm2_session_chip_remove(chip, space->session_tbl[i]);
> +		tpm2_flush_context_cmd(chip, space->session_tbl[i],
> +				       TPM_TRANSMIT_UNLOCKED);
> +	}
> +	if (space->reserved_handle && !tpm2_is_session_force_evict(space)) {
> +		tpm2_flush_context_cmd(chip, space->reserved_handle,
> +				       TPM_TRANSMIT_UNLOCKED);
> +		space->reserved_handle = 0;
> +		/* subtlety here: if force evict is set, we don't clear it */
>  	}
>  }
>  
> @@ -194,6 +363,7 @@ static int tpm2_load_space(struct tpm_chip *chip)
>  				       &offset, &handle);
>  		if (rc == -ENOENT) {
>  			/* load failed, just forget session */
> +			tpm2_session_chip_remove(chip, space->session_tbl[i]);
>  			space->session_tbl[i] = 0;
>  		} else if (rc) {
>  			tpm2_flush_space(chip, space);
> @@ -269,6 +439,7 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space, u32 cc,
>  	       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);
> +	chip->space = space;
>  
>  	rc = tpm2_load_space(chip);
>  	if (rc) {
> @@ -282,16 +453,28 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space, u32 cc,
>  		return rc;
>  	}
>  
> +	if (space->reserved_handle && !tpm2_is_session_force_evict(space)) {
> +		/* this is a trick to allow a previous command which
> +		 * failed because it was out of handle space to
> +		 * succeed.  The handle is still in the TPM, so now we
> +		 * flush it under the tpm_mutex which should ensure we
> +		 * can create a new one
> +		 */
> +		tpm2_flush_context_cmd(chip, space->reserved_handle,
> +				       TPM_TRANSMIT_UNLOCKED);
> +		space->reserved_handle = 0;
> +	}
> +
>  	return 0;
>  }
>  
> -static int tpm2_map_response(struct tpm_chip *chip, u32 cc, u8 *rsp, size_t len)
> +static int tpm2_map_response(struct tpm_chip *chip, u32 cc, u8 *rsp, size_t len,
> +			     u32 return_code)
>  {
>  	struct tpm_space *space = &chip->work_space;
>  	u32 phandle, phandle_type;
>  	u32 vhandle;
>  	u32 attrs;
> -	u32 return_code = get_unaligned_be32((__be32 *)&rsp[6]);
>  	int i;
>  
>  	if (return_code != TPM2_RC_SUCCESS)
> @@ -314,7 +497,7 @@ static int tpm2_map_response(struct tpm_chip *chip, u32 cc, u8 *rsp, size_t len)
>  		return 0;
>  
>  	if (phandle_type != TPM2_HT_TRANSIENT)
> -		return tpm2_session_add(chip, space, phandle);
> +		return tpm2_session_add(chip, phandle);
>  
>  	/* Garbage collect a dead context. */
>  	for (i = 0; i < ARRAY_SIZE(space->context_tbl); i++) {
> @@ -376,6 +559,7 @@ static int tpm2_save_space(struct tpm_chip *chip)
>  
>  		if (rc == -ENOENT) {
>  			/* handle error saving session, just forget it */
> +			tpm2_session_chip_remove(chip, space->session_tbl[i]);
>  			space->session_tbl[i] = 0;
>  		} else if (rc < 0) {
>  			tpm2_flush_space(chip, space);
> @@ -390,11 +574,12 @@ int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space,
>  		      u32 cc, u8 *buf, size_t bufsiz)
>  {
>  	int rc;
> +	u32 return_code = get_unaligned_be32((__be32 *)&buf[6]);
>  
>  	if (!space)
>  		return 0;
>  
> -	rc = tpm2_map_response(chip, cc, buf, bufsiz);
> +	rc = tpm2_map_response(chip, cc, buf, bufsiz, return_code);
>  	if (rc) {
>  		tpm2_flush_space(chip, space);
>  		return rc;
> @@ -412,6 +597,12 @@ int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space,
>  	       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);
> +	chip->space = NULL;
> +
> +	if (return_code == TPM2_RC_SESSION_HANDLES) {
> +		tpm2_session_wait(chip, space);
> +		return -EAGAIN;
> +	}
>  
>  	return 0;
>  }
> diff --git a/drivers/char/tpm/tpms-dev.c b/drivers/char/tpm/tpms-dev.c
> index 6efead3..9766fb4 100644
> --- a/drivers/char/tpm/tpms-dev.c
> +++ b/drivers/char/tpm/tpms-dev.c
> @@ -58,8 +58,23 @@ ssize_t tpms_write(struct file *file, const char __user *buf,
>  {
>  	struct file_priv *fpriv = file->private_data;
>  	struct tpms_priv *priv = container_of(fpriv, struct tpms_priv, priv);
> +	int count = 0;
> +	const int max_count = 3; /* number of retries */
> +	int rc;
> +
> +	for (count = 0; count < max_count; count++) {
> +		rc = tpm_common_write(file, buf, size, off, &priv->space);
> +		if (rc != -EAGAIN)
> +			break;
> +		if (count == max_count - 2)
> +			/* second to last go around, force an eviction if
> +			 * this go fails, so final go should succeed
> +			 */
> +			tpm2_session_force_evict(&priv->space);
> +	}
> +	tpm2_session_clear_reserved(fpriv->chip, &priv->space);
>  
> -	return tpm_common_write(file, buf, size, off, &priv->space);
> +	return rc;
>  }
>  
>  const struct file_operations tpms_fops = {
> -- 
> 2.6.6
> 
> --
> 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: [PATCH 2/2] tpm2-space: add handling for global session exhaustion

[James Bottomley]

On Thu, 2017-01-26 at 14:56 +0200, Jarkko Sakkinen wrote:
> On Mon, Jan 23, 2017 at 09:38:33PM -0800, James Bottomley wrote:
> > In a TPM2, sessions can be globally exhausted once there are
> > TPM_PT_ACTIVE_SESSION_MAX of them (even if they're all context 
> > saved). The Strategy for handling this is to keep a global count of 
> > all the sessions along with their creation time.  Then if we see 
> > the TPM run out of sessions (via the TPM_RC_SESSION_HANDLES) we 
> > first wait for one to become free, but if it doesn't, we forcibly 
> > evict an existing one. The eviction strategy waits until the 
> > current command is repeated to evict the session which should 
> > guarantee there is an available slot.
> > 
> > On the force eviction case, we make sure that the victim session is 
> > at least SESSION_TIMEOUT old (currently 2 seconds).  The wait queue
> > for session slots is a FIFO one, ensuring that once we run out of
> > sessions, everyone will get a session in a bounded time and once 
> > they get one, they'll have SESSION_TIMEOUT to use it before it may 
> > be subject to eviction.
> > 
> > Signed-off-by: James Bottomley <
> > James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org>
> 
> This is not a proper review yet. Just quick question: why do you need
> a real time (i.e. created)? Maybe in the force eviction case it would
> be enough to sleep lets say 500 ms and pick the victim with smallest
> number? I.e. just have increasing u64 counter instead of real time.

So that if the oldest session has already been around for > 2s there's
no need to wait.  In order to guarantee everyone gets a session for at
least 2s without tracking the age of sessions, you'd have to sleep for
2s after you find the oldest session.

> That would simplify this patch a lot and does not prevent to refine
> later on if workloads show need for more complex logic.

An increasing monotonic counter would actually not be much simpler: all
you could really cut out would be the four lines and two comments at
the bottom of the for loop in tpm2_session_wait() which check the age
of the found session.

James


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH 2/2] tpm2-space: add handling for global session exhaustion

[James Bottomley]

On Thu, 2017-01-26 at 14:56 +0200, Jarkko Sakkinen wrote:
> On Mon, Jan 23, 2017 at 09:38:33PM -0800, James Bottomley wrote:
> > In a TPM2, sessions can be globally exhausted once there are
> > TPM_PT_ACTIVE_SESSION_MAX of them (even if they're all context 
> > saved). The Strategy for handling this is to keep a global count of 
> > all the sessions along with their creation time.  Then if we see 
> > the TPM run out of sessions (via the TPM_RC_SESSION_HANDLES) we 
> > first wait for one to become free, but if it doesn't, we forcibly 
> > evict an existing one. The eviction strategy waits until the 
> > current command is repeated to evict the session which should 
> > guarantee there is an available slot.
> > 
> > On the force eviction case, we make sure that the victim session is 
> > at least SESSION_TIMEOUT old (currently 2 seconds).  The wait queue
> > for session slots is a FIFO one, ensuring that once we run out of
> > sessions, everyone will get a session in a bounded time and once 
> > they get one, they'll have SESSION_TIMEOUT to use it before it may 
> > be subject to eviction.
> > 
> > Signed-off-by: James Bottomley <
> > James.Bottomley@HansenPartnership.com>
> 
> This is not a proper review yet. Just quick question: why do you need
> a real time (i.e. created)? Maybe in the force eviction case it would
> be enough to sleep lets say 500 ms and pick the victim with smallest
> number? I.e. just have increasing u64 counter instead of real time.

So that if the oldest session has already been around for > 2s there's
no need to wait.  In order to guarantee everyone gets a session for at
least 2s without tracking the age of sessions, you'd have to sleep for
2s after you find the oldest session.

> That would simplify this patch a lot and does not prevent to refine
> later on if workloads show need for more complex logic.

An increasing monotonic counter would actually not be much simpler: all
you could really cut out would be the four lines and two comments at
the bottom of the for loop in tpm2_session_wait() which check the age
of the found session.

James

Re: [PATCH 2/2] tpm2-space: add handling for global session exhaustion

[Jarkko Sakkinen]

On Thu, Jan 26, 2017 at 04:45:52PM -0800, James Bottomley wrote:
> On Thu, 2017-01-26 at 14:56 +0200, Jarkko Sakkinen wrote:
> > On Mon, Jan 23, 2017 at 09:38:33PM -0800, James Bottomley wrote:
> > > In a TPM2, sessions can be globally exhausted once there are
> > > TPM_PT_ACTIVE_SESSION_MAX of them (even if they're all context 
> > > saved). The Strategy for handling this is to keep a global count of 
> > > all the sessions along with their creation time.  Then if we see 
> > > the TPM run out of sessions (via the TPM_RC_SESSION_HANDLES) we 
> > > first wait for one to become free, but if it doesn't, we forcibly 
> > > evict an existing one. The eviction strategy waits until the 
> > > current command is repeated to evict the session which should 
> > > guarantee there is an available slot.
> > > 
> > > On the force eviction case, we make sure that the victim session is 
> > > at least SESSION_TIMEOUT old (currently 2 seconds).  The wait queue
> > > for session slots is a FIFO one, ensuring that once we run out of
> > > sessions, everyone will get a session in a bounded time and once 
> > > they get one, they'll have SESSION_TIMEOUT to use it before it may 
> > > be subject to eviction.
> > > 
> > > Signed-off-by: James Bottomley <
> > > James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org>
> > 
> > This is not a proper review yet. Just quick question: why do you need
> > a real time (i.e. created)? Maybe in the force eviction case it would
> > be enough to sleep lets say 500 ms and pick the victim with smallest
> > number? I.e. just have increasing u64 counter instead of real time.
> 
> So that if the oldest session has already been around for > 2s there's
> no need to wait.  In order to guarantee everyone gets a session for at
> least 2s without tracking the age of sessions, you'd have to sleep for
> 2s after you find the oldest session.
> 
> > That would simplify this patch a lot and does not prevent to refine
> > later on if workloads show need for more complex logic.
> 
> An increasing monotonic counter would actually not be much simpler: all
> you could really cut out would be the four lines and two comments at
> the bottom of the for loop in tpm2_session_wait() which check the age
> of the found session.
> 
> James

Right. Thanks for explaining this. I'll check the code with more detail
ASAP.

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot

Re: [PATCH 2/2] tpm2-space: add handling for global session exhaustion

[Jarkko Sakkinen]

On Thu, Jan 26, 2017 at 04:45:52PM -0800, James Bottomley wrote:
> On Thu, 2017-01-26 at 14:56 +0200, Jarkko Sakkinen wrote:
> > On Mon, Jan 23, 2017 at 09:38:33PM -0800, James Bottomley wrote:
> > > In a TPM2, sessions can be globally exhausted once there are
> > > TPM_PT_ACTIVE_SESSION_MAX of them (even if they're all context 
> > > saved). The Strategy for handling this is to keep a global count of 
> > > all the sessions along with their creation time.  Then if we see 
> > > the TPM run out of sessions (via the TPM_RC_SESSION_HANDLES) we 
> > > first wait for one to become free, but if it doesn't, we forcibly 
> > > evict an existing one. The eviction strategy waits until the 
> > > current command is repeated to evict the session which should 
> > > guarantee there is an available slot.
> > > 
> > > On the force eviction case, we make sure that the victim session is 
> > > at least SESSION_TIMEOUT old (currently 2 seconds).  The wait queue
> > > for session slots is a FIFO one, ensuring that once we run out of
> > > sessions, everyone will get a session in a bounded time and once 
> > > they get one, they'll have SESSION_TIMEOUT to use it before it may 
> > > be subject to eviction.
> > > 
> > > Signed-off-by: James Bottomley <
> > > James.Bottomley@HansenPartnership.com>
> > 
> > This is not a proper review yet. Just quick question: why do you need
> > a real time (i.e. created)? Maybe in the force eviction case it would
> > be enough to sleep lets say 500 ms and pick the victim with smallest
> > number? I.e. just have increasing u64 counter instead of real time.
> 
> So that if the oldest session has already been around for > 2s there's
> no need to wait.  In order to guarantee everyone gets a session for at
> least 2s without tracking the age of sessions, you'd have to sleep for
> 2s after you find the oldest session.
> 
> > That would simplify this patch a lot and does not prevent to refine
> > later on if workloads show need for more complex logic.
> 
> An increasing monotonic counter would actually not be much simpler: all
> you could really cut out would be the four lines and two comments at
> the bottom of the for loop in tpm2_session_wait() which check the age
> of the found session.
> 
> James

Right. Thanks for explaining this. I'll check the code with more detail
ASAP.

/Jarkko