Re: [RFC PATCH 5/5] efi: esrt: Omit region sanity check when no memory map is available

[Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>]

On Sun, Oct 02, 2022 at 11:56:26AM +0200, Ard Biesheuvel wrote:
> In order to permit the ESRT to be used when doing pseudo-EFI boot
> without a EFI memory map, e.g., when booting inside a Xen dom0 on x86,
> make the sanity checks optional based on whether the memory map is
> available.
> 
> If additional validation is needed, it is up to the Xen EFI glue code to
> implement this in its xen_efi_config_table_is_valid() helper, or provide
> a EFI memory map like it does on other architectures.

I don’t like this.  It is easy to use a hypercall to get the end of the
memory region containing the config table, which is what my one of my
previous patches actually does.  Skipping all of the validation could
easily lead to a regression.  I understand wanting to get Xen-specific
code out of esrt.c, but this isn’t the answer.  Some sort of abstraction
over both cases would be a much better solution.

> Co-developed-by: Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
> ---
>  arch/x86/platform/efi/quirks.c |  3 +
>  drivers/firmware/efi/esrt.c    | 61 +++++++++++---------
>  2 files changed, 37 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
> index b0b848d6933a..9307be2f4afa 100644
> --- a/arch/x86/platform/efi/quirks.c
> +++ b/arch/x86/platform/efi/quirks.c
> @@ -250,6 +250,9 @@ void __init efi_arch_mem_reserve(phys_addr_t addr, u64 
> size)
>       int num_entries;
>       void *new;
>  
> +     if (!efi_enabled(EFI_MEMMAP))
> +             return;
> +

This function does not actually work under Xen, even if EFI_MEMMAP is
set.  When running under Xen, either this function must never be
called (in which case there should be at least a WARN()), or it should
return an error that callers must check for.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature