All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] tpm: check event log version before reading final events
@ 2020-05-12  4:01 Loïc Yhuel
  2020-05-12  6:44 ` Ard Biesheuvel
                   ` (4 more replies)
  0 siblings, 5 replies; 23+ messages in thread
From: Loïc Yhuel @ 2020-05-12  4:01 UTC (permalink / raw)
  To: linux-integrity
  Cc: matthewgarrett, ardb, jarkko.sakkinen, javierm, Loïc Yhuel

This fixes the boot issues since 5.3 on several Dell models when the TPM
is enabled. Depending on the exact grub binary, booting the kernel would
freeze early, or just report an error parsing the final events log.

We get an event log in the SHA-1 format, which doesn't have a
tcg_efi_specid_event_head in the first event, and there is a final events
table which doesn't match the crypto agile format.
__calc_tpm2_event_size reads bad "count" and "efispecid->num_algs", and
either fails, or loops long enough for the machine to be appear frozen.

So we now only parse the final events table, which is per the spec always
supposed to be in the crypto agile format, when we got a event log in this
format.

Fixes: c46f3405692de ("tpm: Reserve the TPM final events table")
Fixes: 166a2809d65b2 ("tpm: Don't duplicate events from the final event log in the TCG2 log")
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1779611
Signed-off-by: Loïc Yhuel <loic.yhuel@gmail.com>
---
 drivers/firmware/efi/libstub/tpm.c | 5 +++--
 drivers/firmware/efi/tpm.c         | 3 ++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c
index 1d59e103a2e3..e9a684637b70 100644
--- a/drivers/firmware/efi/libstub/tpm.c
+++ b/drivers/firmware/efi/libstub/tpm.c
@@ -54,7 +54,7 @@ void efi_retrieve_tpm2_eventlog(void)
 	efi_status_t status;
 	efi_physical_addr_t log_location = 0, log_last_entry = 0;
 	struct linux_efi_tpm_eventlog *log_tbl = NULL;
-	struct efi_tcg2_final_events_table *final_events_table;
+	struct efi_tcg2_final_events_table *final_events_table = NULL;
 	unsigned long first_entry_addr, last_entry_addr;
 	size_t log_size, last_entry_size;
 	efi_bool_t truncated;
@@ -127,7 +127,8 @@ void efi_retrieve_tpm2_eventlog(void)
 	 * Figure out whether any events have already been logged to the
 	 * final events structure, and if so how much space they take up
 	 */
-	final_events_table = get_efi_config_table(LINUX_EFI_TPM_FINAL_LOG_GUID);
+	if (version == EFI_TCG2_EVENT_LOG_FORMAT_TCG_2)
+		final_events_table = get_efi_config_table(LINUX_EFI_TPM_FINAL_LOG_GUID);
 	if (final_events_table && final_events_table->nr_events) {
 		struct tcg_pcr_event2_head *header;
 		int offset;
diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
index 55b031d2c989..77e101a395e7 100644
--- a/drivers/firmware/efi/tpm.c
+++ b/drivers/firmware/efi/tpm.c
@@ -62,7 +62,8 @@ int __init efi_tpm_eventlog_init(void)
 	tbl_size = sizeof(*log_tbl) + log_tbl->size;
 	memblock_reserve(efi.tpm_log, tbl_size);
 
-	if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR)
+	if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR ||
+	    log_tbl->version != EFI_TCG2_EVENT_LOG_FORMAT_TCG_2)
 		goto out;
 
 	final_tbl = early_memremap(efi.tpm_final_log, sizeof(*final_tbl));
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH] tpm: check event log version before reading final events
  2020-05-12  4:01 [PATCH] tpm: check event log version before reading final events Loïc Yhuel
@ 2020-05-12  6:44 ` Ard Biesheuvel
  2020-05-12 11:40   ` Loïc Yhuel
  2020-05-14  1:09   ` Jarkko Sakkinen
  2020-05-12 17:45 ` Javier Martinez Canillas
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 23+ messages in thread
From: Ard Biesheuvel @ 2020-05-12  6:44 UTC (permalink / raw)
  To: Loïc Yhuel; +Cc: linux-integrity, matthewgarrett, Jarkko Sakkinen, javierm

Hi Loïc,

Thanks for the fix.

On Tue, 12 May 2020 at 06:01, Loïc Yhuel <loic.yhuel@gmail.com> wrote:
>
> This fixes the boot issues since 5.3 on several Dell models when the TPM
> is enabled. Depending on the exact grub binary, booting the kernel would
> freeze early, or just report an error parsing the final events log.
>
> We get an event log in the SHA-1 format, which doesn't have a
> tcg_efi_specid_event_head in the first event, and there is a final events
> table which doesn't match the crypto agile format.
> __calc_tpm2_event_size reads bad "count" and "efispecid->num_algs", and
> either fails, or loops long enough for the machine to be appear frozen.
>
> So we now only parse the final events table, which is per the spec always
> supposed to be in the crypto agile format, when we got a event log in this
> format.
>

So what functionality do we lose here? Can we still make meaningful
use of the event log without the final log? I thought one was
incomplete without the other?

> Fixes: c46f3405692de ("tpm: Reserve the TPM final events table")
> Fixes: 166a2809d65b2 ("tpm: Don't duplicate events from the final event log in the TCG2 log")
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1779611
> Signed-off-by: Loïc Yhuel <loic.yhuel@gmail.com>

I can take this as a fix, but I need an ack from Matt as well.

> ---
>  drivers/firmware/efi/libstub/tpm.c | 5 +++--
>  drivers/firmware/efi/tpm.c         | 3 ++-
>  2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c
> index 1d59e103a2e3..e9a684637b70 100644
> --- a/drivers/firmware/efi/libstub/tpm.c
> +++ b/drivers/firmware/efi/libstub/tpm.c
> @@ -54,7 +54,7 @@ void efi_retrieve_tpm2_eventlog(void)
>         efi_status_t status;
>         efi_physical_addr_t log_location = 0, log_last_entry = 0;
>         struct linux_efi_tpm_eventlog *log_tbl = NULL;
> -       struct efi_tcg2_final_events_table *final_events_table;
> +       struct efi_tcg2_final_events_table *final_events_table = NULL;
>         unsigned long first_entry_addr, last_entry_addr;
>         size_t log_size, last_entry_size;
>         efi_bool_t truncated;
> @@ -127,7 +127,8 @@ void efi_retrieve_tpm2_eventlog(void)
>          * Figure out whether any events have already been logged to the
>          * final events structure, and if so how much space they take up
>          */
> -       final_events_table = get_efi_config_table(LINUX_EFI_TPM_FINAL_LOG_GUID);
> +       if (version == EFI_TCG2_EVENT_LOG_FORMAT_TCG_2)
> +               final_events_table = get_efi_config_table(LINUX_EFI_TPM_FINAL_LOG_GUID);
>         if (final_events_table && final_events_table->nr_events) {
>                 struct tcg_pcr_event2_head *header;
>                 int offset;
> diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
> index 55b031d2c989..77e101a395e7 100644
> --- a/drivers/firmware/efi/tpm.c
> +++ b/drivers/firmware/efi/tpm.c
> @@ -62,7 +62,8 @@ int __init efi_tpm_eventlog_init(void)
>         tbl_size = sizeof(*log_tbl) + log_tbl->size;
>         memblock_reserve(efi.tpm_log, tbl_size);
>
> -       if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR)
> +       if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR ||
> +           log_tbl->version != EFI_TCG2_EVENT_LOG_FORMAT_TCG_2)
>                 goto out;
>
>         final_tbl = early_memremap(efi.tpm_final_log, sizeof(*final_tbl));
> --
> 2.26.2
>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] tpm: check event log version before reading final events
  2020-05-12  6:44 ` Ard Biesheuvel
@ 2020-05-12 11:40   ` Loïc Yhuel
  2020-05-12 12:30     ` Ard Biesheuvel
  2020-05-14  1:09   ` Jarkko Sakkinen
  1 sibling, 1 reply; 23+ messages in thread
From: Loïc Yhuel @ 2020-05-12 11:40 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: linux-integrity, Matthew Garrett, Jarkko Sakkinen, javierm

Le mar. 12 mai 2020 à 08:45, Ard Biesheuvel <ardb@kernel.org> a écrit :
> So what functionality do we lose here? Can we still make meaningful
> use of the event log without the final log? I thought one was
> incomplete without the other?
The char driver (drivers/char/tpm/eventlog/efi.c), already ignores
efi.tpm_final_log
if the event log version isn't EFI_TCG2_EVENT_LOG_FORMAT_TCG_2.
So there currently no code making use of the final log contents on
those machines,
besides the two cases I patched which only try to determine its size.

I don't know if the table contains bad data, or just doesn't follow
the specification
and uses the older SHA-1 log format. If this is the case, perhaps we
could try to
support it, and modify the code to allow returning the additional
events it might
contain to the userspace.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] tpm: check event log version before reading final events
  2020-05-12 11:40   ` Loïc Yhuel
@ 2020-05-12 12:30     ` Ard Biesheuvel
  2020-05-14 10:51       ` Jarkko Sakkinen
  0 siblings, 1 reply; 23+ messages in thread
From: Ard Biesheuvel @ 2020-05-12 12:30 UTC (permalink / raw)
  To: Loïc Yhuel
  Cc: linux-integrity, Matthew Garrett, Jarkko Sakkinen, javierm

On Tue, 12 May 2020 at 13:40, Loïc Yhuel <loic.yhuel@gmail.com> wrote:
>
> Le mar. 12 mai 2020 à 08:45, Ard Biesheuvel <ardb@kernel.org> a écrit :
> > So what functionality do we lose here? Can we still make meaningful
> > use of the event log without the final log? I thought one was
> > incomplete without the other?
> The char driver (drivers/char/tpm/eventlog/efi.c), already ignores
> efi.tpm_final_log
> if the event log version isn't EFI_TCG2_EVENT_LOG_FORMAT_TCG_2.
> So there currently no code making use of the final log contents on
> those machines,
> besides the two cases I patched which only try to determine its size.
>

Ah ok, thanks for clarifying. If we never consume it anyway, then I
agree this is the correct fix.



> I don't know if the table contains bad data, or just doesn't follow
> the specification
> and uses the older SHA-1 log format. If this is the case, perhaps we
> could try to
> support it, and modify the code to allow returning the additional
> events it might
> contain to the userspace.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] tpm: check event log version before reading final events
  2020-05-12  4:01 [PATCH] tpm: check event log version before reading final events Loïc Yhuel
  2020-05-12  6:44 ` Ard Biesheuvel
@ 2020-05-12 17:45 ` Javier Martinez Canillas
  2020-05-12 18:45 ` Jerry Snitselaar
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 23+ messages in thread
From: Javier Martinez Canillas @ 2020-05-12 17:45 UTC (permalink / raw)
  To: Loïc Yhuel
  Cc: linux-integrity, Matthew Garrett, Ard Biesheuvel, Jarkko Sakkinen

Hello Loïc,

On Tue, May 12, 2020 at 6:02 AM Loïc Yhuel <loic.yhuel@gmail.com> wrote:
>
> This fixes the boot issues since 5.3 on several Dell models when the TPM
> is enabled. Depending on the exact grub binary, booting the kernel would
> freeze early, or just report an error parsing the final events log.
>
> We get an event log in the SHA-1 format, which doesn't have a
> tcg_efi_specid_event_head in the first event, and there is a final events
> table which doesn't match the crypto agile format.
> __calc_tpm2_event_size reads bad "count" and "efispecid->num_algs", and
> either fails, or loops long enough for the machine to be appear frozen.
>
> So we now only parse the final events table, which is per the spec always
> supposed to be in the crypto agile format, when we got a event log in this
> format.
>
> Fixes: c46f3405692de ("tpm: Reserve the TPM final events table")
> Fixes: 166a2809d65b2 ("tpm: Don't duplicate events from the final event log in the TCG2 log")
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1779611
> Signed-off-by: Loïc Yhuel <loic.yhuel@gmail.com>
> ---

As you mention, that's what the TCG EFI Protocol Specification says
about the EFI Final Events Table so I agree with your patch.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
Javier


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] tpm: check event log version before reading final events
  2020-05-12  4:01 [PATCH] tpm: check event log version before reading final events Loïc Yhuel
  2020-05-12  6:44 ` Ard Biesheuvel
  2020-05-12 17:45 ` Javier Martinez Canillas
@ 2020-05-12 18:45 ` Jerry Snitselaar
  2020-05-12 20:08 ` Matthew Garrett
  2020-05-22 18:30 ` [tip: efi/urgent] " tip-bot2 for Loïc Yhuel
  4 siblings, 0 replies; 23+ messages in thread
From: Jerry Snitselaar @ 2020-05-12 18:45 UTC (permalink / raw)
  To: Loïc Yhuel
  Cc: linux-integrity, matthewgarrett, ardb, jarkko.sakkinen, javierm

On Tue May 12 20, Loïc Yhuel wrote:
>This fixes the boot issues since 5.3 on several Dell models when the TPM
>is enabled. Depending on the exact grub binary, booting the kernel would
>freeze early, or just report an error parsing the final events log.
>
>We get an event log in the SHA-1 format, which doesn't have a
>tcg_efi_specid_event_head in the first event, and there is a final events
>table which doesn't match the crypto agile format.
>__calc_tpm2_event_size reads bad "count" and "efispecid->num_algs", and
>either fails, or loops long enough for the machine to be appear frozen.
>
>So we now only parse the final events table, which is per the spec always
>supposed to be in the crypto agile format, when we got a event log in this
>format.
>
>Fixes: c46f3405692de ("tpm: Reserve the TPM final events table")
>Fixes: 166a2809d65b2 ("tpm: Don't duplicate events from the final event log in the TCG2 log")
>Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1779611
>Signed-off-by: Loïc Yhuel <loic.yhuel@gmail.com>
>---

Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] tpm: check event log version before reading final events
  2020-05-12  4:01 [PATCH] tpm: check event log version before reading final events Loïc Yhuel
                   ` (2 preceding siblings ...)
  2020-05-12 18:45 ` Jerry Snitselaar
@ 2020-05-12 20:08 ` Matthew Garrett
  2020-05-14 10:53   ` Jarkko Sakkinen
  2020-05-22 18:30 ` [tip: efi/urgent] " tip-bot2 for Loïc Yhuel
  4 siblings, 1 reply; 23+ messages in thread
