[PATCH] tpm_tis: Add a check for invalid status

[PATCH] tpm_tis: Add a check for invalid status

[James Bottomley]

Some TIS based TPMs can return 0xff to status reads if the locality
hasn't been properly requested.  Detect this condition by checking the
bits that the TIS spec specifies must return zero are clear and return
zero in that case.  Also drop a warning so the problem can be
identified in the calling path and fixed (usually a missing
try_get_ops()).

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---

This is the patch I've been using to catch and kill all the points in
the stack where we're not properly using get/put ops on the tpm chip.

 drivers/char/tpm/tpm_tis_core.c | 11 +++++++++++
 drivers/char/tpm/tpm_tis_core.h |  1 +
 2 files changed, 12 insertions(+)

diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
index 65ab1b027949..92c51c6cfd1b 100644
--- a/drivers/char/tpm/tpm_tis_core.c
+++ b/drivers/char/tpm/tpm_tis_core.c
@@ -239,6 +239,17 @@ static u8 tpm_tis_status(struct tpm_chip *chip)
 	if (rc < 0)
 		return 0;
 
+	if (unlikely((status & TPM_STS_READ_ZERO) != 0)) {
+		/*
+		 * If this trips, the chances are the read is
+		 * returning 0xff because the locality hasn't been
+		 * acquired.  Usually because tpm_try_get_ops() hasn't
+		 * been called before doing a TPM operation.
+		 */
+		WARN_ONCE(1, "TPM returned invalid status\n");
+		return 0;
+	}
+
 	return status;
 }
 
