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'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). Thanks again. /Jarkko
