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

[Usama Arif]

On 12/09/2024 16:21, Ard Biesheuvel wrote:
> 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.

I have tested above diff, and it works! No memory corruption.

The region comes up as ACPI data:
[    0.000000] BIOS-e820: [mem 0x000000007fb6d000-0x000000007fb7efff] ACPI data                                                                                                                               

and kexec doesnt interfere with it.

Thanks,
Usama