[PATCH v2] tpm: ignore failed selftest in probe

[PATCH v2] 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. 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.

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>

---
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
  *
  * 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 v2] tpm: ignore failed selftest in probe

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

> 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

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

[Dafna Hirschfeld]

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

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

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

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

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

[Dafna Hirschfeld]

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

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.

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

  
> 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

> 	
> /Jarkko
> 

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

[James Bottomley]

On Mon, 2020-12-14 at 10:50 +0100, Dafna Hirschfeld wrote:
> Am 11.12.20 um 18:57 schrieb Jarkko Sakkinen:
[...]
> > 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.

This comes up a lot, so let me give my view that this really has to be
our requirement going forward.  TPM 1.2 is pretty much a dead device
and, thanks to being sha1 only, it's definitely not used to provide
security in any serious devices anymore.  So effectively any patch
which is TPM 1.2 only is either because there's a problem in a legacy
system which was fixed in 2.0 or because for some reason the patch
producer is working on a legacy system, but it also needs to be
provided for TPM 2.0.

On this patch, I've got a TPM 2.0 which I can trigger into failure mode
on demand (just not get it back again without a power cycle).  Since it
does correctly respond TPM_RC_FAILURE to every command, it could be
attached with no command restriction.  The problem is that TPM 2 is an
agile device so we need to probe some of its characteristics on attach,
which we can't if it's in a failed state.  So if we're going to attach
a failed TPM 2, we're going to have to make assumptions about the
probed values.  What do we use as the assumptions for things like
number of PCRS (24 woud be reasonable) and hashes and banks (sha1 and
sha256), attribute table?  The problem here is we only probe this at
attach, so if the TPM somehow unfails, all of the assumptions are
likely to be wrong and we're going to get issues ... this last bit
argues that we should restrict the command list to make sure we don't
send it a command with incorrect assumptions.

James

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

[Jarkko Sakkinen]

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