Re: [RFC PATCH 4/5] efi: Apply allowlist to EFI configuration tables when running under Xen

[Demi Marie Obenour <demi@invisiblethingslab.com>]

[-- Attachment #1: Type: text/plain, Size: 3465 bytes --]

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@invisiblethingslab.com>
> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]