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

[Usama Arif]

On 11/09/2024 12:51, Ard Biesheuvel wrote:
> On Wed, 11 Sept 2024 at 12:41, Usama Arif <usamaarif642@xxxxxxxxx> wrote:
>>
>> Looking at the TPM spec [1]
>>
>> If the ACPI TPM2 table contains the address and size of the Platform
>> Firmware TCG log, firmware “pins” the memory associated with the
>> Platform FirmwareTCG log, and reports this memory as “Reserved” memory
>> via the INT 15h/E820 interface.
>>
>> It looks like the firmware should pass this as reserved in e820 memory
>> map. However, it doesn't seem to. The firmware being tested on is:
>> dmidecode -s bios-version
>> edk2-20240214-2.el9
>>
>> When this area is not reserved, it comes up as usable in
>> /sys/firmware/memmap. This means that kexec, which uses that memmap
>> to find usable memory regions, can select the region where efi.tpm_log
>> is and overwrite it and relocate_kernel.
>>
>> Having a fix in firmware can be difficult to get through. As a secondary
>> fix, this patch marks that region as reserved in e820_table_firmware if it
>> is currently E820_TYPE_RAM so that kexec doesn't use it for kernel segments.
>>
>> [1] https://trustedcomputinggroup.org/wp-content/uploads/PC-ClientPlatform_Profile_for_TPM_2p0_Systems_v49_161114_public-review.pdf
>>
>> Signed-off-by: Usama Arif <usamaarif642@xxxxxxxxx>
Forgot to add:

Reported-by: Breno Leitao <leitao@xxxxxxxxxx>

> 
> I would expect the EFI memory map to E820 conversion implemented in
> the EFI stub to take care of this.
> 
> If you are not booting via the EFI stub, the bootloader is performing
> this conversion, and so it should be done there instead.
> 
I will look into this and report back.
Thanks

> 
>> ---
>>  arch/x86/include/asm/e820/api.h | 2 ++
>>  arch/x86/kernel/e820.c          | 6 ++++++
>>  arch/x86/platform/efi/efi.c     | 9 +++++++++
>>  drivers/firmware/efi/tpm.c      | 2 +-
>>  include/linux/efi.h             | 7 +++++++
>>  5 files changed, 25 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/include/asm/e820/api.h b/arch/x86/include/asm/e820/api.h
>> index 2e74a7f0e935..4e9aa24f03bd 100644
>> --- a/arch/x86/include/asm/e820/api.h
>> +++ b/arch/x86/include/asm/e820/api.h
>> @@ -16,6 +16,8 @@ extern bool e820__mapped_all(u64 start, u64 end, enum e820_type type);
>>
>>  extern void e820__range_add   (u64 start, u64 size, enum e820_type type);
>>  extern u64  e820__range_update(u64 start, u64 size, enum e820_type old_type, enum e820_type new_type);
>> +extern u64  e820__range_update_firmware(u64 start, u64 size, enum e820_type old_type,
>> +                                       enum e820_type new_type);
>>  extern u64  e820__range_remove(u64 start, u64 size, enum e820_type old_type, bool check_type);
>>  extern u64  e820__range_update_table(struct e820_table *t, u64 start, u64 size, enum e820_type old_type, enum e820_type new_type);
>>
>> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
>> index 4893d30ce438..912400161623 100644
>> --- a/arch/x86/kernel/e820.c
>> +++ b/arch/x86/kernel/e820.c
>> @@ -538,6 +538,12 @@ u64 __init e820__range_update_table(struct e820_table *t, u64 start, u64 size,
>>         return __e820__range_update(t, start, size, old_type, new_type);
>>  }
>>
>> +u64 __init e820__range_update_firmware(u64 start, u64 size, enum e820_type old_type,
>> +                                      enum e820_type new_type)
>> +{
>> +       return __e820__range_update(e820_table_firmware, start, size, old_type, new_type);
>> +}
>> +
>>  /* Remove a range of memory from the E820 table: */
>>  u64 __init e820__range_remove(u64 start, u64 size, enum e820_type old_type, bool check_type)
>>  {
>> diff --git a/arch/x86/platform/efi/efi.c b/arch/x86/platform/efi/efi.c
>> index 88a96816de9a..aa95f77d7a30 100644
>> --- a/arch/x86/platform/efi/efi.c
>> +++ b/arch/x86/platform/efi/efi.c
>> @@ -171,6 +171,15 @@ static void __init do_add_efi_memmap(void)
>>         e820__update_table(e820_table);
>>  }
>>
>> +/* Reserve firmware area if it was marked as RAM */
>> +void arch_update_firmware_area(u64 addr, u64 size)
>> +{
>> +       if (e820__get_entry_type(addr, addr + size) == E820_TYPE_RAM) {
>> +               e820__range_update_firmware(addr, size, E820_TYPE_RAM, E820_TYPE_RESERVED);
>> +               e820__update_table(e820_table_firmware);
>> +       }
>> +}
>> +
>>  /*
>>   * Given add_efi_memmap defaults to 0 and there is no alternative
>>   * e820 mechanism for soft-reserved memory, import the full EFI memory
>> diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
>> index e8d69bd548f3..8e6e7131d718 100644
>> --- a/drivers/firmware/efi/tpm.c
>> +++ b/drivers/firmware/efi/tpm.c
>> @@ -60,6 +60,7 @@ int __init efi_tpm_eventlog_init(void)
>>         }
>>
>>         tbl_size = sizeof(*log_tbl) + log_tbl->size;
>> +       arch_update_firmware_area(efi.tpm_log, tbl_size);
>>         memblock_reserve(efi.tpm_log, tbl_size);
>>
>>         if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR) {
>> @@ -107,4 +108,3 @@ int __init efi_tpm_eventlog_init(void)
>>         early_memunmap(log_tbl, sizeof(*log_tbl));
>>         return ret;
>>  }
>> -
>> diff --git a/include/linux/efi.h b/include/linux/efi.h
>> index 6bf3c4fe8511..9c239cdff771 100644
>> --- a/include/linux/efi.h
>> +++ b/include/linux/efi.h
>> @@ -1371,4 +1371,11 @@ extern struct blocking_notifier_head efivar_ops_nh;
>>  void efivars_generic_ops_register(void);
>>  void efivars_generic_ops_unregister(void);
>>
>> +#ifdef CONFIG_X86_64
>> +void __init arch_update_firmware_area(u64 addr, u64 size);
>> +#else
>> +static inline void __init arch_update_firmware_area(u64 addr, u64 size)
>> +{
>> +}
>> +#endif
>>  #endif /* _LINUX_EFI_H */
>> --
>> 2.43.5
>>