All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] tpm_tis: Add a check for invalid status
@ 2020-09-28 18:00 James Bottomley
  2020-09-28 18:33 ` Jason Gunthorpe
  0 siblings, 1 reply; 5+ messages in thread
From: James Bottomley @ 2020-09-28 18:00 UTC (permalink / raw)
  To: linux-integrity; +Cc: Jarkko Sakkinen, Peter Huewe, Jason Gunthorpe

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



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] tpm_tis: Add a check for invalid status
  2020-09-28 18:00 [PATCH] tpm_tis: Add a check for invalid status James Bottomley
@ 2020-09-28 18:33 ` Jason Gunthorpe
  2020-09-28 21:15   ` James Bottomley
  0 siblings, 1 reply; 5+ messages in thread
From: Jason Gunthorpe @ 2020-09-28 18:33 UTC (permalink / raw)
  To: James Bottomley; +Cc: linux-integrity, Jarkko Sakkinen, Peter Huewe

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] tpm_tis: Add a check for invalid status
  2020-09-28 18:33 ` Jason Gunthorpe
@ 2020-09-28 21:15   ` James Bottomley
  2020-09-30  1:37     ` Jarkko Sakkinen
  0 siblings, 1 reply; 5+ messages in thread
From: James Bottomley @ 2020-09-28 21:15 UTC (permalink / raw)
  To: Jason Gunthorpe; +Cc: linux-integrity, Jarkko Sakkinen, Peter Huewe

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



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] tpm_tis: Add a check for invalid status
  2020-09-28 21:15   ` James Bottomley
@ 2020-09-30  1:37     ` Jarkko Sakkinen
  2020-09-30  1:40       ` Jarkko Sakkinen
  0 siblings, 1 reply; 5+ messages in thread
From: Jarkko Sakkinen @ 2020-09-30  1:37 UTC (permalink / raw)
  To: James Bottomley; +Cc: Jason Gunthorpe, linux-integrity, Peter Huewe

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] tpm_tis: Add a check for invalid status
  2020-09-30  1:37     ` Jarkko Sakkinen
@ 2020-09-30  1:40       ` Jarkko Sakkinen
  0 siblings, 0 replies; 5+ messages in thread
From: Jarkko Sakkinen @ 2020-09-30  1:40 UTC (permalink / raw)
  To: James Bottomley; +Cc: Jason Gunthorpe, linux-integrity, Peter Huewe

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-09-30  1:40 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-28 18:00 [PATCH] tpm_tis: Add a check for invalid status James Bottomley
2020-09-28 18:33 ` Jason Gunthorpe
2020-09-28 21:15   ` James Bottomley
2020-09-30  1:37     ` Jarkko Sakkinen
2020-09-30  1:40       ` Jarkko Sakkinen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.