| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4D91EE00.1000801@linux.vnet.ibm.com>
Date: Tue, 29 Mar 2011 11:34:40 -0300
From: Rajiv Andrade <srajiv@...ux.vnet.ibm.com>
To: Stefan Berger <stefanb@...ux.vnet.ibm.com>
CC: debora@...ux.vnet.ibm.com, tpmdd-devel@...ts.sourceforge.net,
linux-kernel@...r.kernel.org, jirislaby@...il.com,
preining@...ic.at
Subject: Re: [patch 1/8] tpm_tis: Use timeouts returned from TPM
Hi Stefan,
Some comments:
On 03/15/2011 08:13 AM, Stefan Berger wrote:
> v3:
> - sysfs entry now called 'durations' to resemble TPM-speak (previously
> was called 'timeouts')
>
> v2:
> - adjusting all timeouts for TPM devices reporting timeouts in msec rather
> than usec
This is a bugfix that's different than the one you originally sent, can you
submit this as a separated patch? This is mainly for easier debug in the
future, so each feature/bugfix/commit can be tested separately.
> - also displaying in sysfs whether the timeouts are 'original' or 'adjusted'
>
Minor comment, it's easier to read the log when the changelog comes after the main
patch description.
> The current TPM TIS driver in git discards the timeout values returned
> from the TPM. The check of the response packet needs to consider that
> the return_code field is 0 on success and the size of the expected
> packet is equivalent to the header size + u32 length indicator for the
> TPM_GetCapability() result + 3 timeout indicators of type u32.
>
> Since some TPMs seem to return timeouts in msec rather than usec,
> I am now adjusting all the timeouts rather than just the one for short
> durations.
>
> I am also adding a sysfs entry 'durations' showing the timeouts that are
> being used.
>
> Signed-off-by: Stefan Berger <stefanb@...ux.vnet.ibm.com>
>
> ---
> drivers/char/tpm/tpm.c | 38 ++++++++++++++++++++++++++++++--------
> drivers/char/tpm/tpm.h | 3 +++
> drivers/char/tpm/tpm_tis.c | 4 +++-
> 3 files changed, 36 insertions(+), 9 deletions(-)
>
> Index: linux-2.6/drivers/char/tpm/tpm.c
> ===================================================================
> --- linux-2.6.orig/drivers/char/tpm/tpm.c
> +++ linux-2.6/drivers/char/tpm/tpm.c
> @@ -575,23 +575,31 @@ duration:
> if (rc)
> return;
>
> - if (be32_to_cpu(tpm_cmd.header.out.return_code)
> - != 3 * sizeof(u32))
> + 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) + 3 * sizeof(u32))
> return;
> +
> duration_cap = &tpm_cmd.params.getcap_out.cap.duration;
> chip->vendor.duration[TPM_SHORT] =
> usecs_to_jiffies(be32_to_cpu(duration_cap->tpm_short));
> + chip->vendor.duration[TPM_MEDIUM] =
> + usecs_to_jiffies(be32_to_cpu(duration_cap->tpm_medium));
> + chip->vendor.duration[TPM_LONG] =
> + usecs_to_jiffies(be32_to_cpu(duration_cap->tpm_long));
> +
> /* The Broadcom BCM0102 chipset in a Dell Latitude D820 gets the above
> * value wrong and apparently reports msecs rather than usecs. So we
> * fix up the resulting too-small TPM_SHORT value to make things work.
> + * We also scale the TPM_MEDIUM and -_LONG values by 1000.
> */
> - if (chip->vendor.duration[TPM_SHORT] < (HZ/100))
> + if (chip->vendor.duration[TPM_SHORT] < (HZ / 100)) {
> chip->vendor.duration[TPM_SHORT] = HZ;
> -
> - chip->vendor.duration[TPM_MEDIUM] =
> - usecs_to_jiffies(be32_to_cpu(duration_cap->tpm_medium));
> - chip->vendor.duration[TPM_LONG] =
> - usecs_to_jiffies(be32_to_cpu(duration_cap->tpm_long));
> + chip->vendor.duration[TPM_MEDIUM] *= 1000;
> + chip->vendor.duration[TPM_LONG] *= 1000;
> + chip->vendor.duration_adjusted = true;
> + dev_info(chip->dev, "Adjusting TPM timeout parameters.");
> + }
> }
> EXPORT_SYMBOL_GPL(tpm_get_timeouts);
>
> @@ -937,6 +945,20 @@ ssize_t tpm_show_caps_1_2(struct device
> }
> EXPORT_SYMBOL_GPL(tpm_show_caps_1_2);
>
> +ssize_t tpm_show_durations(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct tpm_chip *chip = dev_get_drvdata(dev);
> +
> + return sprintf(buf, "%d %d %d [%s]\n",
> + jiffies_to_usecs(chip->vendor.duration[TPM_SHORT]),
> + jiffies_to_usecs(chip->vendor.duration[TPM_MEDIUM]),
> + jiffies_to_usecs(chip->vendor.duration[TPM_LONG]),
> + chip->vendor.duration_adjusted
> + ? "adjusted" : "original");
> +}
> +EXPORT_SYMBOL_GPL(tpm_show_durations);
> +
> ssize_t tpm_store_cancel(struct device *dev, struct device_attribute *attr,
> const char *buf, size_t count)
> {
> Index: linux-2.6/drivers/char/tpm/tpm.h
> ===================================================================
> --- linux-2.6.orig/drivers/char/tpm/tpm.h
> +++ linux-2.6/drivers/char/tpm/tpm.h
> @@ -56,6 +56,8 @@ extern ssize_t tpm_show_owned(struct dev
> char *);
> extern ssize_t tpm_show_temp_deactivated(struct device *,
> struct device_attribute *attr, char *);
> +extern ssize_t tpm_show_durations(struct device *,
> + struct device_attribute *attr, char *);
>
> struct tpm_chip;
>
> @@ -82,6 +84,7 @@ struct tpm_vendor_specific {
> int locality;
> unsigned long timeout_a, timeout_b, timeout_c, timeout_d; /* jiffies */
> unsigned long duration[3]; /* jiffies */
> + bool duration_adjusted;
>
> wait_queue_head_t read_queue;
> wait_queue_head_t int_queue;
> Index: linux-2.6/drivers/char/tpm/tpm_tis.c
> ===================================================================
> --- linux-2.6.orig/drivers/char/tpm/tpm_tis.c
> +++ linux-2.6/drivers/char/tpm/tpm_tis.c
> @@ -376,6 +376,7 @@ static DEVICE_ATTR(temp_deactivated, S_I
> NULL);
> static DEVICE_ATTR(caps, S_IRUGO, tpm_show_caps_1_2, NULL);
> static DEVICE_ATTR(cancel, S_IWUSR | S_IWGRP, NULL, tpm_store_cancel);
> +static DEVICE_ATTR(durations, S_IRUGO, tpm_show_durations, NULL);
>
> static struct attribute *tis_attrs[] = {
> &dev_attr_pubek.attr,
> @@ -385,7 +386,8 @@ static struct attribute *tis_attrs[] = {
> &dev_attr_owned.attr,
> &dev_attr_temp_deactivated.attr,
> &dev_attr_caps.attr,
> - &dev_attr_cancel.attr, NULL,
> + &dev_attr_cancel.attr,
> + &dev_attr_durations.attr, NULL,
> };
>
> static struct attribute_group tis_attr_grp = {
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists