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? > 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; } > + > rc = tpm1_do_selftest(chip); > if (rc) { > - dev_err(&chip->dev, "TPM self test failed\n"); > - goto out; > + dev_err(&chip->dev, "TPM self test failed %d\n", rc); > + if (rc < 0) > + return rc; > } > - > - return rc; > -out: > - if (rc > 0) > - rc = -ENODEV; > - return rc; > + return 0; You don't need to remove the goto-statement. You could just replace the existing condition with what I described above. This is patch is doing changes that are not mandatory for the change. > } > > #define TPM_ORD_SAVESTATE 152 > -- > 2.17.1 /Jarkko
