On Sun, Oct 02, 2022 at 11:56:25AM +0200, Ard Biesheuvel wrote: > As it turns out, Xen does not guarantee that EFI bootservices data > regions in memory are preserved, which means that EFI configuration > tables pointing into such memory regions may be corrupted before the > dom0 OS has had a chance to inspect them. > > Demi Marie reports that this is causing problems for Qubes OS when it > attempts to perform system firmware updates, which requires that the > contents of the ESRT configuration table are valid when the fwupd user > space program runs. > > However, other configuration tables such as the memory attributes > table or the runtime properties table are equally affected, and so we > need a comprehensive workaround that works for any table type. > > So let's first disregard all EFI configuration tables except the ones > that are known (or can be expected) to reside in memory regions of a > type that Xen preserves, i.e., ACPI tables (which are passed in > EfiAcpiReclaimMemory regions) and SMBIOS tables (which are usually > passed in EfiRuntimeServicesData regions, even though the UEFI spec only > mentions this as a recommendation). Then, cross reference unknown tables > against either the EFI memory map (if available) or do a Xen hypercall > to determine the memory type, and allow the config table if the type is > one that is guaranteed to be preserved. > > Future patches can augment the logic in this routine to allow other > table types based on the size of the allocation, or based on a table > specific header size field. > > Co-developed-by: Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx> > Signed-off-by: Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx> > Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx> > --- > drivers/firmware/efi/efi.c | 7 ++ > drivers/xen/efi.c | 69 ++++++++++++++++++++ > include/linux/efi.h | 2 + > 3 files changed, 78 insertions(+) > > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c > index 11857af72859..e8c0747011d7 100644 > --- a/drivers/firmware/efi/efi.c > +++ b/drivers/firmware/efi/efi.c > @@ -556,6 +556,13 @@ static __init int match_config_table(const efi_guid_t *guid, > > for (i = 0; efi_guidcmp(table_types[i].guid, NULL_GUID); i++) { > if (!efi_guidcmp(*guid, table_types[i].guid)) { > + if (IS_ENABLED(CONFIG_XEN_EFI) && > + !xen_efi_config_table_is_usable(guid, table)) { > + if (table_types[i].name[0]) > + pr_cont("(%s=0x%lx) ", > + table_types[i].name, table); > + return 1; > + } > *(table_types[i].ptr) = table; > if (table_types[i].name[0]) > pr_cont("%s=0x%lx ", > diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c > index d1ff2186ebb4..3f1f365b37d0 100644 > --- a/drivers/xen/efi.c > +++ b/drivers/xen/efi.c > @@ -292,3 +292,72 @@ void __init xen_efi_runtime_setup(void) > efi.get_next_high_mono_count = xen_efi_get_next_high_mono_count; > efi.reset_system = xen_efi_reset_system; > } > + > +static const efi_guid_t cfg_table_allow_list[] __initconst = { > + ACPI_20_TABLE_GUID, > + ACPI_TABLE_GUID, > + SMBIOS_TABLE_GUID, > + SMBIOS3_TABLE_GUID, > +}; This allowlist seems redundant. Either the tables are already in memory that Xen will preserve or they aren’t. In both cases the subsequent code will do the right thing. -- Sincerely, Demi Marie Obenour (she/her/hers) Invisible Things Lab
Attachment:
signature.asc
Description: PGP signature