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

[Dave Young]

On Sat, 14 Sept 2024 at 16:31, Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
>
> On Sat, 14 Sept 2024 at 08:46, Dave Young <dyoung@xxxxxxxxxx> wrote:
> >
> > On Fri, 13 Sept 2024 at 18:56, Dave Young <dyoung@xxxxxxxxxx> wrote:
> > >
> > > On Thu, 12 Sept 2024 at 22:15, Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
> > > >
> > > > (cc Dave)
> > >
> > > Thanks for ccing me.
> > >
> > > >
> > > > Full thread here:
> > > > https://lore.kernel.org/all/CAMj1kXG1hbiafKRyC5qM1Vj5X7x-dmLndqqo2AYnHMRxDz-80w@xxxxxxxxxxxxxx/T/#u
> > > >
> > > > On Thu, 12 Sept 2024 at 16:05, Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
> > > > >
> > > > > On Thu, 12 Sept 2024 at 15:55, Usama Arif <usamaarif642@xxxxxxxxx> wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > On 12/09/2024 14:10, Ard Biesheuvel wrote:
> > > > > > > Does the below help at all?
> > > > > > >
> > > > > > > --- a/drivers/firmware/efi/tpm.c
> > > > > > > +++ b/drivers/firmware/efi/tpm.c
> > > > > > > @@ -60,7 +60,7 @@ int __init efi_tpm_eventlog_init(void)
> > > > > > >         }
> > > > > > >
> > > > > > >         tbl_size = sizeof(*log_tbl) + log_tbl->size;
> > > > > > > -       memblock_reserve(efi.tpm_log, tbl_size);
> > > > > > > +       efi_mem_reserve(efi.tpm_log, tbl_size);
> > > > > > >
> > > > > > >         if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR) {
> > > > > > >                 pr_info("TPM Final Events table not present\n");
> > > > > >
> > > > > > Unfortunately not. efi_mem_reserve updates e820_table, while kexec looks at /sys/firmware/memmap
> > > > > > which is e820_table_firmware.

Updating e820_table should be good enough, it depends on where the
corruption is happening.

kexec will find a suitable memory for the kernel via searching through
the system ram resources.   So efi_mem_reserve will update e820_table,
then reserve in the resources list as E820_TYPE_RESERVED, thus it
should not be a problem.
During the 2nd kernel boot phase, it is carried as EFI_LOADER_DATA
with EFI_MEMORY_RUNTIME attribute, I think it is also fine,  and later
efi_mem_reserve will be called as what have been done in previous
kernel.

So I think no need to update the e820_table_kexec and e820_table_firmware

> > > > > >
> > > > > > arch_update_firmware_area introduced in the RFC patch does the same thing as efi_mem_reserve does at
> > > > > > its end, just with e820_table_firmware instead of e820_table.
> > > > > > i.e. efi_mem_reserve does:
> > > > > >         e820__range_update(addr, size, E820_TYPE_RAM, E820_TYPE_RESERVED);
> > > > > >         e820__update_table(e820_table);
> > > > > >
> > > > > > while arch_update_firmware_area does:
> > > > > >         e820__range_update_firmware(addr, size, E820_TYPE_RAM, E820_TYPE_RESERVED);
> > > > > >         e820__update_table(e820_table_firmware);
> > > > > >
> > > > >
> > > > > Shame.
> > > > >
> > > > > Using efi_mem_reserve() is appropriate here in any case, but I guess
> > > > > kexec on x86 needs to be fixed to juggle the EFI memory map, memblock
> > > > > table, and 3 (!) versions of the E820 table in the correct way
> > > > > (e820_table, e820_table_kexec and e820_table_firmware)
> > > > >
> > > > > Perhaps we can put this additional logic in x86's implementation of
> > > > > efi_arch_mem_reserve()? AFAICT, all callers of efi_mem_reserve() deal
> > > > > with configuration tables produced by the firmware that may not be
> > > > > reserved correctly if kexec looks at e820_table_firmware[] only.
> > > >
> > >
> > > I have not read all the conversations,  let me have a look and response later.
> > >
> >
> > I'm still confused after reading the code about why this issue can
> > still happen with a efi_mem_reserve.
> > Usama, Breno, could any of you share the exact steps on how to
> > reproduce this issue with a kvm guest?
> >
>
> The code does not use efi_mem_reserve() only memblock_reserve().

Yes, I see this, I just thought that Usama tested with changes to
efi_mem_reserve and it still did not work, this is what I'm confused
about.

But maybe Usama did not test and only checked the code and assumed
that we have to update the e820_table_kexec and e820_table_firmware.
See my reply inline above.

Thanks
Dave
>