Re: [PATCH] tpm: fix a race condition tpm2_unseal_trusted()

[Jason Gunthorpe <jgunthorpe@obsidianresearch.com>]

On Thu, Aug 25, 2016 at 05:06:10PM -0400, Jarkko Sakkinen wrote:
> On Thu, Aug 25, 2016 at 12:30:59PM -0600, Jason Gunthorpe wrote:
> > On Tue, Aug 23, 2016 at 08:57:22PM -0400, Jarkko Sakkinen wrote:
> > 
> > > +     if (flags & TPM_TRANSMIT_LOCK)
> > > +             mutex_lock(&chip->tpm_mutex);
> > 
> > I think I would invert this. UNLOCKED is the exceptional case, so I'd
> > make the 0 flags lock. If we see UNLOCKED in the caller then we know
> > to audit for locking, 0 is much less obvious.
> 
> I'm fine with either way.
> 
> > > @@ -576,7 +576,7 @@ static int tpm2_load(struct tpm_chip *chip,
> > >  		goto out;
> > >  	}
> > >  
> > > -	rc = tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob");
> > > +	rc = __tpm_transmit_cmd(chip, buf.data, PAGE_SIZE, "loading blob", 0);
> > 
> > All these points should accept a flags too and the caller should pass
> > in the TPM_TRASNMIT_UNLOCKED if it needs it..
> 
> For this bug fix it makes sense to implement it the way I did because it
> needs to be applied to multiple releases (I think I've underlined this
> in my changelog).

You shouldn't compromise the mainline kernel to ease backporting, I'm
not sure why adding a flags to tpm2_load would be a problem for the
-stable kernels?

It is generally better to make the backports move the older kernels
closer to mainline than to have them be something else, it makes it
easier to apply future backport fixes.

> If you think this is high priority, I can make the next revision into
> patch set of two patches. The second patch would implement the change
> you suggested.

Yes, I think it is important the locking requirement be very clear
from the code.

Jason