From: Jerry Snitselaar <jsnitsel@redhat.com>
To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: Alexey Klimov <aklimov@redhat.com>,
linux-integrity@vger.kernel.org, peterhuewe@gmx.de, jgg@ziepe.ca
Subject: Re: [PATCH REVIEW 2/2] tpm_tis: override durations for STM tpm with firmware 1.2.8.28
Date: Mon, 14 Jan 2019 12:39:40 -0700 [thread overview]
Message-ID: <20190114193940.mylziev4xhidmazs@cantor> (raw)
In-Reply-To: <20190103131453.GC10491@linux.intel.com>
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
next prev parent reply other threads:[~2019-01-14 19:39 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-14 13:21 [PATCH REVIEW 1/2] tpm: provide a way to override the chip returned durations Alexey Klimov
2018-12-14 13:21 ` [PATCH REVIEW 2/2] tpm_tis: override durations for STM tpm with firmware 1.2.8.28 Alexey Klimov
2019-01-03 13:14 ` Jarkko Sakkinen
2019-01-14 19:39 ` Jerry Snitselaar [this message]
2019-01-18 14:59 ` Jarkko Sakkinen
2019-01-20 21:30 ` Jerry Snitselaar
2019-01-21 12:29 ` Jarkko Sakkinen
2019-01-03 13:05 ` [PATCH REVIEW 1/2] tpm: provide a way to override the chip returned durations Jarkko Sakkinen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190114193940.mylziev4xhidmazs@cantor \
--to=jsnitsel@redhat.com \
--cc=aklimov@redhat.com \
--cc=jarkko.sakkinen@linux.intel.com \
--cc=jgg@ziepe.ca \
--cc=linux-integrity@vger.kernel.org \
--cc=peterhuewe@gmx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).