[PATCH] tpm: check event log version before reading final events

[PATCH] tpm: check event log version before reading final events

[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

Re: [PATCH] tpm: check event log version before reading final events

[Ard Biesheuvel]

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
>

Re: [PATCH] tpm: check event log version before reading final events

[Loïc Yhuel]

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.

Re: [PATCH] tpm: check event log version before reading final events

[Ard Biesheuvel]

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.

Re: [PATCH] tpm: check event log version before reading final events

[Jarkko Sakkinen]

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

Re: [PATCH] tpm: check event log version before reading final events

[Jarkko Sakkinen]

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

Re: [PATCH] tpm: check event log version before reading final events

[Ard Biesheuvel]

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?

Re: [PATCH] tpm: check event log version before reading final events

[Jarkko Sakkinen]

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

Re: [PATCH] tpm: check event log version before reading final events

[Javier Martinez Canillas]

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

Re: [PATCH] tpm: check event log version before reading final events

[Jerry Snitselaar]

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>

Re: [PATCH] tpm: check event log version before reading final events

[Matthew Garrett]

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>

Re: [PATCH] tpm: check event log version before reading final events

[Jarkko Sakkinen]

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

Re: [PATCH] tpm: check event log version before reading final events

[Loïc Yhuel]

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 ?

Re: [PATCH] tpm: check event log version before reading final events

[Ard Biesheuvel]

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?

Re: [PATCH] tpm: check event log version before reading final events

[Jarkko Sakkinen]

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

Re: [PATCH] tpm: check event log version before reading final events

[Javier Martinez Canillas]

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

Re: [PATCH] tpm: check event log version before reading final events

[Ard Biesheuvel]

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
>

Re: [PATCH] tpm: check event log version before reading final events

[Javier Martinez Canillas]

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

Re: [PATCH] tpm: check event log version before reading final events

[Ard Biesheuvel]

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,

Re: [PATCH] tpm: check event log version before reading final events

[Javier Martinez Canillas]

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

Re: [PATCH] tpm: check event log version before reading final events

[Matthew Garrett]

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.

Re: [PATCH] tpm: check event log version before reading final events

[Loïc Yhuel]

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.