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
