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

[Ard Biesheuvel <ardb@xxxxxxxxxx>]

On Thu, 14 May 2020 at 14:56, Javier Martinez Canillas
<javierm@xxxxxxxxxx> 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,