Re: [PATCH v2 6/6] efi: Apply allowlist to EFI configuration tables when running under Xen

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

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

On Mon, Oct 03, 2022 at 01:26:25PM +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 when running under Xen, check the EFI memory descriptor covering the
> start of the table, and disregard the table if it does not reside in
> memory that is preserved by Xen.
> 
> 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          | 24 ++++++++++++++++++++
>  include/linux/efi.h        |  2 ++
>  3 files changed, 33 insertions(+)
> 
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 2c12b1a06481..0a4583c13a40 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -560,6 +560,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 74f3f6d8cdc8..c275a9c377fe 100644
> --- a/drivers/xen/efi.c
> +++ b/drivers/xen/efi.c
> @@ -326,3 +326,27 @@ int efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md)
>  
>          return 0;
>  }
> +
> +bool __init xen_efi_config_table_is_usable(const efi_guid_t *guid,
> +					   unsigned long table)
> +{
> +	efi_memory_desc_t md;
> +	int rc;
> +
> +	if (!efi_enabled(EFI_PARAVIRT))
> +		return true;
> +
> +	rc = efi_mem_desc_lookup(table, &md);
> +	if (rc)
> +		return false;
> +
> +	switch (md.type) {
> +	case EFI_RUNTIME_SERVICES_CODE:
> +	case EFI_RUNTIME_SERVICES_DATA:
> +	case EFI_ACPI_RECLAIM_MEMORY:
> +	case EFI_RESERVED_TYPE:

Some firmware uses EFI_ACPI_MEMORY_NVS to store ACPI tables, so this
needs to be added to the allowlist.  Otherwise affected systems will not
boot.  Xen treats EFI_ACPI_MEMORY_NVS the way it treats
EFI_ACPI_RECLAIM_MEMORY, so this is safe.

> +		return true;
> +	}
> +
> +	return false;
> +}
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index e0ee6f6da4b4..b0cba86352ce 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1352,4 +1352,6 @@ struct linux_efi_initrd {
>  /* Header of a populated EFI secret area */
>  #define EFI_SECRET_TABLE_HEADER_GUID	EFI_GUID(0x1e74f542, 0x71dd, 0x4d66,  0x96, 0x3e, 0xef, 0x42, 0x87, 0xff, 0x17, 0x3b)
>  
> +bool xen_efi_config_table_is_usable(const efi_guid_t *, unsigned long table);
> +
>  #endif /* _LINUX_EFI_H */
> -- 
> 2.35.1
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]