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

[Jarkko Sakkinen <jarkko.sakkinen@iki.fi>]

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?

> 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;
	}


> +
>  	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