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:

quoted

On Wed, 2019-01-16 at 23:23 +0200, Jarkko Sakkinen wrote:
[...]

quoted

-	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!

quoted

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