Add session isolation and context saving to the space manager

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

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

Quoted 5 lines · James Bottomley

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

Quoted 7 lines

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

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

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

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

Quoted 14 lines · Jarkko Sakkinen

Quoted 5 lines · James Bottomley

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

Quoted 7 lines

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

Quoted 2 lines

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

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

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

Quoted 5 lines · Jarkko Sakkinen

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

Are you saying that, when a process flushes (or continue = false) a session, this code won't actually flush the context? You'll wait until another startauthsession creates a handle, and then delete other occurrences of it?

Quoted 19 lines · James Bottomley

Quoted 15 lines · Jarkko Sakkinen

Quoted 5 lines · James Bottomley

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

Quoted 8 lines

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

Quoted 2 lines

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

Quoted 8 lines

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

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

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

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

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

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

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

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

/Jarkko

Quoted 29 lines · Jarkko Sakkinen

Quoted 19 lines · James Bottomley

Quoted 15 lines · Jarkko Sakkinen

Quoted 5 lines · James Bottomley

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

Quoted 8 lines

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

Quoted 2 lines

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

Quoted 8 lines

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

Quoted 7 lines

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

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

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

Quoted 3 lines

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

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

Quoted 10 lines

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

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

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

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

Quoted 3 lines

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

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

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

Quoted 5 lines

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

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

James

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

Signed-off-by James Bottomley <[email protected]>

---
 drivers/char/tpm/tpm-chip.c   |   6 ++
 drivers/char/tpm/tpm.h        |   1 +
 drivers/char/tpm/tpm2-space.c | 223 ++++++++++++++++++++++++++++--------------
 drivers/char/tpm/tpms-dev.c   |   7 ++
 4 files changed, 164 insertions(+), 73 deletions(-)

drivers/char/tpm/tpm-chip.c +6 -0

index 96ea93e..a625884 100644
@@ -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;
 

drivers/char/tpm/tpm.h +1 -0

index 265b7f5..9923daa 100644
@@ -159,6 +159,7 @@ struct tpm_space {
 	u32 context_tbl[14];
 	u8 *context_buf;
 	u32 session_tbl[6];
+	u8 *session_buf;
 };
 
 enum tpm_chip_flags {

drivers/char/tpm/tpm2-space.c +150 -73

index 49048af..04c9431 100644
@@ -27,6 +27,91 @@ enum tpm2_handle_types {
 
 #define TPM2_HT_TAG_FOR_FLUSH	0xF0000000
 
+struct tpm2_context {
+	__be64 sequence;
+	__be32 saved_handle;
+	__be32 hierarchy;
+	__be16 blob_size;
+} __packed;
+
+static int tpm2_context_save(struct tpm_chip *chip, u8 *area,
+			     int *offset, u32 handle)
+{
+	struct tpm_buf buf;
+	u32 s;
+	int rc;
+
+	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS,
+			  TPM2_CC_CONTEXT_SAVE);
+	if (rc)
+		return rc;
+
+	tpm_buf_append_u32(&buf, handle);
+
+	rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE,
+			      TPM_HEADER_SIZE, TPM_TRANSMIT_UNLOCKED,
+			      NULL);
+	if ((rc & TPM2_RC_HANDLE) == TPM2_RC_HANDLE) {
+		/* no handle to save */
+		rc = 1;
+		goto out;
+	} else if (rc) {
+		dev_warn(&chip->dev, "%s: saving failed with %d\n",
+			 __func__, rc);
+		rc = -EFAULT;
+		goto out;
+	}
+
+	s = tpm_buf_length(&buf) - TPM_HEADER_SIZE;
+	if ((*offset + s) > PAGE_SIZE) {
+		dev_warn(&chip->dev, "out of context storage\n");
+		rc = -ENOMEM;
+		goto out;
+	}
+
+	memcpy(&area[*offset], &buf.data[TPM_HEADER_SIZE], s);
+	*offset += s;
+
+ out:
+	tpm_buf_destroy(&buf);
+	return rc;
+}
+
+static int tpm2_context_load(struct tpm_chip *chip, u8 *area,
+			     int *offset, u32 *handle)
+{
+	struct tpm_buf buf;
+	struct tpm2_context *ctx;
+	int rc;
+	u32 s;
+
+	rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS,
+			  TPM2_CC_CONTEXT_LOAD);
+	if (rc)
+		return rc;
+
+	ctx = (struct tpm2_context *)&area[*offset];
+	s = sizeof(*ctx) + be16_to_cpu(ctx->blob_size);
+	tpm_buf_append(&buf, (const void *)ctx, s);
+
+	rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE,
+			      TPM_HEADER_SIZE + 4,
+			      TPM_TRANSMIT_UNLOCKED, NULL);
+	if (rc) {
+		dev_warn(&chip->dev, "context loading failed with %d\n", rc);
+		rc = -EFAULT;
+		goto out;
+	}
+	*handle = get_unaligned_be32((__be32 *)&buf.data[TPM_HEADER_SIZE]);
+
+	*offset += s;
+
+ out:
+	tpm_buf_destroy(&buf);
+
+	return rc;
+}
+
 static int tpm2_session_find(struct tpm_space *space, u32 handle)
 {
 	int i;

@@ -58,11 +143,35 @@ static int tpm2_session_add(struct tpm_chip *chip,
 	return 0;
 }
 
+static int tpm2_session_forget(struct tpm_space *space, u32 handle)
+{
+	int i, j;
+	struct tpm2_context *ctx;
+
+	for (i = 0, j = 0; i < ARRAY_SIZE(space->session_tbl); i++) {
+		if (space->session_tbl[i] == 0)
+			continue;
+
+		ctx = (struct tpm2_context *)&space->session_buf[j];
+		j += sizeof(*ctx) + get_unaligned_be16(&ctx->blob_size);
+
+		if (space->session_tbl[i] != handle)
+			continue;
+
+		/* forget the session context */
+		memcpy(ctx, &space->session_buf[j], PAGE_SIZE - j);
+		space->session_tbl[i] = 0;
+		break;
+	}
+	if (i == ARRAY_SIZE(space->session_tbl))
+		return -EINVAL;
+	return 0;
+}
+
 /* if a space is active, emulate some commands */
-static int tpm2_intercept(struct tpm_chip *chip, struct tpm_space *space,
-			  u32 cc, u8 *buf, size_t bufsiz)
+static int tpm2_intercept(struct tpm_chip *chip, u32 cc, u8 *buf, size_t bufsiz)
 {
-	int j;
+	struct tpm_space *space = &chip->work_space;
 	u32 handle, handle_type;
 
 	if (!space)

@@ -78,13 +187,7 @@ static int tpm2_intercept(struct tpm_chip *chip, struct tpm_space *space,
 		/* let the TPM figure out and return the error */
 		return 0;
 
-	j = tpm2_session_find(space, handle);
-	if (j < 0)
-		return -EINVAL;
-
-	space->session_tbl[j] |= TPM2_HT_TAG_FOR_FLUSH;
-
-	return 0;
+	return tpm2_session_forget(space, handle);
 }
 
 void tpm2_flush_space(struct tpm_chip *chip, struct tpm_space *space)

@@ -104,22 +207,12 @@ void tpm2_flush_space(struct tpm_chip *chip, struct tpm_space *space)
 	}
 }
 
