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

[Jarkko Sakkinen]

On Tue, Jan 29, 2019 at 11:02:19AM -0800, James Bottomley wrote:
> 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.

Great, thank you! I won't try to land this to v5.1 so there is no any
kind of rush, because there is a show stopper that I need sort out with
v5.0, as you've seen...

/Jarkko