| Message ID | 20240617193408.1234365-1-stefanb@linux.ibm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tpm: ibmvtpm: Call tpm2_sessions_init() to initialize session support | expand |
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
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 >
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
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 >
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);
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
[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);
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
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 > >
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
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
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
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 >
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
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
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
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
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
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
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
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
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);
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(+)