From: Matthew Garrett @ 2020-05-12 20:08 UTC (permalink / raw)
  To: Loïc Yhuel
  Cc: linux-integrity, Ard Biesheuvel, Jarkko Sakkinen,
	Javier Martinez Canillas

On Mon, May 11, 2020 at 9:01 PM Loïc Yhuel <loic.yhuel@gmail.com> wrote:

> We get an event log in the SHA-1 format, which doesn't have a
> tcg_efi_specid_event_head in the first event, and there is a final events
> table which doesn't match the crypto agile format.

This seems like a firmware bug, but we definitely shouldn't fail as a result.

Reviewed-by: Matthew Garrett <mjg59@google.com>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] tpm: check event log version before reading final events
  2020-05-12  6:44 ` Ard Biesheuvel
  2020-05-12 11:40   ` Loïc Yhuel
@ 2020-05-14  1:09   ` Jarkko Sakkinen
  2020-05-14  8:10     ` Ard Biesheuvel
  1 sibling, 1 reply; 23+ messages in thread
From: Jarkko Sakkinen @ 2020-05-14  1:09 UTC (permalink / raw)
  To: Ard Biesheuvel, Loïc Yhuel; +Cc: linux-integrity, matthewgarrett, javierm

On Tue, 2020-05-12 at 08:44 +0200, Ard Biesheuvel wrote:
> Hi Loïc,
> 
> Thanks for the fix.
> 
> On Tue, 12 May 2020 at 06:01, Loïc Yhuel <loic.yhuel@gmail.com> wrote:
> > This fixes the boot issues since 5.3 on several Dell models when the TPM
> > is enabled. Depending on the exact grub binary, booting the kernel would
> > freeze early, or just report an error parsing the final events log.
> > 
> > We get an event log in the SHA-1 format, which doesn't have a
> > tcg_efi_specid_event_head in the first event, and there is a final events
> > table which doesn't match the crypto agile format.
> > __calc_tpm2_event_size reads bad "count" and "efispecid->num_algs", and
> > either fails, or loops long enough for the machine to be appear frozen.
> > 
> > So we now only parse the final events table, which is per the spec always
> > supposed to be in the crypto agile format, when we got a event log in this
> > format.
> > 
> 
> So what functionality do we lose here? Can we still make meaningful
> use of the event log without the final log? I thought one was
> incomplete without the other?


Nope it would be incomplete [*].

So probably would make sense to at least issue a warning in this case.

[*] https://www.kernel.org/doc/Documentation/security/tpm/tpm_event_log.rst

/Jarkko


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] tpm: check event log version before reading final events
  2020-05-14  1:09   ` Jarkko Sakkinen
@ 2020-05-14  8:10     ` Ard Biesheuvel
  2020-05-14 23:44       ` Jarkko Sakkinen
  0 siblings, 1 reply; 23+ messages in thread
From: Ard Biesheuvel @ 2020-05-14  8:10 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Loïc Yhuel, linux-integrity, Matthew Garrett, javierm

On Thu, 14 May 2020 at 03:09, Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Tue, 2020-05-12 at 08:44 +0200, Ard Biesheuvel wrote:
> > Hi Loïc,
> >
> > Thanks for the fix.
> >
> > On Tue, 12 May 2020 at 06:01, Loïc Yhuel <loic.yhuel@gmail.com> wrote:
> > > This fixes the boot issues since 5.3 on several Dell models when the TPM
> > > is enabled. Depending on the exact grub binary, booting the kernel would
> > > freeze early, or just report an error parsing the final events log.
> > >
> > > We get an event log in the SHA-1 format, which doesn't have a
> > > tcg_efi_specid_event_head in the first event, and there is a final events
> > > table which doesn't match the crypto agile format.
> > > __calc_tpm2_event_size reads bad "count" and "efispecid->num_algs", and
> > > either fails, or loops long enough for the machine to be appear frozen.
> > >
> > > So we now only parse the final events table, which is per the spec always
> > > supposed to be in the crypto agile format, when we got a event log in this
> > > format.
> > >
> >
> > So what functionality do we lose here? Can we still make meaningful
> > use of the event log without the final log? I thought one was
> > incomplete without the other?
>
>
> Nope it would be incomplete [*].
>
> So probably would make sense to at least issue a warning in this case.
>
> [*] https://www.kernel.org/doc/Documentation/security/tpm/tpm_event_log.rst
>

Right.

I'll take the current patch as a fix for now.

I agree it makes sense to add a warning, but I'd like to understand
better which exact condition we should warn about.
Currently, tpm_read_log_efi() simply disregards the final log if it is
not in crypto agile format, but is there any point to exposing
anything in that case?

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] tpm: check event log version before reading final events
  2020-05-12 12:30     ` Ard Biesheuvel
@ 2020-05-14 10:51       ` Jarkko Sakkinen
  0 siblings, 0 replies; 23+ messages in thread
From: Jarkko Sakkinen @ 2020-05-14 10:51 UTC (permalink / raw)
  To: Ard Biesheuvel, Loïc Yhuel; +Cc: linux-integrity, Matthew Garrett, javierm

On Tue, 2020-05-12 at 14:30 +0200, Ard Biesheuvel wrote:
> On Tue, 12 May 2020 at 13:40, Loïc Yhuel <loic.yhuel@gmail.com> wrote:
> > Le mar. 12 mai 2020 à 08:45, Ard Biesheuvel <ardb@kernel.org> a écrit :
> > > So what functionality do we lose here? Can we still make meaningful
> > > use of the event log without the final log? I thought one was
> > > incomplete without the other?
> > The char driver (drivers/char/tpm/eventlog/efi.c), already ignores
> > efi.tpm_final_log
> > if the event log version isn't EFI_TCG2_EVENT_LOG_FORMAT_TCG_2.
> > So there currently no code making use of the final log contents on
> > those machines,
> > besides the two cases I patched which only try to determine its size.
> > 
> 
> Ah ok, thanks for clarifying. If we never consume it anyway, then I
> agree this is the correct fix.

I think issuing a warning would not be a bad idea given the incompleteness
of the even log.

/Jarkko


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] tpm: check event log version before reading final events
  2020-05-12 20:08 ` Matthew Garrett
@ 2020-05-14 10:53   ` Jarkko Sakkinen
  2020-05-14 11:28     ` Loïc Yhuel
  0 siblings, 1 reply; 23+ messages in thread
From: Jarkko Sakkinen @ 2020-05-14 10:53 UTC (permalink / raw)
  To: Matthew Garrett, Loïc Yhuel
  Cc: linux-integrity, Ard Biesheuvel, Javier Martinez Canillas

On Tue, 2020-05-12 at 13:08 -0700, Matthew Garrett wrote:
> On Mon, May 11, 2020 at 9:01 PM Loïc Yhuel <loic.yhuel@gmail.com> wrote:
> 
> > We get an event log in the SHA-1 format, which doesn't have a
> > tcg_efi_specid_event_head in the first event, and there is a final events
> > table which doesn't match the crypto agile format.
> 
> This seems like a firmware bug, but we definitely shouldn't fail as a result.
> 
> Reviewed-by: Matthew Garrett <mjg59@google.com>

