[tpmdd-devel,1/4] Add log start/length fields to TPM2 table

Submitted by Petr Vandrovec on March 29, 2017, 7:43 a.m.

Details

Message ID 20170329074323.s6qkb7pk47jqa4q6@petr-dev3.eng.vmware.com
State New
Headers show

Commit Message

Petr Vandrovec March 29, 2017, 7:43 a.m.
From: Petr Vandrovec <petr@vmware.com>

Latest revision of TPM 2.0 ACPI spec adds log start/length
to the TPM2 table.  Add them to our definition.  As few
places were using sizeof(TPM2) to make sure required fields
are present, switch them to use length of table up to and
including start type field, as that is what they are after.

Also change SMC CRB handling to use TPM2 fields rather than
offsets + typecasts.

Signed-off-by: Petr Vandrovec <petr@vmware.com>
---
 drivers/char/tpm/tpm_crb.c | 15 +++------------
 drivers/char/tpm/tpm_tis.c |  2 +-
 include/acpi/actbl2.h      | 30 +++++++++++++++++++++++++-----
 3 files changed, 29 insertions(+), 18 deletions(-)

Comments

Jarkko Sakkinen March 31, 2017, 8:14 a.m.
On Wed, Mar 29, 2017 at 12:43:23AM -0700, Petr Vandrovec wrote:
> From: Petr Vandrovec <petr@vmware.com>
> 
> Latest revision of TPM 2.0 ACPI spec adds log start/length
> to the TPM2 table.  Add them to our definition.  As few
> places were using sizeof(TPM2) to make sure required fields
> are present, switch them to use length of table up to and
> including start type field, as that is what they are after.
> 
> Also change SMC CRB handling to use TPM2 fields rather than
> offsets + typecasts.
> 
> Signed-off-by: Petr Vandrovec <petr@vmware.com>

Looking into these after 4.12 PR. Where is the cover letter?
I cannot find it for some reason.

/Jarkko

