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

[Jarkko Sakkinen <jarkko@kernel.org>]

On Fri, Dec 04, 2020 at 02:56:49PM +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.
> ConstinueSelfTest command results in TPM_FAILEDSELFTEST

I guess ContinueSelfTest? What command is that anywyay
in which context? TPM spec does not have that at least.

Please use the exact same phrasings as they are in the
TPM spec.

> 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 self test, and create the
> device - the chip is out there, it's just in the
> failed state.
> 
> Testing:
> Tested with swtpm as tpm simulator and a patch in libtpms
> to enter failure mode
> 
> With this settings, the 'dev/tpm0' is created but

'dev/tpm0' -> /dev/tpm0

> 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
> Chromeos Elm device.

Please format these paragraphs properly using 75 character
line length. And capitalize acronyms correctly, e.g. "TPM",
not "tpm".

Please explain what "Elm" is as I have zero idea of it.

> 
> Signed-off-by: Andrey Pronin <apronin@chromium.org>
> [change the code to still fail in case of fatal error]
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> 
> ---

Please put all below just before diffstat.

> 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
>   *
>   * 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;
> +
>  	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;
>  }
>  
>  #define TPM_ORD_SAVESTATE 152
> -- 
> 2.17.1
> 
> 

Let's look at the code change once the commit message looks sane...

/Jarkko