Re: [PATCH] tpm: ibmvtpm: Call tpm2_sessions_init() to initialize session support
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-jar...@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
On Wed Jul 3, 2024 at 3:34 AM EEST, Jarkko Sakkinen wrote: > https://lore.kernel.org/linux-integrity/20240703003033.19057-1-jar...@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
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-jar...@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
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
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
James Bottomley writes:
> On Fri, 2024-06-28 at 10:54 +1000, Michael Ellerman wrote:
>> Stefan Berger 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
>> > ---
>> > 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.ca...@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
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-jar...@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
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 :00:02.0: xHCI Host Controller [2.496183] xhci_hcd :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-jar...@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
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
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
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
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
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
Re: [PATCH] tpm: ibmvtpm: Call tpm2_sessions_init() to initialize session support
On 6/28/24 12:39, James Bottomley wrote:
On Fri, 2024-06-28 at 10:54 +1000, Michael Ellerman wrote:
Stefan Berger 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
---
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.ca...@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
On Fri, 2024-06-28 at 10:54 +1000, Michael Ellerman wrote:
> Stefan Berger 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
> > ---
> > 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.ca...@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
[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
>> ---
>> 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
Stefan Berger 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
> ---
> 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
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
---
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
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
---
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
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
---
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
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
> > > ---
> > > 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
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
---
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
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
> ---
> 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
[PATCH] tpm: ibmvtpm: Call tpm2_sessions_init() to initialize session support
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
---
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
