* [PATCH v5 0/2] tpm2: Make TPM2 logs accessible for non-UEFI firmware @ 2020-06-23 12:06 Stefan Berger 2020-06-23 12:06 ` [PATCH v5 1/2] acpi: Extend TPM2 ACPI table with missing log fields Stefan Berger 2020-06-23 12:06 ` [PATCH v5 2/2] tpm: Add support for event log pointer found in TPM2 ACPI table Stefan Berger 0 siblings, 2 replies; 9+ messages in thread From: Stefan Berger @ 2020-06-23 12:06 UTC (permalink / raw) To: linux-integrity, linux-kernel, jarkko.sakkinen, linux-acpi, linux-security-module Cc: Stefan Berger From: Stefan Berger <stfeanb@linux.ibm.com> This series of patches extends the existing TPM2 ACPI table with additional fields found in the TPM2 TCG ACPI specification (reference is in the patch) that allow access to the log's address and its size. We then modify the code that so far only enables access to a TPM 1.2's log for a TPM2 as well. This then enables access to the TPM2's log on non-UEFI system that for example run SeaBIOS. Stefan v4->v5: - Added R-bs and A-bs. v3->v4: - Repost as one series v2->v3: - Split the series into two separate patches - Added comments to ACPI table fields - Added check for null pointer to log area and zero log size v1->v2: - Repost of the series Stefan Berger (2): acpi: Extend TPM2 ACPI table with missing log fields tpm: Add support for event log pointer found in TPM2 ACPI table drivers/char/tpm/eventlog/acpi.c | 56 ++++++++++++++++++++------------ drivers/char/tpm/tpm_crb.c | 13 ++++++-- drivers/char/tpm/tpm_tis.c | 4 ++- include/acpi/actbl3.h | 5 +-- 4 files changed, 51 insertions(+), 27 deletions(-) -- 2.26.2 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v5 1/2] acpi: Extend TPM2 ACPI table with missing log fields 2020-06-23 12:06 [PATCH v5 0/2] tpm2: Make TPM2 logs accessible for non-UEFI firmware Stefan Berger @ 2020-06-23 12:06 ` Stefan Berger 2020-06-25 0:00 ` Jarkko Sakkinen 2020-06-23 12:06 ` [PATCH v5 2/2] tpm: Add support for event log pointer found in TPM2 ACPI table Stefan Berger 1 sibling, 1 reply; 9+ messages in thread From: Stefan Berger @ 2020-06-23 12:06 UTC (permalink / raw) To: linux-integrity, linux-kernel, jarkko.sakkinen, linux-acpi, linux-security-module Cc: Stefan Berger, Rafael J . Wysocki 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(-) 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 */ -- 2.26.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v5 1/2] acpi: Extend TPM2 ACPI table with missing log fields 2020-06-23 12:06 ` [PATCH v5 1/2] acpi: Extend TPM2 ACPI table with missing log fields Stefan Berger @ 2020-06-25 0:00 ` Jarkko Sakkinen 2020-06-25 0:38 ` Stefan Berger 0 siblings, 1 reply; 9+ messages in thread From: Jarkko Sakkinen @ 2020-06-25 0:00 UTC (permalink / raw) To: Stefan Berger, Jiandi An Cc: linux-integrity, linux-kernel, linux-acpi, linux-security-module, Stefan Berger, Rafael J . Wysocki 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5 1/2] acpi: Extend TPM2 ACPI table with missing log fields 2020-06-25 0:00 ` Jarkko Sakkinen @ 2020-06-25 0:38 ` Stefan Berger 2020-06-25 2:34 ` Jarkko Sakkinen 0 siblings, 1 reply; 9+ messages in thread From: Stefan Berger @ 2020-06-25 0:38 UTC (permalink / raw) To: Jarkko Sakkinen, Stefan Berger, Jiandi An Cc: linux-integrity, linux-kernel, linux-acpi, linux-security-module, Rafael J . Wysocki 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5 1/2] acpi: Extend TPM2 ACPI table with missing log fields 2020-06-25 0:38 ` Stefan Berger @ 2020-06-25 2:34 ` Jarkko Sakkinen 2020-06-25 2:52 ` Jarkko Sakkinen 2020-06-25 12:10 ` Stefan Berger 0 siblings, 2 replies; 9+ messages in thread From: Jarkko Sakkinen @ 2020-06-25 2:34 UTC (permalink / raw) To: Stefan Berger Cc: Stefan Berger, Jiandi An, linux-integrity, linux-kernel, linux-acpi, linux-security-module, Rafael J . Wysocki 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5 1/2] acpi: Extend TPM2 ACPI table with missing log fields 2020-06-25 2:34 ` Jarkko Sakkinen @ 2020-06-25 2:52 ` Jarkko Sakkinen 2020-06-25 2:54 ` Jarkko Sakkinen 2020-06-25 12:10 ` Stefan Berger 1 sibling, 1 reply; 9+ messages in thread From: Jarkko Sakkinen @ 2020-06-25 2:52 UTC (permalink / raw) To: Stefan Berger Cc: Stefan Berger, Jiandi An, linux-integrity, linux-kernel, linux-acpi, linux-security-module, Rafael J . Wysocki 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5 1/2] acpi: Extend TPM2 ACPI table with missing log fields 2020-06-25 2:52 ` Jarkko Sakkinen @ 2020-06-25 2:54 ` Jarkko Sakkinen 0 siblings, 0 replies; 9+ messages in thread From: Jarkko Sakkinen @ 2020-06-25 2:54 UTC (permalink / raw) To: Stefan Berger Cc: Stefan Berger, Jiandi An, linux-integrity, linux-kernel, linux-acpi, linux-security-module, Rafael J . Wysocki 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v5 1/2] acpi: Extend TPM2 ACPI table with missing log fields 2020-06-25 2:34 ` Jarkko Sakkinen 2020-06-25 2:52 ` Jarkko Sakkinen @ 2020-06-25 12:10 ` Stefan Berger 1 sibling, 0 replies; 9+ messages in thread From: Stefan Berger @ 2020-06-25 12:10 UTC (permalink / raw) To: Jarkko Sakkinen Cc: Stefan Berger, Jiandi An, linux-integrity, linux-kernel, linux-acpi, linux-security-module, Rafael J . Wysocki 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v5 2/2] tpm: Add support for event log pointer found in TPM2 ACPI table 2020-06-23 12:06 [PATCH v5 0/2] tpm2: Make TPM2 logs accessible for non-UEFI firmware Stefan Berger 2020-06-23 12:06 ` [PATCH v5 1/2] acpi: Extend TPM2 ACPI table with missing log fields Stefan Berger @ 2020-06-23 12:06 ` Stefan Berger 1 sibling, 0 replies; 9+ messages in thread From: Stefan Berger @ 2020-06-23 12:06 UTC (permalink / raw) To: linux-integrity, linux-kernel, jarkko.sakkinen, linux-acpi, linux-security-module Cc: Stefan Berger From: Stefan Berger <stefanb@linux.ibm.com> In case a TPM2 is attached, search for a TPM2 ACPI table when trying to get the event log from ACPI. If one is found, use it to get the start and length of the log area. This allows non-UEFI systems, such as SeaBIOS, to pass an event log when using a TPM2. Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> --- drivers/char/tpm/eventlog/acpi.c | 56 ++++++++++++++++++++------------ 1 file changed, 35 insertions(+), 21 deletions(-) diff --git a/drivers/char/tpm/eventlog/acpi.c b/drivers/char/tpm/eventlog/acpi.c index 63ada5e53f13..e714a2bd0423 100644 --- a/drivers/char/tpm/eventlog/acpi.c +++ b/drivers/char/tpm/eventlog/acpi.c @@ -49,9 +49,8 @@ int tpm_read_log_acpi(struct tpm_chip *chip) void __iomem *virt; u64 len, start; struct tpm_bios_log *log; - - if (chip->flags & TPM_CHIP_FLAG_TPM2) - return -ENODEV; + struct acpi_table_tpm2 *tbl; + int format; log = &chip->log; @@ -61,23 +60,38 @@ int tpm_read_log_acpi(struct tpm_chip *chip) if (!chip->acpi_dev_handle) return -ENODEV; - /* Find TCPA entry in RSDT (ACPI_LOGICAL_ADDRESSING) */ - status = acpi_get_table(ACPI_SIG_TCPA, 1, - (struct acpi_table_header **)&buff); - - if (ACPI_FAILURE(status)) - return -ENODEV; - - switch(buff->platform_class) { - case BIOS_SERVER: - len = buff->server.log_max_len; - start = buff->server.log_start_addr; - break; - case BIOS_CLIENT: - default: - len = buff->client.log_max_len; - start = buff->client.log_start_addr; - break; + if (chip->flags & TPM_CHIP_FLAG_TPM2) { + status = acpi_get_table("TPM2", 1, + (struct acpi_table_header **)&tbl); + if (ACPI_FAILURE(status)) + return -ENODEV; + if (tbl->header.length < sizeof(*tbl)) + return -ENODEV; + len = tbl->log_area_minimum_length; + start = tbl->log_area_start_address; + if (!start || !len) + return -ENODEV; + format = EFI_TCG2_EVENT_LOG_FORMAT_TCG_2; + } else { + /* Find TCPA entry in RSDT (ACPI_LOGICAL_ADDRESSING) */ + status = acpi_get_table(ACPI_SIG_TCPA, 1, + (struct acpi_table_header **)&buff); + + if (ACPI_FAILURE(status)) + return -ENODEV; + + switch (buff->platform_class) { + case BIOS_SERVER: + len = buff->server.log_max_len; + start = buff->server.log_start_addr; + break; + case BIOS_CLIENT: + default: + len = buff->client.log_max_len; + start = buff->client.log_start_addr; + break; + } + format = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2; } if (!len) { dev_warn(&chip->dev, "%s: TCPA log area empty\n", __func__); @@ -98,7 +112,7 @@ int tpm_read_log_acpi(struct tpm_chip *chip) memcpy_fromio(log->bios_event_log, virt, len); acpi_os_unmap_iomem(virt, len); - return EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2; + return format; err: kfree(log->bios_event_log); -- 2.26.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-06-25 12:11 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-23 12:06 [PATCH v5 0/2] tpm2: Make TPM2 logs accessible for non-UEFI firmware Stefan Berger 2020-06-23 12:06 ` [PATCH v5 1/2] acpi: Extend TPM2 ACPI table with missing log fields Stefan Berger 2020-06-25 0:00 ` Jarkko Sakkinen 2020-06-25 0:38 ` Stefan Berger 2020-06-25 2:34 ` Jarkko Sakkinen 2020-06-25 2:52 ` Jarkko Sakkinen 2020-06-25 2:54 ` Jarkko Sakkinen 2020-06-25 12:10 ` Stefan Berger 2020-06-23 12:06 ` [PATCH v5 2/2] tpm: Add support for event log pointer found in TPM2 ACPI table Stefan Berger
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).