Am 11.12.20 um 18:57 schrieb Jarkko Sakkinen:
On Fri, Dec 11, 2020 at 05:56:24PM +0100, Dafna Hirschfeld wrote:
Hi,
Am 08.12.20 um 18:34 schrieb Jarkko Sakkinen:
On Mon, Dec 07, 2020 at 02:57:10PM +0100, Dafna Hirschfeld wrote:
From: Andrey Pronin <apronin@xxxxxxxxxxxx>
If a TPM firmware update is interrupted, e.g due to loss of power or a
reset while installing the update, you end with the TPM chip in failure
mode. TPM_ContinueSelfTest command is called when the device is probed.
It results in TPM_FAILEDSELFTEST error, and probe fails. The TPM device
is not created, and that prevents the OS from attempting any further
recover operations with the TPM. Instead, ignore the error code of the
TPM_ContinueSelfTest command, and create the device - the chip is out
there, it's just in failure mode.
Testing:
Tested with the swtpm as TPM simulator and a patch in 'libtpms'
to enter failure mode
With this settings, the '/dev/tpm0' is created but the tcsd daemon fails
to run. In addition, the commands TPM_GetTestResult, TPM_GetCapability
and TPM_GetRandom were tested.
A normal operation was tested with an Acer Chromebook R13 device
(also called Elm) running Debian.
Move testing part to the stuff before diffstat.
Signed-off-by: Andrey Pronin <apronin@xxxxxxxxxxxx>
[change the code to still fail in case of fatal error]
What is this?
In the original patch, any return value from 'tpm1_do_selftest'
is ignored. I change the original patch so that in case of system
error (rc < 0) the error is not ignored since this error did not
come from the TPM but from the system.
Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@xxxxxxxxxxxxx>
---
changes since v1:
- rewriting the commit message
This commit comes from chromeos:
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/1065c2fe54d6%5E%21/
In Chromeos, the selftest fails if the TPM firmware is updated during EC
reset. In that case the userspace wants to access the TPM for recovery.
This patch is for TPM 1.2 only, I can also send a patch for TPM 2 if it
is required that the behaviour stays consistent among the versions.
libtpms patch:
https://gitlab.collabora.com/dafna/libtpms/-/commit/42848f4a838636d01ddb5ed353b3990dad3f601d
TPM tests:
https://gitlab.collabora.com/dafna/test-tpm1.git
drivers/char/tpm/tpm1-cmd.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/drivers/char/tpm/tpm1-cmd.c b/drivers/char/tpm/tpm1-cmd.c
index ca7158fa6e6c..8b7997ef8d1c 100644
--- a/drivers/char/tpm/tpm1-cmd.c
+++ b/drivers/char/tpm/tpm1-cmd.c
@@ -697,6 +697,8 @@ EXPORT_SYMBOL_GPL(tpm1_do_selftest);
/**
* tpm1_auto_startup - Perform the standard automatic TPM initialization
* sequence
+ * NOTE: if tpm1_do_selftest returns with a TPM error code, we return 0 (success)
+ * to allow userspace interaction with the TPM when it is on failure mode.
* @chip: TPM chip to use
Please do not use "we ...". Use imperative form.
Also that is wrong place for the description:
https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt
*
* Returns 0 on success, < 0 in case of fatal error.
@@ -707,18 +709,15 @@ int tpm1_auto_startup(struct tpm_chip *chip)
rc = tpm1_get_timeouts(chip);
if (rc)
- goto out;
+ return rc < 0 ? rc : -ENODEV;
Do not use ternary operators. Also we are interested on
TPM_SELFTESTFAILED only (according to the commit message).
I.e. afaik should be
if (rc) {
if (rc == TPM_SELFTESTFAILED)
return -ENODEV;
else
return rc;
}
I read the description of TPM_ContinueSelfTest
in the spec file
https://trustedcomputinggroup.org/wp-content/uploads/TPM-Main-Part-3-Commands_v1.2_rev116_01032011.pdf
It is stated there that when running a command C1 before running TPM_ContinueSelfTest
then the command might return error codes TPM_NEEDS_SELFTEST/TPM_DOING_SELFTEST.
In those cases the command tpm1_get_timeouts should be called again after calling
'tpm1_continue_selftest'.
So it seems that we can just move the the call to 'tpm1_get_timeouts' to
after the call to 'tpm1_continue_selftest'.
I guess that the ChromeOS's TPM can support TPM_GetCapability for TPM_CAP_PROP_TIS_TIMEOUT, also
when it is on failure mode and this is why their patch ignores only the
result of 'tpm1_do_selftest' and not the result of 'tpm1_get_timeouts'.
The idea of the patch is opposite than what you suggest.
If 'tpm1_get_timeouts' returns 'TPM_SELFTESTFAILED' then the code should not return '-ENODEV'
since we do want to have '/dev/tpm*' in that case.
Thanks,
Dafna
My mistake.
You only need to add two lines of code:
out:
if (rc == TPM_SELFTESTFAILED)
rc = 0;
if (rc > 0)
rc = -ENODEV;
return rc;
}
But how does this patch deal with TPM2?
It doesn't, I was not sure if there is need to keep consistent behavior
between 1.2 and 2. I can send next version with the same behavior for TPM 2.
This should be opt-in feature or restricted to a narrow subset of TPM
commands. Please rephrase this for next iteration:
"The TPM device is not created, and that prevents the OS from attempting
any further recover operations with the TPM."
In failure mode, the TPM should fail for almost all commands except for
TPM_GetTestResult, and some params of TPM_GetCapability. So I don't see a
reason to restrict the commands in the kernel.
Can you be more clear, what should I rephrase in the above sentence?
should I describe in detail the recovery steps?
What you've sent works only as a PoC.
Can you give more details of what should be added to the patch so
it is not just a PoC ?
Thanks,
Dafna
/Jarkko
