[tpmdd-devel,RFC] tpm, tpm_tis: detect TPM2 devices

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(-)

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 */
> 

------------------------------------------------------------------------------