| Message ID | 20181214132115.26223-2-aklimov@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [REVIEW,1/2] tpm: provide a way to override the chip returned durations | expand |
On Fri, Dec 14, 2018 at 01:21:15PM +0000, Alexey Klimov wrote: > There was revealed a bug in the STM TPM chipset used in Dell R415s. > Bug is observed so far only on chipset firmware 1.2.8.28 > (1.2 TPM, device-id 0x0, rev-id 78). After some number of > operations chipset hangs and stays in inconsistent state: > > tpm_tis 00:09: Operation Timed out > tpm_tis 00:09: tpm_transmit: tpm_send: error -5 > > Durations returned by the chip are the same like on other > firmware revisions but apparently with specifically 1.2.8.28 fw > durations should be reset to 2 minutes to enable tpm chip work > properly. No working way of updating firmware was found. > > This patch adds implementation of ->update_durations method > that matches only STM devices with specific firmware version. > > Cc: Jerry Snitselaar <jsnitsel@redhat.com> > Signed-off-by: Alexey Klimov <aklimov@redhat.com> > --- > drivers/char/tpm/tpm_tis_core.c | 90 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 90 insertions(+) > > diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c > index d2345d9fd7b5..e0bdca647460 100644 > --- a/drivers/char/tpm/tpm_tis_core.c > +++ b/drivers/char/tpm/tpm_tis_core.c > @@ -514,6 +514,95 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len) > return rc; > } > > +struct tis_vendor_durations_override { > + u32 did_vid; > + struct tpm_version_t tpm_version; > + unsigned long durs[3]; I would rather have just "unsigned long durations[3];". > +}; > + > +static > +const struct tis_vendor_durations_override vendor_dur_overrides[] = { > + /* STMicroelectronics 0x104a */ > + { 0x0000104A, > + { 1, 2, 8, 28 }, > + { (2 * 60 * HZ), (2 * 60 * HZ), (2 * 60 * HZ) } }, > +}; > + > +static bool tpm_tis_update_durations(struct tpm_chip *chip, > + unsigned long *duration_cap) > +{ > + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); > + u32 did_vid; > + int i, rc; > + cap_t cap; > + > + if (chip->ops->clk_enable != NULL) > + chip->ops->clk_enable(chip, true); > + > + rc = tpm_tis_read32(priv, TPM_DID_VID(0), &did_vid); > + if (rc < 0) > + goto out; > + > + for (i = 0; i != ARRAY_SIZE(vendor_dur_overrides); i++) { > + if (vendor_dur_overrides[i].did_vid != did_vid) > + continue; > + > + /* Try to get a TPM version 1.2 TPM_CAP_VERSION_INFO */ > + rc = tpm_getcap(chip, TPM_CAP_VERSION_1_2, &cap, > + "attempting to determine the 1.2 version", > + sizeof(cap.tpm_version_1_2)); Not properly aligned. > + if (!rc) { > + if ((cap.tpm_version_1_2.Major == > + vendor_dur_overrides[i].tpm_version.Major) && > + (cap.tpm_version_1_2.Minor == > + vendor_dur_overrides[i].tpm_version.Minor) && > + (cap.tpm_version_1_2.revMajor == > + vendor_dur_overrides[i].tpm_version.revMajor) && > + (cap.tpm_version_1_2.revMinor == > + vendor_dur_overrides[i].tpm_version.revMinor)) { Same. > + > + memcpy(duration_cap, > + vendor_dur_overrides[i].durs, > + sizeof(vendor_dur_overrides[i].durs)); Same. > + rc = true; > + goto out; > + } > + } else { > + rc = tpm_getcap(chip, TPM_CAP_VERSION_1_1, &cap, > + "attempting to determine the 1.1 version", > + sizeof(cap.tpm_version)); Same. > + if (rc) { > + rc = false; > + goto out; > + } > + if ((cap.tpm_version.Major == > + vendor_dur_overrides[i].tpm_version.Major) && > + (cap.tpm_version.Minor == > + vendor_dur_overrides[i].tpm_version.Minor) && > + (cap.tpm_version.revMajor == > + vendor_dur_overrides[i].tpm_version.revMajor) && > + (cap.tpm_version.revMinor == > + vendor_dur_overrides[i].tpm_version.revMinor)) { Same. > + > + memcpy(duration_cap, > + vendor_dur_overrides[i].durs, > + sizeof(vendor_dur_overrides[i].durs)); Same. > + rc = true; > + goto out; > + } > + } > + } > + > + rc = false; Shoud return proper rc instead of bool. > + > +out: > + if (chip->ops->clk_enable != NULL) > + chip->ops->clk_enable(chip, false); > + > + return rc; > +} > + > + > struct tis_vendor_timeout_override { > u32 did_vid; > unsigned long timeout_us[4]; > @@ -847,6 +936,7 @@ static const struct tpm_class_ops tpm_tis = { > .send = tpm_tis_send, > .cancel = tpm_tis_ready, > .update_timeouts = tpm_tis_update_timeouts, > + .update_durations = tpm_tis_update_durations, > .req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID, > .req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID, > .req_canceled = tpm_tis_req_canceled, > -- > 2.14.4 > /Jarkko
On Thu Jan 03 19, Jarkko Sakkinen wrote: >On Fri, Dec 14, 2018 at 01:21:15PM +0000, Alexey Klimov wrote: >> There was revealed a bug in the STM TPM chipset used in Dell R415s. >> Bug is observed so far only on chipset firmware 1.2.8.28 >> (1.2 TPM, device-id 0x0, rev-id 78). After some number of >> operations chipset hangs and stays in inconsistent state: >> >> tpm_tis 00:09: Operation Timed out >> tpm_tis 00:09: tpm_transmit: tpm_send: error -5 >> >> Durations returned by the chip are the same like on other >> firmware revisions but apparently with specifically 1.2.8.28 fw >> durations should be reset to 2 minutes to enable tpm chip work >> properly. No working way of updating firmware was found. >> >> This patch adds implementation of ->update_durations method >> that matches only STM devices with specific firmware version. >> >> Cc: Jerry Snitselaar <jsnitsel@redhat.com> >> Signed-off-by: Alexey Klimov <aklimov@redhat.com> >> --- >> drivers/char/tpm/tpm_tis_core.c | 90 +++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 90 insertions(+) >> >> diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c >> index d2345d9fd7b5..e0bdca647460 100644 >> --- a/drivers/char/tpm/tpm_tis_core.c >> +++ b/drivers/char/tpm/tpm_tis_core.c >> @@ -514,6 +514,95 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len) >> return rc; >> } >> >> +struct tis_vendor_durations_override { >> + u32 did_vid; >> + struct tpm_version_t tpm_version; >> + unsigned long durs[3]; > >I would rather have just "unsigned long durations[3];". > >> +}; >> + >> +static >> +const struct tis_vendor_durations_override vendor_dur_overrides[] = { >> + /* STMicroelectronics 0x104a */ >> + { 0x0000104A, >> + { 1, 2, 8, 28 }, >> + { (2 * 60 * HZ), (2 * 60 * HZ), (2 * 60 * HZ) } }, >> +}; >> + >> +static bool tpm_tis_update_durations(struct tpm_chip *chip, >> + unsigned long *duration_cap) >> +{ >> + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); >> + u32 did_vid; >> + int i, rc; >> + cap_t cap; >> + >> + if (chip->ops->clk_enable != NULL) >> + chip->ops->clk_enable(chip, true); >> + >> + rc = tpm_tis_read32(priv, TPM_DID_VID(0), &did_vid); >> + if (rc < 0) >> + goto out; >> + >> + for (i = 0; i != ARRAY_SIZE(vendor_dur_overrides); i++) { >> + if (vendor_dur_overrides[i].did_vid != did_vid) >> + continue; >> + >> + /* Try to get a TPM version 1.2 TPM_CAP_VERSION_INFO */ >> + rc = tpm_getcap(chip, TPM_CAP_VERSION_1_2, &cap, >> + "attempting to determine the 1.2 version", >> + sizeof(cap.tpm_version_1_2)); > >Not properly aligned. > >> + if (!rc) { >> + if ((cap.tpm_version_1_2.Major == >> + vendor_dur_overrides[i].tpm_version.Major) && >> + (cap.tpm_version_1_2.Minor == >> + vendor_dur_overrides[i].tpm_version.Minor) && >> + (cap.tpm_version_1_2.revMajor == >> + vendor_dur_overrides[i].tpm_version.revMajor) && >> + (cap.tpm_version_1_2.revMinor == >> + vendor_dur_overrides[i].tpm_version.revMinor)) { > >Same. > >> + >> + memcpy(duration_cap, >> + vendor_dur_overrides[i].durs, >> + sizeof(vendor_dur_overrides[i].durs)); > >Same. > >> + rc = true; >> + goto out; >> + } >> + } else { >> + rc = tpm_getcap(chip, TPM_CAP_VERSION_1_1, &cap, >> + "attempting to determine the 1.1 version", >> + sizeof(cap.tpm_version)); > >Same. > >> + if (rc) { >> + rc = false; >> + goto out; >> + } >> + if ((cap.tpm_version.Major == >> + vendor_dur_overrides[i].tpm_version.Major) && >> + (cap.tpm_version.Minor == >> + vendor_dur_overrides[i].tpm_version.Minor) && >> + (cap.tpm_version.revMajor == >> + vendor_dur_overrides[i].tpm_version.revMajor) && >> + (cap.tpm_version.revMinor == >> + vendor_dur_overrides[i].tpm_version.revMinor)) { > >Same. > >> + >> + memcpy(duration_cap, >> + vendor_dur_overrides[i].durs, >> + sizeof(vendor_dur_overrides[i].durs)); > >Same. > >> + rc = true; >> + goto out; >> + } >> + } >> + } >> + >> + rc = false; > >Shoud return proper rc instead of bool. > Alexey was following the example of tpm_tis_update_timeouts() which returns true if the timeouts were updated, and otherwise returns false. The bool here makes sense to me, but what rc would you suggest in this case? Regards, Jerry >> + >> +out: >> + if (chip->ops->clk_enable != NULL) >> + chip->ops->clk_enable(chip, false); >> + >> + return rc; >> +} >> + >> + >> struct tis_vendor_timeout_override { >> u32 did_vid; >> unsigned long timeout_us[4]; >> @@ -847,6 +936,7 @@ static const struct tpm_class_ops tpm_tis = { >> .send = tpm_tis_send, >> .cancel = tpm_tis_ready, >> .update_timeouts = tpm_tis_update_timeouts, >> + .update_durations = tpm_tis_update_durations, >> .req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID, >> .req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID, >> .req_canceled = tpm_tis_req_canceled, >> -- >> 2.14.4 >> > >/Jarkko
On Mon, Jan 14, 2019 at 12:39:40PM -0700, Jerry Snitselaar wrote: > Alexey was following the example of tpm_tis_update_timeouts() which > returns true if the timeouts were updated, and otherwise returns > false. The bool here makes sense to me, but what rc would you suggest > in this case? Maybe the pattern used there is not that great then. The callback should simply be update_durations(chip), and it would do whatever updates needed and either return zero or -errno. And of course update durations_adjusted flag because that is needed in sysfs. /Jarkko
On Fri Jan 18 19, Jarkko Sakkinen wrote: >On Mon, Jan 14, 2019 at 12:39:40PM -0700, Jerry Snitselaar wrote: >> Alexey was following the example of tpm_tis_update_timeouts() which >> returns true if the timeouts were updated, and otherwise returns >> false. The bool here makes sense to me, but what rc would you suggest >> in this case? > >Maybe the pattern used there is not that great then. > >The callback should simply be update_durations(chip), and it would do >whatever updates needed and either return zero or -errno. And of course >update durations_adjusted flag because that is needed in sysfs. > >/Jarkko Taking a quick look, they already track whether the adjustment occurred in the tpm_chip struct, so that could be used instead for what the bool return was being used for. I'll post a patch for the timeout updates code, and work with Alexey to rework his patchset. Regards, Jerry
On Sun, Jan 20, 2019 at 02:30:12PM -0700, Jerry Snitselaar wrote: > On Fri Jan 18 19, Jarkko Sakkinen wrote: > > On Mon, Jan 14, 2019 at 12:39:40PM -0700, Jerry Snitselaar wrote: > > > Alexey was following the example of tpm_tis_update_timeouts() which > > > returns true if the timeouts were updated, and otherwise returns > > > false. The bool here makes sense to me, but what rc would you suggest > > > in this case? > > > > Maybe the pattern used there is not that great then. > > > > The callback should simply be update_durations(chip), and it would do > > whatever updates needed and either return zero or -errno. And of course > > update durations_adjusted flag because that is needed in sysfs. > > > > /Jarkko > > Taking a quick look, they already track whether the adjustment > occurred in the tpm_chip struct, so that could be used instead for > what the bool return was being used for. I'll post a patch for the > timeout updates code, and work with Alexey to rework his patchset. Awesome, thank you! /Jarkko
diff --git a/drivers/char/tpm/tpm_tis_core.c b/drivers/char/tpm/tpm_tis_core.c index d2345d9fd7b5..e0bdca647460 100644 --- a/drivers/char/tpm/tpm_tis_core.c +++ b/drivers/char/tpm/tpm_tis_core.c @@ -514,6 +514,95 @@ static int tpm_tis_send(struct tpm_chip *chip, u8 *buf, size_t len) return rc; } +struct tis_vendor_durations_override { + u32 did_vid; + struct tpm_version_t tpm_version; + unsigned long durs[3]; +}; + +static +const struct tis_vendor_durations_override vendor_dur_overrides[] = { + /* STMicroelectronics 0x104a */ + { 0x0000104A, + { 1, 2, 8, 28 }, + { (2 * 60 * HZ), (2 * 60 * HZ), (2 * 60 * HZ) } }, +}; + +static bool tpm_tis_update_durations(struct tpm_chip *chip, + unsigned long *duration_cap) +{ + struct tpm_tis_data *priv = dev_get_drvdata(&chip->dev); + u32 did_vid; + int i, rc; + cap_t cap; + + if (chip->ops->clk_enable != NULL) + chip->ops->clk_enable(chip, true); + + rc = tpm_tis_read32(priv, TPM_DID_VID(0), &did_vid); + if (rc < 0) + goto out; + + for (i = 0; i != ARRAY_SIZE(vendor_dur_overrides); i++) { + if (vendor_dur_overrides[i].did_vid != did_vid) + continue; + + /* Try to get a TPM version 1.2 TPM_CAP_VERSION_INFO */ + rc = tpm_getcap(chip, TPM_CAP_VERSION_1_2, &cap, + "attempting to determine the 1.2 version", + sizeof(cap.tpm_version_1_2)); + if (!rc) { + if ((cap.tpm_version_1_2.Major == + vendor_dur_overrides[i].tpm_version.Major) && + (cap.tpm_version_1_2.Minor == + vendor_dur_overrides[i].tpm_version.Minor) && + (cap.tpm_version_1_2.revMajor == + vendor_dur_overrides[i].tpm_version.revMajor) && + (cap.tpm_version_1_2.revMinor == + vendor_dur_overrides[i].tpm_version.revMinor)) { + + memcpy(duration_cap, + vendor_dur_overrides[i].durs, + sizeof(vendor_dur_overrides[i].durs)); + rc = true; + goto out; + } + } else { + rc = tpm_getcap(chip, TPM_CAP_VERSION_1_1, &cap, + "attempting to determine the 1.1 version", + sizeof(cap.tpm_version)); + if (rc) { + rc = false; + goto out; + } + if ((cap.tpm_version.Major == + vendor_dur_overrides[i].tpm_version.Major) && + (cap.tpm_version.Minor == + vendor_dur_overrides[i].tpm_version.Minor) && + (cap.tpm_version.revMajor == + vendor_dur_overrides[i].tpm_version.revMajor) && + (cap.tpm_version.revMinor == + vendor_dur_overrides[i].tpm_version.revMinor)) { + + memcpy(duration_cap, + vendor_dur_overrides[i].durs, + sizeof(vendor_dur_overrides[i].durs)); + rc = true; + goto out; + } + } + } + + rc = false; + +out: + if (chip->ops->clk_enable != NULL) + chip->ops->clk_enable(chip, false); + + return rc; +} + + struct tis_vendor_timeout_override { u32 did_vid; unsigned long timeout_us[4]; @@ -847,6 +936,7 @@ static const struct tpm_class_ops tpm_tis = { .send = tpm_tis_send, .cancel = tpm_tis_ready, .update_timeouts = tpm_tis_update_timeouts, + .update_durations = tpm_tis_update_durations, .req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID, .req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID, .req_canceled = tpm_tis_req_canceled,
There was revealed a bug in the STM TPM chipset used in Dell R415s. Bug is observed so far only on chipset firmware 1.2.8.28 (1.2 TPM, device-id 0x0, rev-id 78). After some number of operations chipset hangs and stays in inconsistent state: tpm_tis 00:09: Operation Timed out tpm_tis 00:09: tpm_transmit: tpm_send: error -5 Durations returned by the chip are the same like on other firmware revisions but apparently with specifically 1.2.8.28 fw durations should be reset to 2 minutes to enable tpm chip work properly. No working way of updating firmware was found. This patch adds implementation of ->update_durations method that matches only STM devices with specific firmware version. Cc: Jerry Snitselaar <jsnitsel@redhat.com> Signed-off-by: Alexey Klimov <aklimov@redhat.com> --- drivers/char/tpm/tpm_tis_core.c | 90 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 90 insertions(+)