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

[Jarkko Sakkinen <jarkko@kernel.org>]

On Sat, Oct 24, 2020 at 03:07:44PM +0300, Jarkko Sakkinen wrote:
> 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.

This can be merged immediately once the remarks have been taken care
of (i.e. do not except tested-by for 1/5).

/Jarkko