diff mbox series

[v5,1/2] acpi: Extend TPM2 ACPI table with missing log fields

Message ID 20200623120636.1453470-2-stefanb@linux.vnet.ibm.com (mailing list archive)
State New, archived
Headers show
Series tpm2: Make TPM2 logs accessible for non-UEFI firmware | expand

Commit Message

Stefan Berger June 23, 2020, 12:06 p.m. UTC 
From: Stefan Berger <stefanb@linux.ibm.com>

Recent extensions of the TPM2 ACPI table added 3 more fields
including 12 bytes of start method specific parameters and Log Area
Minimum Length (u32) and Log Area Start Address (u64). So, we extend
the existing structure with these fields to allow non-UEFI systems
to access the TPM2's log.

The specification that has the new fields is the following:
  TCG ACPI Specification
  Family "1.2" and "2.0"
  Version 1.2, Revision 8

Adapt all existing table size calculations to use
offsetof(struct acpi_table_tpm2, start_method_specific)
[where start_method_specific is a newly added field]
rather than sizeof(struct acpi_table_tpm2) so that the addition
of the new fields does not affect current systems that may not
have them.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
Cc: linux-acpi@vger.kernel.org
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm_crb.c | 13 ++++++++++---
 drivers/char/tpm/tpm_tis.c |  4 +++-
 include/acpi/actbl3.h      |  5 +++--
 3 files changed, 16 insertions(+), 6 deletions(-)

Comments

Jarkko Sakkinen June 25, 2020, midnight UTC | #1 
On Tue, Jun 23, 2020 at 08:06:35AM -0400, Stefan Berger wrote:
> From: Stefan Berger <stefanb@linux.ibm.com>
> 
> Recent extensions of the TPM2 ACPI table added 3 more fields
> including 12 bytes of start method specific parameters and Log Area
> Minimum Length (u32) and Log Area Start Address (u64). So, we extend
> the existing structure with these fields to allow non-UEFI systems
> to access the TPM2's log.
> 
> The specification that has the new fields is the following:
>   TCG ACPI Specification
>   Family "1.2" and "2.0"
>   Version 1.2, Revision 8
> 
> Adapt all existing table size calculations to use
> offsetof(struct acpi_table_tpm2, start_method_specific)
> [where start_method_specific is a newly added field]
> rather than sizeof(struct acpi_table_tpm2) so that the addition
> of the new fields does not affect current systems that may not
> have them.
> 

I found at least one regression from this patch. Please remove my
reviewed-by comment form the next version.

Should have:

  Link: https://trustedcomputinggroup.org/wp-content/uploads/TCG_ACPIGeneralSpecification_v1.20_r8.pdf

Please, add this.

> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> Cc: linux-acpi@vger.kernel.org
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  drivers/char/tpm/tpm_crb.c | 13 ++++++++++---
>  drivers/char/tpm/tpm_tis.c |  4 +++-
>  include/acpi/actbl3.h      |  5 +++--
>  3 files changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index a9dcf31eadd2..0565aa5482f9 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -669,7 +669,9 @@ 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 <
> +			offsetof(struct acpi_table_tpm2,
> +				 start_method_specific)) {
>  		dev_err(dev, FW_BUG "failed to get TPM2 ACPI table\n");
>  		return -EINVAL;
>  	}
> @@ -684,14 +686,19 @@ static int crb_acpi_add(struct acpi_device *device)
>  		return -ENOMEM;
>  
>  	if (sm == ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC) {
> -		if (buf->header.length < (sizeof(*buf) + sizeof(*crb_smc))) {
> +		if (buf->header.length <
> +			(offsetof(struct acpi_table_tpm2,
> +				  start_method_specific) +

Should be

  offsetof(struct acpti_table_tpm2, log_area_minimum_length)

> +			 sizeof(*crb_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_ARM_SMC);
>  			return -EINVAL;
>  		}
> -		crb_smc = ACPI_ADD_PTR(struct tpm2_crb_smc, buf, sizeof(*buf));
> +		crb_smc = ACPI_ADD_PTR(struct tpm2_crb_smc, buf,
> +			   offsetof(struct acpi_table_tpm2,
> +				    start_method_specific));
>  		priv->smc_func_id = crb_smc->smc_func_id;
>  	}
>  
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index e7df342a317d..a80f36860bac 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -111,7 +111,9 @@ static int check_acpi_tpm2(struct device *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 <
> +			offsetof(struct acpi_table_tpm2,
> +				 start_method_specific)) {
>  		dev_err(dev, FW_BUG "failed to get TPM2 ACPI table\n");
>  		return -EINVAL;
>  	}
> diff --git a/include/acpi/actbl3.h b/include/acpi/actbl3.h
> index b0b163b9efc6..8b55426bbcf6 100644
> --- a/include/acpi/actbl3.h
> +++ b/include/acpi/actbl3.h
> @@ -411,8 +411,9 @@ struct acpi_table_tpm2 {
>  	u16 reserved;
>  	u64 control_address;
>  	u32 start_method;
> -
> -	/* Platform-specific data follows */
> +	u8  start_method_specific[12];
> +	u32 log_area_minimum_length;		/* optional */
> +	u64 log_area_start_address;		/* optional */

'start_method_specific' is also optional. Please remove the optional
comments altogether as platform-specific data is by definition optional.

>  };
>  
>  /* Values for start_method above */
> -- 
> 2.26.2
> 

Please add this to the next version since we do not want to break ARM
side:

  Cc: Jiandi An <anjiandi@codeaurora.org>

/Jarkko
Stefan Berger June 25, 2020, 12:38 a.m. UTC | #2 
On 6/24/20 8:00 PM, Jarkko Sakkinen wrote:
> On Tue, Jun 23, 2020 at 08:06:35AM -0400, Stefan Berger wrote:
>> From: Stefan Berger <stefanb@linux.ibm.com>
>>
>> Recent extensions of the TPM2 ACPI table added 3 more fields
>> including 12 bytes of start method specific parameters and Log Area
>> Minimum Length (u32) and Log Area Start Address (u64). So, we extend
>> the existing structure with these fields to allow non-UEFI systems
>> to access the TPM2's log.
>>
>> The specification that has the new fields is the following:
>>    TCG ACPI Specification
>>    Family "1.2" and "2.0"
>>    Version 1.2, Revision 8
>>
>> Adapt all existing table size calculations to use
>> offsetof(struct acpi_table_tpm2, start_method_specific)
>> [where start_method_specific is a newly added field]
>> rather than sizeof(struct acpi_table_tpm2) so that the addition
>> of the new fields does not affect current systems that may not
>> have them.
>>
> I found at least one regression from this patch. Please remove my
> reviewed-by comment form the next version.
>
> Should have:
>
>    Link: https://trustedcomputinggroup.org/wp-content/uploads/TCG_ACPIGeneralSpecification_v1.20_r8.pdf
>
> Please, add this.
>
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>> Cc: linux-acpi@vger.kernel.org
>> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>> ---
>>   drivers/char/tpm/tpm_crb.c | 13 ++++++++++---
>>   drivers/char/tpm/tpm_tis.c |  4 +++-
>>   include/acpi/actbl3.h      |  5 +++--
>>   3 files changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
>> index a9dcf31eadd2..0565aa5482f9 100644
>> --- a/drivers/char/tpm/tpm_crb.c
>> +++ b/drivers/char/tpm/tpm_crb.c
>> @@ -669,7 +669,9 @@ 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 <
>> +			offsetof(struct acpi_table_tpm2,
>> +				 start_method_specific)) {
>>   		dev_err(dev, FW_BUG "failed to get TPM2 ACPI table\n");
>>   		return -EINVAL;
>>   	}
>> @@ -684,14 +686,19 @@ static int crb_acpi_add(struct acpi_device *device)
>>   		return -ENOMEM;
>>   
>>   	if (sm == ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC) {
>> -		if (buf->header.length < (sizeof(*buf) + sizeof(*crb_smc))) {
>> +		if (buf->header.length <
>> +			(offsetof(struct acpi_table_tpm2,
>> +				  start_method_specific) +
> Should be
>
>    offsetof(struct acpti_table_tpm2, log_area_minimum_length)


The old code had sizeof(*buf) with buf being 'struct acpi_table_tpm2' 
and that was equivalent to offsetof(struct acpi_table_tpm2, 
start_method_specific) since 'start_method_specific' is the first new 
field that we are adding right here. Also see 3rd paragraph in the patch 
description. The replacement rule described there should apply to all 
sizeof() calculations on 'struct acpi_table_tpm2.'

I'll address the other issues.


    Stefan
Jarkko Sakkinen June 25, 2020, 2:34 a.m. UTC | #3 
On Wed, Jun 24, 2020 at 08:38:25PM -0400, Stefan Berger wrote:
> On 6/24/20 8:00 PM, Jarkko Sakkinen wrote:
> > On Tue, Jun 23, 2020 at 08:06:35AM -0400, Stefan Berger wrote:
> > > From: Stefan Berger <stefanb@linux.ibm.com>
> > > 
> > > Recent extensions of the TPM2 ACPI table added 3 more fields
> > > including 12 bytes of start method specific parameters and Log Area
> > > Minimum Length (u32) and Log Area Start Address (u64). So, we extend
> > > the existing structure with these fields to allow non-UEFI systems
> > > to access the TPM2's log.
> > > 
> > > The specification that has the new fields is the following:
> > >    TCG ACPI Specification
> > >    Family "1.2" and "2.0"
> > >    Version 1.2, Revision 8
> > > 
> > > Adapt all existing table size calculations to use
> > > offsetof(struct acpi_table_tpm2, start_method_specific)
> > > [where start_method_specific is a newly added field]
> > > rather than sizeof(struct acpi_table_tpm2) so that the addition
> > > of the new fields does not affect current systems that may not
> > > have them.
> > > 
> > I found at least one regression from this patch. Please remove my
> > reviewed-by comment form the next version.
> > 
> > Should have:
> > 
> >    Link: https://trustedcomputinggroup.org/wp-content/uploads/TCG_ACPIGeneralSpecification_v1.20_r8.pdf
> > 
> > Please, add this.
> > 
> > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > > Cc: linux-acpi@vger.kernel.org
> > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > ---
> > >   drivers/char/tpm/tpm_crb.c | 13 ++++++++++---
> > >   drivers/char/tpm/tpm_tis.c |  4 +++-
> > >   include/acpi/actbl3.h      |  5 +++--
> > >   3 files changed, 16 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> > > index a9dcf31eadd2..0565aa5482f9 100644
> > > --- a/drivers/char/tpm/tpm_crb.c
> > > +++ b/drivers/char/tpm/tpm_crb.c
> > > @@ -669,7 +669,9 @@ 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 <
> > > +			offsetof(struct acpi_table_tpm2,
> > > +				 start_method_specific)) {
> > >   		dev_err(dev, FW_BUG "failed to get TPM2 ACPI table\n");
> > >   		return -EINVAL;
> > >   	}
> > > @@ -684,14 +686,19 @@ static int crb_acpi_add(struct acpi_device *device)
> > >   		return -ENOMEM;
> > >   	if (sm == ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC) {
> > > -		if (buf->header.length < (sizeof(*buf) + sizeof(*crb_smc))) {
> > > +		if (buf->header.length <
> > > +			(offsetof(struct acpi_table_tpm2,
> > > +				  start_method_specific) +
> > Should be
> > 
> >    offsetof(struct acpti_table_tpm2, log_area_minimum_length)
> 
> 
> The old code had sizeof(*buf) with buf being 'struct acpi_table_tpm2' and
> that was equivalent to offsetof(struct acpi_table_tpm2,
> start_method_specific) since 'start_method_specific' is the first new field
> that we are adding right here. Also see 3rd paragraph in the patch
> description. The replacement rule described there should apply to all
> sizeof() calculations on 'struct acpi_table_tpm2.'

Aren't you ignoring sizeof(*crb_smc) then?

> 
> I'll address the other issues.
> 
> 
>    Stefan
Jarkko Sakkinen June 25, 2020, 2:52 a.m. UTC | #4 
On Thu, Jun 25, 2020 at 05:34:38AM +0300, Jarkko Sakkinen wrote:
> On Wed, Jun 24, 2020 at 08:38:25PM -0400, Stefan Berger wrote:
> > On 6/24/20 8:00 PM, Jarkko Sakkinen wrote:
> > > On Tue, Jun 23, 2020 at 08:06:35AM -0400, Stefan Berger wrote:
> > > > From: Stefan Berger <stefanb@linux.ibm.com>
> > > > 
> > > > Recent extensions of the TPM2 ACPI table added 3 more fields
> > > > including 12 bytes of start method specific parameters and Log Area
> > > > Minimum Length (u32) and Log Area Start Address (u64). So, we extend
> > > > the existing structure with these fields to allow non-UEFI systems
> > > > to access the TPM2's log.
> > > > 
> > > > The specification that has the new fields is the following:
> > > >    TCG ACPI Specification
> > > >    Family "1.2" and "2.0"
> > > >    Version 1.2, Revision 8
> > > > 
> > > > Adapt all existing table size calculations to use
> > > > offsetof(struct acpi_table_tpm2, start_method_specific)
> > > > [where start_method_specific is a newly added field]
> > > > rather than sizeof(struct acpi_table_tpm2) so that the addition
> > > > of the new fields does not affect current systems that may not
> > > > have them.
> > > > 
> > > I found at least one regression from this patch. Please remove my
> > > reviewed-by comment form the next version.
> > > 
> > > Should have:
> > > 
> > >    Link: https://trustedcomputinggroup.org/wp-content/uploads/TCG_ACPIGeneralSpecification_v1.20_r8.pdf
> > > 
> > > Please, add this.
> > > 
> > > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > > > Cc: linux-acpi@vger.kernel.org
> > > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > ---
> > > >   drivers/char/tpm/tpm_crb.c | 13 ++++++++++---
> > > >   drivers/char/tpm/tpm_tis.c |  4 +++-
> > > >   include/acpi/actbl3.h      |  5 +++--
> > > >   3 files changed, 16 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> > > > index a9dcf31eadd2..0565aa5482f9 100644
> > > > --- a/drivers/char/tpm/tpm_crb.c
> > > > +++ b/drivers/char/tpm/tpm_crb.c
> > > > @@ -669,7 +669,9 @@ 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 <
> > > > +			offsetof(struct acpi_table_tpm2,
> > > > +				 start_method_specific)) {
> > > >   		dev_err(dev, FW_BUG "failed to get TPM2 ACPI table\n");
> > > >   		return -EINVAL;
> > > >   	}
> > > > @@ -684,14 +686,19 @@ static int crb_acpi_add(struct acpi_device *device)
> > > >   		return -ENOMEM;
> > > >   	if (sm == ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC) {
> > > > -		if (buf->header.length < (sizeof(*buf) + sizeof(*crb_smc))) {
> > > > +		if (buf->header.length <
> > > > +			(offsetof(struct acpi_table_tpm2,
> > > > +				  start_method_specific) +
> > > Should be
> > > 
> > >    offsetof(struct acpti_table_tpm2, log_area_minimum_length)
> > 
> > 
> > The old code had sizeof(*buf) with buf being 'struct acpi_table_tpm2' and
> > that was equivalent to offsetof(struct acpi_table_tpm2,
> > start_method_specific) since 'start_method_specific' is the first new field
> > that we are adding right here. Also see 3rd paragraph in the patch
> > description. The replacement rule described there should apply to all
> > sizeof() calculations on 'struct acpi_table_tpm2.'
> 
> Aren't you ignoring sizeof(*crb_smc) then?

Duh, it's there I see. Sorry, my mistake.

Please put the new fields in a separate struct:

struct acpi_tpm2_phy {
	u8  start_method_specific[12];
	u32 log_area_minimum_length;
	u64 log_area_start_address;
};

This way we don't have to obfuscate all the calculations and zero out
the need for 1/2 in this patch set.

/Jarkko
Jarkko Sakkinen June 25, 2020, 2:54 a.m. UTC | #5 
On Thu, Jun 25, 2020 at 05:52:56AM +0300, Jarkko Sakkinen wrote:
> On Thu, Jun 25, 2020 at 05:34:38AM +0300, Jarkko Sakkinen wrote:
> > On Wed, Jun 24, 2020 at 08:38:25PM -0400, Stefan Berger wrote:
> > > On 6/24/20 8:00 PM, Jarkko Sakkinen wrote:
> > > > On Tue, Jun 23, 2020 at 08:06:35AM -0400, Stefan Berger wrote:
> > > > > From: Stefan Berger <stefanb@linux.ibm.com>
> > > > > 
> > > > > Recent extensions of the TPM2 ACPI table added 3 more fields
> > > > > including 12 bytes of start method specific parameters and Log Area
> > > > > Minimum Length (u32) and Log Area Start Address (u64). So, we extend
> > > > > the existing structure with these fields to allow non-UEFI systems
> > > > > to access the TPM2's log.
> > > > > 
> > > > > The specification that has the new fields is the following:
> > > > >    TCG ACPI Specification
> > > > >    Family "1.2" and "2.0"
> > > > >    Version 1.2, Revision 8
> > > > > 
> > > > > Adapt all existing table size calculations to use
> > > > > offsetof(struct acpi_table_tpm2, start_method_specific)
> > > > > [where start_method_specific is a newly added field]
> > > > > rather than sizeof(struct acpi_table_tpm2) so that the addition
> > > > > of the new fields does not affect current systems that may not
> > > > > have them.
> > > > > 
> > > > I found at least one regression from this patch. Please remove my
> > > > reviewed-by comment form the next version.
> > > > 
> > > > Should have:
> > > > 
> > > >    Link: https://trustedcomputinggroup.org/wp-content/uploads/TCG_ACPIGeneralSpecification_v1.20_r8.pdf
> > > > 
> > > > Please, add this.
> > > > 
> > > > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > > > > Cc: linux-acpi@vger.kernel.org
> > > > > Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > > > ---
> > > > >   drivers/char/tpm/tpm_crb.c | 13 ++++++++++---
> > > > >   drivers/char/tpm/tpm_tis.c |  4 +++-
> > > > >   include/acpi/actbl3.h      |  5 +++--
> > > > >   3 files changed, 16 insertions(+), 6 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> > > > > index a9dcf31eadd2..0565aa5482f9 100644
> > > > > --- a/drivers/char/tpm/tpm_crb.c
> > > > > +++ b/drivers/char/tpm/tpm_crb.c
> > > > > @@ -669,7 +669,9 @@ 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 <
> > > > > +			offsetof(struct acpi_table_tpm2,
> > > > > +				 start_method_specific)) {
> > > > >   		dev_err(dev, FW_BUG "failed to get TPM2 ACPI table\n");
> > > > >   		return -EINVAL;
> > > > >   	}
> > > > > @@ -684,14 +686,19 @@ static int crb_acpi_add(struct acpi_device *device)
> > > > >   		return -ENOMEM;
> > > > >   	if (sm == ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC) {
> > > > > -		if (buf->header.length < (sizeof(*buf) + sizeof(*crb_smc))) {
> > > > > +		if (buf->header.length <
> > > > > +			(offsetof(struct acpi_table_tpm2,
> > > > > +				  start_method_specific) +
> > > > Should be
> > > > 
> > > >    offsetof(struct acpti_table_tpm2, log_area_minimum_length)
> > > 
> > > 
> > > The old code had sizeof(*buf) with buf being 'struct acpi_table_tpm2' and
> > > that was equivalent to offsetof(struct acpi_table_tpm2,
> > > start_method_specific) since 'start_method_specific' is the first new field
> > > that we are adding right here. Also see 3rd paragraph in the patch
> > > description. The replacement rule described there should apply to all
> > > sizeof() calculations on 'struct acpi_table_tpm2.'
> > 
> > Aren't you ignoring sizeof(*crb_smc) then?
> 
> Duh, it's there I see. Sorry, my mistake.
> 
> Please put the new fields in a separate struct:
> 
> struct acpi_tpm2_phy {
> 	u8  start_method_specific[12];
> 	u32 log_area_minimum_length;
> 	u64 log_area_start_address;
> };
> 
> This way we don't have to obfuscate all the calculations and zero out
> the need for 1/2 in this patch set.

Also remark that, if you continue the current patch, that would need
tested-by from ARM whereas a new struct does not because the ARM code
is intact.

/Jarkko
Stefan Berger June 25, 2020, 12:10 p.m. UTC | #6 
On 6/24/20 10:34 PM, Jarkko Sakkinen wrote:
> On Wed, Jun 24, 2020 at 08:38:25PM -0400, Stefan Berger wrote:
>> On 6/24/20 8:00 PM, Jarkko Sakkinen wrote:
>>> On Tue, Jun 23, 2020 at 08:06:35AM -0400, Stefan Berger wrote:
>>>> From: Stefan Berger <stefanb@linux.ibm.com>
>>>>
>>>> Recent extensions of the TPM2 ACPI table added 3 more fields
>>>> including 12 bytes of start method specific parameters and Log Area
>>>> Minimum Length (u32) and Log Area Start Address (u64). So, we extend
>>>> the existing structure with these fields to allow non-UEFI systems
>>>> to access the TPM2's log.
>>>>
>>>> The specification that has the new fields is the following:
>>>>     TCG ACPI Specification
>>>>     Family "1.2" and "2.0"
>>>>     Version 1.2, Revision 8
>>>>
>>>> Adapt all existing table size calculations to use
>>>> offsetof(struct acpi_table_tpm2, start_method_specific)
>>>> [where start_method_specific is a newly added field]
>>>> rather than sizeof(struct acpi_table_tpm2) so that the addition
>>>> of the new fields does not affect current systems that may not
>>>> have them.
>>>>
>>> I found at least one regression from this patch. Please remove my
>>> reviewed-by comment form the next version.
>>>
>>> Should have:
>>>
>>>     Link: https://trustedcomputinggroup.org/wp-content/uploads/TCG_ACPIGeneralSpecification_v1.20_r8.pdf
>>>
>>> Please, add this.
>>>
>>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>>>> Cc: linux-acpi@vger.kernel.org
>>>> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>>>> ---
>>>>    drivers/char/tpm/tpm_crb.c | 13 ++++++++++---
>>>>    drivers/char/tpm/tpm_tis.c |  4 +++-
>>>>    include/acpi/actbl3.h      |  5 +++--
>>>>    3 files changed, 16 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
>>>> index a9dcf31eadd2..0565aa5482f9 100644
>>>> --- a/drivers/char/tpm/tpm_crb.c
>>>> +++ b/drivers/char/tpm/tpm_crb.c
>>>> @@ -669,7 +669,9 @@ 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 <
>>>> +			offsetof(struct acpi_table_tpm2,
>>>> +				 start_method_specific)) {
>>>>    		dev_err(dev, FW_BUG "failed to get TPM2 ACPI table\n");
>>>>    		return -EINVAL;
>>>>    	}
>>>> @@ -684,14 +686,19 @@ static int crb_acpi_add(struct acpi_device *device)
>>>>    		return -ENOMEM;
>>>>    	if (sm == ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC) {
>>>> -		if (buf->header.length < (sizeof(*buf) + sizeof(*crb_smc))) {
>>>> +		if (buf->header.length <
>>>> +			(offsetof(struct acpi_table_tpm2,
>>>> +				  start_method_specific) +
>>> Should be
>>>
>>>     offsetof(struct acpti_table_tpm2, log_area_minimum_length)
>>
>> The old code had sizeof(*buf) with buf being 'struct acpi_table_tpm2' and
>> that was equivalent to offsetof(struct acpi_table_tpm2,
>> start_method_specific) since 'start_method_specific' is the first new field
>> that we are adding right here. Also see 3rd paragraph in the patch
>> description. The replacement rule described there should apply to all
>> sizeof() calculations on 'struct acpi_table_tpm2.'
> Aren't you ignoring sizeof(*crb_smc) then?

It's still there.
diff mbox series

Patch

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index a9dcf31eadd2..0565aa5482f9 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -669,7 +669,9 @@  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 <
+			offsetof(struct acpi_table_tpm2,
+				 start_method_specific)) {
 		dev_err(dev, FW_BUG "failed to get TPM2 ACPI table\n");
 		return -EINVAL;
 	}
@@ -684,14 +686,19 @@  static int crb_acpi_add(struct acpi_device *device)
 		return -ENOMEM;
 
 	if (sm == ACPI_TPM2_COMMAND_BUFFER_WITH_ARM_SMC) {
-		if (buf->header.length < (sizeof(*buf) + sizeof(*crb_smc))) {
+		if (buf->header.length <
+			(offsetof(struct acpi_table_tpm2,
+				  start_method_specific) +
+			 sizeof(*crb_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_ARM_SMC);
 			return -EINVAL;
 		}
-		crb_smc = ACPI_ADD_PTR(struct tpm2_crb_smc, buf, sizeof(*buf));
+		crb_smc = ACPI_ADD_PTR(struct tpm2_crb_smc, buf,
+			   offsetof(struct acpi_table_tpm2,
+				    start_method_specific));
 		priv->smc_func_id = crb_smc->smc_func_id;
 	}
 
diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
index e7df342a317d..a80f36860bac 100644
--- a/drivers/char/tpm/tpm_tis.c
+++ b/drivers/char/tpm/tpm_tis.c
@@ -111,7 +111,9 @@  static int check_acpi_tpm2(struct device *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 <
+			offsetof(struct acpi_table_tpm2,
+				 start_method_specific)) {
 		dev_err(dev, FW_BUG "failed to get TPM2 ACPI table\n");
 		return -EINVAL;
 	}
diff --git a/include/acpi/actbl3.h b/include/acpi/actbl3.h
index b0b163b9efc6..8b55426bbcf6 100644
--- a/include/acpi/actbl3.h
+++ b/include/acpi/actbl3.h
@@ -411,8 +411,9 @@  struct acpi_table_tpm2 {
 	u16 reserved;
 	u64 control_address;
 	u32 start_method;
-
-	/* Platform-specific data follows */
+	u8  start_method_specific[12];
+	u32 log_area_minimum_length;		/* optional */
+	u64 log_area_start_address;		/* optional */
 };
 
 /* Values for start_method above */