| Message ID | 1440858966-12431-1-git-send-email-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > ------------------------------------------------------------------------------
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 */ > ------------------------------------------------------------------------------
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 */
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(-)