|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2] Grab the EFI System Resource Table and check it
On Wed, Apr 06, 2022 at 12:10:17PM +0200, Jan Beulich wrote: > On 02.04.2022 01:14, Demi Marie Obenour wrote: > > The EFI System Resource Table (ESRT) is necessary for fwupd to identify > > firmware updates to install. According to the UEFI specification §23.4, > > the table shall be stored in memory of type EfiBootServicesData. > > Therefore, Xen must avoid reusing that memory for other purposes, so > > that Linux can access the ESRT. Additionally, Xen must mark the memory > > as reserved, so that Linux knows accessing it is safe. > > > > See https://lore.kernel.org/xen-devel/20200818184018.GN1679@mail-itl/T/ > > for details. > > > > Signed-off-by: Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx> > > First a note on patch submission: Patches are sent To: the list, with > Cc: to involved maintainers (and perhaps other interested parties). Sorry about that — I am used to Linux which does things the other way around. > > --- > > xen/arch/arm/efi/efi-boot.h | 9 +++-- > > xen/arch/x86/efi/efi-boot.h | 5 ++- > > xen/common/efi/boot.c | 77 +++++++++++++++++++++++++++++++++++-- > > xen/common/efi/efi.h | 2 +- > > xen/common/efi/runtime.c | 5 ++- > > xen/include/efi/efiapi.h | 3 ++ > > 6 files changed, 89 insertions(+), 12 deletions(-) > > This being v2 (oddly enough with another v2 sent on Mar 30), you will > want to add a brief revision log clarifying to reviewers what has > changed. Together with the duplicate v2 it's not really clear whether > this was a plain resend, or whether anything actually changed (it > looks like you did address the Arm build issue). Only change was to break the ARM build IIRC. > > --- a/xen/arch/arm/efi/efi-boot.h > > +++ b/xen/arch/arm/efi/efi-boot.h > > @@ -176,7 +176,8 @@ static bool __init meminfo_add_bank(struct meminfo *mem, > > > > static EFI_STATUS __init > > efi_process_memory_map_bootinfo(EFI_MEMORY_DESCRIPTOR *map, > > UINTN mmap_size, > > - UINTN desc_size) > > + UINTN desc_size, > > + const > > EFI_MEMORY_DESCRIPTOR *const esrt_desc) > > Please omit the 2nd const (here and elsewhere) - no other parameters are > modified like this. Will fix in v3. > > --- a/xen/arch/x86/efi/efi-boot.h > > +++ b/xen/arch/x86/efi/efi-boot.h > > @@ -154,7 +154,8 @@ static void __init > > efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable, > > void *map, > > UINTN map_size, > > UINTN desc_size, > > - UINT32 desc_ver) > > + UINT32 desc_ver, > > + const EFI_MEMORY_DESCRIPTOR > > *const esrt_desc) > > { > > struct e820entry *e; > > unsigned int i; > > @@ -171,7 +172,7 @@ static void __init > > efi_arch_process_memory_map(EFI_SYSTEM_TABLE *SystemTable, > > { > > case EfiBootServicesCode: > > case EfiBootServicesData: > > - if ( map_bs ) > > + if ( map_bs || desc == esrt_desc ) > > { > > default: > > type = E820_RESERVED; > > How certain is it that this descriptor doesn't cover (much) more than > just ESRT? This could be quite wasteful in terms of memory which > wouldn't be reclaimed just because of the (perhaps small) ESRT. I can’t be certain at all, but this has the advantage of a much simpler and smaller patch. I don’t intend to fix this in v3, but I will send a follow-up patch that fixes it for x86. > > --- a/xen/common/efi/boot.c > > +++ b/xen/common/efi/boot.c > > @@ -93,6 +93,23 @@ typedef struct _EFI_LOAD_OPTION { > > CHAR16 Description[]; > > } EFI_LOAD_OPTION; > > > > +struct esrt_entry { > > + EFI_GUID fw_class; > > + UINT32 fw_type; > > + UINT32 fw_version; > > + UINT32 fw_lowest_supported_version; > > + UINT32 fw_capsule_flags; > > + UINT32 fw_last_attempt_version; > > + UINT32 fw_last_attempt_status; > > +}; > > + > > +struct esrt { > > + UINT32 esrt_count; > > + UINT32 esrt_max; > > + UINT64 esrt_version; > > + struct esrt_entry esrt_entries[]; > > +}; > > Please follow the naming and naming convention used by immediately > preceding structure definitions as well as the specification: All > names matching the spec and using typedef. That's how such structures > would eventually be added to the EFI headers we're inheriting from > the gnu-efi package. At such a point we would want to be able to > delete the declarations here without needing to touch any other code. > > > #define LOAD_OPTION_ACTIVE 0x00000001 > > Also please don't insert in the middle of two related definitions. > > > @@ -567,6 +584,38 @@ static int __init efi_check_dt_boot(const > > EFI_LOADED_IMAGE *loaded_image) > > } > > #endif > > > > +static UINTN __initdata esrt; > > Don't you need to initialize this to EFI_INVALID_TABLE_ADDR in order > for ... > > > +static bool __init is_esrt_valid( > > + const EFI_MEMORY_DESCRIPTOR *const desc) > > +{ > > + size_t available_len, esrt_len, len; > > + const UINTN physical_start = desc->PhysicalStart; > > + const struct esrt *esrt_ptr; > > + > > + len = desc->NumberOfPages << EFI_PAGE_SHIFT; > > + if ( esrt == EFI_INVALID_TABLE_ADDR ) > > + return false; > > ... this check to actually be useful? > > > + if ( physical_start > esrt || esrt - physical_start >= len ) > > + return false; > > + /* > > + * The specification requires EfiBootServicesData, but accept > > + * EfiRuntimeServicesData for compatibility > > + */ > > + if ( (desc->Type != EfiRuntimeServicesData) && > > + (desc->Type != EfiBootServicesData) ) > > + return false; > > + available_len = len - (esrt - physical_start); > > + if ( available_len < sizeof(*esrt_ptr) ) > > + return false; > > + esrt_ptr = (const struct esrt *)esrt; > > + if ( esrt_ptr->esrt_version != 1 || esrt_ptr->esrt_count <= 0 ) > > Nit: I think unsigned values would better not be compared for "<= 0"; > this wants to be "== 0" (or simply use the ! operator as we tend to > do elsewhere). > > > + return false; > > + esrt_len = esrt_ptr->esrt_count * sizeof(esrt_ptr->esrt_entries[0]); > > While presently we support EFI only for 64-bit architectures, this > can overflow for 32-bit ones. Better to guard against this right > away. This could be achieved implicitly by ... > > > + return esrt_len > available_len - sizeof(*esrt_ptr); > > return esrt_ptr->esrt_count <= > (available_len - sizeof(*esrt_ptr)) / > sizeof(esrt_ptr->esrt_entries[0]); > > (also correcting > to <= at the same time). > > > @@ -846,6 +895,10 @@ static void __init efi_tables(void) > > { > > unsigned int i; > > > > + BUILD_BUG_ON(sizeof(struct esrt_entry) != 40); > > + BUILD_BUG_ON(__alignof(struct esrt_entry) != 4); > > + BUILD_BUG_ON(sizeof(struct esrt) != 16); > > What are these about? I don't think we have similar checks for > other interface definitions, and I don't see why we would need > such. These are there because I wanted to make sure I didn’t mess up when writing the struct definitions. Will remove in v3. > > @@ -854,6 +907,7 @@ static void __init efi_tables(void) > > static EFI_GUID __initdata mps_guid = MPS_TABLE_GUID; > > static EFI_GUID __initdata smbios_guid = SMBIOS_TABLE_GUID; > > static EFI_GUID __initdata smbios3_guid = SMBIOS3_TABLE_GUID; > > + static EFI_GUID __initdata esrt_guid = ESRT_GUID; > > > > if ( match_guid(&acpi2_guid, &efi_ct[i].VendorGuid) ) > > efi.acpi20 = (long)efi_ct[i].VendorTable; > > @@ -865,6 +919,8 @@ static void __init efi_tables(void) > > efi.smbios = (long)efi_ct[i].VendorTable; > > if ( match_guid(&smbios3_guid, &efi_ct[i].VendorGuid) ) > > efi.smbios3 = (long)efi_ct[i].VendorTable; > > + if ( match_guid(&esrt_guid, &efi_ct[i].VendorGuid) ) > > + esrt = (long)efi_ct[i].VendorTable; > > I think all the casts to "long" are bogus here. At the very least > this ought to be "unsigned long", but I think in your case it > actually wants to be UINTN (the destination variable's type). Also, > while I guess you simply derived the two new lines by copy-and- > paste, please avoid introducing yet another instance of the pre- > exisiting indentation issues (hard tab and too deep when tabs > expand to the usual 8 blank positions). Will fix in v3. > > @@ -1053,19 +1109,19 @@ static void __init efi_exit_boot(EFI_HANDLE > > ImageHandle, EFI_SYSTEM_TABLE *Syste > > EFI_STATUS status; > > UINTN info_size = 0, map_key; > > bool retry; > > -#ifdef CONFIG_EFI_SET_VIRTUAL_ADDRESS_MAP > > unsigned int i; > > -#endif > > > > efi_bs->GetMemoryMap(&info_size, NULL, &map_key, > > &efi_mdesc_size, &mdesc_ver); > > - info_size += 8 * efi_mdesc_size; > > + info_size += 8 * (efi_mdesc_size + 1); > > efi_memmap = efi_arch_allocate_mmap_buffer(info_size); > > if ( !efi_memmap ) > > blexit(L"Unable to allocate memory for EFI memory map"); > > > > for ( retry = false; ; retry = true ) > > { > > + esrt_desc = (EFI_MEMORY_DESCRIPTOR*)EFI_INVALID_TABLE_ADDR; > > Nit: Missing blank before * and perhaps wants to cast to pointer-to- > const. Will change in v3. > > @@ -1074,8 +1130,21 @@ static void __init efi_exit_boot(EFI_HANDLE > > ImageHandle, EFI_SYSTEM_TABLE *Syste > > if ( EFI_ERROR(status) ) > > PrintErrMesg(L"Cannot obtain memory map", status); > > > > + for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size ) > > + { > > + if ( is_esrt_valid(efi_memmap + i) ) > > + { > > + esrt_desc = efi_memmap + i; > > + break; > > + } > > + } > > + > > + /* > > + * We cannot pass esrt because we need to explicitly compare the > > + * descriptor pointers for equality. > > + */ > > efi_arch_process_memory_map(SystemTable, efi_memmap, > > efi_memmap_size, > > - efi_mdesc_size, mdesc_ver); > > + efi_mdesc_size, mdesc_ver, esrt_desc); > > Since esrt_desc is a global variable, why do you pass it as an argument > here? I was trying to minimize the use of globals. Will change in v3. > > --- a/xen/common/efi/runtime.c > > +++ b/xen/common/efi/runtime.c > > @@ -52,6 +52,7 @@ static unsigned int efi_rs_on_cpu = NR_CPUS; > > UINTN __read_mostly efi_memmap_size; > > UINTN __read_mostly efi_mdesc_size; > > void *__read_mostly efi_memmap; > > +void *__read_mostly esrt_desc; > > Why "void *" and not pointer-to-const? > > > @@ -269,14 +270,14 @@ int efi_get_info(uint32_t idx, union xenpf_efi_info > > *info) > > case XEN_FW_EFI_MEM_INFO: > > for ( i = 0; i < efi_memmap_size; i += efi_mdesc_size ) > > { > > - EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i; > > + const EFI_MEMORY_DESCRIPTOR *desc = efi_memmap + i; > > u64 len = desc->NumberOfPages << EFI_PAGE_SHIFT; > > > > if ( info->mem.addr >= desc->PhysicalStart && > > info->mem.addr < desc->PhysicalStart + len ) > > { > > info->mem.type = desc->Type; > > - info->mem.attr = desc->Attribute; > > + info->mem.attr = desc == esrt_desc ? EFI_MEMORY_RUNTIME : > > desc->Attribute; > > Why this override? This tells the dom0 kernel that the memory was reserved by Xen, even though it is of type EfiBootServicesData. Otherwise, dom0 might think that the memory has been overwritten with garbage. I will use a different solution. -- Sincerely, Demi Marie Obenour (she/her/hers) Invisible Things Lab Attachment:
signature.asc
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |