Re: [PATCH v10 08/17] tpm: call tpm2_flush_space() on error in tpm_try_transmit()

From: James Bottomley <James.Bottomley@HansenPartnership.com>
To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: linux-integrity@vger.kernel.org,
	linux-security-module@vger.kernel.org,
	Peter Huewe <PeterHuewe@gmx.de>, Jason Gunthorpe <jgg@ziepe.ca>,
	Tomas Winkler <tomas.winkler@intel.com>,
	Tadeusz Struk <tadeusz.struk@intel.com>,
	Stefan Berger <stefanb@linux.vnet.ibm.com>,
	Nayna Jain <nayna@linux.ibm.com>,
	stable@vger.kernel.org
Subject: Re: [PATCH v10 08/17] tpm: call tpm2_flush_space() on error in tpm_try_transmit()
Date: Tue, 29 Jan 2019 11:02:19 -0800	[thread overview]
Message-ID: <1548788539.2888.40.camel@HansenPartnership.com> (raw)
In-Reply-To: <20190129185330.GA11615@linux.intel.com>

On Tue, 2019-01-29 at 20:53 +0200, Jarkko Sakkinen wrote:
> On Tue, Jan 29, 2019 at 09:06:01AM -0800, James Bottomley wrote:
> > On Wed, 2019-01-16 at 23:23 +0200, Jarkko Sakkinen wrote:
> > [...]
> > > -	rc = tpm2_commit_space(chip, space, ordinal, buf, &len);
> > > +out_space:
> > > +	if (rc)
> > > +		tpm2_flush_space(chip);
> > > +	else
> > > +		rc = tpm2_commit_space(chip, space, ordinal,
> > > buf,
> > > &len);
> > 
> > I don't think this is quite right.  tpm2_flush_space only flushes
> > the handles it knows about and those are the ones from before the
> > TPM operation was attempted.  If the operation has altered the
> > internal state we could miss a created handle in this flush and it
> > would effectively reside forever in the TPM.  We should be able to
> > rely on the TPM preserving the original state if it returns an
> > error, so I think your patch works for that part.  However rc is
> > also set to -EFAULT on a transmission error and if that's on the
> > receive path, the TPM may have changed state before the error
> > occurred.
> 
> If TPM is working properly in the first place, tpm2_commit_space() is
> always called (e.g. in a situation where TPM gives a TPM error). Your
> deduction about the opposite is absolutely correct. Thanks!
> 
> > If the object is to move the TPM back to where it was before the
> > error occurred, even in the case of transmit errors, then I think
> > we need to invent a new kind of flush that queries the current TPM
> > state and then flushes everything.
> 
> I think this consideration is anyway out of scope for this patch set.

I certainly agree the problem existed before and this makes it no
worse.

> I'd hope you would also skim through v11 as soon as I get it
> prepared, at least the patches where I've added an explicit CC (one
> or two at most).

Sure, as you can see, I'm up to 8.  I'll complete the review and then
set up an environment to test.

James