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

[Ard Biesheuvel <ardb@xxxxxxxxxx>]

On Sun, 2 Oct 2022 at 18:28, Demi Marie Obenour
<demi@xxxxxxxxxxxxxxxxxxxxxx> wrote:
>
> 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 don't like putting Xen specific hacks left and right because Xen on
x86 cannot be bothered to provide an EFI memory map. And as for
regressions, ESRT does not work at all under Xen (given the lack of a
memory map) and so I fail to see how this could break a currently
working case.

>  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.
>

We have such an abstraction already, it is called the EFI memory map.

So there are two options here:
- expose a EFI memory map
- add a ESRT specific check to xen_efi_config_table_is_valid() so that
the ESRT is withheld from dom0 if there is something wrong with it.

And frankly, the validation itself could use some attention as well:

"""
rc = efi_mem_desc_lookup(efi.esrt, &md);
...
max = efi_mem_desc_end(&md);
if (max < efi.esrt) {
"""

Unless I am missing something, this can never occur so the check is
pointless and the pr_err() that follows is unreachable.

Then we have

"""
size = sizeof(*esrt);
max -= efi.esrt;

if (max < size) {
"""

'size' is 16 bytes here, so the only way this can become true is if
the memory descriptor describes a region of 0 pages in length, which
is explicitly forbidden by the EFI spec. If such a descriptor exists
in spite of that, this is a memory map problem not a ESRT problem.

So actually, instead of making these checks conditional on EFI_MEMMAP
being set, I might just rip them out entirely and be done with it.



> > 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