[PATCH] tpm: ignore failed selftest in probe

[PATCH] tpm: ignore failed selftest in probe

[Dafna Hirschfeld]

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

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>

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

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

[Jarkko Sakkinen]

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