Re: [PATCH v2] Add support for ESRT loading under Xen

[Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>]

On Mon, Sep 05, 2022 at 01:46:54PM +0200, Ard Biesheuvel wrote:
> On Sun, 28 Aug 2022 at 04:52, Demi Marie Obenour
> <demi@xxxxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > This is needed for fwupd to work in Qubes OS.
> >
> 
> Please elaborate on:

Will do in v3.

> - the current situation

The ESRT is not available in dom0 under Xen.

> - why this is a problem

fwupd requires the ESRT to be available in dom0.  Without it, users
cannot update their firmware.

> - why your approach is a reasonable solution.

It is the approach already chosen by Xen upstream.  See below for
details.

> > Signed-off-by: Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>
> > ---
> > Changes since v1:
> >
> > - Use a different type (struct xen_efi_mem_info) for memory information
> >   provided by Xen, as Xen reports it in a different way than the
> >   standard Linux functions do.
> >
> >  drivers/firmware/efi/esrt.c | 49 +++++++++++++++++++++++++++----------
> >  drivers/xen/efi.c           | 32 ++++++++++++++++++++++++++
> >  include/linux/efi.h         | 18 ++++++++++++++
> >  3 files changed, 86 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
> > index 
> > 2a2f52b017e736dd995c69e8aeb5fbd7761732e5..c0fc149a838044cc16bb08a374a0c8ea6b7dcbff
> >  100644
> > --- a/drivers/firmware/efi/esrt.c
> > +++ b/drivers/firmware/efi/esrt.c
> > @@ -243,27 +243,50 @@ void __init efi_esrt_init(void)
> >         void *va;
> >         struct efi_system_resource_table tmpesrt;
> >         size_t size, max, entry_size, entries_size;
> > -       efi_memory_desc_t md;
> > -       int rc;
> >         phys_addr_t end;
> > -
> > -       if (!efi_enabled(EFI_MEMMAP))
> > -               return;
> > +       uint32_t type;
> >
> >         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");
> > +       if (efi_enabled(EFI_MEMMAP)) {
> > +               efi_memory_desc_t md;
> > +
> > +               if (efi_mem_desc_lookup(efi.esrt, &md) < 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;
> > +               }
> > +
> > +               type = md.type;
> > +               max = efi_mem_desc_end(&md);
> > +       } else if (IS_ENABLED(CONFIG_XEN_EFI) && efi_enabled(EFI_PARAVIRT)) 
> > {
> > +               struct xen_efi_mem_info info;
> > +
> > +               if (!xen_efi_mem_info_query(efi.esrt, &info)) {
> > +                       pr_warn("Failed to lookup ESRT header in Xen memory 
> > map\n");
> > +                       return;
> > +               }
> > +
> > +               type = info.type;
> > +               max = info.addr + info.size;
> > +
> > +               /* Recent Xen versions relocate the ESRT to memory of type
> > +                * EfiRuntimeServicesData, which Xen will not reuse.  If 
> > the ESRT
> 
> This violates the EFI spec, which spells out very clearly that the
> ESRT must be in EfiBootServicesData memory. Why are you deviating from
> this?

Xen will freely use EfiBootServicesData memory for its own purposes
after calling ExitBootServices().  In particular, such memory may be
allocated to, and become writable by, other guests.  Since the ESRT is
(of necessity) trusted, it cannot be used by Linux unless it is
guaranteed to not be writable by other guests.

Earlier patches to Xen just reserved the region containing the ESRT in
the EFI memory map.  However, this was tricky to implement correctly and
required a new platform op to alert dom0 that the ESRT had been reserved
by Xen.  The final patch accepted upstream instead checks if the ESRT is
in EfiBootServicesData memory, and if it is, relocates it to
EfiRuntimeServicesData memory.  This allowes using existing hypercalls
to check if the ESRT has been reserved by Xen, which is exactly what
this patch does.

If I recall correctly, some firmware already allocates the ESRT from
EfiRuntimeServicesData memory, so operating systems already need to
support this case.  Furthermore, the ESRT must not be clobbered by the
OS or hypervisor, so EfiRuntimeServicesData is a more logical choice
anyway.
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature