[PATCH v10 08/17] tpm: call tpm2_flush_space() on error in tpm_try_transmit()

[PATCH v10 08/17] tpm: call tpm2_flush_space() on error in tpm_try_transmit()

[Jarkko Sakkinen]

Always call tpm2_flush_space() on failure in tpm_try_transmit() so that
the volatile memory of the TPM gets cleared. If /dev/tpm0 does not have
sufficient permissions (usually it has), this could lead to the leakage
of TPM objects. Through /dev/tpmrm0 this issue does not raise any new
security concerns.

Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Cc: stable@vger.kernel.org
Fixes: 745b361e989a ("tpm:tpm: infrastructure for TPM spaces")
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Tested-by: Stefan Berger <stefanb@linux.ibm.com>
---
 drivers/char/tpm/tpm-interface.c | 29 ++++++++++++-----------------
 drivers/char/tpm/tpm.h           |  1 +
 drivers/char/tpm/tpm2-space.c    |  2 +-
 3 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 689d07e67643..2c409d19a9b9 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -220,14 +220,14 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, struct tpm_space *space,
 
 	rc = tpm2_prepare_space(chip, space, ordinal, buf);
 	if (rc)
-		goto out;
+		goto out_idle;
 
 	rc = chip->ops->send(chip, buf, count);
 	if (rc < 0) {
 		if (rc != -EPIPE)
 			dev_err(&chip->dev,
 				"%s: tpm_send: error %d\n", __func__, rc);
-		goto out;
+		goto out_space;
 	}
 
 	if (chip->flags & TPM_CHIP_FLAG_IRQ)
@@ -243,7 +243,7 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, struct tpm_space *space,
 		if (chip->ops->req_canceled(chip, status)) {
 			dev_err(&chip->dev, "Operation Canceled\n");
 			rc = -ECANCELED;
-			goto out;
+			goto out_space;
 		}
 
 		tpm_msleep(TPM_TIMEOUT_POLL);
@@ -253,28 +253,23 @@ static ssize_t tpm_try_transmit(struct tpm_chip *chip, struct tpm_space *space,
 	chip->ops->cancel(chip);
 	dev_err(&chip->dev, "Operation Timed out\n");
 	rc = -ETIME;
-	goto out;
+	goto out_space;
 
 out_recv:
 	len = chip->ops->recv(chip, buf, bufsiz);
 	if (len < 0) {
 		rc = len;
-		dev_err(&chip->dev,
-			"tpm_transmit: tpm_recv: error %d\n", rc);
-		goto out;
-	} else if (len < TPM_HEADER_SIZE) {
+		dev_err(&chip->dev, "tpm_transmit: tpm_recv: error %d\n", rc);
+	} else if (len < TPM_HEADER_SIZE || len != be32_to_cpu(header->length))
 		rc = -EFAULT;
-		goto out;
-	}
 
-	if (len != be32_to_cpu(header->length)) {
-		rc = -EFAULT;
-		goto out;
-	}
-
-	rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
+out_space:
+	if (rc)
+		tpm2_flush_space(chip);
+	else
+		rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
 
-out:
+out_idle:
 	/* may fail but do not override previous error value in rc */
 	tpm_go_idle(chip, flags);
 
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index 1454ef19d2f4..6eb67ccad2a3 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -576,6 +576,7 @@ int tpm2_probe(struct tpm_chip *chip);
 int tpm2_find_cc(struct tpm_chip *chip, u32 cc);
 int tpm2_init_space(struct tpm_space *space);
 void tpm2_del_space(struct tpm_chip *chip, struct tpm_space *space);
+void tpm2_flush_space(struct tpm_chip *chip);
 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,
diff --git a/drivers/char/tpm/tpm2-space.c b/drivers/char/tpm/tpm2-space.c
index 39cb3915771e..5d6487575074 100644
--- a/drivers/char/tpm/tpm2-space.c
+++ b/drivers/char/tpm/tpm2-space.c
@@ -162,7 +162,7 @@ static int tpm2_save_context(struct tpm_chip *chip, u32 handle, u8 *buf,
 	return 0;
 }
 
-static void tpm2_flush_space(struct tpm_chip *chip)
+void tpm2_flush_space(struct tpm_chip *chip)
 {
 	struct tpm_space *space = &chip->work_space;
 	int i;
-- 
2.19.1

Re: [PATCH v10 08/17] tpm: call tpm2_flush_space() on error in tpm_try_transmit()

[James Bottomley]

On Wed, 2019-01-16 at 23:23 +0200, Jarkko Sakkinen wrote:
[...]
> -	rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
> +out_space:
> +	if (rc)
> +		tpm2_flush_space(chip);
> +	else
> +		rc = tpm2_commit_space(chip, space, ordinal, buf,
> &len);

I don't think this is quite right.  tpm2_flush_space only flushes the
handles it knows about and those are the ones from before the TPM
operation was attempted.  If the operation has altered the internal
state we could miss a created handle in this flush and it would
effectively reside forever in the TPM.  We should be able to rely on
the TPM preserving the original state if it returns an error, so I
think your patch works for that part.  However rc is also set to
-EFAULT on a transmission error and if that's on the receive path, the
TPM may have changed state before the error occurred.

If the object is to move the TPM back to where it was before the error
occurred, even in the case of transmit errors, then I think we need to
invent a new kind of flush that queries the current TPM state and then
flushes everything.

James

Re: [PATCH v10 08/17] tpm: call tpm2_flush_space() on error in tpm_try_transmit()

[Jarkko Sakkinen]

On Tue, Jan 29, 2019 at 09:06:01AM -0800, James Bottomley wrote:
> On Wed, 2019-01-16 at 23:23 +0200, Jarkko Sakkinen wrote:
> [...]
> > -	rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
> > +out_space:
> > +	if (rc)
> > +		tpm2_flush_space(chip);
> > +	else
> > +		rc = tpm2_commit_space(chip, space, ordinal, buf,
> > &len);
> 
> I don't think this is quite right.  tpm2_flush_space only flushes the
> handles it knows about and those are the ones from before the TPM
> operation was attempted.  If the operation has altered the internal
> state we could miss a created handle in this flush and it would
> effectively reside forever in the TPM.  We should be able to rely on
> the TPM preserving the original state if it returns an error, so I
> think your patch works for that part.  However rc is also set to
> -EFAULT on a transmission error and if that's on the receive path, the
> TPM may have changed state before the error occurred.

If TPM is working properly in the first place, tpm2_commit_space() is
always called (e.g. in a situation where TPM gives a TPM error). Your
deduction about the opposite is absolutely correct. Thanks!

> If the object is to move the TPM back to where it was before the error
> occurred, even in the case of transmit errors, then I think we need to
> invent a new kind of flush that queries the current TPM state and then
> flushes everything.

I think this consideration is anyway out of scope for this patch set.
I'd hope you would also skim through v11 as soon as I get it prepared,
at least the patches where I've added an explicit CC (one or two at
most).

