Message ID | 20200204132706.3220416-4-stefanb@linux.vnet.ibm.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Enable vTPM 2.0 for the IBM vTPM driver | expand |
On 2/4/20 8:27 AM, Stefan Berger wrote: > From: Stefan Berger <stefanb@linux.ibm.com> > > Support TPM 2 in the IBM vTPM driver. The hypervisor tells us what > version of TPM is connected through the vio_device_id. > > In case a TPM 2 is found, we set the TPM_OPS_AUTO_STARTUP flag to > have properly initialize the TPM and driver. > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > --- > drivers/char/tpm/tpm_ibmvtpm.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c > index eee566eddb35..d479d64a65aa 100644 > --- a/drivers/char/tpm/tpm_ibmvtpm.c > +++ b/drivers/char/tpm/tpm_ibmvtpm.c > @@ -29,6 +29,7 @@ static const char tpm_ibmvtpm_driver_name[] = "tpm_ibmvtpm"; > > static const struct vio_device_id tpm_ibmvtpm_device_table[] = { > { "IBM,vtpm", "IBM,vtpm"}, > + { "IBM,vtpm", "IBM,vtpm20"}, > { "", "" } > }; > MODULE_DEVICE_TABLE(vio, tpm_ibmvtpm_device_table); > @@ -443,7 +444,7 @@ static bool tpm_ibmvtpm_req_canceled(struct tpm_chip *chip, u8 status) > return (status == 0); > } > > -static const struct tpm_class_ops tpm_ibmvtpm = { > +static struct tpm_class_ops tpm_ibmvtpm = { > .recv = tpm_ibmvtpm_recv, > .send = tpm_ibmvtpm_send, > .cancel = tpm_ibmvtpm_cancel, > @@ -672,6 +673,11 @@ static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev, > if (rc) > goto init_irq_cleanup; > > + if (!strcmp(id->compat, "IBM,vtpm20")) { > + chip->flags |= TPM_CHIP_FLAG_TPM2; > + tpm_ibmvtpm.flags = TPM_OPS_AUTO_STARTUP; TPM_OPS_AUTO_STARTUP flag isn't set for vTPM 1.2. What is different in case of vTPM 2.0 ? Thanks & Regards, - Nayna
On 2/13/20 12:53 PM, Nayna wrote: > > On 2/4/20 8:27 AM, Stefan Berger wrote: >> From: Stefan Berger <stefanb@linux.ibm.com> >> >> Support TPM 2 in the IBM vTPM driver. The hypervisor tells us what >> version of TPM is connected through the vio_device_id. >> >> In case a TPM 2 is found, we set the TPM_OPS_AUTO_STARTUP flag to >> have properly initialize the TPM and driver. >> >> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> >> --- >> drivers/char/tpm/tpm_ibmvtpm.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/char/tpm/tpm_ibmvtpm.c >> b/drivers/char/tpm/tpm_ibmvtpm.c >> index eee566eddb35..d479d64a65aa 100644 >> --- a/drivers/char/tpm/tpm_ibmvtpm.c >> +++ b/drivers/char/tpm/tpm_ibmvtpm.c >> @@ -29,6 +29,7 @@ static const char tpm_ibmvtpm_driver_name[] = >> "tpm_ibmvtpm"; >> >> static const struct vio_device_id tpm_ibmvtpm_device_table[] = { >> { "IBM,vtpm", "IBM,vtpm"}, >> + { "IBM,vtpm", "IBM,vtpm20"}, >> { "", "" } >> }; >> MODULE_DEVICE_TABLE(vio, tpm_ibmvtpm_device_table); >> @@ -443,7 +444,7 @@ static bool tpm_ibmvtpm_req_canceled(struct >> tpm_chip *chip, u8 status) >> return (status == 0); >> } >> >> -static const struct tpm_class_ops tpm_ibmvtpm = { >> +static struct tpm_class_ops tpm_ibmvtpm = { >> .recv = tpm_ibmvtpm_recv, >> .send = tpm_ibmvtpm_send, >> .cancel = tpm_ibmvtpm_cancel, >> @@ -672,6 +673,11 @@ static int tpm_ibmvtpm_probe(struct vio_dev >> *vio_dev, >> if (rc) >> goto init_irq_cleanup; >> >> + if (!strcmp(id->compat, "IBM,vtpm20")) { >> + chip->flags |= TPM_CHIP_FLAG_TPM2; >> + tpm_ibmvtpm.flags = TPM_OPS_AUTO_STARTUP; > > TPM_OPS_AUTO_STARTUP flag isn't set for vTPM 1.2. What is different in > case of vTPM 2.0 ? I don't want side effects for the TPM 1.2 case here, so I am only modifying the flag for the case where the new TPM 2 is being used. Here's the code where it shows the effect. int tpm_auto_startup(struct tpm_chip *chip) { int rc; if (!(chip->ops->flags & TPM_OPS_AUTO_STARTUP)) return 0; if (chip->flags & TPM_CHIP_FLAG_TPM2) rc = tpm2_auto_startup(chip); else rc = tpm1_auto_startup(chip); return rc; } In the TPM 2 case we then get timeouts, do the TPM self test, send TPM2_STARTUP if necessary and get attributes of the TPM 2 command from the device. All necessary to start it up. https://elixir.bootlin.com/linux/latest/source/drivers/char/tpm/tpm2-cmd.c#L719 Does this answer your question ? Stefan > > Thanks & Regards, > > - Nayna >
On Thu, Feb 13, 2020 at 01:20:12PM -0500, Stefan Berger wrote: > I don't want side effects for the TPM 1.2 case here, so I am only modifying > the flag for the case where the new TPM 2 is being used. Here's the code > where it shows the effect. I'm surprised this driver is using AUTO_STARTUP, it was intended for embedded cases where their is no firmware to boot the TPM. Chips using AUTO_STARTUP are basically useless for PCRs/etc. I'd expect somthing called vtpm to have been started and PCRs working before Linux is started?? Jason
On 2/13/20 1:35 PM, Jason Gunthorpe wrote: > On Thu, Feb 13, 2020 at 01:20:12PM -0500, Stefan Berger wrote: > >> I don't want side effects for the TPM 1.2 case here, so I am only modifying >> the flag for the case where the new TPM 2 is being used. Here's the code >> where it shows the effect. > I'm surprised this driver is using AUTO_STARTUP, it was intended for > embedded cases where their is no firmware to boot the TPM. The TIS is also using it on any device. static const struct tpm_class_ops tpm_tis = { .flags = TPM_OPS_AUTO_STARTUP, .status = tpm_tis_status, https://elixir.bootlin.com/linux/latest/source/drivers/char/tpm/tpm_tis_core.c#L917 > > Chips using AUTO_STARTUP are basically useless for PCRs/etc. > > I'd expect somthing called vtpm to have been started and PCRs working > before Linux is started?? Yes, there's supposed to be firmware. I only see one caller to tpm2_get_cc_attrs_tbl(chip), which is necessary to call. This caller happens to be in tpm2_auto_startup. Stefan > > Jason
On Thu, Feb 13, 2020 at 02:04:12PM -0500, Stefan Berger wrote: > On 2/13/20 1:35 PM, Jason Gunthorpe wrote: > > On Thu, Feb 13, 2020 at 01:20:12PM -0500, Stefan Berger wrote: > > > > > I don't want side effects for the TPM 1.2 case here, so I am only modifying > > > the flag for the case where the new TPM 2 is being used. Here's the code > > > where it shows the effect. > > I'm surprised this driver is using AUTO_STARTUP, it was intended for > > embedded cases where their is no firmware to boot the TPM. > > The TIS is also using it on any device. TIS is a generic driver, and can run on TPMs without firmware support. It doesn't know either way > > Chips using AUTO_STARTUP are basically useless for PCRs/etc. > > > > I'd expect somthing called vtpm to have been started and PCRs working > > before Linux is started?? > > Yes, there's supposed to be firmware. > > I only see one caller to tpm2_get_cc_attrs_tbl(chip), which is necessary to > call. This caller happens to be in tpm2_auto_startup. That seems to be a mistake, proper startup of the driver should never require auto_startup. Jason
On 2/13/20 2:11 PM, Jason Gunthorpe wrote: > On Thu, Feb 13, 2020 at 02:04:12PM -0500, Stefan Berger wrote: >> On 2/13/20 1:35 PM, Jason Gunthorpe wrote: >>> On Thu, Feb 13, 2020 at 01:20:12PM -0500, Stefan Berger wrote: >>> >>>> I don't want side effects for the TPM 1.2 case here, so I am only modifying >>>> the flag for the case where the new TPM 2 is being used. Here's the code >>>> where it shows the effect. >>> I'm surprised this driver is using AUTO_STARTUP, it was intended for >>> embedded cases where their is no firmware to boot the TPM. >> The TIS is also using it on any device. > TIS is a generic driver, and can run on TPMs without firmware > support. It doesn't know either way The following drivers are all using it: drivers/char/tpm/st33zp24/st33zp24.c, line 493 drivers/char/tpm/tpm-interface.c, line 374 drivers/char/tpm/tpm_crb.c, line 421 drivers/char/tpm/tpm_ftpm_tee.c, line 184 drivers/char/tpm/tpm_i2c_atmel.c, line 139 drivers/char/tpm/tpm_i2c_infineon.c, line 602 drivers/char/tpm/tpm_i2c_nuvoton.c, line 465 drivers/char/tpm/tpm_tis_core.c, line 917 drivers/char/tpm/tpm_vtpm_proxy.c, line 435 https://elixir.bootlin.com/linux/latest/ident/TPM_OPS_AUTO_STARTUP > >>> Chips using AUTO_STARTUP are basically useless for PCRs/etc. >>> >>> I'd expect somthing called vtpm to have been started and PCRs working >>> before Linux is started?? >> Yes, there's supposed to be firmware. >> >> I only see one caller to tpm2_get_cc_attrs_tbl(chip), which is necessary to >> call. This caller happens to be in tpm2_auto_startup. > That seems to be a mistake, proper startup of the driver should never > require auto_startup. Is this IBM vTPM driver special that it should do things differently than all those drivers listed above? From looking at the code is seems it is to be set for the TPM 2.0 case. Stefan
On Thu, Feb 13, 2020 at 02:15:03PM -0500, Stefan Berger wrote: > On 2/13/20 2:11 PM, Jason Gunthorpe wrote: > > On Thu, Feb 13, 2020 at 02:04:12PM -0500, Stefan Berger wrote: > > > On 2/13/20 1:35 PM, Jason Gunthorpe wrote: > > > > On Thu, Feb 13, 2020 at 01:20:12PM -0500, Stefan Berger wrote: > > > > > > > > > I don't want side effects for the TPM 1.2 case here, so I am only modifying > > > > > the flag for the case where the new TPM 2 is being used. Here's the code > > > > > where it shows the effect. > > > > I'm surprised this driver is using AUTO_STARTUP, it was intended for > > > > embedded cases where their is no firmware to boot the TPM. > > > The TIS is also using it on any device. > > TIS is a generic driver, and can run on TPMs without firmware > > support. It doesn't know either way > > The following drivers are all using it: > > > drivers/char/tpm/st33zp24/st33zp24.c, line 493 > drivers/char/tpm/tpm-interface.c, line 374 > drivers/char/tpm/tpm_crb.c, line 421 > drivers/char/tpm/tpm_ftpm_tee.c, line 184 > drivers/char/tpm/tpm_i2c_atmel.c, line 139 > drivers/char/tpm/tpm_i2c_infineon.c, line 602 > drivers/char/tpm/tpm_i2c_nuvoton.c, line 465 > drivers/char/tpm/tpm_tis_core.c, line 917 > drivers/char/tpm/tpm_vtpm_proxy.c, line 435 > > https://elixir.bootlin.com/linux/latest/ident/TPM_OPS_AUTO_STARTUP These are all general purpose drivers. Though perhaps vtpm_proxy shouldn't include it, not sure. > > > > Chips using AUTO_STARTUP are basically useless for PCRs/etc. > > > > > > > > I'd expect somthing called vtpm to have been started and PCRs working > > > > before Linux is started?? > > > Yes, there's supposed to be firmware. > > > > > > I only see one caller to tpm2_get_cc_attrs_tbl(chip), which is necessary to > > > call. This caller happens to be in tpm2_auto_startup. > > That seems to be a mistake, proper startup of the driver should never > > require auto_startup. > > Is this IBM vTPM driver special that it should do things differently than > all those drivers listed above? From looking at the code is seems it is to > be set for the TPM 2.0 case. Any driver that knows the TPM must be started prior to Linux booting should not use the flag. vtpm drivers in general would seem to be the case where we can make this statement. If it was mandatory then it would not be a flag the driver has to specify. Jason
On 2/13/20 2:39 PM, Jason Gunthorpe wrote: > On Thu, Feb 13, 2020 at 02:15:03PM -0500, Stefan Berger wrote: >> On 2/13/20 2:11 PM, Jason Gunthorpe wrote: >>> On Thu, Feb 13, 2020 at 02:04:12PM -0500, Stefan Berger wrote: >>>> On 2/13/20 1:35 PM, Jason Gunthorpe wrote: >>>>> On Thu, Feb 13, 2020 at 01:20:12PM -0500, Stefan Berger wrote: >>>>> >>>>>> I don't want side effects for the TPM 1.2 case here, so I am only modifying >>>>>> the flag for the case where the new TPM 2 is being used. Here's the code >>>>>> where it shows the effect. >>>>> I'm surprised this driver is using AUTO_STARTUP, it was intended for >>>>> embedded cases where their is no firmware to boot the TPM. >>>> The TIS is also using it on any device. >>> TIS is a generic driver, and can run on TPMs without firmware >>> support. It doesn't know either way >> The following drivers are all using it: >> >> >> drivers/char/tpm/st33zp24/st33zp24.c, line 493 >> drivers/char/tpm/tpm-interface.c, line 374 >> drivers/char/tpm/tpm_crb.c, line 421 >> drivers/char/tpm/tpm_ftpm_tee.c, line 184 >> drivers/char/tpm/tpm_i2c_atmel.c, line 139 >> drivers/char/tpm/tpm_i2c_infineon.c, line 602 >> drivers/char/tpm/tpm_i2c_nuvoton.c, line 465 >> drivers/char/tpm/tpm_tis_core.c, line 917 >> drivers/char/tpm/tpm_vtpm_proxy.c, line 435 >> >> https://elixir.bootlin.com/linux/latest/ident/TPM_OPS_AUTO_STARTUP > These are all general purpose drivers. > > Though perhaps vtpm_proxy shouldn't include it, not sure. > >>>>> Chips using AUTO_STARTUP are basically useless for PCRs/etc. >>>>> >>>>> I'd expect somthing called vtpm to have been started and PCRs working >>>>> before Linux is started?? >>>> Yes, there's supposed to be firmware. >>>> >>>> I only see one caller to tpm2_get_cc_attrs_tbl(chip), which is necessary to >>>> call. This caller happens to be in tpm2_auto_startup. >>> That seems to be a mistake, proper startup of the driver should never >>> require auto_startup. >> Is this IBM vTPM driver special that it should do things differently than >> all those drivers listed above? From looking at the code is seems it is to >> be set for the TPM 2.0 case. > Any driver that knows the TPM must be started prior to Linux > booting should not use the flag. vtpm drivers in general would seem > to be the case where we can make this statement. Wouldn't this statement apply to all systems, including embedded ones? Basically all firmwares should implement the CRTM and do the TPM initialization. > > If it was mandatory then it would not be a flag the driver has to specify. I'll add a case where we call into tpm2_get_cc_attrs_tbl(chip) in case the auto startup flag is not set. I believe this is the only part that's missing for TPM 2 to work if not autostarted. Stefan > > Jason
On Thu, Feb 13, 2020 at 02:45:49PM -0500, Stefan Berger wrote: > > Any driver that knows the TPM must be started prior to Linux > > booting should not use the flag. vtpm drivers in general would seem > > to be the case where we can make this statement. > > Wouldn't this statement apply to all systems, including embedded ones? > Basically all firmwares should implement the CRTM and do the TPM > initialization. It is not mandatory that systems with TPMs start it in the firmware. That is only required if the TPM PCR feature is going to be used. A TPM can quite happily be used for key storage without FW support. Arguably this is sort of done wrong. eg if the platform has provided TPM information through uEFI or something then we shouldn't try to auto start the system TPM. At least for TPM1 detecting a non-started TPM was harmless, so nobody really cared. I wonder if TPM2 is much different.. But certainly auto startup should *not* be required to have a working TPM driver, that is just fundamentally wrong. Jason
diff --git a/drivers/char/tpm/tpm_ibmvtpm.c b/drivers/char/tpm/tpm_ibmvtpm.c index eee566eddb35..d479d64a65aa 100644 --- a/drivers/char/tpm/tpm_ibmvtpm.c +++ b/drivers/char/tpm/tpm_ibmvtpm.c @@ -29,6 +29,7 @@ static const char tpm_ibmvtpm_driver_name[] = "tpm_ibmvtpm"; static const struct vio_device_id tpm_ibmvtpm_device_table[] = { { "IBM,vtpm", "IBM,vtpm"}, + { "IBM,vtpm", "IBM,vtpm20"}, { "", "" } }; MODULE_DEVICE_TABLE(vio, tpm_ibmvtpm_device_table); @@ -443,7 +444,7 @@ static bool tpm_ibmvtpm_req_canceled(struct tpm_chip *chip, u8 status) return (status == 0); } -static const struct tpm_class_ops tpm_ibmvtpm = { +static struct tpm_class_ops tpm_ibmvtpm = { .recv = tpm_ibmvtpm_recv, .send = tpm_ibmvtpm_send, .cancel = tpm_ibmvtpm_cancel, @@ -672,6 +673,11 @@ static int tpm_ibmvtpm_probe(struct vio_dev *vio_dev, if (rc) goto init_irq_cleanup; + if (!strcmp(id->compat, "IBM,vtpm20")) { + chip->flags |= TPM_CHIP_FLAG_TPM2; + tpm_ibmvtpm.flags = TPM_OPS_AUTO_STARTUP; + } + if (!wait_event_timeout(ibmvtpm->crq_queue.wq, ibmvtpm->rtce_buf != NULL, HZ)) {