So it is clear that "pr_warn(FW_BUG ..." would be a sane to have there.

/Jarkko


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] tpm: check event log version before reading final events
  2020-05-14 10:53   ` Jarkko Sakkinen
@ 2020-05-14 11:28     ` Loïc Yhuel
  2020-05-14 11:31       ` Ard Biesheuvel
  2020-05-14 11:33       ` Javier Martinez Canillas
  0 siblings, 2 replies; 23+ messages in thread
From: Loïc Yhuel @ 2020-05-14 11:28 UTC (permalink / raw)
  To: Jarkko Sakkinen
  Cc: Matthew Garrett, linux-integrity, Ard Biesheuvel,
	Javier Martinez Canillas

Le jeu. 14 mai 2020 à 12:54, Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> a écrit :
> So it is clear that "pr_warn(FW_BUG ..." would be a sane to have there.
So only to tell the UEFI might have logged events the kernel can't read ?
There is no warning if the table is missing, which would have the same result.

I can try to dump it, perhaps it is using the SHA-1 log format.
If so, would a patch to support this non-standard behavior be accepted ?

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] tpm: check event log version before reading final events
  2020-05-14 11:28     ` Loïc Yhuel
@ 2020-05-14 11:31       ` Ard Biesheuvel
  2020-05-15  0:03         ` Jarkko Sakkinen
  2020-05-14 11:33       ` Javier Martinez Canillas
  1 sibling, 1 reply; 23+ messages in thread
From: Ard Biesheuvel @ 2020-05-14 11:31 UTC (permalink / raw)
  To: Loïc Yhuel
  Cc: Jarkko Sakkinen, Matthew Garrett, linux-integrity,
	Javier Martinez Canillas

On Thu, 14 May 2020 at 13:28, Loïc Yhuel <loic.yhuel@gmail.com> wrote:
>
> Le jeu. 14 mai 2020 à 12:54, Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> a écrit :
> > So it is clear that "pr_warn(FW_BUG ..." would be a sane to have there.
> So only to tell the UEFI might have logged events the kernel can't read ?
> There is no warning if the table is missing, which would have the same result.
>
> I can try to dump it, perhaps it is using the SHA-1 log format.
> If so, would a patch to support this non-standard behavior be accepted ?

That is why I was asking the question: what exact condition should we
warn about? And at which point?

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] tpm: check event log version before reading final events
  2020-05-14 11:28     ` Loïc Yhuel
  2020-05-14 11:31       ` Ard Biesheuvel
@ 2020-05-14 11:33       ` Javier Martinez Canillas
  2020-05-14 12:28         ` Ard Biesheuvel
  2020-05-15 15:55         ` Loïc Yhuel
  1 sibling, 2 replies; 23+ messages in thread
From: Javier Martinez Canillas @ 2020-05-14 11:33 UTC (permalink / raw)
  To: Loïc Yhuel, Jarkko Sakkinen
  Cc: Matthew Garrett, linux-integrity, Ard Biesheuvel

Hello Loïc,

On 5/14/20 1:28 PM, Loïc Yhuel wrote:
> Le jeu. 14 mai 2020 à 12:54, Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> a écrit :
>> So it is clear that "pr_warn(FW_BUG ..." would be a sane to have there.
> So only to tell the UEFI might have logged events the kernel can't read ?
> There is no warning if the table is missing, which would have the same result.
> 
> I can try to dump it, perhaps it is using the SHA-1 log format.
> If so, would a patch to support this non-standard behavior be accepted ?
>

I was thinking the same and wrote the following (untested) patch that should
expose the logs from this Final Events Table that is not following the spec.

From a08ec2b99b62b3ce97c0906527d5d11f41c255a6 Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas <javierm@redhat.com>
Date: Thu, 14 May 2020 13:29:42 +0200
Subject: [RFC PATCH] tpm: Append logs from the Final Events table also for SHA1
 format

The Final Events Table contains the logs for any events that are triggered
by ExitBootServices(). According to the TCG EFI Protocol Specification the
table will only contains log entries using the crypto agile log format.

But some EFI firmwares seems to expose a Final Events Table even when the
SHA-1 log format is used. This is not supported according to the TCG spec,
but there is also no other way to get a complete TPM event log for SHA-1
if ExitBootServices() extends PCR5.

Currently the kernel only queries for the Final Events Log when using the
crypto agile log format, which is the correct behaviour to be compliant
with the spec. But since there are firmwares that provide the table even
for the SHA-1 log format, attempt to get it and append the final logs to
the TPM event log that is exposed to user-space.

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/char/tpm/eventlog/efi.c    |  3 +--
 drivers/firmware/efi/libstub/tpm.c | 18 ++++++++++++------
 drivers/firmware/efi/tpm.c         | 21 +++++++++++++++------
 3 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/drivers/char/tpm/eventlog/efi.c b/drivers/char/tpm/eventlog/efi.c
index 6bb023de17f..4a8200c9445 100644
--- a/drivers/char/tpm/eventlog/efi.c
+++ b/drivers/char/tpm/eventlog/efi.c
@@ -61,8 +61,7 @@ int tpm_read_log_efi(struct tpm_chip *chip)
 	ret = tpm_log_version;
 
 	if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR ||
-	    efi_tpm_final_log_size == 0 ||
-	    tpm_log_version != EFI_TCG2_EVENT_LOG_FORMAT_TCG_2)
+	    efi_tpm_final_log_size == 0)
 		goto out;
 
 	final_tbl = memremap(efi.tpm_final_log,
diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c
index e9a684637b7..ba9d3ac2e19 100644
--- a/drivers/firmware/efi/libstub/tpm.c
+++ b/drivers/firmware/efi/libstub/tpm.c
@@ -127,10 +127,9 @@ void efi_retrieve_tpm2_eventlog(void)
 	 * Figure out whether any events have already been logged to the
 	 * final events structure, and if so how much space they take up
 	 */
-	if (version == EFI_TCG2_EVENT_LOG_FORMAT_TCG_2)
-		final_events_table = get_efi_config_table(LINUX_EFI_TPM_FINAL_LOG_GUID);
+	final_events_table = get_efi_config_table(LINUX_EFI_TPM_FINAL_LOG_GUID);
 	if (final_events_table && final_events_table->nr_events) {
-		struct tcg_pcr_event2_head *header;
+		void *header;
 		int offset;
 		void *data;
 		int event_size;
@@ -142,9 +141,16 @@ void efi_retrieve_tpm2_eventlog(void)
 
 		while (i > 0) {
 			header = data + offset + final_events_size;
-			event_size = __calc_tpm2_event_size(header,
-						   (void *)(long)log_location,
-						   false);
+			if (version == EFI_TCG2_EVENT_LOG_FORMAT_TCG_2) {
+				event_size = __calc_tpm2_event_size(header,
+						(void *)(long)log_location,
+								    false);
+			} else {
+				struct tcpa_event *event =
+					(struct tcpa_event *)header;
+				event_size = sizeof(*event) + event->event_size;
+			}
+
 			final_events_size += event_size;
 			i--;
 		}
diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
index 77e101a395e..0962d566481 100644
--- a/drivers/firmware/efi/tpm.c
+++ b/drivers/firmware/efi/tpm.c
@@ -16,14 +16,24 @@
 int efi_tpm_final_log_size;
 EXPORT_SYMBOL(efi_tpm_final_log_size);
 
-static int __init tpm2_calc_event_log_size(void *data, int count, void *size_info)
+static int __init tpm2_calc_event_log_size(void *data, int count,
+					   struct linux_efi_tpm_eventlog *log)
 {
-	struct tcg_pcr_event2_head *header;
+	void *header;
 	int event_size, size = 0;
+	struct tcpa_event *event;
 
 	while (count > 0) {
 		header = data + size;
-		event_size = __calc_tpm2_event_size(header, size_info, true);
+		if (log->version == EFI_TCG2_EVENT_LOG_FORMAT_TCG_2) {
+			event_size = __calc_tpm2_event_size(header,
+							    (void *)log->log,
+							    true);
+		} else {
+			event = (struct tcpa_event *)header;
+			event_size = sizeof(*event) + event->event_size;
+		}
+
 		if (event_size == 0)
 			return -1;
 		size += event_size;
@@ -62,8 +72,7 @@ int __init efi_tpm_eventlog_init(void)
 	tbl_size = sizeof(*log_tbl) + log_tbl->size;
 	memblock_reserve(efi.tpm_log, tbl_size);
 
-	if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR ||
-	    log_tbl->version != EFI_TCG2_EVENT_LOG_FORMAT_TCG_2)
+	if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR)
 		goto out;
 
 	final_tbl = early_memremap(efi.tpm_final_log, sizeof(*final_tbl));
@@ -84,7 +93,7 @@ int __init efi_tpm_eventlog_init(void)
 
 		tbl_size = tpm2_calc_event_log_size(events,
 						    final_tbl->nr_events,
-						    log_tbl->log);
+						    log_tbl);
 	}
 
 	if (tbl_size < 0) {
-- 
2.26.2

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH] tpm: check event log version before reading final events
  2020-05-14 11:33       ` Javier Martinez Canillas
