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