diff --git a/drivers/char/tpm/tpm_tis_core.h b/drivers/char/tpm/tpm_tis_core.h
index 7337819f5d7b..9b2d32a59f67 100644
--- a/drivers/char/tpm/tpm_tis_core.h
+++ b/drivers/char/tpm/tpm_tis_core.h
@@ -34,6 +34,7 @@ enum tis_status {
 	TPM_STS_GO = 0x20,
 	TPM_STS_DATA_AVAIL = 0x10,
 	TPM_STS_DATA_EXPECT = 0x08,
+	TPM_STS_READ_ZERO = 0x23, /* bits that must be zero on read */
 };
 
 enum tis_int_flags {
-- 
2.26.2

Re: [PATCH] tpm_tis: Add a check for invalid status

[Jason Gunthorpe]

On Mon, Sep 28, 2020 at 11:00:12AM -0700, James Bottomley wrote:
> Some TIS based TPMs can return 0xff to status reads if the locality
> hasn't been properly requested.  Detect this condition by checking the
> bits that the TIS spec specifies must return zero are clear and return
> zero in that case.  Also drop a warning so the problem can be
> identified in the calling path and fixed (usually a missing
> try_get_ops()).
> 
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> 
> This is the patch I've been using to catch and kill all the points in
> the stack where we're not properly using get/put ops on the tpm chip.

If this is a problem add a lockdep on ops_sem in various places too?

Jason

Re: [PATCH] tpm_tis: Add a check for invalid status

[James Bottomley]

On Mon, 2020-09-28 at 15:33 -0300, Jason Gunthorpe wrote:
> On Mon, Sep 28, 2020 at 11:00:12AM -0700, James Bottomley wrote:
> > Some TIS based TPMs can return 0xff to status reads if the locality
> > hasn't been properly requested.  Detect this condition by checking
> > the bits that the TIS spec specifies must return zero are clear and
> > return zero in that case.  Also drop a warning so the problem can
> > be identified in the calling path and fixed (usually a missing
> > try_get_ops()).
> > 
> > Signed-off-by: James Bottomley <
> > James.Bottomley@HansenPartnership.com>
> > 
> > This is the patch I've been using to catch and kill all the points
> > in the stack where we're not properly using get/put ops on the tpm
> > chip.
> 
> If this is a problem add a lockdep on ops_sem in various places too?

It's not really possible because of the init issue with checking the
interrupt.  That originally had no locking at all (it doesn't need any
because the TPM device isn't publicly exposed at the point the check is
done).  If the patch to add get/put around the tpm2_get_tpm_pt is
acceptable, then perhaps we could because I think that's the last
unguarded use of tpm_tis_status.

James

Re: [PATCH] tpm_tis: Add a check for invalid status

[Jarkko Sakkinen]

On Mon, Sep 28, 2020 at 02:15:33PM -0700, James Bottomley wrote:
> On Mon, 2020-09-28 at 15:33 -0300, Jason Gunthorpe wrote:
> > On Mon, Sep 28, 2020 at 11:00:12AM -0700, James Bottomley wrote:
> > > Some TIS based TPMs can return 0xff to status reads if the locality
> > > hasn't been properly requested.  Detect this condition by checking
> > > the bits that the TIS spec specifies must return zero are clear and
> > > return zero in that case.  Also drop a warning so the problem can
> > > be identified in the calling path and fixed (usually a missing
> > > try_get_ops()).
> > > 
> > > Signed-off-by: James Bottomley <
> > > James.Bottomley@HansenPartnership.com>
> > > 
> > > This is the patch I've been using to catch and kill all the points
> > > in the stack where we're not properly using get/put ops on the tpm
> > > chip.
> > 
> > If this is a problem add a lockdep on ops_sem in various places too?
> 
> It's not really possible because of the init issue with checking the
> interrupt.  That originally had no locking at all (it doesn't need any
> because the TPM device isn't publicly exposed at the point the check is
> done).  If the patch to add get/put around the tpm2_get_tpm_pt is
> acceptable, then perhaps we could because I think that's the last
> unguarded use of tpm_tis_status.
> 
> James

I think this sanity check would not hurt at all.

We should also improve the testing coverage. E.g. there is zero coverage
for trusted keys. It can easily lead weird conclusions if there is no
common test to run.

I touched that over here:

https://lore.kernel.org/linux-integrity/20200929225841.GA805025@linux.intel.com/

Anyway, I will apply this for sure.

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

Re: [PATCH] tpm_tis: Add a check for invalid status

[Jarkko Sakkinen]

On Wed, Sep 30, 2020 at 04:37:58AM +0300, Jarkko Sakkinen wrote:
> On Mon, Sep 28, 2020 at 02:15:33PM -0700, James Bottomley wrote:
> > On Mon, 2020-09-28 at 15:33 -0300, Jason Gunthorpe wrote:
> > > On Mon, Sep 28, 2020 at 11:00:12AM -0700, James Bottomley wrote:
> > > > Some TIS based TPMs can return 0xff to status reads if the locality
> > > > hasn't been properly requested.  Detect this condition by checking
> > > > the bits that the TIS spec specifies must return zero are clear and
> > > > return zero in that case.  Also drop a warning so the problem can
> > > > be identified in the calling path and fixed (usually a missing
> > > > try_get_ops()).
> > > > 
> > > > Signed-off-by: James Bottomley <
> > > > James.Bottomley@HansenPartnership.com>
> > > > 
> > > > This is the patch I've been using to catch and kill all the points
> > > > in the stack where we're not properly using get/put ops on the tpm
> > > > chip.
> > > 
> > > If this is a problem add a lockdep on ops_sem in various places too?
> > 
> > It's not really possible because of the init issue with checking the
> > interrupt.  That originally had no locking at all (it doesn't need any
> > because the TPM device isn't publicly exposed at the point the check is
> > done).  If the patch to add get/put around the tpm2_get_tpm_pt is
> > acceptable, then perhaps we could because I think that's the last
> > unguarded use of tpm_tis_status.
> > 
> > James
> 
> I think this sanity check would not hurt at all.
> 
> We should also improve the testing coverage. E.g. there is zero coverage
> for trusted keys. It can easily lead weird conclusions if there is no
> common test to run.
> 
> I touched that over here:
> 
> https://lore.kernel.org/linux-integrity/20200929225841.GA805025@linux.intel.com/
> 
> Anyway, I will apply this for sure.
> 
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

Has been applied.

/Jarkko