[PATCH] tpm: ibmvtpm: Call tpm2_sessions_init() to initialize session support

[PATCH] tpm: ibmvtpm: Call tpm2_sessions_init() to initialize session support

[Stefan Berger]

Fix the following type of error message caused by a missing call to
tpm2_sessions_init() in the IBM vTPM driver:

[    2.987131] tpm tpm0: tpm2_load_context: failed with a TPM error 0x01C4
[    2.987140] ima: Error Communicating to TPM chip, result: -14

Fixes: d2add27cf2b8 ("tpm: Add NULL primary creation")
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 drivers/char/tpm/tpm_ibmvtpm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
index d3989b257f42..1e5b107d1f3b 100644
--- a/drivers/char/tpm/tpm_ibmvtpm.c
+++ b/drivers/char/tpm/tpm_ibmvtpm.c
@@ -698,6 +698,10 @@ static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev,
 		rc = tpm2_get_cc_attrs_tbl(chip);
 		if (rc)
 			goto init_irq_cleanup;
+
+		rc = tpm2_sessions_init(chip);
+		if (rc)
+			goto init_irq_cleanup;
 	}
 
 	return tpm_chip_register(chip);
-- 
2.45.2

Re: [PATCH] tpm: ibmvtpm: Call tpm2_sessions_init() to initialize session support

[James Bottomley]

