|
[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 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). > --- > 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). > --- 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. > --- 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. > --- 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. > @@ -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). > @@ -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. > @@ -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? > --- 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? Jan
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |