| Message ID | YkeHXFvgB3MwXnuR@itl-email (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] Grab the EFI System Resource Table and check it | expand |
> On 2 Apr 2022, at 00:14, Demi Marie Obenour <demi@invisiblethingslab.com> 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@invisiblethingslab.com> Hi, I’ve tested the patch on an arm machine booting Xen+Dom0 through EFI, unfortunately I could not test the functionality. Cheers, Luca
On Mon, 4 Apr 2022, Luca Fancellu wrote: > > On 2 Apr 2022, at 00:14, Demi Marie Obenour <demi@invisiblethingslab.com> 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@invisiblethingslab.com> > > Hi, > > I’ve tested the patch on an arm machine booting Xen+Dom0 through EFI, unfortunately > I could not test the functionality. I understand you couldn't test ESRT but did the basic Xen+Dom0 boot via EFI on ARM work?
> On 5 Apr 2022, at 20:21, Stefano Stabellini <sstabellini@kernel.org> wrote: > > On Mon, 4 Apr 2022, Luca Fancellu wrote: >>> On 2 Apr 2022, at 00:14, Demi Marie Obenour <demi@invisiblethingslab.com> 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@invisiblethingslab.com> >> >> Hi, >> >> I’ve tested the patch on an arm machine booting Xen+Dom0 through EFI, unfortunately >> I could not test the functionality. > > I understand you couldn't test ESRT but did the basic Xen+Dom0 boot via > EFI on ARM work? Yes, I realise now I should have added *and it works* before the comma, without it the sentence is misleading. Cheers, Luca
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@invisiblethingslab.com> 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
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@invisiblethingslab.com> > > 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.
diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h index ae8627134e..767b2c9154 100644 --- 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) { int Index; EFI_MEMORY_DESCRIPTOR *desc_ptr = map; @@ -188,6 +189,7 @@ static EFI_STATUS __init efi_process_memory_map_bootinfo(EFI_MEMORY_DESCRIPTOR * desc_ptr->Type == EfiLoaderCode || desc_ptr->Type == EfiLoaderData || (!map_bs && + desc_ptr != esrt_desc && (desc_ptr->Type == EfiBootServicesCode || desc_ptr->Type == EfiBootServicesData))) ) { @@ -393,11 +395,12 @@ 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) { EFI_STATUS status; - status = efi_process_memory_map_bootinfo(map, map_size, desc_size); + status = efi_process_memory_map_bootinfo(map, map_size, desc_size, esrt_desc); if ( EFI_ERROR(status) ) blexit(L"EFI memory map processing failed"); diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h index d91eb5a537..6e6a5d2224 100644 --- 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; diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c index 4dd5ea6a06..ad5883133d 100644 --- 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[]; +}; + #define LOAD_OPTION_ACTIVE 0x00000001 union string { @@ -567,6 +584,38 @@ static int __init efi_check_dt_boot(const EFI_LOADED_IMAGE *loaded_image) } #endif +static UINTN __initdata esrt; + +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; + 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 ) + return false; + esrt_len = esrt_ptr->esrt_count * sizeof(esrt_ptr->esrt_entries[0]); + + return esrt_len > available_len - sizeof(*esrt_ptr); +} + /* * Include architecture specific implementation here, which references the * static globals defined above. @@ -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); + /* Obtain basic table pointers. */ for ( i = 0; i < efi_num_ct; ++i ) { @@ -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; } #ifndef CONFIG_ARM /* TODO - disabled until implemented on ARM */ @@ -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; + efi_memmap_size = info_size; status = SystemTable->BootServices->GetMemoryMap(&efi_memmap_size, efi_memmap, &map_key, @@ -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); efi_arch_pre_exit_boot(); diff --git a/xen/common/efi/efi.h b/xen/common/efi/efi.h index c9aa65d506..498c8dc1fa 100644 --- a/xen/common/efi/efi.h +++ b/xen/common/efi/efi.h @@ -27,7 +27,7 @@ extern const CHAR16 *efi_fw_vendor; extern const EFI_RUNTIME_SERVICES *efi_rs; extern UINTN efi_memmap_size, efi_mdesc_size; -extern void *efi_memmap; +extern void *efi_memmap, *esrt_desc; #ifdef CONFIG_X86 extern mfn_t efi_l4_mfn; diff --git a/xen/common/efi/runtime.c b/xen/common/efi/runtime.c index 13b0975866..9b6a06e651 100644 --- 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; UINT64 __read_mostly efi_boot_max_var_store_size; UINT64 __read_mostly efi_boot_remain_var_store_size; @@ -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; if ( info->mem.addr + info->mem.size < info->mem.addr || info->mem.addr + info->mem.size > desc->PhysicalStart + len ) diff --git a/xen/include/efi/efiapi.h b/xen/include/efi/efiapi.h index a616d1238a..42ef3e1c8c 100644 --- a/xen/include/efi/efiapi.h +++ b/xen/include/efi/efiapi.h @@ -882,6 +882,9 @@ typedef struct _EFI_BOOT_SERVICES { #define SAL_SYSTEM_TABLE_GUID \ { 0xeb9d2d32, 0x2d88, 0x11d3, {0x9a, 0x16, 0x0, 0x90, 0x27, 0x3f, 0xc1, 0x4d} } +#define ESRT_GUID \ + { 0xb122a263, 0x3661, 0x4f68, {0x99, 0x29, 0x78, 0xf8, 0xb0, 0xd6, 0x21, 0x80} } + typedef struct _EFI_CONFIGURATION_TABLE { EFI_GUID VendorGuid;
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@invisiblethingslab.com> --- 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(-)