On Mon, 2024-06-17 at 15:34 -0400, Stefan Berger wrote:
> Fix the following type of error message caused by a missing call to
> tpm2_sessions_init() in the IBM vTPM driver:
> 
> [    2.987131] tpm tpm0: tpm2_load_context: failed with a TPM error
> 0x01C4
> [    2.987140] ima: Error Communicating to TPM chip, result: -14
> 
> Fixes: d2add27cf2b8 ("tpm: Add NULL primary creation")
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>  drivers/char/tpm/tpm_ibmvtpm.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/char/tpm/tpm_ibmvtpm.c
> b/drivers/char/tpm/tpm_ibmvtpm.c
> index d3989b257f42..1e5b107d1f3b 100644
> --- a/drivers/char/tpm/tpm_ibmvtpm.c
> +++ b/drivers/char/tpm/tpm_ibmvtpm.c
> @@ -698,6 +698,10 @@ static int tpm_ibmvtpm_probe(struct vio_dev
> *vio_dev,
>                 rc = tpm2_get_cc_attrs_tbl(chip);
>                 if (rc)
>                         goto init_irq_cleanup;
> +
> +               rc = tpm2_sessions_init(chip);
> +               if (rc)
> +                       goto init_irq_cleanup;

This looks wrong: the whole thing is designed to occur in the bootstrap
phase from tpm_chip_register() (which tpm_ibmvtpm.c definitely calls),
so why isn't it happening?

James

Re: [PATCH] tpm: ibmvtpm: Call tpm2_sessions_init() to initialize session support

[Stefan Berger]

On 6/17/24 15:42, James Bottomley wrote:
> On Mon, 2024-06-17 at 15:34 -0400, Stefan Berger wrote:
>> Fix the following type of error message caused by a missing call to
>> tpm2_sessions_init() in the IBM vTPM driver:
>>
>> [    2.987131] tpm tpm0: tpm2_load_context: failed with a TPM error
>> 0x01C4
>> [    2.987140] ima: Error Communicating to TPM chip, result: -14
>>
>> Fixes: d2add27cf2b8 ("tpm: Add NULL primary creation")
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>> ---
>>   drivers/char/tpm/tpm_ibmvtpm.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/char/tpm/tpm_ibmvtpm.c
>> b/drivers/char/tpm/tpm_ibmvtpm.c
>> index d3989b257f42..1e5b107d1f3b 100644
>> --- a/drivers/char/tpm/tpm_ibmvtpm.c
>> +++ b/drivers/char/tpm/tpm_ibmvtpm.c
>> @@ -698,6 +698,10 @@ static int tpm_ibmvtpm_probe(struct vio_dev
>> *vio_dev,
>>                  rc = tpm2_get_cc_attrs_tbl(chip);
>>                  if (rc)
>>                          goto init_irq_cleanup;
>> +
>> +               rc = tpm2_sessions_init(chip);
>> +               if (rc)
>> +                       goto init_irq_cleanup;
> 
> This looks wrong: the whole thing is designed to occur in the bootstrap
> phase from tpm_chip_register() (which tpm_ibmvtpm.c definitely calls),
> so why isn't it happening?

Because flags = TPM_OPS_AUTO_STARTUP has not been set for this driver.
> 
> James
> 

Re: [PATCH] tpm: ibmvtpm: Call tpm2_sessions_init() to initialize session support

[James Bottomley]

On Mon, 2024-06-17 at 15:56 -0400, Stefan Berger wrote:
> 
> 
> On 6/17/24 15:42, James Bottomley wrote:
> > On Mon, 2024-06-17 at 15:34 -0400, Stefan Berger wrote:
> > > Fix the following type of error message caused by a missing call
> > > to
> > > tpm2_sessions_init() in the IBM vTPM driver:
> > > 
> > > [    2.987131] tpm tpm0: tpm2_load_context: failed with a TPM
> > > error
> > > 0x01C4
> > > [    2.987140] ima: Error Communicating to TPM chip, result: -14
> > > 
> > > Fixes: d2add27cf2b8 ("tpm: Add NULL primary creation")
> > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > > ---
> > >   drivers/char/tpm/tpm_ibmvtpm.c | 4 ++++
> > >   1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/drivers/char/tpm/tpm_ibmvtpm.c
> > > b/drivers/char/tpm/tpm_ibmvtpm.c
> > > index d3989b257f42..1e5b107d1f3b 100644
> > > --- a/drivers/char/tpm/tpm_ibmvtpm.c
> > > +++ b/drivers/char/tpm/tpm_ibmvtpm.c
> > > @@ -698,6 +698,10 @@ static int tpm_ibmvtpm_probe(struct vio_dev
> > > *vio_dev,
> > >                  rc = tpm2_get_cc_attrs_tbl(chip);
> > >                  if (rc)
> > >                          goto init_irq_cleanup;
> > > +
> > > +               rc = tpm2_sessions_init(chip);
> > > +               if (rc)
> > > +                       goto init_irq_cleanup;
> > 
> > This looks wrong: the whole thing is designed to occur in the
> > bootstrap
> > phase from tpm_chip_register() (which tpm_ibmvtpm.c definitely
> > calls),
> > so why isn't it happening?
> 
> Because flags = TPM_OPS_AUTO_STARTUP has not been set for this
> driver.
> 

In that case, wouldn't the fix be to move tpm_sessions_init() to
somewhere in tpm_chip_register() that would then be called by this
driver?  Having to special case it for every driver that doesn't set
this flag is going to be a huge pain.

I think the only reason it's down that far is that it should only be
called for TPM2 code so it was avoiding doing the check twice, so
something like this?

James

---

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 5da134f12c9a..4280cbb0f0b1 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -347,6 +347,12 @@ int tpm_auto_startup(struct tpm_chip *chip)
 {
 	int rc;
 
+	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
+		rc = tpm2_sessions_init(chip);
+		if (rc)
+			return rc;
+	}
+
 	if (!(chip->ops->flags & TPM_OPS_AUTO_STARTUP))
 		return 0;
 
diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
index 1e856259219e..b4f85c8cdbb6 100644
--- a/drivers/char/tpm/tpm2-cmd.c
+++ b/drivers/char/tpm/tpm2-cmd.c
@@ -773,11 +773,6 @@ int tpm2_auto_startup(struct tpm_chip *chip)
 		rc = 0;
 	}
 
-	if (rc)
-		goto out;
-
-	rc = tpm2_sessions_init(chip);
-
 out:
 	/*
 	 * Infineon TPM in field upgrade mode will return no data for the number

Re: [PATCH] tpm: ibmvtpm: Call tpm2_sessions_init() to initialize session support

[Stefan Berger]

On 6/17/24 16:05, James Bottomley wrote:
> On Mon, 2024-06-17 at 15:56 -0400, Stefan Berger wrote:
>>
>>
>> On 6/17/24 15:42, James Bottomley wrote:
>>> On Mon, 2024-06-17 at 15:34 -0400, Stefan Berger wrote:
>>>> Fix the following type of error message caused by a missing call
>>>> to
>>>> tpm2_sessions_init() in the IBM vTPM driver:
>>>>
>>>> [    2.987131] tpm tpm0: tpm2_load_context: failed with a TPM
>>>> error
>>>> 0x01C4
>>>> [    2.987140] ima: Error Communicating to TPM chip, result: -14
>>>>
>>>> Fixes: d2add27cf2b8 ("tpm: Add NULL primary creation")
>>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>>>> ---
>>>>    drivers/char/tpm/tpm_ibmvtpm.c | 4 ++++
>>>>    1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/char/tpm/tpm_ibmvtpm.c
>>>> b/drivers/char/tpm/tpm_ibmvtpm.c
>>>> index d3989b257f42..1e5b107d1f3b 100644
>>>> --- a/drivers/char/tpm/tpm_ibmvtpm.c
>>>> +++ b/drivers/char/tpm/tpm_ibmvtpm.c
>>>> @@ -698,6 +698,10 @@ static int tpm_ibmvtpm_probe(struct vio_dev
>>>> *vio_dev,
>>>>                   rc = tpm2_get_cc_attrs_tbl(chip);
>>>>                   if (rc)
>>>>                           goto init_irq_cleanup;
>>>> +
>>>> +               rc = tpm2_sessions_init(chip);
>>>> +               if (rc)
>>>> +                       goto init_irq_cleanup;
>>>
>>> This looks wrong: the whole thing is designed to occur in the
>>> bootstrap
>>> phase from tpm_chip_register() (which tpm_ibmvtpm.c definitely
>>> calls),
>>> so why isn't it happening?
>>
>> Because flags = TPM_OPS_AUTO_STARTUP has not been set for this
>> driver.
>>
> 
> In that case, wouldn't the fix be to move tpm_sessions_init() to
> somewhere in tpm_chip_register() that would then be called by this
> driver?  Having to special case it for every driver that doesn't set
> this flag is going to be a huge pain.

I think the 2nd fix is to set TPM_OPS_AUTO_STARTUP also for the ibmvtpm 
driver like the following patch on top of this one, but after more testing:

 From c6bcd3890f1bdc43d9549fbb39fe388adf756358 Mon Sep 17 00:00:00 2001
From: Stefan Berger <stefanb@linux.ibm.com>
Date: Mon, 17 Jun 2024 16:05:54 -0400
Subject: [PATCH] tpm: ibmvtpm: Set TPM_OPS_AUTO_STARTUP flag for
  initialization

Set the TPM_OPS_AUTO_STARTUP flag for using common initialization code.
The difference between the old initialization and the new one is that
for TPM 1.2 tpm1_do_selftest and for TPM 2 tpm2_do_selftest will be called.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
  drivers/char/tpm/tpm_ibmvtpm.c | 15 +--------------
  1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
index 1e5b107d1f3b..76d048f63d55 100644
--- a/drivers/char/tpm/tpm_ibmvtpm.c
+++ b/drivers/char/tpm/tpm_ibmvtpm.c
@@ -450,6 +450,7 @@ static bool tpm_ibmvtpm_req_canceled(struct tpm_chip 
*chip, u8 status)
  }

  static const struct tpm_class_ops tpm_ibmvtpm = {
+       .flags = TPM_OPS_AUTO_STARTUP,
         .recv = tpm_ibmvtpm_recv,
         .send = tpm_ibmvtpm_send,
         .cancel = tpm_ibmvtpm_cancel,
@@ -690,20 +691,6 @@ static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev,
         if (!strcmp(id->compat, "IBM,vtpm20"))
                 chip->flags |= TPM_CHIP_FLAG_TPM2;

-       rc = tpm_get_timeouts(chip);
-       if (rc)
-               goto init_irq_cleanup;
-
-       if (chip->flags & TPM_CHIP_FLAG_TPM2) {
-               rc = tpm2_get_cc_attrs_tbl(chip);
-               if (rc)
-                       goto init_irq_cleanup;
-
-               rc = tpm2_sessions_init(chip);
-               if (rc)
-                       goto init_irq_cleanup;
-       }
-
         return tpm_chip_register(chip);
  init_irq_cleanup:
         do {
--
2.45.2

Regards,
    Stefan

> 
> I think the only reason it's down that far is that it should only be
> called for TPM2 code so it was avoiding doing the check twice, so
> something like this >
> James
> 
> ---
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 5da134f12c9a..4280cbb0f0b1 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -347,6 +347,12 @@ int tpm_auto_startup(struct tpm_chip *chip)
>   {
>   	int rc;
>   
> +	if (chip->flags & TPM_CHIP_FLAG_TPM2) {
> +		rc = tpm2_sessions_init(chip);
> +		if (rc)
> +			return rc;
> +	}
> +
>   	if (!(chip->ops->flags & TPM_OPS_AUTO_STARTUP))
>   		return 0;
>   
> diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c
> index 1e856259219e..b4f85c8cdbb6 100644
> --- a/drivers/char/tpm/tpm2-cmd.c
> +++ b/drivers/char/tpm/tpm2-cmd.c
> @@ -773,11 +773,6 @@ int tpm2_auto_startup(struct tpm_chip *chip)
>   		rc = 0;
>   	}
>   
> -	if (rc)
> -		goto out;
> -
> -	rc = tpm2_sessions_init(chip);
> -
>   out:
>   	/*
>   	 * Infineon TPM in field upgrade mode will return no data for the number
> 

Re: [PATCH] tpm: ibmvtpm: Call tpm2_sessions_init() to initialize session support

[Stefan Berger]

Jarkko,
   are you ok with this patch?

   Stefan

On 6/17/24 15:34, Stefan Berger wrote:
> Fix the following type of error message caused by a missing call to
> tpm2_sessions_init() in the IBM vTPM driver:
> 
> [    2.987131] tpm tpm0: tpm2_load_context: failed with a TPM error 0x01C4
> [    2.987140] ima: Error Communicating to TPM chip, result: -14
> 
> Fixes: d2add27cf2b8 ("tpm: Add NULL primary creation")
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>   drivers/char/tpm/tpm_ibmvtpm.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
> index d3989b257f42..1e5b107d1f3b 100644
> --- a/drivers/char/tpm/tpm_ibmvtpm.c
> +++ b/drivers/char/tpm/tpm_ibmvtpm.c
> @@ -698,6 +698,10 @@ static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev,
>   		rc = tpm2_get_cc_attrs_tbl(chip);
>   		if (rc)
>   			goto init_irq_cleanup;
> +
> +		rc = tpm2_sessions_init(chip);
> +		if (rc)
> +			goto init_irq_cleanup;
>   	}
>   
>   	return tpm_chip_register(chip);

Re: [PATCH] tpm: ibmvtpm: Call tpm2_sessions_init() to initialize session support

[Linux regression tracking (Thorsten Leemhuis)]

[CCing the regression list]

On 20.06.24 00:34, Stefan Berger wrote:
> Jarkko,
>   are you ok with this patch?

Hmmm, hope I did not miss anythng, but looks like nothing happened for
about 10 days here. Hence:

Jarkko, looks like some feedback from your side really would help to
find a path to get this regression resolved before 6.10 is released.

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

#regzbot poke

> On 6/17/24 15:34, Stefan Berger wrote:
>> Fix the following type of error message caused by a missing call to
>> tpm2_sessions_init() in the IBM vTPM driver:
>>
>> [    2.987131] tpm tpm0: tpm2_load_context: failed with a TPM error
>> 0x01C4
>> [    2.987140] ima: Error Communicating to TPM chip, result: -14
>>
>> Fixes: d2add27cf2b8 ("tpm: Add NULL primary creation")
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>> ---
>>   drivers/char/tpm/tpm_ibmvtpm.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/char/tpm/tpm_ibmvtpm.c
>> b/drivers/char/tpm/tpm_ibmvtpm.c
>> index d3989b257f42..1e5b107d1f3b 100644
>> --- a/drivers/char/tpm/tpm_ibmvtpm.c
>> +++ b/drivers/char/tpm/tpm_ibmvtpm.c
>> @@ -698,6 +698,10 @@ static int tpm_ibmvtpm_probe(struct vio_dev
>> *vio_dev,
>>           rc = tpm2_get_cc_attrs_tbl(chip);
>>           if (rc)
>>               goto init_irq_cleanup;
>> +
>> +        rc = tpm2_sessions_init(chip);
>> +        if (rc)
>> +            goto init_irq_cleanup;
>>       }
>>         return tpm_chip_register(chip);

Re: [PATCH] tpm: ibmvtpm: Call tpm2_sessions_init() to initialize session support

[Jarkko Sakkinen]

On Fri, 2024-06-28 at 17:00 +0200, Linux regression tracking (Thorsten Leemhuis) wrote:
> [CCing the regression list]
> 
> On 20.06.24 00:34, Stefan Berger wrote:
> > Jarkko,
> >   are you ok with this patch?
> 
> Hmmm, hope I did not miss anythng, but looks like nothing happened for
> about 10 days here. Hence:
> 
> Jarkko, looks like some feedback from your side really would help to
> find a path to get this regression resolved before 6.10 is released.
> 
> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)

Sorry for latency, and except a bit more slow phase also during
July because I'm most of this month on Holiday, except taking care
6.11 release.

This really is a bug in the HMAC code not in the IBM driver as 
it should not break because of a new feature, i.e. this is only
correct conclusions, give the "no regressions" rule.

Since HMAC is by default only for x86_64 and it does not break
defconfig's, we should take time and fix the actual issue.

BR, Jarkko

Re: [PATCH] tpm: ibmvtpm: Call tpm2_sessions_init() to initialize session support

[Stefan Berger]

On 7/1/24 11:22, Jarkko Sakkinen wrote:
> On Fri, 2024-06-28 at 17:00 +0200, Linux regression tracking (Thorsten Leemhuis) wrote:
>> [CCing the regression list]
>>
>> On 20.06.24 00:34, Stefan Berger wrote:
>>> Jarkko,
>>>    are you ok with this patch?
>>
>> Hmmm, hope I did not miss anythng, but looks like nothing happened for
>> about 10 days here. Hence:
>>
>> Jarkko, looks like some feedback from your side really would help to
>> find a path to get this regression resolved before 6.10 is released.
>>
>> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
> 
> Sorry for latency, and except a bit more slow phase also during
> July because I'm most of this month on Holiday, except taking care
> 6.11 release.
> 
> This really is a bug in the HMAC code not in the IBM driver as
> it should not break because of a new feature, i.e. this is only
> correct conclusions, give the "no regressions" rule.
> 
> Since HMAC is by default only for x86_64 and it does not break
> defconfig's, we should take time and fix the actual issue.

It was enabled it on my ppc64 system after a git pull -- at least I did 
not enable it explicitly. Besides that others can enable it on any arch 
unless you now change the 'default x86_64' to a 'depends x86_64' iiuc 
otherwise the usage of a Fixes: , as I used in my patch, would be justified.

config TCG_TPM2_HMAC
	bool "Use HMAC and encrypted transactions on the TPM bus"
	default X86_64
	select CRYPTO_ECDH
	select CRYPTO_LIB_AESCFB
	select CRYPTO_LIB_SHA256

https://elixir.bootlin.com/linux/v6.10-rc6/source/drivers/char/tpm/Kconfig

> 
> BR, Jarkko
> 

Re: [PATCH] tpm: ibmvtpm: Call tpm2_sessions_init() to initialize session support

[Jarkko Sakkinen]

On Mon Jul 1, 2024 at 6:29 PM UTC, Stefan Berger wrote:
>
>
> On 7/1/24 11:22, Jarkko Sakkinen wrote:
> > On Fri, 2024-06-28 at 17:00 +0200, Linux regression tracking (Thorsten Leemhuis) wrote:
> >> [CCing the regression list]
> >>
> >> On 20.06.24 00:34, Stefan Berger wrote:
> >>> Jarkko,
> >>>    are you ok with this patch?
> >>
> >> Hmmm, hope I did not miss anythng, but looks like nothing happened for
> >> about 10 days here. Hence:
> >>
> >> Jarkko, looks like some feedback from your side really would help to
> >> find a path to get this regression resolved before 6.10 is released.
> >>
> >> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
> > 
> > Sorry for latency, and except a bit more slow phase also during
> > July because I'm most of this month on Holiday, except taking care
> > 6.11 release.
> > 
> > This really is a bug in the HMAC code not in the IBM driver as
> > it should not break because of a new feature, i.e. this is only
> > correct conclusions, give the "no regressions" rule.
> > 
> > Since HMAC is by default only for x86_64 and it does not break
> > defconfig's, we should take time and fix the actual issue.
>
> It was enabled it on my ppc64 system after a git pull -- at least I did 
> not enable it explicitly. Besides that others can enable it on any arch 
> unless you now change the 'default x86_64' to a 'depends x86_64' iiuc 
> otherwise the usage of a Fixes: , as I used in my patch, would be justified.
>
> config TCG_TPM2_HMAC
> 	bool "Use HMAC and encrypted transactions on the TPM bus"
> 	default X86_64
> 	select CRYPTO_ECDH
> 	select CRYPTO_LIB_AESCFB
> 	select CRYPTO_LIB_SHA256
>
> https://elixir.bootlin.com/linux/v6.10-rc6/source/drivers/char/tpm/Kconfig

Yep, it is still a bug, and unmodified IBM vtpm driver must be expected
to work. I was merely saying that there is some window to  fix it properly
instead of duct tape since it is not yet widely enable feature.

I was shocked to see that the implementation has absolutely no checks
whether chip->auth was allocated. I mean anything that would cause
tpm2_sessions_init() not called could trigger null dereference.

So can you test this and see how your test hardware behaves:

https://lore.kernel.org/linux-integrity/20240701170735.109583-1-jarkko@kernel.org/T/#u

I'll modify it accrodingly if problems persist. Please put your feedback
over there. I cannot anything but compile test so it could be that
I've ignored something.

BR, Jarkko

Re: [PATCH] tpm: ibmvtpm: Call tpm2_sessions_init() to initialize session support

[Stefan Berger]

On 7/1/24 15:01, Jarkko Sakkinen wrote:
> On Mon Jul 1, 2024 at 6:29 PM UTC, Stefan Berger wrote:
>>
>>
>> On 7/1/24 11:22, Jarkko Sakkinen wrote:
>>> On Fri, 2024-06-28 at 17:00 +0200, Linux regression tracking (Thorsten Leemhuis) wrote:
>>>> [CCing the regression list]
>>>>
>>>> On 20.06.24 00:34, Stefan Berger wrote:
>>>>> Jarkko,
>>>>>     are you ok with this patch?
>>>>
>>>> Hmmm, hope I did not miss anythng, but looks like nothing happened for
>>>> about 10 days here. Hence:
>>>>
>>>> Jarkko, looks like some feedback from your side really would help to
>>>> find a path to get this regression resolved before 6.10 is released.
>>>>
>>>> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
>>>
>>> Sorry for latency, and except a bit more slow phase also during
>>> July because I'm most of this month on Holiday, except taking care
>>> 6.11 release.
>>>
>>> This really is a bug in the HMAC code not in the IBM driver as
>>> it should not break because of a new feature, i.e. this is only
>>> correct conclusions, give the "no regressions" rule.
>>>
>>> Since HMAC is by default only for x86_64 and it does not break
>>> defconfig's, we should take time and fix the actual issue.
>>
>> It was enabled it on my ppc64 system after a git pull -- at least I did
>> not enable it explicitly. Besides that others can enable it on any arch
>> unless you now change the 'default x86_64' to a 'depends x86_64' iiuc
>> otherwise the usage of a Fixes: , as I used in my patch, would be justified.
>>
>> config TCG_TPM2_HMAC
>> 	bool "Use HMAC and encrypted transactions on the TPM bus"
>> 	default X86_64
>> 	select CRYPTO_ECDH
>> 	select CRYPTO_LIB_AESCFB
>> 	select CRYPTO_LIB_SHA256
>>
>> https://elixir.bootlin.com/linux/v6.10-rc6/source/drivers/char/tpm/Kconfig
> 
> Yep, it is still a bug, and unmodified IBM vtpm driver must be expected
> to work. I was merely saying that there is some window to  fix it properly
> instead of duct tape since it is not yet widely enable feature.
> 
> I was shocked to see that the implementation has absolutely no checks
> whether chip->auth was allocated. I mean anything that would cause
> tpm2_sessions_init() not called could trigger null dereference.
> 
> So can you test this and see how your test hardware behaves:

I just tested it and it does NOT resolve the issue on my ppc64 machine. 
I see this here:

[    1.549798] tpm_ibmvtpm 5000: CRQ initialized
[    1.549871] tpm_ibmvtpm 5000: CRQ initialization completed
[    2.569446] tpm2_start_auth_session: encryption is not active
[    2.570544] tpm tpm0: A TPM error (154) occurred attempting get random
....
[  330.188826] tpm tpm0: A TPM error (149) occurred attempting extend a 
PCR value
[  330.189438] ima: Error Communicating to TPM chip, result: 149
[  330.195007] tpm tpm0: A TPM error (149) occurred attempting extend a 
PCR value
[  330.195550] ima: Error Communicating to TPM chip, result: 149
[  330.197246] tpm tpm0: A TPM error (149) occurred attempting extend a 
PCR value
[  330.197727] ima: Error Communicating to TPM chip, result: 149
[  330.199378] tpm tpm0: A TPM error (149) occurred attempting extend a 
PCR value
[  330.199962] ima: Error Communicating to TPM chip, result: 149
[  330.201452] tpm tpm0: A TPM error (149) occurred attempting extend a 
PCR value
[  330.201917] ima: Error Communicating to TPM chip, result: 149


My 4-liner patch, applied on top of it, does resolve the issue:

[    1.462079] tpm_ibmvtpm 5000: CRQ initialized
[    1.462125] tpm_ibmvtpm 5000: CRQ initialization completed
[    2.496024] xhci_hcd 0000:00:02.0: xHCI Host Controller
[    2.496183] xhci_hcd 0000:00:02.0: new USB bus registered, assigned 
bus number 1

Applying it is probably the better path forward than restricting HMAC to 
x86_64 now and enabling it on a per-architecture basis afterwards ...

    Stefan

> 
> https://lore.kernel.org/linux-integrity/20240701170735.109583-1-jarkko@kernel.org/T/#u
> 
> I'll modify it accrodingly if problems persist. Please put your feedback
> over there. I cannot anything but compile test so it could be that
> I've ignored something.
> 
> BR, Jarkko

Re: [PATCH] tpm: ibmvtpm: Call tpm2_sessions_init() to initialize session support

[Jarkko Sakkinen]

On Mon, 2024-07-01 at 15:14 -0400, Stefan Berger wrote:
> Applying it is probably the better path forward than restricting HMAC to 
> x86_64 now and enabling it on a per-architecture basis afterwards ...

Why is this here and not in the associated patch?

Any, what argue against is already done for v6.10.

The actual bug needs to be fixed before anything
else.

I can look at the patch when in August (back from
holiday) but please response to the correct patch
next time, thanks.

BR, Jarkko

Re: [PATCH] tpm: ibmvtpm: Call tpm2_sessions_init() to initialize session support

[Jarkko Sakkinen]

On Wed, 2024-07-03 at 02:48 +0300, Jarkko Sakkinen wrote:
> On Mon, 2024-07-01 at 15:14 -0400, Stefan Berger wrote:
> > Applying it is probably the better path forward than restricting HMAC to 
> > x86_64 now and enabling it on a per-architecture basis afterwards ...
> 
> Why is this here and not in the associated patch?
> 
> Any, what argue against is already done for v6.10.
> 
> The actual bug needs to be fixed before anything
> else.
> 
> I can look at the patch when in August (back from
> holiday) but please response to the correct patch
> next time, thanks.

Next steps forward:

1  Comment out sessions_init().
2. See what happens on x86 in QEMU.
3. All errors were some sort size errors, so look into failing
   sites and fixup the use of hmac shenanigans.

BR, Jarkko

Re: [PATCH] tpm: ibmvtpm: Call tpm2_sessions_init() to initialize session support

[Jarkko Sakkinen]

On Wed Jul 3, 2024 at 2:57 AM EEST, Jarkko Sakkinen wrote:
> On Wed, 2024-07-03 at 02:48 +0300, Jarkko Sakkinen wrote:
> > On Mon, 2024-07-01 at 15:14 -0400, Stefan Berger wrote:
> > > Applying it is probably the better path forward than restricting HMAC to 
> > > x86_64 now and enabling it on a per-architecture basis afterwards ...
> > 
> > Why is this here and not in the associated patch?
> > 
> > Any, what argue against is already done for v6.10.
> > 
> > The actual bug needs to be fixed before anything
> > else.
> > 
> > I can look at the patch when in August (back from
> > holiday) but please response to the correct patch
> > next time, thanks.
>
> Next steps forward:
>
> 1  Comment out sessions_init().
> 2. See what happens on x86 in QEMU.
> 3. All errors were some sort size errors, so look into failing
>    sites and fixup the use of hmac shenanigans.

For anything "fast" or "quick" I think this really the only
possible sane thing to do right now:

https://lore.kernel.org/linux-integrity/20240703003033.19057-1-jarkko@kernel.org/T/#u

There's also bunch of other drivers than tpm_ibmvtpm so better
to limit it to known good drivers.

I can take at the actual issue in August and will review any
possible patches then. This one I'll send after my current PR
for TPM has been merged.

BR, Jarkko

Re: [PATCH] tpm: ibmvtpm: Call tpm2_sessions_init() to initialize session support

[Jarkko Sakkinen]

On Wed Jul 3, 2024 at 3:34 AM EEST, Jarkko Sakkinen wrote:
> https://lore.kernel.org/linux-integrity/20240703003033.19057-1-jarkko@kernel.org/T/#u
>
> There's also bunch of other drivers than tpm_ibmvtpm so better
> to limit it to known good drivers.
>
> I can take at the actual issue in August and will review any
> possible patches then. This one I'll send after my current PR
> for TPM has been merged.

After this patch has been merged to mainline, you can send your change
as a feature patch for tpm_ibmvtpm and replace Kconfig line with
"depends on ... || TCG_IBMVTPM".

This zeros the risk other drivers than tpm_tis, tpm_crb and tpm_ibmvtpm,
and thus is the only possible solution that I'm willing to accept in
*fast phase*". I.e. the most conservative and guaranteed route, like
anyone with sane mind should really.

BR, Jarkko

Re: [PATCH] tpm: ibmvtpm: Call tpm2_sessions_init() to initialize session support

[Jarkko Sakkinen]

On Wed Jul 3, 2024 at 3:48 AM EEST, Jarkko Sakkinen wrote:
> On Wed Jul 3, 2024 at 3:34 AM EEST, Jarkko Sakkinen wrote:
> > https://lore.kernel.org/linux-integrity/20240703003033.19057-1-jarkko@kernel.org/T/#u
> >
> > There's also bunch of other drivers than tpm_ibmvtpm so better
> > to limit it to known good drivers.
> >
> > I can take at the actual issue in August and will review any
> > possible patches then. This one I'll send after my current PR
> > for TPM has been merged.
>
> After this patch has been merged to mainline, you can send your change
> as a feature patch for tpm_ibmvtpm and replace Kconfig line with
> "depends on ... || TCG_IBMVTPM".
>
> This zeros the risk other drivers than tpm_tis, tpm_crb and tpm_ibmvtpm,
> and thus is the only possible solution that I'm willing to accept in
> *fast phase*". I.e. the most conservative and guaranteed route, like
> anyone with sane mind should really.

Ouch, that won't obviously work so please ignore this! :-) Sorry.
It really needs to be !TCG_IBMVTPM because otherwise TCG_TIS_CORE
or TCG_CRB would leak the code over there.

BR, Jarkko

Re: [PATCH] tpm: ibmvtpm: Call tpm2_sessions_init() to initialize session support

[Jarkko Sakkinen]

On Wed, 2024-06-19 at 18:34 -0400, Stefan Berger wrote:
> Jarkko,
>    are you ok with this patch?

Nope :-) It masks a bug, does not fix it.

BR, Jarkko

Re: [PATCH] tpm: ibmvtpm: Call tpm2_sessions_init() to initialize session support

[Michael Ellerman]

Stefan Berger <stefanb@linux.ibm.com> writes:
> Fix the following type of error message caused by a missing call to
> tpm2_sessions_init() in the IBM vTPM driver:
>
> [    2.987131] tpm tpm0: tpm2_load_context: failed with a TPM error 0x01C4
> [    2.987140] ima: Error Communicating to TPM chip, result: -14
>
> Fixes: d2add27cf2b8 ("tpm: Add NULL primary creation")
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>  drivers/char/tpm/tpm_ibmvtpm.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c
> index d3989b257f42..1e5b107d1f3b 100644
> --- a/drivers/char/tpm/tpm_ibmvtpm.c
> +++ b/drivers/char/tpm/tpm_ibmvtpm.c
> @@ -698,6 +698,10 @@ static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev,
>  		rc = tpm2_get_cc_attrs_tbl(chip);
>  		if (rc)
>  			goto init_irq_cleanup;
> +
> +		rc = tpm2_sessions_init(chip);
> +		if (rc)
> +			goto init_irq_cleanup;
>  	}
>  
>  	return tpm_chip_register(chip);

#regzbot ^introduced: d2add27cf2b8 

Re: [PATCH] tpm: ibmvtpm: Call tpm2_sessions_init() to initialize session support

[James Bottomley]

On Fri, 2024-06-28 at 10:54 +1000, Michael Ellerman wrote:
> Stefan Berger <stefanb@linux.ibm.com> writes:
> > Fix the following type of error message caused by a missing call to
> > tpm2_sessions_init() in the IBM vTPM driver:
> > 
> > [    2.987131] tpm tpm0: tpm2_load_context: failed with a TPM error
> > 0x01C4
> > [    2.987140] ima: Error Communicating to TPM chip, result: -14
> > 
> > Fixes: d2add27cf2b8 ("tpm: Add NULL primary creation")
> > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > ---
> >  drivers/char/tpm/tpm_ibmvtpm.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/char/tpm/tpm_ibmvtpm.c
> > b/drivers/char/tpm/tpm_ibmvtpm.c
> > index d3989b257f42..1e5b107d1f3b 100644
> > --- a/drivers/char/tpm/tpm_ibmvtpm.c
> > +++ b/drivers/char/tpm/tpm_ibmvtpm.c
> > @@ -698,6 +698,10 @@ static int tpm_ibmvtpm_probe(struct vio_dev
> > *vio_dev,
> >                 rc = tpm2_get_cc_attrs_tbl(chip);
> >                 if (rc)
> >                         goto init_irq_cleanup;
> > +
> > +               rc = tpm2_sessions_init(chip);
> > +               if (rc)
> > +                       goto init_irq_cleanup;
> >         }
> >  
> >         return tpm_chip_register(chip);
> 
> #regzbot ^introduced: d2add27cf2b8 

Could you please test out the patch I proposed for this:

https://lore.kernel.org/linux-integrity/1302b413a2d7bf3b275133e7fdb04b44bfe2d5e3.camel@HansenPartnership.com/

Because it's not just tmp_ibmvtpm that doesn't call autostart.  From
inspection xen-tpmfront, tmp_nsc, tpm_infineon and tpm_atmel also
don't, so it would be better to fix this for everyone rather than just
for you and have to do a separate fix for each of them.

James

Re: [PATCH] tpm: ibmvtpm: Call tpm2_sessions_init() to initialize session support

[Stefan Berger]

On 6/28/24 12:39, James Bottomley wrote:
> On Fri, 2024-06-28 at 10:54 +1000, Michael Ellerman wrote:
>> Stefan Berger <stefanb@linux.ibm.com> writes:
>>> Fix the following type of error message caused by a missing call to
>>> tpm2_sessions_init() in the IBM vTPM driver:
>>>
>>> [    2.987131] tpm tpm0: tpm2_load_context: failed with a TPM error
>>> 0x01C4
>>> [    2.987140] ima: Error Communicating to TPM chip, result: -14
>>>
>>> Fixes: d2add27cf2b8 ("tpm: Add NULL primary creation")
>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>>> ---
>>>   drivers/char/tpm/tpm_ibmvtpm.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/char/tpm/tpm_ibmvtpm.c
>>> b/drivers/char/tpm/tpm_ibmvtpm.c
>>> index d3989b257f42..1e5b107d1f3b 100644
>>> --- a/drivers/char/tpm/tpm_ibmvtpm.c
>>> +++ b/drivers/char/tpm/tpm_ibmvtpm.c
>>> @@ -698,6 +698,10 @@ static int tpm_ibmvtpm_probe(struct vio_dev
>>> *vio_dev,
>>>                  rc = tpm2_get_cc_attrs_tbl(chip);
>>>                  if (rc)
>>>                          goto init_irq_cleanup;
>>> +
>>> +               rc = tpm2_sessions_init(chip);
>>> +               if (rc)
>>> +                       goto init_irq_cleanup;
>>>          }
>>>   
>>>          return tpm_chip_register(chip);
>>
>> #regzbot ^introduced: d2add27cf2b8
> 
> Could you please test out the patch I proposed for this:
> 
> https://lore.kernel.org/linux-integrity/1302b413a2d7bf3b275133e7fdb04b44bfe2d5e3.camel@HansenPartnership.com/
> 
> Because it's not just tmp_ibmvtpm that doesn't call autostart.  From
> inspection xen-tpmfront, tmp_nsc, tpm_infineon and tpm_atmel also

afaik tpm_infineon is a TPM 1.2 driver; same holds for tpm_atmel and 
tpm_ns. Neither needs this new call from what I understand. The new TPM2 
drivers have the TPM_OPS_AUTO_STARTUP flag set.

$ grep -r AUTO drivers/char/tpm/*.c | grep =
drivers/char/tpm/tpm_crb.c:     .flags = TPM_OPS_AUTO_STARTUP,
drivers/char/tpm/tpm_ftpm_tee.c:        .flags = TPM_OPS_AUTO_STARTUP,
drivers/char/tpm/tpm_i2c_atmel.c:       .flags = TPM_OPS_AUTO_STARTUP,
drivers/char/tpm/tpm_i2c_infineon.c:    .flags = TPM_OPS_AUTO_STARTUP,
drivers/char/tpm/tpm_i2c_nuvoton.c:     .flags = TPM_OPS_AUTO_STARTUP,
drivers/char/tpm/tpm_ibmvtpm.c: .flags = TPM_OPS_AUTO_STARTUP,
drivers/char/tpm/tpm_tis_core.c:        .flags = TPM_OPS_AUTO_STARTUP,
drivers/char/tpm/tpm_tis_i2c_cr50.c:    .flags = TPM_OPS_AUTO_STARTUP,
drivers/char/tpm/tpm_vtpm_proxy.c:      .flags = TPM_OPS_AUTO_STARTUP,

With xen-tpmfront I am not sure where something like chip->flags |= 
TPM_CHIP_FLAG_TPM2 is done -- tpm2-cmd.c::tpm2_probe is not called from 
this driver but only from tpm_tis_core.c::tpm_tis_core_init and 
otherwise driver set it themselves.

$ grep -r TPM_CHIP_FLAG_TPM2 drivers/char/tpm/*.c | grep =
drivers/char/tpm/tpm2-cmd.c:                    chip->flags |= 
TPM_CHIP_FLAG_TPM2;
drivers/char/tpm/tpm-chip.c:    rc = (chip->flags & TPM_CHIP_FLAG_TPM2) ?
drivers/char/tpm/tpm_crb.c:     chip->flags = TPM_CHIP_FLAG_TPM2;
drivers/char/tpm/tpm_ftpm_tee.c:        pvt_data->chip->flags |= 
TPM_CHIP_FLAG_TPM2;
drivers/char/tpm/tpm_i2c_nuvoton.c:             chip->flags |= 
TPM_CHIP_FLAG_TPM2;
drivers/char/tpm/tpm_ibmvtpm.c:         chip->flags |= TPM_CHIP_FLAG_TPM2;
drivers/char/tpm/tpm-interface.c:       rc = (chip->flags & 
TPM_CHIP_FLAG_TPM2) != 0;
drivers/char/tpm/tpm_tis_i2c_cr50.c:    chip->flags |= TPM_CHIP_FLAG_TPM2;
drivers/char/tpm/tpm_vtpm_proxy.c:              proxy_dev->chip->flags 
|= TPM_CHIP_FLAG_TPM2;





> don't, so it would be better to fix this for everyone rather than just
> for you and have to do a separate fix for each of them.

I am not sure whether any one of the mentioned drivers actually need 
this call and if they need it they should probably move towards setting 
TPM_OPS_AUTO_STARTUP.

   Stefan
> 
> James
> 
> 

Re: [PATCH] tpm: ibmvtpm: Call tpm2_sessions_init() to initialize session support

[Michael Ellerman]

James Bottomley <James.Bottomley@HansenPartnership.com> writes:
> On Fri, 2024-06-28 at 10:54 +1000, Michael Ellerman wrote:
>> Stefan Berger <stefanb@linux.ibm.com> writes:
>> > Fix the following type of error message caused by a missing call to
>> > tpm2_sessions_init() in the IBM vTPM driver:
>> > 
>> > [    2.987131] tpm tpm0: tpm2_load_context: failed with a TPM error
>> > 0x01C4
>> > [    2.987140] ima: Error Communicating to TPM chip, result: -14
>> > 
>> > Fixes: d2add27cf2b8 ("tpm: Add NULL primary creation")
>> > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>> > ---
>> >  drivers/char/tpm/tpm_ibmvtpm.c | 4 ++++
>> >  1 file changed, 4 insertions(+)
>> > 
>> > diff --git a/drivers/char/tpm/tpm_ibmvtpm.c
>> > b/drivers/char/tpm/tpm_ibmvtpm.c
>> > index d3989b257f42..1e5b107d1f3b 100644
>> > --- a/drivers/char/tpm/tpm_ibmvtpm.c
>> > +++ b/drivers/char/tpm/tpm_ibmvtpm.c
>> > @@ -698,6 +698,10 @@ static int tpm_ibmvtpm_probe(struct vio_dev
>> > *vio_dev,
>> >                 rc = tpm2_get_cc_attrs_tbl(chip);
>> >                 if (rc)
>> >                         goto init_irq_cleanup;
>> > +
>> > +               rc = tpm2_sessions_init(chip);
>> > +               if (rc)
>> > +                       goto init_irq_cleanup;
>> >         }
>> >  
>> >         return tpm_chip_register(chip);
>> 
>> #regzbot ^introduced: d2add27cf2b8 
>
> Could you please test out the patch I proposed for this:
>
> https://lore.kernel.org/linux-integrity/1302b413a2d7bf3b275133e7fdb04b44bfe2d5e3.camel@HansenPartnership.com/

Your patch does fix the issue on my PowerVM system, as does Stefan's.

cheers

Re: [PATCH] tpm: ibmvtpm: Call tpm2_sessions_init() to initialize session support

[Jarkko Sakkinen]

On Mon, 2024-06-17 at 15:34 -0400, Stefan Berger wrote:
> Fix the following type of error message caused by a missing call to
> tpm2_sessions_init() in the IBM vTPM driver:
> 
> [    2.987131] tpm tpm0: tpm2_load_context: failed with a TPM error 0x01C4
> [    2.987140] ima: Error Communicating to TPM chip, result: -14
> 
> Fixes: d2add27cf2b8 ("tpm: Add NULL primary creation")
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>

This is a bug in the hmac encryption. It should be robust enough
to only be enabled if tpm_sessions_init() was called.

It is fine to enable the feature IBM vTPM driver but definitely
not as a bug fix.

Missed this one in the code review.

BR, Jarkko