Re: [PATCH] efistub/tpm: Use ACPI reclaim memory for event log to avoid corruption

[Usama Arif <usamaarif642@gmail.com>]

On 25/10/2024 06:09, Jiri Slaby wrote:
> On 25. 10. 24, 7:07, Jiri Slaby wrote:
>> On 24. 10. 24, 18:20, Jiri Slaby wrote:
>>> On 12. 09. 24, 17:52, Ard Biesheuvel wrote:
>>>> From: Ard Biesheuvel <ardb@kernel.org>
>>>>
>>>> The TPM event log table is a Linux specific construct, where the data
>>>> produced by the GetEventLog() boot service is cached in memory, and
>>>> passed on to the OS using a EFI configuration table.
>>>>
>>>> The use of EFI_LOADER_DATA here results in the region being left
>>>> unreserved in the E820 memory map constructed by the EFI stub, and this
>>>> is the memory description that is passed on to the incoming kernel by
>>>> kexec, which is therefore unaware that the region should be reserved.
>>>>
>>>> Even though the utility of the TPM2 event log after a kexec is
>>>> questionable, any corruption might send the parsing code off into the
>>>> weeds and crash the kernel. So let's use EFI_ACPI_RECLAIM_MEMORY
>>>> instead, which is always treated as reserved by the E820 conversion
>>>> logic.
>>>>
>>>> Cc: <stable@vger.kernel.org>
>>>> Reported-by: Breno Leitao <leitao@debian.org>
>>>> Tested-by: Usama Arif <usamaarif642@gmail.com>
>>>> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>>>> ---
>>>>   drivers/firmware/efi/libstub/tpm.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/ efi/libstub/tpm.c
>>>> index df3182f2e63a..1fd6823248ab 100644
>>>> --- 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);
>>>
>>> Hi,
>>>
>>> this, for some reason, corrupts system configuration table. On good boots, memattr points to 0x77535018, on bad boots (this commit applied), it points to 0x77526018.
>>>
>>> And the good content at 0x77526018:
>>> tab=0x77526018 size=16+45*48=0x0000000000000880
>>>
>>> bad content at 0x77535018:
>>> tab=0x77535018 size=16+2*1705353216=0x00000000cb4b4010
>>>
>>> This happens only on cold boots. Subsequent boots (having the commit or not) are all fine.
>>>
>>> Any ideas?
>>
>> ====
>> EFI_ACPI_RECLAIM_MEMORY
>>
>> This memory is to be preserved by the UEFI OS loader and OS until ACPI
>> is enabled. Once ACPI is enabled, the memory in this range is available for general use.
>> ====
>>
>> BTW doesn't the above mean it is released by the time TPM actually reads it?
>>
>> Isn't the proper fix to actually memblock_reserve() that TPM portion. The same as memattr in efi_memattr_init()?
> 
> And this is actually done in efi_tpm_eventlog_init().
> 
>>> DMI: Dell Inc. Latitude 7290/09386V, BIOS 1.39.0 07/04/2024
>>>
>>> This was reported downstream at:
>>> https://bugzilla.suse.com/show_bug.cgi?id=1231465
>>>
>>> thanks,
> 

Could you share the e820 map, reserve setup_data and TPMEventLog address with and without the patch?
All of these should be just be in the dmesg.

Thanks,
Usama