> ---
>  drivers/char/tpm/tpm_crb.c | 15 +++------------
>  drivers/char/tpm/tpm_tis.c |  2 +-
>  include/acpi/actbl2.h      | 30 +++++++++++++++++++++++++-----
>  3 files changed, 29 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index 72b03c328198..43bec842e013 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -110,14 +110,6 @@ struct crb_priv {
>  	u32 smc_func_id;
>  };
>  
> -struct tpm2_crb_smc {
> -	u32 interrupt;
> -	u8 interrupt_flags;
> -	u8 op_flags;
> -	u16 reserved2;
> -	u32 smc_func_id;
> -};
> -
>  /**
>   * crb_go_idle - request tpm crb device to go the idle state
>   *
> @@ -538,7 +530,7 @@ static int crb_acpi_add(struct acpi_device *device)
>  
>  	status = acpi_get_table(ACPI_SIG_TPM2, 1,
>  				(struct acpi_table_header **) &buf);
> -	if (ACPI_FAILURE(status) || buf->header.length < sizeof(*buf)) {
> +	if (ACPI_FAILURE(status) || buf->header.length < ACPI_TPM2_SIZE_WITH_START) {
>  		dev_err(dev, FW_BUG "failed to get TPM2 ACPI table\n");
>  		return -EINVAL;
>  	}
> @@ -565,15 +557,14 @@ static int crb_acpi_add(struct acpi_device *device)
>  		priv->flags |= CRB_FL_ACPI_START;
>  
>  	if (sm == ACPI_TPM2_COMMAND_BUFFER_WITH_SMC) {
> -		if (buf->header.length < (sizeof(*buf) + sizeof(*crb_smc))) {
> +		if (buf->header.length < ACPI_TPM2_SIZE_WITH_SMC) {
>  			dev_err(dev,
>  				FW_BUG "TPM2 ACPI table has wrong size %u for start method type %d\n",
>  				buf->header.length,
>  				ACPI_TPM2_COMMAND_BUFFER_WITH_SMC);
>  			return -EINVAL;
>  		}
> -		crb_smc = ACPI_ADD_PTR(struct tpm2_crb_smc, buf,
> -				       ACPI_TPM2_START_METHOD_PARAMETER_OFFSET);
> +		crb_smc = &buf->platform_specific_data.smc;
>  		priv->smc_func_id = crb_smc->smc_func_id;
>  		priv->flags |= CRB_FL_CRB_SMC_START;
>  	}
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index c7e1384f1b08..f513a116e195 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -257,7 +257,7 @@ static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
>  
>  	st = acpi_get_table(ACPI_SIG_TPM2, 1,
>  			    (struct acpi_table_header **) &tbl);
> -	if (ACPI_FAILURE(st) || tbl->header.length < sizeof(*tbl)) {
> +	if (ACPI_FAILURE(st) || tbl->header.length < ACPI_TPM2_SIZE_WITH_START) {
>  		dev_err(&acpi_dev->dev,
>  			FW_BUG "failed to get TPM2 ACPI table\n");
>  		return -EINVAL;
> diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
> index 2b4af0769a28..645961f998ef 100644
> --- a/include/acpi/actbl2.h
> +++ b/include/acpi/actbl2.h
> @@ -1273,20 +1273,42 @@ struct acpi_table_tcpa_server {
>   *        Version 4
>   *
>   * Conforms to "TCG ACPI Specification, Family 1.2 and 2.0",
> - * December 19, 2014
> + * Version 1.2, Revision 8, February 27, 2017, Committee Draft
>   *
>   ******************************************************************************/
>  
> +struct tpm2_crb_smc {
> +	u32 interrupt;
> +	u8 interrupt_flags;
> +	u8 op_flags;
> +	u16 reserved2;
> +	u32 smc_func_id;
> +};
> +
>  struct acpi_table_tpm2 {
>  	struct acpi_table_header header;	/* Common ACPI table header */
>  	u16 platform_class;
>  	u16 reserved;
>  	u64 control_address;
>  	u32 start_method;
> -
> -	/* Platform-specific data follows */
> +	/* End of Version 3 or minimal version 4 table. */
> +        union {
> +		u8 raw[12];
> +		u32 acpi;  /* ACPI start method should have 4 zero bytes here. */
> +		struct tpm2_crb_smc smc;
> +	} platform_specific_data;	/* Added in Level 00 Version 00.37 */
> +	u32 minimum_log_length;		/* Added in Version 1.2 */
> +	u64 log_address;		/* Added in Version 1.2 */
>  };
>  
> +/* TPM2 table sizes. */
> +
> +#define ACPI_TPM2_SIZE_WITH_START	offsetofend(struct acpi_table_tpm2, \
> +						    start_method)
> +#define ACPI_TPM2_SIZE_WITH_SMC		offsetofend(struct acpi_table_tpm2, \
> +						    platform_specific_data)
> +#define ACPI_TPM2_SIZE_WITH_LOG		sizeof(struct acpi_table_tpm2)
> +
>  /* Values for start_method above */
>  
>  #define ACPI_TPM2_NOT_ALLOWED                       0
> @@ -1296,8 +1318,6 @@ struct acpi_table_tpm2 {
>  #define ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD  8
>  #define ACPI_TPM2_COMMAND_BUFFER_WITH_SMC          11
>  
> -#define ACPI_TPM2_START_METHOD_PARAMETER_OFFSET    52
> -
>  /*******************************************************************************
>   *
>   * UEFI - UEFI Boot optimization Table
> -- 
> 2.11.0
> 

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Jarkko Sakkinen April 5, 2017, 11:14 a.m.
On Wed, Mar 29, 2017 at 12:43:23AM -0700, Petr Vandrovec wrote:
> From: Petr Vandrovec <petr@vmware.com>
> 
> Latest revision of TPM 2.0 ACPI spec adds log start/length
> to the TPM2 table.  Add them to our definition.  As few
> places were using sizeof(TPM2) to make sure required fields
> are present, switch them to use length of table up to and
> including start type field, as that is what they are after.
> 
> Also change SMC CRB handling to use TPM2 fields rather than
> offsets + typecasts.
> 
> Signed-off-by: Petr Vandrovec <petr@vmware.com>
> ---
>  drivers/char/tpm/tpm_crb.c | 15 +++------------
>  drivers/char/tpm/tpm_tis.c |  2 +-
>  include/acpi/actbl2.h      | 30 +++++++++++++++++++++++++-----
>  3 files changed, 29 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index 72b03c328198..43bec842e013 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -110,14 +110,6 @@ struct crb_priv {
>  	u32 smc_func_id;
>  };
>  
> -struct tpm2_crb_smc {
> -	u32 interrupt;
> -	u8 interrupt_flags;
> -	u8 op_flags;
> -	u16 reserved2;
> -	u32 smc_func_id;
> -};
> -
>  /**
>   * crb_go_idle - request tpm crb device to go the idle state
>   *
> @@ -538,7 +530,7 @@ static int crb_acpi_add(struct acpi_device *device)
>  
>  	status = acpi_get_table(ACPI_SIG_TPM2, 1,
>  				(struct acpi_table_header **) &buf);
> -	if (ACPI_FAILURE(status) || buf->header.length < sizeof(*buf)) {
> +	if (ACPI_FAILURE(status) || buf->header.length < ACPI_TPM2_SIZE_WITH_START) {
>  		dev_err(dev, FW_BUG "failed to get TPM2 ACPI table\n");
>  		return -EINVAL;
>  	}
> @@ -565,15 +557,14 @@ static int crb_acpi_add(struct acpi_device *device)
>  		priv->flags |= CRB_FL_ACPI_START;
>  
>  	if (sm == ACPI_TPM2_COMMAND_BUFFER_WITH_SMC) {
> -		if (buf->header.length < (sizeof(*buf) + sizeof(*crb_smc))) {
> +		if (buf->header.length < ACPI_TPM2_SIZE_WITH_SMC) {
>  			dev_err(dev,
>  				FW_BUG "TPM2 ACPI table has wrong size %u for start method type %d\n",
>  				buf->header.length,
>  				ACPI_TPM2_COMMAND_BUFFER_WITH_SMC);
>  			return -EINVAL;
>  		}
> -		crb_smc = ACPI_ADD_PTR(struct tpm2_crb_smc, buf,
> -				       ACPI_TPM2_START_METHOD_PARAMETER_OFFSET);
> +		crb_smc = &buf->platform_specific_data.smc;
>  		priv->smc_func_id = crb_smc->smc_func_id;
>  		priv->flags |= CRB_FL_CRB_SMC_START;
>  	}
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index c7e1384f1b08..f513a116e195 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -257,7 +257,7 @@ static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
>  
>  	st = acpi_get_table(ACPI_SIG_TPM2, 1,
>  			    (struct acpi_table_header **) &tbl);
> -	if (ACPI_FAILURE(st) || tbl->header.length < sizeof(*tbl)) {
> +	if (ACPI_FAILURE(st) || tbl->header.length < ACPI_TPM2_SIZE_WITH_START) {
>  		dev_err(&acpi_dev->dev,
>  			FW_BUG "failed to get TPM2 ACPI table\n");
>  		return -EINVAL;
> diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
> index 2b4af0769a28..645961f998ef 100644
> --- a/include/acpi/actbl2.h
> +++ b/include/acpi/actbl2.h
> @@ -1273,20 +1273,42 @@ struct acpi_table_tcpa_server {
>   *        Version 4
>   *
>   * Conforms to "TCG ACPI Specification, Family 1.2 and 2.0",
> - * December 19, 2014
> + * Version 1.2, Revision 8, February 27, 2017, Committee Draft
>   *
>   ******************************************************************************/
>  
> +struct tpm2_crb_smc {
> +	u32 interrupt;
> +	u8 interrupt_flags;
> +	u8 op_flags;
> +	u16 reserved2;
> +	u32 smc_func_id;
> +};
> +

I want to have this in tpm_crb, not in here. It will be most pratical
both for ACPICA maintainers and for me.

>  struct acpi_table_tpm2 {
>  	struct acpi_table_header header;	/* Common ACPI table header */
>  	u16 platform_class;
>  	u16 reserved;
>  	u64 control_address;
>  	u32 start_method;
> -
> -	/* Platform-specific data follows */
> +	/* End of Version 3 or minimal version 4 table. */
> +        union {
> +		u8 raw[12];
> +		u32 acpi;  /* ACPI start method should have 4 zero bytes here. */
> +		struct tpm2_crb_smc smc;
> +	} platform_specific_data;	/* Added in Level 00 Version 00.37 */
> +	u32 minimum_log_length;		/* Added in Version 1.2 */
> +	u64 log_address;		/* Added in Version 1.2 */
>  };
>  
> +/* TPM2 table sizes. */
> +
> +#define ACPI_TPM2_SIZE_WITH_START	offsetofend(struct acpi_table_tpm2, \
> +						    start_method)
> +#define ACPI_TPM2_SIZE_WITH_SMC		offsetofend(struct acpi_table_tpm2, \
> +						    platform_specific_data)
> +#define ACPI_TPM2_SIZE_WITH_LOG		sizeof(struct acpi_table_tpm2)

