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

[Jiri Slaby <jirislaby@kernel.org>]

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,

-- 
js
suse labs