Re: [RFC] efi/tpm: add efi.tpm_log as a reserved region in 820_table_firmware

[Ard Biesheuvel]

On Thu, 12 Sept 2024 at 16:29, Breno Leitao <leitao@xxxxxxxxxx> wrote:
>
> On Thu, Sep 12, 2024 at 03:10:43PM +0200, Ard Biesheuvel wrote:
> > On Thu, 12 Sept 2024 at 15:03, Breno Leitao <leitao@xxxxxxxxxx> wrote:
> > > On Thu, Sep 12, 2024 at 12:51:57PM +0200, Ard Biesheuvel wrote:
> > > > I don't see how this could be an EFI bug, given that it does not deal
> > > > with E820 tables at all.
> > >
> > > I want to back up a little bit and make sure I am following the
> > > discussion.
> > >
> > > From what I understand from previous discussion, we have an EFI bug as
> > > the root cause of this issue.
> > >
> > > This happens because the EFI does NOT mark the EFI TPM event log memory
> > > region as reserved (EFI_RESERVED_TYPE).
> >
> > Why do you think EFI should use EFI_RESERVED_TYPE in this case?
> >
> > The EFI spec is very clear that EFI_RESERVED_TYPE really shouldn't be
> > used for anything by EFI itself. It is quite common for EFI
> > configuration tables to be passed as EfiRuntimeServicesData (SMBIOS),
> > EfiBootServicesData (ESRT) or EFiAcpiReclaim (ACPI tables).
> >
> > Reserved memory is mostly for memory that even the firmware does not
> > know what it is for, i.e., particular platform specific uses.
> >
> > In general, it is up to the OS to ensure that EFI configuration tables
> > that it cares about should be reserved in the correct way.
>
> Thanks for the explanation.
>
> So, if I understand what you meant here, the TPM event log memory range
> shouldn't be listed as a memory region in EFI memory map (as passed by
> the firmware to the OS).
>
> Hence, this is not a EFI firmware bug, but a OS/Kernel bug.
>
> Am I correct with the statements above?
>

No, not entirely. But I also missed the face that this table is
actually created by the EFI stub in Linux, not the firmware. It is
*not* the same memory region that the TPM2 ACPI table describes, and
so what the various specs say about that is entirely irrelevant.

The TPM event log configuration table is created by the EFI stub,
which uses the TCG2::GetEventLog () protocol method to obtain it. This
must be done by the EFI stub because these protocols will no longer be
available once the OS boots. But the data is not used by the EFI stub,
only by the OS, which is why it is passed in memory like this.

The memory in question is allocated as EFI_LOADER_DATA, and so we are
relying on the OS to know that this memory is special, and needs to be
preserved.

I think the solution here is to use a different memory type:

--- a/drivers/firmware/efi/libstub/tpm.c
+++ b/drivers/firmware/efi/libstub/tpm.c
@@ -96,7 +96,7 @@ static void efi_retrieve_tcg2_eventlog(int version,
efi_physical_addr_t log_loca
        }

        /* Allocate space for the logs and copy them. */
-       status = efi_bs_call(allocate_pool, EFI_LOADER_DATA,
+       status = efi_bs_call(allocate_pool, EFI_ACPI_RECLAIM_MEMORY,
                             sizeof(*log_tbl) + log_size, (void **)&log_tbl);

        if (status != EFI_SUCCESS) {

which will be treated appropriately by the existing EFI-to-E820
conversion logic.