Re: [PATCH REVIEW 2/2] tpm_tis: override durations for STM tpm with firmware 1.2.8.28

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
Signed-off-by: Alexey Klimov <aklimov@xxxxxxxxxx>
---
 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



[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Linux Kernel]     [Linux Kernel Hardening]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux SCSI]

  Powered by Linux