@ 2020-05-14 12:28         ` Ard Biesheuvel
  2020-05-14 12:56           ` Javier Martinez Canillas
  2020-05-15 15:55         ` Loïc Yhuel
  1 sibling, 1 reply; 23+ messages in thread
From: Ard Biesheuvel @ 2020-05-14 12:28 UTC (permalink / raw)
  To: Javier Martinez Canillas
  Cc: Loïc Yhuel, Jarkko Sakkinen, Matthew Garrett, linux-integrity

On Thu, 14 May 2020 at 13:33, Javier Martinez Canillas
<javierm@redhat.com> wrote:
>
> Hello Loïc,
>
> On 5/14/20 1:28 PM, Loïc Yhuel wrote:
> > Le jeu. 14 mai 2020 à 12:54, Jarkko Sakkinen
> > <jarkko.sakkinen@linux.intel.com> a écrit :
> >> So it is clear that "pr_warn(FW_BUG ..." would be a sane to have there.
> > So only to tell the UEFI might have logged events the kernel can't read ?
> > There is no warning if the table is missing, which would have the same result.
> >
> > I can try to dump it, perhaps it is using the SHA-1 log format.
> > If so, would a patch to support this non-standard behavior be accepted ?
> >
>
> I was thinking the same and wrote the following (untested) patch that should
> expose the logs from this Final Events Table that is not following the spec.
>
> From a08ec2b99b62b3ce97c0906527d5d11f41c255a6 Mon Sep 17 00:00:00 2001
> From: Javier Martinez Canillas <javierm@redhat.com>
> Date: Thu, 14 May 2020 13:29:42 +0200
> Subject: [RFC PATCH] tpm: Append logs from the Final Events table also for SHA1
>  format
>
> The Final Events Table contains the logs for any events that are triggered
> by ExitBootServices().

This is inaccurate afaik. The final events table contains all events
that were logged since the first call to Tcg2::GetEventLog()

> According to the TCG EFI Protocol Specification the
> table will only contains log entries using the crypto agile log format.
>
> But some EFI firmwares seems to expose a Final Events Table even when the
> SHA-1 log format is used. This is not supported according to the TCG spec,
> but there is also no other way to get a complete TPM event log for SHA-1
> if ExitBootServices() extends PCR5.
>
> Currently the kernel only queries for the Final Events Log when using the
> crypto agile log format, which is the correct behaviour to be compliant
> with the spec. But since there are firmwares that provide the table even
> for the SHA-1 log format, attempt to get it and append the final logs to
> the TPM event log that is exposed to user-space.
>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>  drivers/char/tpm/eventlog/efi.c    |  3 +--
>  drivers/firmware/efi/libstub/tpm.c | 18 ++++++++++++------
>  drivers/firmware/efi/tpm.c         | 21 +++++++++++++++------
>  3 files changed, 28 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/char/tpm/eventlog/efi.c b/drivers/char/tpm/eventlog/efi.c
> index 6bb023de17f..4a8200c9445 100644
> --- a/drivers/char/tpm/eventlog/efi.c
> +++ b/drivers/char/tpm/eventlog/efi.c
> @@ -61,8 +61,7 @@ int tpm_read_log_efi(struct tpm_chip *chip)
>         ret = tpm_log_version;
>
>         if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR ||
> -           efi_tpm_final_log_size == 0 ||
> -           tpm_log_version != EFI_TCG2_EVENT_LOG_FORMAT_TCG_2)
> +           efi_tpm_final_log_size == 0)
>                 goto out;
>
>         final_tbl = memremap(efi.tpm_final_log,
> diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c
> index e9a684637b7..ba9d3ac2e19 100644
> --- a/drivers/firmware/efi/libstub/tpm.c
> +++ b/drivers/firmware/efi/libstub/tpm.c
> @@ -127,10 +127,9 @@ void efi_retrieve_tpm2_eventlog(void)
>          * Figure out whether any events have already been logged to the
>          * final events structure, and if so how much space they take up
>          */
> -       if (version == EFI_TCG2_EVENT_LOG_FORMAT_TCG_2)
> -               final_events_table = get_efi_config_table(LINUX_EFI_TPM_FINAL_LOG_GUID);
> +       final_events_table = get_efi_config_table(LINUX_EFI_TPM_FINAL_LOG_GUID);
>         if (final_events_table && final_events_table->nr_events) {
> -               struct tcg_pcr_event2_head *header;
> +               void *header;
>                 int offset;
>                 void *data;
>                 int event_size;
> @@ -142,9 +141,16 @@ void efi_retrieve_tpm2_eventlog(void)
>
>                 while (i > 0) {
>                         header = data + offset + final_events_size;
> -                       event_size = __calc_tpm2_event_size(header,
> -                                                  (void *)(long)log_location,
> -                                                  false);
> +                       if (version == EFI_TCG2_EVENT_LOG_FORMAT_TCG_2) {
> +                               event_size = __calc_tpm2_event_size(header,
> +                                               (void *)(long)log_location,
> +                                                                   false);
> +                       } else {
> +                               struct tcpa_event *event =
> +                                       (struct tcpa_event *)header;
> +                               event_size = sizeof(*event) + event->event_size;
> +                       }
> +
>                         final_events_size += event_size;
>                         i--;
>                 }
> diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
> index 77e101a395e..0962d566481 100644
> --- a/drivers/firmware/efi/tpm.c
> +++ b/drivers/firmware/efi/tpm.c
> @@ -16,14 +16,24 @@
>  int efi_tpm_final_log_size;
>  EXPORT_SYMBOL(efi_tpm_final_log_size);
>
> -static int __init tpm2_calc_event_log_size(void *data, int count, void *size_info)
> +static int __init tpm2_calc_event_log_size(void *data, int count,
> +                                          struct linux_efi_tpm_eventlog *log)
>  {
> -       struct tcg_pcr_event2_head *header;
> +       void *header;
>         int event_size, size = 0;
> +       struct tcpa_event *event;
>
>         while (count > 0) {
>                 header = data + size;
> -               event_size = __calc_tpm2_event_size(header, size_info, true);
> +               if (log->version == EFI_TCG2_EVENT_LOG_FORMAT_TCG_2) {
> +                       event_size = __calc_tpm2_event_size(header,
> +                                                           (void *)log->log,
> +                                                           true);
> +               } else {
> +                       event = (struct tcpa_event *)header;
> +                       event_size = sizeof(*event) + event->event_size;
> +               }
> +
>                 if (event_size == 0)
>                         return -1;
>                 size += event_size;
> @@ -62,8 +72,7 @@ int __init efi_tpm_eventlog_init(void)
>         tbl_size = sizeof(*log_tbl) + log_tbl->size;
>         memblock_reserve(efi.tpm_log, tbl_size);
>
> -       if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR ||
> -           log_tbl->version != EFI_TCG2_EVENT_LOG_FORMAT_TCG_2)
> +       if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR)
>                 goto out;
>
>         final_tbl = early_memremap(efi.tpm_final_log, sizeof(*final_tbl));
> @@ -84,7 +93,7 @@ int __init efi_tpm_eventlog_init(void)
>
>                 tbl_size = tpm2_calc_event_log_size(events,
>                                                     final_tbl->nr_events,
> -                                                   log_tbl->log);
> +                                                   log_tbl);
>         }
>
>         if (tbl_size < 0) {
> --
> 2.26.2
>
> Best regards,
> --
> Javier Martinez Canillas
> Software Engineer - Desktop Hardware Enablement
> Red Hat
>

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] tpm: check event log version before reading final events
  2020-05-14 12:28         ` Ard Biesheuvel
@ 2020-05-14 12:56           ` Javier Martinez Canillas
  2020-05-14 13:04             ` Ard Biesheuvel
  0 siblings, 1 reply; 23+ messages in thread
From: Javier Martinez Canillas @ 2020-05-14 12:56 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Loïc Yhuel, Jarkko Sakkinen, Matthew Garrett, linux-integrity

Hello Ard,

On 5/14/20 2:28 PM, Ard Biesheuvel wrote:
> On Thu, 14 May 2020 at 13:33, Javier Martinez Canillas

[snip]

>>
>> The Final Events Table contains the logs for any events that are triggered
>> by ExitBootServices().
> 
> This is inaccurate afaik. The final events table contains all events
> that were logged since the first call to Tcg2::GetEventLog()
> 

Yes, you are correct. After the first call to GetEventLog(), all events
are logged in both the event log and the Final Events Table IIUC.

But what I tried to say is that only the Final Events Table can be used to
obtain the logs for the events triggered by ExitBootServices(). I'll try
to make it more clear if I post this as a proper patch.

I still don't know if something like that would be acceptable or if we
should just consider a bug if a firmware doesn't conform with the spec.

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] tpm: check event log version before reading final events
  2020-05-14 12:56           ` Javier Martinez Canillas
@ 2020-05-14 13:04             ` Ard Biesheuvel
  2020-05-14 13:51               ` Javier Martinez Canillas
  0 siblings, 1 reply; 23+ messages in thread
From: Ard Biesheuvel @ 2020-05-14 13:04 UTC (permalink / raw)
  To: Javier Martinez Canillas
  Cc: Loïc Yhuel, Jarkko Sakkinen, Matthew Garrett, linux-integrity

On Thu, 14 May 2020 at 14:56, Javier Martinez Canillas
<javierm@redhat.com> wrote:
>
> Hello Ard,
>
> On 5/14/20 2:28 PM, Ard Biesheuvel wrote:
> > On Thu, 14 May 2020 at 13:33, Javier Martinez Canillas
>
> [snip]
>
> >>
> >> The Final Events Table contains the logs for any events that are triggered
> >> by ExitBootServices().
> >
> > This is inaccurate afaik. The final events table contains all events
> > that were logged since the first call to Tcg2::GetEventLog()
> >
>
> Yes, you are correct. After the first call to GetEventLog(), all events
> are logged in both the event log and the Final Events Table IIUC.
>
> But what I tried to say is that only the Final Events Table can be used to
> obtain the logs for the events triggered by ExitBootServices(). I'll try
> to make it more clear if I post this as a proper patch.
>
> I still don't know if something like that would be acceptable or if we
> should just consider a bug if a firmware doesn't conform with the spec.
>

I'd like Matt's and Jarkko's take on this - for me, considering it a
bug is just fine. I just want to understand what exactly to warn
about, since we currently silently ignore the lack of a final events
table, while it apparently defeats the purpose of having a log in the
first.

So given that the ordinary event log will contain everything *except*
the events that were logged during EBS(), I agree that the log may
still be useful if those final events only affected PCRs that you
don't care about.

Something like the below perhaps? Should we suppress the warning for
tables of size 0x0?

diff --git a/drivers/char/tpm/eventlog/efi.c b/drivers/char/tpm/eventlog/efi.c
index 6bb023de17f1..65bfee8e636d 100644
--- a/drivers/char/tpm/eventlog/efi.c
+++ b/drivers/char/tpm/eventlog/efi.c
@@ -62,8 +62,10 @@ int tpm_read_log_efi(struct tpm_chip *chip)

        if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR ||
            efi_tpm_final_log_size == 0 ||
-           tpm_log_version != EFI_TCG2_EVENT_LOG_FORMAT_TCG_2)
+           tpm_log_version != EFI_TCG2_EVENT_LOG_FORMAT_TCG_2) {
+               pr_warn(FW_BUG "TPM Final Event table not found - some
events may be missing\n");
                goto out;
+       }

        final_tbl = memremap(efi.tpm_final_log,
                             sizeof(*final_tbl) + efi_tpm_final_log_size,

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH] tpm: check event log version before reading final events
  2020-05-14 13:04             ` Ard Biesheuvel
@ 2020-05-14 13:51               ` Javier Martinez Canillas
  2020-05-14 18:06                 ` Matthew Garrett
  0 siblings, 1 reply; 23+ messages in thread
From: Javier Martinez Canillas @ 2020-05-14 13:51 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Loïc Yhuel, Jarkko Sakkinen, Matthew Garrett, linux-integrity

