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

[Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>]

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