linux-kernel - Re: [PATCH] tpm: fix a race condition tpm2_unseal

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 25 Aug 2016 15:09:42 -0600
From:   Jason Gunthorpe <jgunthorpe@...idianresearch.com>
To:     Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
Cc:     Peter Huewe <peterhuewe@....de>,
        linux-security-module@...r.kernel.org, stable@...r.kernel.org,
        Marcel Selhorst <tpmdd@...horst.net>,
        "moderated list:TPM DEVICE DRIVER" 
        <tpmdd-devel@...ts.sourceforge.net>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] tpm: fix a race condition tpm2_unseal_trusted()

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