Thanks again.

/Jarkko

Re: [PATCH v10 08/17] tpm: call tpm2_flush_space() on error in tpm_try_transmit()

[James Bottomley]

On Tue, 2019-01-29 at 20:53 +0200, Jarkko Sakkinen wrote:
> On Tue, Jan 29, 2019 at 09:06:01AM -0800, James Bottomley wrote:
> > On Wed, 2019-01-16 at 23:23 +0200, Jarkko Sakkinen wrote:
> > [...]
> > > -	rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
> > > +out_space:
> > > +	if (rc)
> > > +		tpm2_flush_space(chip);
> > > +	else
> > > +		rc = tpm2_commit_space(chip, space, ordinal,
> > > buf,
> > > &len);
> > 
> > I don't think this is quite right.  tpm2_flush_space only flushes
> > the handles it knows about and those are the ones from before the
> > TPM operation was attempted.  If the operation has altered the
> > internal state we could miss a created handle in this flush and it
> > would effectively reside forever in the TPM.  We should be able to
> > rely on the TPM preserving the original state if it returns an
> > error, so I think your patch works for that part.  However rc is
> > also set to -EFAULT on a transmission error and if that's on the
> > receive path, the TPM may have changed state before the error
> > occurred.
> 
> If TPM is working properly in the first place, tpm2_commit_space() is
> always called (e.g. in a situation where TPM gives a TPM error). Your
> deduction about the opposite is absolutely correct. Thanks!
> 
> > If the object is to move the TPM back to where it was before the
> > error occurred, even in the case of transmit errors, then I think
> > we need to invent a new kind of flush that queries the current TPM
> > state and then flushes everything.
> 
> I think this consideration is anyway out of scope for this patch set.

I certainly agree the problem existed before and this makes it no
worse.

> I'd hope you would also skim through v11 as soon as I get it
> prepared, at least the patches where I've added an explicit CC (one
> or two at most).

Sure, as you can see, I'm up to 8.  I'll complete the review and then
set up an environment to test.

James

Re: [PATCH v10 08/17] tpm: call tpm2_flush_space() on error in tpm_try_transmit()

[Jarkko Sakkinen]

On Tue, Jan 29, 2019 at 11:02:19AM -0800, James Bottomley wrote:
> On Tue, 2019-01-29 at 20:53 +0200, Jarkko Sakkinen wrote:
> > On Tue, Jan 29, 2019 at 09:06:01AM -0800, James Bottomley wrote:
> > > On Wed, 2019-01-16 at 23:23 +0200, Jarkko Sakkinen wrote:
> > > [...]
> > > > -	rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
> > > > +out_space:
> > > > +	if (rc)
> > > > +		tpm2_flush_space(chip);
> > > > +	else
> > > > +		rc = tpm2_commit_space(chip, space, ordinal,
> > > > buf,
> > > > &len);
> > > 
> > > I don't think this is quite right.  tpm2_flush_space only flushes
> > > the handles it knows about and those are the ones from before the
> > > TPM operation was attempted.  If the operation has altered the
> > > internal state we could miss a created handle in this flush and it
> > > would effectively reside forever in the TPM.  We should be able to
> > > rely on the TPM preserving the original state if it returns an
> > > error, so I think your patch works for that part.  However rc is
> > > also set to -EFAULT on a transmission error and if that's on the
> > > receive path, the TPM may have changed state before the error
> > > occurred.
> > 
> > If TPM is working properly in the first place, tpm2_commit_space() is
> > always called (e.g. in a situation where TPM gives a TPM error). Your
> > deduction about the opposite is absolutely correct. Thanks!
> > 
> > > If the object is to move the TPM back to where it was before the
> > > error occurred, even in the case of transmit errors, then I think
> > > we need to invent a new kind of flush that queries the current TPM
> > > state and then flushes everything.
> > 
> > I think this consideration is anyway out of scope for this patch set.
> 
> I certainly agree the problem existed before and this makes it no
> worse.
> 
> > I'd hope you would also skim through v11 as soon as I get it
> > prepared, at least the patches where I've added an explicit CC (one
> > or two at most).
> 
> Sure, as you can see, I'm up to 8.  I'll complete the review and then
> set up an environment to test.

Great, thank you! I won't try to land this to v5.1 so there is no any
kind of rush, because there is a show stopper that I need sort out with
v5.0, as you've seen...

/Jarkko