Re: [PATCH v2 1/5] tpm_tis: Fix check_locality for correct locality acquisition

From: Jarkko Sakkinen <jarkko@kernel.org>
To: Jerry Snitselaar <jsnitsel@redhat.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>,
	linux-integrity@vger.kernel.org, Jason Gunthorpe <jgg@ziepe.ca>,
	Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
	Peter Huewe <peterhuewe@gmx.de>
Subject: Re: [PATCH v2 1/5] tpm_tis: Fix check_locality for correct locality acquisition
Date: Sat, 24 Oct 2020 15:07:44 +0300	[thread overview]
Message-ID: <20201024120744.GA32607@kernel.org> (raw)
In-Reply-To: <878sc1hkmn.fsf@redhat.com>

On Mon, Oct 19, 2020 at 04:16:32PM -0700, Jerry Snitselaar wrote:
> 
> James Bottomley @ 2020-10-01 11:09 MST:
> 
> > The TPM TIS specification says the TPM signals the acquisition of
> > locality when the TMP_ACCESS_REQUEST_USE bit goes to one *and* the
> > TPM_ACCESS_REQUEST_USE bit goes to zero.  Currently we only check the
> > former not the latter, so check both.  Adding the check on
> > TPM_ACCESS_REQUEST_USE should fix the case where the locality is
> > re-requested before the TPM has released it.  In this case the
> > locality may get released briefly before it is reacquired, which
> > causes all sorts of problems. However, with the added check,
> > TPM_ACCESS_REQUEST_USE should remain 1 until the second request for
> > the locality is granted.
> >
> > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> >
> > ---
> >
> > v2: added this patch
> > ---
> >  drivers/char/tpm/tpm_tis_core.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c
> > index 92c51c6cfd1b..f3ecde8df47d 100644
> > --- a/drivers/char/tpm/tpm_tis_core.c
> > +++ b/drivers/char/tpm/tpm_tis_core.c
> > @@ -125,7 +125,8 @@ static bool check_locality(struct tpm_chip *chip, int l)
> >  	if (rc < 0)
> >  		return false;
> >  
> > -	if ((access & (TPM_ACCESS_ACTIVE_LOCALITY | TPM_ACCESS_VALID)) ==
> > +	if ((access & (TPM_ACCESS_ACTIVE_LOCALITY | TPM_ACCESS_VALID
> > +		       | TPM_ACCESS_REQUEST_USE)) ==
> >  	    (TPM_ACCESS_ACTIVE_LOCALITY | TPM_ACCESS_VALID)) {
> >  		priv->locality = l;
> >  		return true;
> 
> Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>

Thank you.

James, few minor remarks.

Given that the failing commit is in the GIT, just for reference

  Fixes: 27084efee0c3 ("[PATCH] tpm: driver for next generation TPM chips")
  Cc: stable@ger.kernel.org

I want fixes tags for everything that has a legit Git commit, even
if it spans through all the existing stable kernels. It's valuable
information to have in the Git log.

Another thing I noticed is that would be less ugly put everything
in the same line, as checkpatch requirement have been relaxed.

Finally, please put a verbose inline comment before the condition.
tpm_tis driver is complicated enough that it should be better
documented. After months pass things tend to fade away and wrong
decisions might be made because of that. You can probably derive
it from the already nice and verbose commit message, so let's
take advantage of it.

About recent debate on patch changelogs. I talked with Dave Hansen
about this, and he said that in x86 tree, they are the standard, but
the practice depends per tree.

After thinking about this, and writing per patch changelogs for the
next iteration of the SGX patch set, my standing point is that either
works as it is properly written and maintained.

/Jarkko