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? 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." What you've sent works only as a PoC. /Jarkko
