Re: [PATCH] efi: discover ESRT table on Xen PV too

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Mon, Aug 17, 2020 at 11:00:13AM +0200, Roger Pau Monné wrote:
> On Sun, Aug 16, 2020 at 02:19:49AM +0200, Marek Marczykowski-Górecki wrote:
> > In case of Xen PV dom0, Xen passes along info about system tables (see
> > arch/x86/xen/efi.c), but not the memory map from EFI.
> 
> I think that's because the memory map returned by
> XENMEM_machine_memory_map is in e820 form, and doesn't contain the
> required information about the EFI regions due to the translation done
> by efi_arch_process_memory_map in Xen?

Yes, I think so.

> > This makes sense
> > as it is Xen responsible for managing physical memory address space.
> > In this case, it doesn't make sense to condition using ESRT table on
> > availability of EFI memory map, as it isn't Linux kernel responsible for
> > it.
> 
> PV dom0 is kind of special in that regard as it can create mappings to
> (almost) any MMIO regions, and hence can change it's memory map
> substantially.

Do you mean PV dom0 should receive full EFI memory map? Jan already
objected this as it would be a layering violation.

> > Skip this part on Xen PV (let Xen do the right thing if it deems
> > necessary) and use ESRT table normally.
> 
> Maybe it would be better to introduce a new hypercall (or add a
> parameter to XENMEM_machine_memory_map) in order to be able to fetch
> the EFI memory map?
>
> That should allow a PV dom0 to check the ESRT is correct and thus not
> diverge from bate metal.

Note the EFI memory map there is used not just to check things, but to
actually modify it to reserve the region. I think that's rather Xen
responsibility, not dom0. See the comment from Ard.
 
> > 
> > This is a requirement for using fwupd in PV dom0 to update UEFI using
> > capsules.
> > 
> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> > ---
> >  drivers/firmware/efi/esrt.c | 47 ++++++++++++++++++++-----------------
> >  1 file changed, 25 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
> > index d5915272141f..5c49f2aaa4b1 100644
> > --- a/drivers/firmware/efi/esrt.c
> > +++ b/drivers/firmware/efi/esrt.c
> > @@ -245,36 +245,38 @@ void __init efi_esrt_init(void)
> >     int rc;
> >     phys_addr_t end;
> >  
> > -   if (!efi_enabled(EFI_MEMMAP))
> > +   if (!efi_enabled(EFI_MEMMAP) && !efi_enabled(EFI_PARAVIRT))
> >             return;
> >  
> >     pr_debug("esrt-init: loading.\n");
> >     if (!esrt_table_exists())
> >             return;
> >  
> > -   rc = efi_mem_desc_lookup(efi.esrt, &md);
> > -   if (rc < 0 ||
> > -       (!(md.attribute & EFI_MEMORY_RUNTIME) &&
> > -        md.type != EFI_BOOT_SERVICES_DATA &&
> > -        md.type != EFI_RUNTIME_SERVICES_DATA)) {
> > -           pr_warn("ESRT header is not in the memory map.\n");
> > -           return;
> > -   }
> > +   if (efi_enabled(EFI_MEMMAP)) {
> > +           rc = efi_mem_desc_lookup(efi.esrt, &md);
> > +           if (rc < 0 ||
> > +               (!(md.attribute & EFI_MEMORY_RUNTIME) &&
> > +                md.type != EFI_BOOT_SERVICES_DATA &&
> > +                md.type != EFI_RUNTIME_SERVICES_DATA)) {
> > +                   pr_warn("ESRT header is not in the memory map.\n");
> > +                   return;
> > +           }
> 
> Here you blindly trust the data in the ESRT in the PV case, without
> checking it matches the regions on the memory map, which could lead to
> errors if ESRT turns to be wrong.

I don't think checking merely if ESRT lives somewhere in
EFI_{BOOT,RUNTIME}_SERVICES_DATA area guarantees its correctness.

On the other hand, this seems to be done to prevent overwriting that
memory with something else (see that in case of EFI_BOOT_SERVICES_DATA
it is later marked as reserved. I think it should be rather done by Xen,
not dom0. Either by moving this whole check into dom0 (for this table
only? or perhaps for other tables too?), or by simply reserving the whole
EFI_BOOT_SERVICES_DATA (like the /mapbs boot switch does).
Then, I think dom0 could use e820 map to verify that this is reserved,
but do not modify the map anymore.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

Attachment: signature.asc
Description: PGP signature