-struct tpm2_context {
-	__be64 sequence;
-	__be32 saved_handle;
-	__be32 hierarchy;
-	__be16 blob_size;
-} __packed;
-
 static int tpm2_load_space(struct tpm_chip *chip)
 {
 	struct tpm_space *space = &chip->work_space;
-	struct tpm2_context *ctx;
-	struct tpm_buf buf;
 	int i;
 	int j;
 	int rc;
-	u32 s;
 
 	for (i = 0, j = 0; i < ARRAY_SIZE(space->context_tbl); i++) {
 		if (!space->context_tbl[i])

@@ -131,37 +224,33 @@ static int tpm2_load_space(struct tpm_chip *chip)
 			return -EFAULT;
 		}
 
-		rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS,
-				 TPM2_CC_CONTEXT_LOAD);
+		rc = tpm2_context_load(chip, space->context_buf,
+				       &j, &space->context_tbl[i]);
 		if (rc)
-			return rc;
-
-		ctx = (struct tpm2_context *)&space->context_buf[j];
-		s = sizeof(*ctx) + be16_to_cpu(ctx->blob_size);
-		tpm_buf_append(&buf, &space->context_buf[j], s);
-
-		rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE,
-				      TPM_HEADER_SIZE + 4,
-				      TPM_TRANSMIT_UNLOCKED, NULL);
-		if (rc) {
-			dev_warn(&chip->dev, "%s: loading failed with %d\n",
-				 __func__, rc);
-			rc = -EFAULT;
 			goto out_err;
-		}
 
-		space->context_tbl[i] =
-			be32_to_cpup((__be32 *)&buf.data[TPM_HEADER_SIZE]);
+	}
 
-		j += s;
+	for (i = 0, j = 0; i < ARRAY_SIZE(space->session_tbl); i++) {
+		u32 handle;
 
-		tpm_buf_destroy(&buf);
+		if (!space->session_tbl[i])
+			continue;
+
+		rc = tpm2_context_load(chip, space->session_buf,
+				       &j, &handle);
+		if (rc)
+			goto out_err;
+		if (handle != (space->session_tbl[i] & ~TPM2_HT_TAG_FOR_FLUSH)) {
+			dev_warn(&chip->dev, "session restored to wrong handle\n");
+			rc = -EFAULT;
+			goto out_err;
+		}
 	}
 
 	return 0;
 
 out_err:
-	tpm_buf_destroy(&buf);
 	tpm2_flush_space(chip, space);
 	return rc;
 }

@@ -297,8 +386,9 @@ int tpm2_prepare_space(struct tpm_chip *chip, struct tpm_space *space,
 	memcpy(&chip->work_space.session_tbl, &space->session_tbl,
 	       sizeof(space->session_tbl));
 	memcpy(chip->work_space.context_buf, space->context_buf, PAGE_SIZE);
+	memcpy(chip->work_space.session_buf, space->session_buf, PAGE_SIZE);
 
-	rc = tpm2_intercept(chip, space, cc, buf, bufsiz);
+	rc = tpm2_intercept(chip, cc, buf, bufsiz);
 	if (rc)
 		return rc;
 

@@ -384,59 +474,45 @@ static int tpm2_map_response(struct tpm_chip *chip, u32 cc, u8 *rsp, size_t len)
 static int tpm2_save_space(struct tpm_chip *chip)
 {
 	struct tpm_space *space = &chip->work_space;
-	struct tpm_buf buf;
 	int i;
 	int j;
 	int rc;
-	u32 s;
 
 	for (i = 0, j = 0; i < ARRAY_SIZE(space->context_tbl); i++) {
 		if (!(space->context_tbl[i] && ~space->context_tbl[i]))
 			continue;
 
-		rc = tpm_buf_init(&buf, TPM2_ST_NO_SESSIONS,
-				  TPM2_CC_CONTEXT_SAVE);
-		if (rc)
-			return rc;
-
-		tpm_buf_append_u32(&buf, space->context_tbl[i]);
-
-		rc = tpm_transmit_cmd(chip, NULL, buf.data, PAGE_SIZE,
-				      TPM_HEADER_SIZE, TPM_TRANSMIT_UNLOCKED,
-				      NULL);
-		if ((rc & TPM2_RC_HANDLE) == TPM2_RC_HANDLE) {
+		rc = tpm2_context_save(chip, space->context_buf, &j,
+				       space->context_tbl[i]);
+		if (rc < 0)
+			goto out_err;
+		if (rc > 0) {
 			space->context_tbl[i] = 0;
 			continue;
-		} else if (rc) {
-			dev_warn(&chip->dev, "%s: saving failed with %d\n",
-				 __func__, rc);
-			rc = -EFAULT;
-			goto out_err;
 		}
 
-		s = tpm_buf_length(&buf) - TPM_HEADER_SIZE;
-		if ((j + s) > PAGE_SIZE) {
-			dev_warn(&chip->dev, "%s: out of backing storage\n",
-				 __func__);
-			rc = -ENOMEM;
-			goto out_err;
-		}
-
-		memcpy(&space->context_buf[j], &buf.data[TPM_HEADER_SIZE], s);
-
 		tpm2_flush_context_cmd(chip, space->context_tbl[i],
 				       TPM_TRANSMIT_UNLOCKED);
 
 		space->context_tbl[i] = ~0;
+	}
 
-		j += s;
+	for (i = 0, j = 0; i < ARRAY_SIZE(space->session_tbl); i++) {
+		if (!space->session_tbl[i])
+			continue;
 
-		tpm_buf_destroy(&buf);
+		rc = tpm2_context_save(chip, space->session_buf, &j,
+				       space->session_tbl[i]);
+		if (rc < 0)
+			goto out_err;
+		if (rc > 0) {
+			space->context_tbl[i] = 0;
+			continue;
+		}
 	}
 
 	return 0;
 out_err:
-	tpm_buf_destroy(&buf);
 	tpm2_flush_space(chip, space);
 	return rc;
 }

@@ -462,6 +538,7 @@ int tpm2_commit_space(struct tpm_chip *chip, struct tpm_space *space,
 	memcpy(&space->session_tbl, &chip->work_space.session_tbl,
 	       sizeof(space->session_tbl));
 	memcpy(space->context_buf, chip->work_space.context_buf, PAGE_SIZE);
+	memcpy(space->session_buf, chip->work_space.session_buf, PAGE_SIZE);
 
 	return 0;
 }

drivers/char/tpm/tpms-dev.c +7 -0

index d6e3491..12b6e34 100644
@@ -25,6 +25,12 @@ static int tpms_open(struct inode *inode, struct file *file)
 		kfree(priv);
 		return -ENOMEM;
 	}
+	priv->space.session_buf = kzalloc(PAGE_SIZE, GFP_KERNEL);
+	if (priv->space.session_buf == NULL) {
+		kfree(priv->space.context_buf);
+		kfree(priv);
+		return -ENOMEM;
+	}
 
 	tpm_common_open(file, chip, &priv->priv);
 

@@ -39,6 +45,7 @@ static int tpms_release(struct inode *inode, struct file *file)
 	tpm2_flush_space(fpriv->chip, &priv->space);
 	tpm_common_release(file, fpriv);
 	kfree(priv->space.context_buf);
+	kfree(priv->space.session_buf);
 	kfree(priv);
 
 	return 0;

Signature · 10 lines

2.6.6


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________
tpmdd-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel

Quoted 5 lines · James Bottomley

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

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

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