On 5/14/20 3:04 PM, Ard Biesheuvel wrote:
> On Thu, 14 May 2020 at 14:56, Javier Martinez Canillas

[snip]

>>
>> I still don't know if something like that would be acceptable or if we
>> should just consider a bug if a firmware doesn't conform with the spec.
>>
> 
> I'd like Matt's and Jarkko's take on this - for me, considering it a
> bug is just fine. I just want to understand what exactly to warn
> about, since we currently silently ignore the lack of a final events
> table, while it apparently defeats the purpose of having a log in the
> first.
>
> So given that the ordinary event log will contain everything *except*
> the events that were logged during EBS(), I agree that the log may
> still be useful if those final events only affected PCRs that you
> don't care about.
>

Agreed.

> Something like the below perhaps? Should we suppress the warning for
> tables of size 0x0?
>

That's what is not clear to me. For example, if a firmware follows the
spec what happens during EBS() when a TPM2 only supports the SHA-1 format?

Does the firmware always log events during EBS() even when it won't provide
a Final Events Table? If so, are the SHA-1 logs always incomplete?

What I wonder is if the firmware bug error should always be printed if the
crypto agile log format isn't used or only if isn't used and the firmware
provides a Final Events Table, which is a behavior that's out of the spec.

> diff --git a/drivers/char/tpm/eventlog/efi.c b/drivers/char/tpm/eventlog/efi.c
> index 6bb023de17f1..65bfee8e636d 100644
> --- a/drivers/char/tpm/eventlog/efi.c
> +++ b/drivers/char/tpm/eventlog/efi.c
> @@ -62,8 +62,10 @@ int tpm_read_log_efi(struct tpm_chip *chip)
> 
>         if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR ||
>             efi_tpm_final_log_size == 0 ||
> -           tpm_log_version != EFI_TCG2_EVENT_LOG_FORMAT_TCG_2)
> +           tpm_log_version != EFI_TCG2_EVENT_LOG_FORMAT_TCG_2) {
> +               pr_warn(FW_BUG "TPM Final Event table not found - some
> events may be missing\n");

Maybe mentioning that the missing logs are for events that happen during EBS()?

>                 goto out;
> +       }
> 
>         final_tbl = memremap(efi.tpm_final_log,
>                              sizeof(*final_tbl) + efi_tpm_final_log_size,
> 

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] tpm: check event log version before reading final events
  2020-05-14 13:51               ` Javier Martinez Canillas
@ 2020-05-14 18:06                 ` Matthew Garrett
  0 siblings, 0 replies; 23+ messages in thread
From: Matthew Garrett @ 2020-05-14 18:06 UTC (permalink / raw)
  To: Javier Martinez Canillas
  Cc: Ard Biesheuvel, Loïc Yhuel, Jarkko Sakkinen, linux-integrity

On Thu, May 14, 2020 at 6:51 AM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> That's what is not clear to me. For example, if a firmware follows the
> spec what happens during EBS() when a TPM2 only supports the SHA-1 format?
>
> Does the firmware always log events during EBS() even when it won't provide
> a Final Events Table? If so, are the SHA-1 logs always incomplete?

In my experience, yes. We deal with this by just brute forcing all
possible PCR5 events until we find a solution that matches.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] tpm: check event log version before reading final events
  2020-05-14  8:10     ` Ard Biesheuvel
@ 2020-05-14 23:44       ` Jarkko Sakkinen
  0 siblings, 0 replies; 23+ messages in thread
From: Jarkko Sakkinen @ 2020-05-14 23:44 UTC (permalink / raw)
  To: Ard Biesheuvel; +Cc: Loïc Yhuel, linux-integrity, Matthew Garrett, javierm

On Thu, 2020-05-14 at 10:10 +0200, Ard Biesheuvel wrote:
> On Thu, 14 May 2020 at 03:09, Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> > On Tue, 2020-05-12 at 08:44 +0200, Ard Biesheuvel wrote:
> > > Hi Loïc,
> > > 
> > > Thanks for the fix.
> > > 
> > > On Tue, 12 May 2020 at 06:01, Loïc Yhuel <loic.yhuel@gmail.com> wrote:
> > > > This fixes the boot issues since 5.3 on several Dell models when the TPM
> > > > is enabled. Depending on the exact grub binary, booting the kernel would
> > > > freeze early, or just report an error parsing the final events log.
> > > > 
> > > > We get an event log in the SHA-1 format, which doesn't have a
> > > > tcg_efi_specid_event_head in the first event, and there is a final events
> > > > table which doesn't match the crypto agile format.
> > > > __calc_tpm2_event_size reads bad "count" and "efispecid->num_algs", and
> > > > either fails, or loops long enough for the machine to be appear frozen.
> > > > 
> > > > So we now only parse the final events table, which is per the spec always
> > > > supposed to be in the crypto agile format, when we got a event log in this
> > > > format.
> > > > 
> > > 
> > > So what functionality do we lose here? Can we still make meaningful
> > > use of the event log without the final log? I thought one was
> > > incomplete without the other?
> > 
> > Nope it would be incomplete [*].
> > 
> > So probably would make sense to at least issue a warning in this case.
> > 
> > [*] https://www.kernel.org/doc/Documentation/security/tpm/tpm_event_log.rst
> > 
> 
> Right.
> 
> I'll take the current patch as a fix for now.
> 
> I agree it makes sense to add a warning, but I'd like to understand
> better which exact condition we should warn about.
> Currently, tpm_read_log_efi() simply disregards the final log if it is
> not in crypto agile format, but is there any point to exposing
> anything in that case?


IMHO just a brief warning that "tail is missing" would be sufficient,
no other info.

/Jarkko


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] tpm: check event log version before reading final events
  2020-05-14 11:31       ` Ard Biesheuvel
@ 2020-05-15  0:03         ` Jarkko Sakkinen
  0 siblings, 0 replies; 23+ messages in thread
From: Jarkko Sakkinen @ 2020-05-15  0:03 UTC (permalink / raw)
  To: Ard Biesheuvel, Loïc Yhuel
  Cc: Matthew Garrett, linux-integrity, Javier Martinez Canillas

On Thu, 2020-05-14 at 13:31 +0200, Ard Biesheuvel wrote:
> On Thu, 14 May 2020 at 13:28, Loïc Yhuel <loic.yhuel@gmail.com> wrote:
> > Le jeu. 14 mai 2020 à 12:54, Jarkko Sakkinen
> > <jarkko.sakkinen@linux.intel.com> a écrit :
> > > So it is clear that "pr_warn(FW_BUG ..." would be a sane to have there.
> > So only to tell the UEFI might have logged events the kernel can't read ?
> > There is no warning if the table is missing, which would have the same result.
> > 
> > I can try to dump it, perhaps it is using the SHA-1 log format.
> > If so, would a patch to support this non-standard behavior be accepted ?
> 
> That is why I was asking the question: what exact condition should we
> warn about? And at which point?

Always when final table is missing there should be some sort of notification
because the event log is incomplete.

I.e. it misses PCR5 extends from GetEventLog().

No additional info, just a note that we don't have the tail. I'm fine with
info level message too.

