Re: [PATCH v2] tpm: ignore failed selftest in probe

[Jarkko Sakkinen <jarkko@kernel.org>]

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@chromium.org>
> > > 
> > > 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@chromium.org>
> > > [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@collabora.com>
> > > 
> > > ---
> > > 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