* [PATCH RFC] tpm, tpm_tis: detect TPM2 devices
@ 2015-08-29 14:36 Jarkko Sakkinen
2015-08-29 14:38 ` Jarkko Sakkinen
2015-09-03 20:21 ` Jethro Beekman
0 siblings, 2 replies; 3+ messages in thread
From: Jarkko Sakkinen @ 2015-08-29 14:36 UTC (permalink / raw)
To: tpmdd-devel, linux-kernel
Cc: Jarkko Sakkinen, Peter Huewe, Marcel Selhorst, Jason Gunthorpe
It turned out that the root cause in b371616b8 was not entirely correct
for https://bugzilla.kernel.org/show_bug.cgi?id=98181.
All TPM 2.0 FIFO and CRB devices have the same HID. For FIFO devices
the start method is 6 as it is defined in
http://www.trustedcomputinggroup.org/resources/tcg_acpi_specification
This patch changes FIFO and CRB drivers so that they check start method
value and initialize only if the start method has a proper value for
that particular interface.
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
drivers/char/tpm/tpm.h | 7 +++++++
drivers/char/tpm/tpm_crb.c | 20 +++++---------------
drivers/char/tpm/tpm_tis.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
3 files changed, 55 insertions(+), 17 deletions(-)
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index f8319a0..39be5ac 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -115,6 +115,13 @@ enum tpm2_startup_types {
TPM2_SU_STATE = 0x0001,
};
+enum tpm2_start_method {
+ TPM2_START_ACPI = 2,
+ TPM2_START_FIFO = 6,
+ TPM2_START_CRB = 7,
+ TPM2_START_CRB_WITH_ACPI = 8,
+};
+
struct tpm_chip;
struct tpm_vendor_specific {
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 1267322..b4564b6 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -34,12 +34,6 @@ enum crb_defaults {
CRB_ACPI_START_INDEX = 1,
};
-enum crb_start_method {
- CRB_SM_ACPI_START = 2,
- CRB_SM_CRB = 7,
- CRB_SM_CRB_WITH_ACPI_START = 8,
-};
-
struct acpi_tpm2 {
struct acpi_table_header hdr;
u16 platform_class;
@@ -233,13 +227,9 @@ static int crb_acpi_add(struct acpi_device *device)
return -ENODEV;
}
- /* At least some versions of AMI BIOS have a bug that TPM2 table has
- * zero address for the control area and therefore we must fail.
- */
- if (!buf->control_area_pa) {
- dev_err(dev, "TPM2 ACPI table has a zero address for the control area\n");
- return -EINVAL;
- }
+ /* Should the FIFO driver handle this? */
+ if (buf->start_method == TPM2_START_FIFO)
+ return -ENODEV;
if (buf->hdr.length < sizeof(struct acpi_tpm2)) {
dev_err(dev, "TPM2 ACPI table has wrong size");
@@ -259,11 +249,11 @@ static int crb_acpi_add(struct acpi_device *device)
* report only ACPI start but in practice seems to require both
* ACPI start and CRB start.
*/
- if (sm == CRB_SM_CRB || sm == CRB_SM_CRB_WITH_ACPI_START ||
+ if (sm == TPM2_START_CRB || sm == TPM2_START_FIFO ||
!strcmp(acpi_device_hid(device), "MSFT0101"))
priv->flags |= CRB_FL_CRB_START;
- if (sm == CRB_SM_ACPI_START || sm == CRB_SM_CRB_WITH_ACPI_START)
+ if (sm == TPM2_START_ACPI || sm == TPM2_START_CRB_WITH_ACPI)
priv->flags |= CRB_FL_ACPI_START;
priv->cca = (struct crb_control_area __iomem *)
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index f2dffa7..4760804 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -28,6 +28,7 @@
#include <linux/wait.h>
#include <linux/acpi.h>
#include <linux/freezer.h>
+#include <acpi/actbl2.h>
#include "tpm.h"
enum tis_access {
@@ -91,7 +92,7 @@ struct priv_data {
};
#if defined(CONFIG_PNP) && defined(CONFIG_ACPI)
-static int is_itpm(struct pnp_dev *dev)
+static int has_hid(struct pnp_dev *dev, const char *hid)
{
struct acpi_device *acpi = pnp_acpi_device(dev);
struct acpi_hardware_id *id;
@@ -100,17 +101,49 @@ static int is_itpm(struct pnp_dev *dev)
return 0;
list_for_each_entry(id, &acpi->pnp.ids, list) {
- if (!strcmp("INTC0102", id->id))
+ if (!strcmp(hid, id->id))
return 1;
}
return 0;
}
+
+static inline int is_itpm(struct pnp_dev *dev)
+{
+ return has_hid(dev, "INTC0102");
+}
+
+static inline int validate_pnp_dev(struct pnp_dev *dev)
+{
+ struct acpi_table_tpm2 *tbl;
+ acpi_status st;
+
+ if (!has_hid(dev, "MSFT0101"))
+ return 0;
+
+ st = acpi_get_table(ACPI_SIG_TPM2, 1,
+ (struct acpi_table_header **) &tbl);
+ if (ACPI_FAILURE(st)) {
+ dev_err(&dev->dev, "failed to get TPM2 ACPI table\n");
+ return -ENODEV;
+ }
+
+ /* Should the CRB driver handle this? */
+ if (le32_to_cpu(tbl->start_method) != TPM2_START_FIFO)
+ return -ENODEV:
+
+ retuurn 0;
+}
#else
static inline int is_itpm(struct pnp_dev *dev)
{
return 0;
}
+
+static inline int validate_pnp_dev(struct pnp_dev *dev)
+{
+ return 0;
+}
#endif
/* Before we attempt to access the TPM we must see that the valid bit is set.
@@ -893,6 +926,11 @@ static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
resource_size_t start, len;
unsigned int irq = 0;
acpi_handle acpi_dev_handle = NULL;
+ int ret;
+
+ ret = validate_pnp_dev(pnp_dev);
+ if (ret)
+ return ret;
start = pnp_mem_start(pnp_dev, 0);
len = pnp_mem_len(pnp_dev, 0);
@@ -921,6 +959,9 @@ static struct pnp_device_id tpm_pnp_tbl[] = {
{"BCM0102", 0}, /* Broadcom */
{"NSC1200", 0}, /* National */
{"ICO0102", 0}, /* Intel */
+#ifdef CONFIG_ACPI
+ {"MSFT0101", 0}, /* TPM 2.0 */
+#endif
/* Add new here */
{"", 0}, /* User Specified */
{"", 0} /* Terminator */
--
2.5.0
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH RFC] tpm, tpm_tis: detect TPM2 devices
2015-08-29 14:36 [PATCH RFC] tpm, tpm_tis: detect TPM2 devices Jarkko Sakkinen
@ 2015-08-29 14:38 ` Jarkko Sakkinen
2015-09-03 20:21 ` Jethro Beekman
1 sibling, 0 replies; 3+ messages in thread
From: Jarkko Sakkinen @ 2015-08-29 14:38 UTC (permalink / raw)
To: Jarkko Sakkinen
Cc: tpmdd-devel, linux-kernel, Peter Huewe, Marcel Selhorst,
Jason Gunthorpe
Did this already over two weeks ago but haven't been able to test this
at all since I'm still waiting the delivery of NUC5i5MYHE. It's a NUC
with a discrete TPM 2.0 chip (ST Electronics if I remember right). Very
useful machine for TPM2 testing...
/Jarkko
On Sat, Aug 29, 2015 at 05:36:06PM +0300, Jarkko Sakkinen wrote:
> It turned out that the root cause in b371616b8 was not entirely correct
> for https://bugzilla.kernel.org/show_bug.cgi?id=98181.
>
> All TPM 2.0 FIFO and CRB devices have the same HID. For FIFO devices
> the start method is 6 as it is defined in
>
> http://www.trustedcomputinggroup.org/resources/tcg_acpi_specification
>
> This patch changes FIFO and CRB drivers so that they check start method
> value and initialize only if the start method has a proper value for
> that particular interface.
>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
> drivers/char/tpm/tpm.h | 7 +++++++
> drivers/char/tpm/tpm_crb.c | 20 +++++---------------
> drivers/char/tpm/tpm_tis.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
> 3 files changed, 55 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index f8319a0..39be5ac 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -115,6 +115,13 @@ enum tpm2_startup_types {
> TPM2_SU_STATE = 0x0001,
> };
>
> +enum tpm2_start_method {
> + TPM2_START_ACPI = 2,
> + TPM2_START_FIFO = 6,
> + TPM2_START_CRB = 7,
> + TPM2_START_CRB_WITH_ACPI = 8,
> +};
> +
> struct tpm_chip;
>
> struct tpm_vendor_specific {
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index 1267322..b4564b6 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -34,12 +34,6 @@ enum crb_defaults {
> CRB_ACPI_START_INDEX = 1,
> };
>
> -enum crb_start_method {
> - CRB_SM_ACPI_START = 2,
> - CRB_SM_CRB = 7,
> - CRB_SM_CRB_WITH_ACPI_START = 8,
> -};
> -
> struct acpi_tpm2 {
> struct acpi_table_header hdr;
> u16 platform_class;
> @@ -233,13 +227,9 @@ static int crb_acpi_add(struct acpi_device *device)
> return -ENODEV;
> }
>
> - /* At least some versions of AMI BIOS have a bug that TPM2 table has
> - * zero address for the control area and therefore we must fail.
> - */
> - if (!buf->control_area_pa) {
> - dev_err(dev, "TPM2 ACPI table has a zero address for the control area\n");
> - return -EINVAL;
> - }
> + /* Should the FIFO driver handle this? */
> + if (buf->start_method == TPM2_START_FIFO)
> + return -ENODEV;
>
> if (buf->hdr.length < sizeof(struct acpi_tpm2)) {
> dev_err(dev, "TPM2 ACPI table has wrong size");
> @@ -259,11 +249,11 @@ static int crb_acpi_add(struct acpi_device *device)
> * report only ACPI start but in practice seems to require both
> * ACPI start and CRB start.
> */
> - if (sm == CRB_SM_CRB || sm == CRB_SM_CRB_WITH_ACPI_START ||
> + if (sm == TPM2_START_CRB || sm == TPM2_START_FIFO ||
> !strcmp(acpi_device_hid(device), "MSFT0101"))
> priv->flags |= CRB_FL_CRB_START;
>
> - if (sm == CRB_SM_ACPI_START || sm == CRB_SM_CRB_WITH_ACPI_START)
> + if (sm == TPM2_START_ACPI || sm == TPM2_START_CRB_WITH_ACPI)
> priv->flags |= CRB_FL_ACPI_START;
>
> priv->cca = (struct crb_control_area __iomem *)
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index f2dffa7..4760804 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -28,6 +28,7 @@
> #include <linux/wait.h>
> #include <linux/acpi.h>
> #include <linux/freezer.h>
> +#include <acpi/actbl2.h>
> #include "tpm.h"
>
> enum tis_access {
> @@ -91,7 +92,7 @@ struct priv_data {
> };
>
> #if defined(CONFIG_PNP) && defined(CONFIG_ACPI)
> -static int is_itpm(struct pnp_dev *dev)
> +static int has_hid(struct pnp_dev *dev, const char *hid)
> {
> struct acpi_device *acpi = pnp_acpi_device(dev);
> struct acpi_hardware_id *id;
> @@ -100,17 +101,49 @@ static int is_itpm(struct pnp_dev *dev)
> return 0;
>
> list_for_each_entry(id, &acpi->pnp.ids, list) {
> - if (!strcmp("INTC0102", id->id))
> + if (!strcmp(hid, id->id))
> return 1;
> }
>
> return 0;
> }
> +
> +static inline int is_itpm(struct pnp_dev *dev)
> +{
> + return has_hid(dev, "INTC0102");
> +}
> +
> +static inline int validate_pnp_dev(struct pnp_dev *dev)
> +{
> + struct acpi_table_tpm2 *tbl;
> + acpi_status st;
> +
> + if (!has_hid(dev, "MSFT0101"))
> + return 0;
> +
> + st = acpi_get_table(ACPI_SIG_TPM2, 1,
> + (struct acpi_table_header **) &tbl);
> + if (ACPI_FAILURE(st)) {
> + dev_err(&dev->dev, "failed to get TPM2 ACPI table\n");
> + return -ENODEV;
> + }
> +
> + /* Should the CRB driver handle this? */
> + if (le32_to_cpu(tbl->start_method) != TPM2_START_FIFO)
> + return -ENODEV:
> +
> + retuurn 0;
> +}
> #else
> static inline int is_itpm(struct pnp_dev *dev)
> {
> return 0;
> }
> +
> +static inline int validate_pnp_dev(struct pnp_dev *dev)
> +{
> + return 0;
> +}
> #endif
>
> /* Before we attempt to access the TPM we must see that the valid bit is set.
> @@ -893,6 +926,11 @@ static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
> resource_size_t start, len;
> unsigned int irq = 0;
> acpi_handle acpi_dev_handle = NULL;
> + int ret;
> +
> + ret = validate_pnp_dev(pnp_dev);
> + if (ret)
> + return ret;
>
> start = pnp_mem_start(pnp_dev, 0);
> len = pnp_mem_len(pnp_dev, 0);
> @@ -921,6 +959,9 @@ static struct pnp_device_id tpm_pnp_tbl[] = {
> {"BCM0102", 0}, /* Broadcom */
> {"NSC1200", 0}, /* National */
> {"ICO0102", 0}, /* Intel */
> +#ifdef CONFIG_ACPI
> + {"MSFT0101", 0}, /* TPM 2.0 */
> +#endif
> /* Add new here */
> {"", 0}, /* User Specified */
> {"", 0} /* Terminator */
> --
> 2.5.0
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH RFC] tpm, tpm_tis: detect TPM2 devices
2015-08-29 14:36 [PATCH RFC] tpm, tpm_tis: detect TPM2 devices Jarkko Sakkinen
2015-08-29 14:38 ` Jarkko Sakkinen
@ 2015-09-03 20:21 ` Jethro Beekman
1 sibling, 0 replies; 3+ messages in thread
From: Jethro Beekman @ 2015-09-03 20:21 UTC (permalink / raw)
To: Jarkko Sakkinen, tpmdd-devel, linux-kernel
Cc: Peter Huewe, Marcel Selhorst, Jason Gunthorpe
On 29-08-15 07:36, Jarkko Sakkinen wrote:
> It turned out that the root cause in b371616b8 was not entirely correct
> for https://bugzilla.kernel.org/show_bug.cgi?id=98181.
>
> All TPM 2.0 FIFO and CRB devices have the same HID. For FIFO devices
> the start method is 6 as it is defined in
>
> http://www.trustedcomputinggroup.org/resources/tcg_acpi_specification
>
> This patch changes FIFO and CRB drivers so that they check start method
> value and initialize only if the start method has a proper value for
> that particular interface.
>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
> drivers/char/tpm/tpm.h | 7 +++++++
> drivers/char/tpm/tpm_crb.c | 20 +++++---------------
> drivers/char/tpm/tpm_tis.c | 45 +++++++++++++++++++++++++++++++++++++++++++--
> 3 files changed, 55 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index f8319a0..39be5ac 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -115,6 +115,13 @@ enum tpm2_startup_types {
> TPM2_SU_STATE = 0x0001,
> };
>
> +enum tpm2_start_method {
> + TPM2_START_ACPI = 2,
> + TPM2_START_FIFO = 6,
> + TPM2_START_CRB = 7,
> + TPM2_START_CRB_WITH_ACPI = 8,
> +};
> +
> struct tpm_chip;
>
> struct tpm_vendor_specific {
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index 1267322..b4564b6 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -34,12 +34,6 @@ enum crb_defaults {
> CRB_ACPI_START_INDEX = 1,
> };
>
> -enum crb_start_method {
> - CRB_SM_ACPI_START = 2,
> - CRB_SM_CRB = 7,
> - CRB_SM_CRB_WITH_ACPI_START = 8,
> -};
> -
> struct acpi_tpm2 {
> struct acpi_table_header hdr;
> u16 platform_class;
> @@ -233,13 +227,9 @@ static int crb_acpi_add(struct acpi_device *device)
> return -ENODEV;
> }
>
> - /* At least some versions of AMI BIOS have a bug that TPM2 table has
> - * zero address for the control area and therefore we must fail.
> - */
> - if (!buf->control_area_pa) {
> - dev_err(dev, "TPM2 ACPI table has a zero address for the control area\n");
> - return -EINVAL;
> - }
> + /* Should the FIFO driver handle this? */
> + if (buf->start_method == TPM2_START_FIFO)
> + return -ENODEV;
>
> if (buf->hdr.length < sizeof(struct acpi_tpm2)) {
> dev_err(dev, "TPM2 ACPI table has wrong size");
> @@ -259,11 +249,11 @@ static int crb_acpi_add(struct acpi_device *device)
> * report only ACPI start but in practice seems to require both
> * ACPI start and CRB start.
> */
> - if (sm == CRB_SM_CRB || sm == CRB_SM_CRB_WITH_ACPI_START ||
> + if (sm == TPM2_START_CRB || sm == TPM2_START_FIFO ||
> !strcmp(acpi_device_hid(device), "MSFT0101"))
> priv->flags |= CRB_FL_CRB_START;
>
> - if (sm == CRB_SM_ACPI_START || sm == CRB_SM_CRB_WITH_ACPI_START)
> + if (sm == TPM2_START_ACPI || sm == TPM2_START_CRB_WITH_ACPI)
> priv->flags |= CRB_FL_ACPI_START;
>
> priv->cca = (struct crb_control_area __iomem *)
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index f2dffa7..4760804 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -28,6 +28,7 @@
> #include <linux/wait.h>
> #include <linux/acpi.h>
> #include <linux/freezer.h>
> +#include <acpi/actbl2.h>
> #include "tpm.h"
>
> enum tis_access {
> @@ -91,7 +92,7 @@ struct priv_data {
> };
>
> #if defined(CONFIG_PNP) && defined(CONFIG_ACPI)
> -static int is_itpm(struct pnp_dev *dev)
> +static int has_hid(struct pnp_dev *dev, const char *hid)
> {
> struct acpi_device *acpi = pnp_acpi_device(dev);
> struct acpi_hardware_id *id;
> @@ -100,17 +101,49 @@ static int is_itpm(struct pnp_dev *dev)
> return 0;
>
> list_for_each_entry(id, &acpi->pnp.ids, list) {
> - if (!strcmp("INTC0102", id->id))
> + if (!strcmp(hid, id->id))
> return 1;
> }
>
> return 0;
> }
> +
> +static inline int is_itpm(struct pnp_dev *dev)
> +{
> + return has_hid(dev, "INTC0102");
> +}
> +
> +static inline int validate_pnp_dev(struct pnp_dev *dev)
> +{
> + struct acpi_table_tpm2 *tbl;
> + acpi_status st;
> +
> + if (!has_hid(dev, "MSFT0101"))
> + return 0;
> +
> + st = acpi_get_table(ACPI_SIG_TPM2, 1,
> + (struct acpi_table_header **) &tbl);
> + if (ACPI_FAILURE(st)) {
> + dev_err(&dev->dev, "failed to get TPM2 ACPI table\n");
> + return -ENODEV;
> + }
> +
> + /* Should the CRB driver handle this? */
> + if (le32_to_cpu(tbl->start_method) != TPM2_START_FIFO)
> + return -ENODEV:
This should end in a semicolon.
> +
> + retuurn 0;
This should spell 'return'.
> +}
> #else
> static inline int is_itpm(struct pnp_dev *dev)
> {
> return 0;
> }
> +
> +static inline int validate_pnp_dev(struct pnp_dev *dev)
> +{
> + return 0;
> +}
> #endif
>
> /* Before we attempt to access the TPM we must see that the valid bit is set.
> @@ -893,6 +926,11 @@ static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
> resource_size_t start, len;
> unsigned int irq = 0;
> acpi_handle acpi_dev_handle = NULL;
> + int ret;
> +
> + ret = validate_pnp_dev(pnp_dev);
> + if (ret)
> + return ret;
>
> start = pnp_mem_start(pnp_dev, 0);
> len = pnp_mem_len(pnp_dev, 0);
> @@ -921,6 +959,9 @@ static struct pnp_device_id tpm_pnp_tbl[] = {
> {"BCM0102", 0}, /* Broadcom */
> {"NSC1200", 0}, /* National */
> {"ICO0102", 0}, /* Intel */
> +#ifdef CONFIG_ACPI
> + {"MSFT0101", 0}, /* TPM 2.0 */
> +#endif
> /* Add new here */
> {"", 0}, /* User Specified */
> {"", 0} /* Terminator */
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-09-03 20:46 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-08-29 14:36 [PATCH RFC] tpm, tpm_tis: detect TPM2 devices Jarkko Sakkinen
2015-08-29 14:38 ` Jarkko Sakkinen
2015-09-03 20:21 ` Jethro Beekman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox