[PATCH] tpm: Start the tpm2 before running a self test.

[PATCH] tpm: Start the tpm2 before running a self test.

[Hermin Anggawijaya]

Before sending a command to attempt the self test, the TPM
may need to be started, otherwise the self test returns
TPM2_RC_INITIALIZE value causing a log as follows:
"tpm tpm0: A TPM error (256) occurred attempting the self test".

Signed-off-by: Hermin Anggawijaya <hermin.anggawijaya@alliedtelesis.co.nz>
---
 drivers/char/tpm/tpm2-cmd.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 93545be190a5..0530f3b5f86a 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -737,15 +737,15 @@ int tpm2_auto_startup(struct tpm_chip *chip)
 	if (rc)
 		goto out;
 
+	rc = tpm2_startup(chip);
+	if (rc && rc != TPM2_RC_INITIALIZE)
+		goto out;
+
 	rc = tpm2_do_selftest(chip);
 	if (rc && rc != TPM2_RC_INITIALIZE)
 		goto out;
 
 	if (rc == TPM2_RC_INITIALIZE) {
-		rc = tpm2_startup(chip);
-		if (rc)
-			goto out;
-
 		rc = tpm2_do_selftest(chip);
 		if (rc)
 			goto out;
-- 
2.43.0

Re: [PATCH] tpm: Start the tpm2 before running a self test.

[Paul Menzel]

Dear Hermin,


Thank you for your patch.

It’d be great if you removed the dot/period from the end of the commit 
message summary/title.

Am 22.11.23 um 07:55 schrieb Hermin Anggawijaya:
> Before sending a command to attempt the self test, the TPM
> may need to be started, otherwise the self test returns
> TPM2_RC_INITIALIZE value causing a log as follows:
> "tpm tpm0: A TPM error (256) occurred attempting the self test".

Please document on what platform this happens.

> Signed-off-by: Hermin Anggawijaya <hermin.anggawijaya@alliedtelesis.co.nz>
> ---
>   drivers/char/tpm/tpm2-cmd.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index 93545be190a5..0530f3b5f86a 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -737,15 +737,15 @@ int tpm2_auto_startup(struct tpm_chip *chip)
>   	if (rc)
>   		goto out;
>   
> +	rc = tpm2_startup(chip);
> +	if (rc && rc != TPM2_RC_INITIALIZE)
> +		goto out;
> +
>   	rc = tpm2_do_selftest(chip);
>   	if (rc && rc != TPM2_RC_INITIALIZE)
>   		goto out;
>   
>   	if (rc == TPM2_RC_INITIALIZE) {
> -		rc = tpm2_startup(chip);
> -		if (rc)
> -			goto out;
> -
>   		rc = tpm2_do_selftest(chip);
>   		if (rc)
>   			goto out;


Kind regards,

Paul

Re: [PATCH] tpm: Start the tpm2 before running a self test.

[Stefan Berger]

On 11/22/23 01:55, Hermin Anggawijaya wrote:
> Before sending a command to attempt the self test, the TPM
> may need to be started, otherwise the self test returns
> TPM2_RC_INITIALIZE value causing a log as follows:
> "tpm tpm0: A TPM error (256) occurred attempting the self test".
> 
> Signed-off-by: Hermin Anggawijaya <hermin.anggawijaya@alliedtelesis.co.nz>
> ---
>   drivers/char/tpm/tpm2-cmd.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index 93545be190a5..0530f3b5f86a 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -737,15 +737,15 @@ int tpm2_auto_startup(struct tpm_chip *chip)
>   	if (rc)
>   		goto out;
>   
> +	rc = tpm2_startup(chip);
> +	if (rc && rc != TPM2_RC_INITIALIZE)
> +		goto out;
> +

Most platforms should have firmware initialize the TPM 2 these days. 
Therefore, a selftest should work and in case it doesn't work you fall 
back to the tpm2_startup below and if you get an error message in the 
log you at least know that you firmware is not up-to-date.

>   	rc = tpm2_do_selftest(chip);
>   	if (rc && rc != TPM2_RC_INITIALIZE)
>   		goto out;
>   
>   	if (rc == TPM2_RC_INITIALIZE) {
> -		rc = tpm2_startup(chip);
> -		if (rc)
> -			goto out;
> -
>   		rc = tpm2_do_selftest(chip);
>   		if (rc)
>   			goto out;

Re: [PATCH] tpm: Start the tpm2 before running a self test.

[Jarkko Sakkinen]

On Wed Nov 22, 2023 at 8:55 AM EET, Hermin Anggawijaya wrote:
> Before sending a command to attempt the self test, the TPM
> may need to be started, otherwise the self test returns
> TPM2_RC_INITIALIZE value causing a log as follows:
> "tpm tpm0: A TPM error (256) occurred attempting the self test".
>
> Signed-off-by: Hermin Anggawijaya <hermin.anggawijaya@alliedtelesis.co.nz>

Firmware does TPM power on.

BR, Jarkko

Re: [PATCH] tpm: Start the tpm2 before running a self test.

[Angga]

On 22/11/2023 8:10 pm, Paul Menzel wrote:
>
> Dear Hermin,
>
>
> Thank you for your patch.
>
> It’d be great if you removed the dot/period from the end of the commit 
> message summary/title.
>
> Am 22.11.23 um 07:55 schrieb Hermin Anggawijaya:
>> Before sending a command to attempt the self test, the TPM
>> may need to be started, otherwise the self test returns
>> TPM2_RC_INITIALIZE value causing a log as follows:
>> "tpm tpm0: A TPM error (256) occurred attempting the self test".
>
> Please document on what platform this happens.
>
>> Signed-off-by: Hermin Anggawijaya 
>> <hermin.anggawijaya@alliedtelesis.co.nz>
>> ---
>>   drivers/char/tpm/tpm2-cmd.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>> index 93545be190a5..0530f3b5f86a 100644
>> --- a/drivers/char/tpm/tpm2-cmd.c
>> +++ b/drivers/char/tpm/tpm2-cmd.c
>> @@ -737,15 +737,15 @@ int tpm2_auto_startup(struct tpm_chip *chip)
>>       if (rc)
>>           goto out;
>>   +    rc = tpm2_startup(chip);
>> +    if (rc && rc != TPM2_RC_INITIALIZE)
>> +        goto out;
>> +
>>       rc = tpm2_do_selftest(chip);
>>       if (rc && rc != TPM2_RC_INITIALIZE)
>>           goto out;
>>         if (rc == TPM2_RC_INITIALIZE) {
>> -        rc = tpm2_startup(chip);
>> -        if (rc)
>> -            goto out;
>> -
>>           rc = tpm2_do_selftest(chip);
>>           if (rc)
>>               goto out;
>
>
> Kind regards,
>
> Paul

Hello Paul

Thank you for your comments.

 >Please document on what platform this happens.

The error log message is observed on a custom hardware design (a router) 
with an Infineon SLM 9670 TPM2.0.
The patch is useful for us, as the firmware (boot loader) of the router 
does not support TPM yet, thus the kernel
needs to start the TPM before starting the self test.

I will reply to Jarkko's and Stefan's comments separately, and if in 
principle, the patch is accepted, I will send the
second version of the patch with your comments addressed.

Kind regards

Hermin

Re: [PATCH] tpm: Start the tpm2 before running a self test.

[Angga]

On 23/11/2023 1:34 am, Stefan Berger wrote:
>
>
> On 11/22/23 01:55, Hermin Anggawijaya wrote:
>> Before sending a command to attempt the self test, the TPM
>> may need to be started, otherwise the self test returns
>> TPM2_RC_INITIALIZE value causing a log as follows:
>> "tpm tpm0: A TPM error (256) occurred attempting the self test".
>>
>> Signed-off-by: Hermin Anggawijaya 
>> <hermin.anggawijaya@alliedtelesis.co.nz>
>> ---
>>   drivers/char/tpm/tpm2-cmd.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
>> index 93545be190a5..0530f3b5f86a 100644
>> --- a/drivers/char/tpm/tpm2-cmd.c
>> +++ b/drivers/char/tpm/tpm2-cmd.c
>> @@ -737,15 +737,15 @@ int tpm2_auto_startup(struct tpm_chip *chip)
>>       if (rc)
>>           goto out;
>>   +    rc = tpm2_startup(chip);
>> +    if (rc && rc != TPM2_RC_INITIALIZE)
>> +        goto out;
>> +
>
> Most platforms should have firmware initialize the TPM 2 these days. 
> Therefore, a selftest should work and in case it doesn't work you fall 
> back to the tpm2_startup below and if you get an error message in the 
> log you at least know that you firmware is not up-to-date.
>
>>       rc = tpm2_do_selftest(chip);
>>       if (rc && rc != TPM2_RC_INITIALIZE)
>>           goto out;
>>         if (rc == TPM2_RC_INITIALIZE) {
>> -        rc = tpm2_startup(chip);
>> -        if (rc)
>> -            goto out;
>> -
>>           rc = tpm2_do_selftest(chip);
>>           if (rc)
>>               goto out;

Hello Stefan

Thank you for your comments.

Unfortunately our platforms (custom hardware design) are the ones which 
do not initialize/start the TPM2 from boot loader yet, and because of 
that the
self test in tpm2_auto_startup always produce a log error message on the 
platform start up.

While I understand your point about the log being useful for "pointing 
out not up-to-date firmware", but it might also generate unnecessary support
queries from some users on such platforms ? And maybe the kernel being 
able to deal with TPM being started more than once is better ?

If wanted, I have the second version of the patch which consist of code 
changes as in v1, plus ability for tpm2_transmit_cmd to handle multiple
attempts to start up the TPM silently, for example, once by the firmware 
and another by the kernel during tpm2 auto-startup.

Kind regards

Hermin Anggawijaya

Re: [PATCH] tpm: Start the tpm2 before running a self test.

[Angga]

On 24/11/2023 2:42 pm, Jarkko Sakkinen wrote:
> On Wed Nov 22, 2023 at 8:55 AM EET, Hermin Anggawijaya wrote:
>> Before sending a command to attempt the self test, the TPM
>> may need to be started, otherwise the self test returns
>> TPM2_RC_INITIALIZE value causing a log as follows:
>> "tpm tpm0: A TPM error (256) occurred attempting the self test".
>>
>> Signed-off-by: Hermin Anggawijaya <hermin.anggawijaya@alliedtelesis.co.nz>
> Firmware does TPM power on.
>
> BR, Jarkko

Hello Jarkko

Thank you for your comment on the patch. As indicated in my previous 
reply to Stefan's comment,
I have v2 version of the patch which also deals with multiple attempts 
to start up the TPM gracefully,
for example, once by the firmware and another by the kernel during tpm2 
auto-startup.

If you think the idea is OK, I can send the v2 of the patch.


Kind regards

Hermin

Re: [PATCH] tpm: Start the tpm2 before running a self test.

[Jarkko Sakkinen]

On Mon Nov 27, 2023 at 4:02 AM EET, Angga wrote:
> On 23/11/2023 1:34 am, Stefan Berger wrote:
> >
> >
> > On 11/22/23 01:55, Hermin Anggawijaya wrote:
> >> Before sending a command to attempt the self test, the TPM
> >> may need to be started, otherwise the self test returns
> >> TPM2_RC_INITIALIZE value causing a log as follows:
> >> "tpm tpm0: A TPM error (256) occurred attempting the self test".
> >>
> >> Signed-off-by: Hermin Anggawijaya 
> >> <hermin.anggawijaya@alliedtelesis.co.nz>
> >> ---
> >>   drivers/char/tpm/tpm2-cmd.c | 8 ++++----
> >>   1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> >> index 93545be190a5..0530f3b5f86a 100644
> >> --- a/drivers/char/tpm/tpm2-cmd.c
> >> +++ b/drivers/char/tpm/tpm2-cmd.c
> >> @@ -737,15 +737,15 @@ int tpm2_auto_startup(struct tpm_chip *chip)
> >>       if (rc)
> >>           goto out;
> >>   +    rc = tpm2_startup(chip);
> >> +    if (rc && rc != TPM2_RC_INITIALIZE)
> >> +        goto out;
> >> +
> >
> > Most platforms should have firmware initialize the TPM 2 these days. 
> > Therefore, a selftest should work and in case it doesn't work you fall 
> > back to the tpm2_startup below and if you get an error message in the 
> > log you at least know that you firmware is not up-to-date.
> >
> >>       rc = tpm2_do_selftest(chip);
> >>       if (rc && rc != TPM2_RC_INITIALIZE)
> >>           goto out;
> >>         if (rc == TPM2_RC_INITIALIZE) {
> >> -        rc = tpm2_startup(chip);
> >> -        if (rc)
> >> -            goto out;
> >> -
> >>           rc = tpm2_do_selftest(chip);
> >>           if (rc)
> >>               goto out;
>
> Hello Stefan
>
> Thank you for your comments.
>
> Unfortunately our platforms (custom hardware design) are the ones which 
> do not initialize/start the TPM2 from boot loader yet, and because of 
> that the
> self test in tpm2_auto_startup always produce a log error message on the 
> platform start up.
>
> While I understand your point about the log being useful for "pointing 
> out not up-to-date firmware", but it might also generate unnecessary support
> queries from some users on such platforms ? And maybe the kernel being 
> able to deal with TPM being started more than once is better ?
>
> If wanted, I have the second version of the patch which consist of code 
> changes as in v1, plus ability for tpm2_transmit_cmd to handle multiple
> attempts to start up the TPM silently, for example, once by the firmware 
> and another by the kernel during tpm2 auto-startup.

To save your time: no.

Mainline kernel is not modified based hardware prototypes.

You have freedom to maintain your own kernel tree for whatever changes
you need but this is totally wrong place for these type of patches.

>
> Kind regards
>
> Hermin Anggawijaya

BR, Jarkko