| Message ID | 20190116212342.24524-9-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Remove nested TPM operations | expand |
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
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
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
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
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;
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(-)