This is even a bigger mess than typecasts.

> +
>  /* Values for start_method above */
>  
>  #define ACPI_TPM2_NOT_ALLOWED                       0
> @@ -1296,8 +1318,6 @@ struct acpi_table_tpm2 {
>  #define ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD  8
>  #define ACPI_TPM2_COMMAND_BUFFER_WITH_SMC          11
>  
> -#define ACPI_TPM2_START_METHOD_PARAMETER_OFFSET    52
> -

Having such constant was a regression in 69c558de63c7. I just
sent a patch to sort out this problem.

[1] https://patchwork.kernel.org/patch/9663779/

>  /*******************************************************************************
>   *
>   * UEFI - UEFI Boot optimization Table
> -- 
> 2.11.0
> 

/Jarkko

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Patch hide | download patch | download mbox

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 72b03c328198..43bec842e013 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -110,14 +110,6 @@  struct crb_priv {
 	u32 smc_func_id;
 };
 
-struct tpm2_crb_smc {
-	u32 interrupt;
-	u8 interrupt_flags;
-	u8 op_flags;
-	u16 reserved2;
-	u32 smc_func_id;
-};
-
 /**
  * crb_go_idle - request tpm crb device to go the idle state
  *
@@ -538,7 +530,7 @@  static int crb_acpi_add(struct acpi_device *device)
 
 	status = acpi_get_table(ACPI_SIG_TPM2, 1,
 				(struct acpi_table_header **) &buf);
-	if (ACPI_FAILURE(status) || buf->header.length < sizeof(*buf)) {
+	if (ACPI_FAILURE(status) || buf->header.length < ACPI_TPM2_SIZE_WITH_START) {
 		dev_err(dev, FW_BUG "failed to get TPM2 ACPI table\n");
 		return -EINVAL;
 	}
@@ -565,15 +557,14 @@  static int crb_acpi_add(struct acpi_device *device)
 		priv->flags |= CRB_FL_ACPI_START;
 
 	if (sm == ACPI_TPM2_COMMAND_BUFFER_WITH_SMC) {
-		if (buf->header.length < (sizeof(*buf) + sizeof(*crb_smc))) {
+		if (buf->header.length < ACPI_TPM2_SIZE_WITH_SMC) {
 			dev_err(dev,
 				FW_BUG "TPM2 ACPI table has wrong size %u for start method type %d\n",
 				buf->header.length,
 				ACPI_TPM2_COMMAND_BUFFER_WITH_SMC);
 			return -EINVAL;
 		}
-		crb_smc = ACPI_ADD_PTR(struct tpm2_crb_smc, buf,
-				       ACPI_TPM2_START_METHOD_PARAMETER_OFFSET);
+		crb_smc = &buf->platform_specific_data.smc;
 		priv->smc_func_id = crb_smc->smc_func_id;
 		priv->flags |= CRB_FL_CRB_SMC_START;
 	}
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index c7e1384f1b08..f513a116e195 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -257,7 +257,7 @@  static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
 
 	st = acpi_get_table(ACPI_SIG_TPM2, 1,
 			    (struct acpi_table_header **) &tbl);
-	if (ACPI_FAILURE(st) || tbl->header.length < sizeof(*tbl)) {
+	if (ACPI_FAILURE(st) || tbl->header.length < ACPI_TPM2_SIZE_WITH_START) {
 		dev_err(&acpi_dev->dev,
 			FW_BUG "failed to get TPM2 ACPI table\n");
 		return -EINVAL;
diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
index 2b4af0769a28..645961f998ef 100644
--- a/include/acpi/actbl2.h
+++ b/include/acpi/actbl2.h
@@ -1273,20 +1273,42 @@  struct acpi_table_tcpa_server {
  *        Version 4
  *
  * Conforms to "TCG ACPI Specification, Family 1.2 and 2.0",
- * December 19, 2014
+ * Version 1.2, Revision 8, February 27, 2017, Committee Draft
  *
  ******************************************************************************/
 
+struct tpm2_crb_smc {
+	u32 interrupt;
+	u8 interrupt_flags;
+	u8 op_flags;
+	u16 reserved2;
+	u32 smc_func_id;
+};
+
 struct acpi_table_tpm2 {
 	struct acpi_table_header header;	/* Common ACPI table header */
 	u16 platform_class;
 	u16 reserved;
 	u64 control_address;
 	u32 start_method;
-
-	/* Platform-specific data follows */
+	/* End of Version 3 or minimal version 4 table. */
+        union {
+		u8 raw[12];
+		u32 acpi;  /* ACPI start method should have 4 zero bytes here. */
+		struct tpm2_crb_smc smc;
+	} platform_specific_data;	/* Added in Level 00 Version 00.37 */
+	u32 minimum_log_length;		/* Added in Version 1.2 */
+	u64 log_address;		/* Added in Version 1.2 */
 };
 
+/* TPM2 table sizes. */
+
+#define ACPI_TPM2_SIZE_WITH_START	offsetofend(struct acpi_table_tpm2, \
+						    start_method)
+#define ACPI_TPM2_SIZE_WITH_SMC		offsetofend(struct acpi_table_tpm2, \
+						    platform_specific_data)
+#define ACPI_TPM2_SIZE_WITH_LOG		sizeof(struct acpi_table_tpm2)
+
 /* Values for start_method above */
 
 #define ACPI_TPM2_NOT_ALLOWED                       0
@@ -1296,8 +1318,6 @@  struct acpi_table_tpm2 {
 #define ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD  8
 #define ACPI_TPM2_COMMAND_BUFFER_WITH_SMC          11
 
-#define ACPI_TPM2_START_METHOD_PARAMETER_OFFSET    52
-
 /*******************************************************************************
  *
  * UEFI - UEFI Boot optimization Table