From: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
To: Ed Swierk <eswierk@skyportsystems.com>
Cc: tpmdd-devel@lists.sourceforge.net,
Stefan Berger <stefanb@us.ibm.com>,
linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org,
Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Subject: Re: [PATCH v5 3/4] tpm: Allow TPM chip drivers to override reported command durations
Date: Fri, 10 Jun 2016 22:42:48 +0300 [thread overview]
Message-ID: <20160610194248.GA7011@intel.com> (raw)
In-Reply-To: <CAO_EM_m_GsEVUcoUxb8Cg7-46W-mm9CY8PG6J=ZtYuTENrDm9A@mail.gmail.com>
On Fri, Jun 10, 2016 at 10:34:15AM -0700, Ed Swierk wrote:
> On Fri, Jun 10, 2016 at 5:19 AM, Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> > On Wed, Jun 08, 2016 at 04:00:17PM -0700, Ed Swierk wrote:
> >> Some TPM chips report bogus command durations in their capabilities,
> >> just as others report incorrect timeouts. Rework tpm_get_timeouts()
> >> to allow chip drivers to override either via a single callback.
> >> Also clean up handling of TPMs that report milliseconds instead of
> >> microseconds.
> >>
> >> Signed-off-by: Ed Swierk <eswierk@skyportsystems.com>
> >> ---
> >> drivers/char/tpm/tpm-interface.c | 177 +++++++++++++++++++++------------------
> >> drivers/char/tpm/tpm_tis.c | 35 ++------
> >> include/linux/tpm.h | 3 +-
> >> 3 files changed, 106 insertions(+), 109 deletions(-)
> >>
> >> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> >> index cc1e5bc..b8a08bb 100644
> >> --- a/drivers/char/tpm/tpm-interface.c
> >> +++ b/drivers/char/tpm/tpm-interface.c
> >> @@ -502,123 +502,138 @@ static int tpm_startup(struct tpm_chip *chip, __be16 startup_type)
> >> "attempting to start the TPM");
> >> }
> >>
> >> -int tpm_get_timeouts(struct tpm_chip *chip)
> >> +static int tpm_get_cap_prop(struct tpm_chip *chip, __be32 type, int size,
> >> + cap_t *cap, char *desc)
> >> {
> >> struct tpm_cmd_t tpm_cmd;
> >> - unsigned long new_timeout[4];
> >> - unsigned long old_timeout[4];
> >> - struct duration_t *duration_cap;
> >> ssize_t rc;
> >>
> >> tpm_cmd.header.in = tpm_getcap_header;
> >> tpm_cmd.params.getcap_in.cap = TPM_CAP_PROP;
> >> tpm_cmd.params.getcap_in.subcap_size = cpu_to_be32(4);
> >> - tpm_cmd.params.getcap_in.subcap = TPM_CAP_PROP_TIS_TIMEOUT;
> >> + tpm_cmd.params.getcap_in.subcap = type;
> >> rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE, NULL);
> >>
> >> if (rc == TPM_ERR_INVALID_POSTINIT) {
> >> /* The TPM is not started, we are the first to talk to it.
> >> Execute a startup command. */
> >> - dev_info(chip->pdev, "Issuing TPM_STARTUP");
> >> + dev_info(chip->pdev, "Issuing TPM_STARTUP\n");
> >> if (tpm_startup(chip, TPM_ST_CLEAR))
> >> return rc;
> >>
> >> tpm_cmd.header.in = tpm_getcap_header;
> >> tpm_cmd.params.getcap_in.cap = TPM_CAP_PROP;
> >> tpm_cmd.params.getcap_in.subcap_size = cpu_to_be32(4);
> >> - tpm_cmd.params.getcap_in.subcap = TPM_CAP_PROP_TIS_TIMEOUT;
> >> + tpm_cmd.params.getcap_in.subcap = type;
> >> rc = tpm_transmit_cmd(chip, &tpm_cmd, TPM_INTERNAL_RESULT_SIZE,
> >> NULL);
> >> }
> >> +
> >> if (rc) {
> >> dev_err(chip->pdev,
> >> - "A TPM error (%zd) occurred attempting to determine the timeouts\n",
> >> - rc);
> >> - goto duration;
> >> + "Error %zd reading %s\n", rc, desc);
> >> + return -EINVAL;
> >> }
> >>
> >> if (be32_to_cpu(tpm_cmd.header.out.return_code) != 0 ||
> >> be32_to_cpu(tpm_cmd.header.out.length)
> >> - != sizeof(tpm_cmd.header.out) + sizeof(u32) + 4 * sizeof(u32))
> >> + != sizeof(tpm_cmd.header.out) + sizeof(u32) + size * sizeof(u32)) {
> >> + dev_err(chip->pdev,
> >> + "Bad return code or length reading %s\n", desc);
> >> return -EINVAL;
> >> -
> >> - old_timeout[0] = be32_to_cpu(tpm_cmd.params.getcap_out.cap.timeout.a);
> >> - old_timeout[1] = be32_to_cpu(tpm_cmd.params.getcap_out.cap.timeout.b);
> >> - old_timeout[2] = be32_to_cpu(tpm_cmd.params.getcap_out.cap.timeout.c);
> >> - old_timeout[3] = be32_to_cpu(tpm_cmd.params.getcap_out.cap.timeout.d);
> >> - memcpy(new_timeout, old_timeout, sizeof(new_timeout));
> >> -
> >> - /*
> >> - * Provide ability for vendor overrides of timeout values in case
> >> - * of misreporting.
> >> - */
> >> - if (chip->ops->update_timeouts != NULL)
> >> - chip->vendor.timeout_adjusted =
> >> - chip->ops->update_timeouts(chip, new_timeout);
> >> -
> >> - if (!chip->vendor.timeout_adjusted) {
> >> - /* Don't overwrite default if value is 0 */
> >> - if (new_timeout[0] != 0 && new_timeout[0] < 1000) {
> >> - int i;
> >> -
> >> - /* timeouts in msec rather usec */
> >> - for (i = 0; i != ARRAY_SIZE(new_timeout); i++)
> >> - new_timeout[i] *= 1000;
> >> - chip->vendor.timeout_adjusted = true;
> >> - }
> >> }
> >>
> >> - /* Report adjusted timeouts */
> >> - if (chip->vendor.timeout_adjusted) {
> >> - dev_info(chip->pdev,
> >> - HW_ERR "Adjusting reported timeouts: A %lu->%luus B %lu->%luus C %lu->%luus D %lu->%luus\n",
> >> - old_timeout[0], new_timeout[0],
> >> - old_timeout[1], new_timeout[1],
> >> - old_timeout[2], new_timeout[2],
> >> - old_timeout[3], new_timeout[3]);
> >> - }
> >> + memcpy(cap, &tpm_cmd.params.getcap_out.cap, sizeof(cap_t));
> >>
> >> - chip->vendor.timeout_a = usecs_to_jiffies(new_timeout[0]);
> >> - chip->vendor.timeout_b = usecs_to_jiffies(new_timeout[1]);
> >> - chip->vendor.timeout_c = usecs_to_jiffies(new_timeout[2]);
> >> - chip->vendor.timeout_d = usecs_to_jiffies(new_timeout[3]);
> >> + return 0;
> >> +}
> >>
> >> -duration:
> >> - tpm_cmd.header.in = tpm_getcap_header;
> >> - tpm_cmd.params.getcap_in.cap = TPM_CAP_PROP;
> >> - tpm_cmd.params.getcap_in.subcap_size = cpu_to_be32(4);
> >> - tpm_cmd.params.getcap_in.subcap = TPM_CAP_PROP_TIS_DURATION;
> >> +int tpm_get_timeouts(struct tpm_chip *chip)
> >> +{
> >> + cap_t cap1, cap2;
> >> + int rc1, rc2;
> >> + struct tpm_vendor_specific orig_vendor;
> >> +
> >> + rc1 = tpm_get_cap_prop(chip, TPM_CAP_PROP_TIS_TIMEOUT, 4, &cap1,
> >> + "timeouts");
> >> + if (rc1 == 0) {
> >> + be32_to_cpus(&cap1.timeout.a);
> >> + be32_to_cpus(&cap1.timeout.b);
> >> + be32_to_cpus(&cap1.timeout.c);
> >> + be32_to_cpus(&cap1.timeout.d);
> >> + chip->vendor.timeout_a = usecs_to_jiffies(cap1.timeout.a);
> >> + chip->vendor.timeout_b = usecs_to_jiffies(cap1.timeout.b);
> >> + chip->vendor.timeout_c = usecs_to_jiffies(cap1.timeout.c);
> >> + chip->vendor.timeout_d = usecs_to_jiffies(cap1.timeout.d);
> >> + }
> >> + rc2 = tpm_get_cap_prop(chip, TPM_CAP_PROP_TIS_DURATION, 3, &cap2,
> >> + "durations");
> >> + if (rc2 == 0) {
> >> + be32_to_cpus(&cap2.duration.tpm_short);
> >> + be32_to_cpus(&cap2.duration.tpm_medium);
> >> + be32_to_cpus(&cap2.duration.tpm_long);
> >> + chip->vendor.duration[TPM_SHORT] =
> >> + usecs_to_jiffies(cap2.duration.tpm_short);
> >> + chip->vendor.duration[TPM_MEDIUM] =
> >> + usecs_to_jiffies(cap2.duration.tpm_medium);
> >> + chip->vendor.duration[TPM_LONG] =
> >> + usecs_to_jiffies(cap2.duration.tpm_long);
> >> + }
> >
> > This is major change to the semantics. Before -EINVAL would have been
> > return on error condition.
> >
> > PS If you want to encapsulate tpm_get_cap_prop(), that step should be
> > a separate commit (prepend this one).
>
> Good points.
>
> I'm confused about the error semantics in the first (timeouts) part of
> tpm_get_timeouts(). If tpm_transmit_cmd() returns zero and a header
> check fails, it returns -EINVAL. But if tpm_transmit_cmd() returns
> nonzero, it swallows the error.
>
> In contrast, in the second (durations) part, an error is returned in
> either case.
>
> Is this difference intentional, or should tpm_get_timeouts() return
> errors immediately in all cases?
Sometimes these kinds of things are just "evolutional" :)
There are also couple of other things that don't look right:
* tpm_transmit_cmd() already prints the TPM error
* If you use dev_err(), you must fail. If we ought to continue, it
should be at most dev_warn().
In my opinion it is just plain wrong to continue to the durations part
in the case of TPM error. I would like to hear a second opinion, though.
> --Ed
/Jarkko
next prev parent reply other threads:[~2016-06-10 19:43 UTC|newest]
Thread overview: 68+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-06-08 0:45 [PATCH v4 0/4] tpm: Command duration logging and chip-specific override Ed Swierk
2016-06-08 0:45 ` [PATCH v4 1/4] tpm_tis: Improve reporting of IO errors Ed Swierk
2016-06-08 0:45 ` [PATCH v4 2/4] tpm: Add optional logging of TPM command durations Ed Swierk
2016-06-08 0:45 ` [PATCH v4 3/4] tpm: Allow TPM chip drivers to override reported " Ed Swierk
2016-06-08 19:05 ` [tpmdd-devel] " Jason Gunthorpe
2016-06-08 20:41 ` Ed Swierk
2016-06-08 0:45 ` [PATCH v4 4/4] tpm_tis: Increase ST19NP18 TPM command duration to avoid chip lockup Ed Swierk
2016-06-08 23:00 ` [PATCH v5 0/4] tpm: Command duration logging and chip-specific override Ed Swierk
2016-06-08 23:00 ` [PATCH v5 1/4] tpm_tis: Improve reporting of IO errors Ed Swierk
2016-06-08 23:00 ` [PATCH v5 2/4] tpm: Add optional logging of TPM command durations Ed Swierk
2016-06-08 23:00 ` [PATCH v5 3/4] tpm: Allow TPM chip drivers to override reported " Ed Swierk
2016-06-10 12:19 ` Jarkko Sakkinen
2016-06-10 17:34 ` Ed Swierk
2016-06-10 19:42 ` Jarkko Sakkinen [this message]
2016-06-11 1:54 ` Ed Swierk
2016-06-08 23:00 ` [PATCH v5 4/4] tpm_tis: Increase ST19NP18 TPM command duration to avoid chip lockup Ed Swierk
2016-06-11 1:55 ` [PATCH v6 0/5] tpm: Command duration logging and chip-specific override Ed Swierk
2016-06-11 1:55 ` [PATCH v6 1/5] tpm_tis: Improve reporting of IO errors Ed Swierk
2016-06-11 1:55 ` [PATCH v6 2/5] tpm: Add optional logging of TPM command durations Ed Swierk
2016-06-11 1:55 ` [PATCH v6 3/5] tpm: Factor out reading of timeout and duration capabilities Ed Swierk
2016-06-16 20:20 ` Jarkko Sakkinen
2016-06-19 12:12 ` Jarkko Sakkinen
2016-06-11 1:55 ` [PATCH v6 4/5] tpm: Allow TPM chip drivers to override reported command durations Ed Swierk
2016-06-16 20:26 ` Jarkko Sakkinen
2016-06-11 1:55 ` [PATCH v6 5/5] tpm_tis: Increase ST19NP18 TPM command duration to avoid chip lockup Ed Swierk
2016-06-21 1:53 ` [PATCH v7 0/5] tpm: Command duration logging and chip-specific override Ed Swierk
2016-06-21 1:53 ` [PATCH v7 1/5] tpm_tis: Improve reporting of IO errors Ed Swierk
2016-06-21 1:53 ` [PATCH v7 2/5] tpm: Add optional logging of TPM command durations Ed Swierk
2016-06-21 1:54 ` [PATCH v7 3/5] tpm: Clean up reading of timeout and duration capabilities Ed Swierk
2016-06-21 20:52 ` Jarkko Sakkinen
2016-06-22 0:21 ` Ed Swierk
2016-06-22 10:46 ` Jarkko Sakkinen
2016-06-21 1:54 ` [PATCH v7 4/5] tpm: Allow TPM chip drivers to override reported command durations Ed Swierk
2016-06-21 20:54 ` Jarkko Sakkinen
2016-06-21 1:54 ` [PATCH v7 5/5] tpm_tis: Increase ST19NP18 TPM command duration to avoid chip lockup Ed Swierk
2016-06-21 20:55 ` Jarkko Sakkinen
2016-06-22 1:10 ` [PATCH v8 0/5] tpm: Command duration logging and chip-specific override Ed Swierk
2016-06-22 1:10 ` [PATCH v8 1/5] tpm_tis: Improve reporting of IO errors Ed Swierk
2016-06-24 18:25 ` Jason Gunthorpe
2016-06-24 20:21 ` Jarkko Sakkinen
2016-06-24 20:23 ` Jarkko Sakkinen
2016-06-24 20:26 ` Jason Gunthorpe
2016-06-25 15:24 ` Jarkko Sakkinen
2016-06-25 15:47 ` Jarkko Sakkinen
2016-06-27 17:55 ` Jason Gunthorpe
2016-06-22 1:10 ` [PATCH v8 2/5] tpm: Add optional logging of TPM command durations Ed Swierk
2016-06-24 18:27 ` Jason Gunthorpe
2016-06-24 20:24 ` Jarkko Sakkinen
2016-06-22 1:10 ` [PATCH v8 3/5] tpm: Clean up reading of timeout and duration capabilities Ed Swierk
2016-06-22 1:10 ` [PATCH v8 4/5] tpm: Allow TPM chip drivers to override reported command durations Ed Swierk
2016-06-22 1:10 ` [PATCH v8 5/5] tpm_tis: Increase ST19NP18 TPM command duration to avoid chip lockup Ed Swierk
2016-07-13 16:19 ` [PATCH v9 0/5] tpm: Command duration logging and chip-specific override Ed Swierk
2016-07-13 16:19 ` [PATCH v9 1/5] tpm_tis: Improve reporting of IO errors Ed Swierk
2016-07-13 16:19 ` [PATCH v9 2/5] tpm: Add optional logging of TPM command durations Ed Swierk
2016-07-13 16:19 ` [PATCH v9 3/5] tpm: Clean up reading of timeout and duration capabilities Ed Swierk
2016-07-18 18:15 ` Jarkko Sakkinen
2016-07-18 18:19 ` Jarkko Sakkinen
2016-07-18 18:20 ` Jarkko Sakkinen
2016-07-13 16:19 ` [PATCH v9 4/5] tpm: Allow TPM chip drivers to override reported command durations Ed Swierk
2016-07-13 17:04 ` kbuild test robot
2016-07-18 18:40 ` Jarkko Sakkinen
2016-07-13 16:19 ` [PATCH v9 5/5] tpm_tis: Increase ST19NP18 TPM command duration to avoid chip lockup Ed Swierk
2016-07-13 16:44 ` [PATCH v9 0/5] tpm: Command duration logging and chip-specific override Ed Swierk
2016-07-13 17:36 ` Jason Gunthorpe
2016-07-13 20:00 ` Ed Swierk
2016-07-13 20:58 ` Eric W. Biederman
2016-07-13 20:59 ` Jason Gunthorpe
2016-07-18 18:07 ` 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=20160610194248.GA7011@intel.com \
--to=jarkko.sakkinen@linux.intel.com \
--cc=eswierk@skyportsystems.com \
--cc=jgunthorpe@obsidianresearch.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=stefanb@us.ibm.com \
--cc=tpmdd-devel@lists.sourceforge.net \
/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).