/Jarkko


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH] tpm: check event log version before reading final events
  2020-05-14 11:33       ` Javier Martinez Canillas
  2020-05-14 12:28         ` Ard Biesheuvel
@ 2020-05-15 15:55         ` Loïc Yhuel
  1 sibling, 0 replies; 23+ messages in thread
From: Loïc Yhuel @ 2020-05-15 15:55 UTC (permalink / raw)
  To: Javier Martinez Canillas
  Cc: Jarkko Sakkinen, Matthew Garrett, linux-integrity, Ard Biesheuvel

Le jeu. 14 mai 2020 à 13:33, Javier Martinez Canillas
<javierm@redhat.com> a écrit :
> I was thinking the same and wrote the following (untested) patch that should
> expose the logs from this Final Events Table that is not following the spec.
> [...]
Thanks, I tried it, and added the missing early_memremap.
But I still got bad values, so I dumped the first 4KB of the table.
It seems to contain events, but they overlap : the last 16 bytes of an
event are overwritten by the following one.
So it seems the UEFI miscalculated the offsets when writing the table,
so gives us data which cannot be used.

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [tip: efi/urgent] tpm: check event log version before reading final events
  2020-05-12  4:01 [PATCH] tpm: check event log version before reading final events Loïc Yhuel
                   ` (3 preceding siblings ...)
  2020-05-12 20:08 ` Matthew Garrett
@ 2020-05-22 18:30 ` tip-bot2 for Loïc Yhuel
  4 siblings, 0 replies; 23+ messages in thread
From: tip-bot2 for Loïc Yhuel @ 2020-05-22 18:30 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: loic.yhuel, Javier Martinez Canillas, Jerry Snitselaar,
	Matthew Garrett, Ard Biesheuvel, x86, LKML

The following commit has been merged into the efi/urgent branch of tip:

Commit-ID:     b4f1874c62168159fdb419ced4afc77c1b51c475
Gitweb:        https://git.kernel.org/tip/b4f1874c62168159fdb419ced4afc77c1b51c475
Author:        Loïc Yhuel <loic.yhuel@gmail.com>
AuthorDate:    Tue, 12 May 2020 06:01:13 +02:00
Committer:     Ard Biesheuvel <ardb@kernel.org>
CommitterDate: Sun, 17 May 2020 11:46:50 +02:00

tpm: check event log version before reading final events

This fixes the boot issues since 5.3 on several Dell models when the TPM
is enabled. Depending on the exact grub binary, booting the kernel would
freeze early, or just report an error parsing the final events log.

We get an event log in the SHA-1 format, which doesn't have a
tcg_efi_specid_event_head in the first event, and there is a final events
table which doesn't match the crypto agile format.
__calc_tpm2_event_size reads bad "count" and "efispecid->num_algs", and
either fails, or loops long enough for the machine to be appear frozen.

So we now only parse the final events table, which is per the spec always
supposed to be in the crypto agile format, when we got a event log in this
format.

Fixes: c46f3405692de ("tpm: Reserve the TPM final events table")
Fixes: 166a2809d65b2 ("tpm: Don't duplicate events from the final event log in the TCG2 log")
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1779611
Signed-off-by: Loïc Yhuel <loic.yhuel@gmail.com>
Link: https://lore.kernel.org/r/20200512040113.277768-1-loic.yhuel@gmail.com
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
Reviewed-by: Matthew Garrett <mjg59@google.com>
[ardb: warn when final events table is missing or in the wrong format]
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/firmware/efi/libstub/tpm.c | 5 +++--
 drivers/firmware/efi/tpm.c         | 5 ++++-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c
index 1d59e10..e9a6846 100644
--- a/drivers/firmware/efi/libstub/tpm.c
+++ b/drivers/firmware/efi/libstub/tpm.c
@@ -54,7 +54,7 @@ void efi_retrieve_tpm2_eventlog(void)
 	efi_status_t status;
 	efi_physical_addr_t log_location = 0, log_last_entry = 0;
 	struct linux_efi_tpm_eventlog *log_tbl = NULL;
-	struct efi_tcg2_final_events_table *final_events_table;
+	struct efi_tcg2_final_events_table *final_events_table = NULL;
 	unsigned long first_entry_addr, last_entry_addr;
 	size_t log_size, last_entry_size;
 	efi_bool_t truncated;
@@ -127,7 +127,8 @@ void efi_retrieve_tpm2_eventlog(void)
 	 * Figure out whether any events have already been logged to the
 	 * final events structure, and if so how much space they take up
 	 */
-	final_events_table = get_efi_config_table(LINUX_EFI_TPM_FINAL_LOG_GUID);
+	if (version == EFI_TCG2_EVENT_LOG_FORMAT_TCG_2)
+		final_events_table = get_efi_config_table(LINUX_EFI_TPM_FINAL_LOG_GUID);
 	if (final_events_table && final_events_table->nr_events) {
 		struct tcg_pcr_event2_head *header;
 		int offset;
diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
index 31f9f0e..0543fbf 100644
--- a/drivers/firmware/efi/tpm.c
+++ b/drivers/firmware/efi/tpm.c
@@ -62,8 +62,11 @@ int __init efi_tpm_eventlog_init(void)
 	tbl_size = sizeof(*log_tbl) + log_tbl->size;
 	memblock_reserve(efi.tpm_log, tbl_size);
 
-	if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR)
+	if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR ||
+	    log_tbl->version != EFI_TCG2_EVENT_LOG_FORMAT_TCG_2) {
+		pr_warn(FW_BUG "TPM Final Events table missing or invalid\n");
 		goto out;
+	}
 
 	final_tbl = early_memremap(efi.tpm_final_log, sizeof(*final_tbl));
 

^ permalink raw reply related	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2020-05-22 18:30 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-12  4:01 [PATCH] tpm: check event log version before reading final events Loïc Yhuel
2020-05-12  6:44 ` Ard Biesheuvel
2020-05-12 11:40   ` Loïc Yhuel
2020-05-12 12:30     ` Ard Biesheuvel
2020-05-14 10:51       ` Jarkko Sakkinen
2020-05-14  1:09   ` Jarkko Sakkinen
2020-05-14  8:10     ` Ard Biesheuvel
2020-05-14 23:44       ` Jarkko Sakkinen
2020-05-12 17:45 ` Javier Martinez Canillas
2020-05-12 18:45 ` Jerry Snitselaar
2020-05-12 20:08 ` Matthew Garrett
2020-05-14 10:53   ` Jarkko Sakkinen
2020-05-14 11:28     ` Loïc Yhuel
2020-05-14 11:31       ` Ard Biesheuvel
2020-05-15  0:03         ` Jarkko Sakkinen
2020-05-14 11:33       ` Javier Martinez Canillas
2020-05-14 12:28         ` Ard Biesheuvel
2020-05-14 12:56           ` Javier Martinez Canillas
2020-05-14 13:04             ` Ard Biesheuvel
2020-05-14 13:51               ` Javier Martinez Canillas
2020-05-14 18:06                 ` Matthew Garrett
2020-05-15 15:55         ` Loïc Yhuel
2020-05-22 18:30 ` [tip: efi/urgent] " tip-bot2 for Loïc Yhuel

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.