On Mon, Dec 14, 2020 at 10:50:25AM +0100, Dafna Hirschfeld wrote: > > > 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. Yeah, it would make sense have consistent behaviour. > > 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? This changes the ABI behaviour from previous kernel versions. > > 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 You don't make it explicit how the "broken" TPM is used by the user space. This makes it imposibble to decipher whether this the right way to change the kernel or not. /Jarkko
