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

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

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