[v2] Add support for ESRT loading under Xen

This is needed for fwupd to work in Qubes OS.

Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
---
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(-)

On Sun, 28 Aug 2022 at 04:52, Demi Marie Obenour
<demi@invisiblethingslab.com> wrote:
>
> This is needed for fwupd to work in Qubes OS.
>

Please elaborate on:
- the current situation
- why this is a problem
- why your approach is a reasonable solution.

> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> ---
> 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?

> +                * is not in EfiRuntimeServicesData memory, it has not been reserved
> +                * by Xen and might be allocated to other guests, so it cannot
> +                * safely be used. */
> +               if (type != EFI_RUNTIME_SERVICES_DATA) {
> +                       pr_warn("Xen did not reserve ESRT, ignoring it\n");
> +                       return;
> +               }
> +       } else {
>                 return;
>         }
>
> -       max = efi_mem_desc_end(&md);
>         if (max < efi.esrt) {
>                 pr_err("EFI memory descriptor is invalid. (esrt: %p max: %p)\n",
>                        (void *)efi.esrt, (void *)max);
> @@ -333,7 +356,7 @@ void __init efi_esrt_init(void)
>
>         end = esrt_data + size;
>         pr_info("Reserving ESRT space from %pa to %pa.\n", &esrt_data, &end);
> -       if (md.type == EFI_BOOT_SERVICES_DATA)
> +       if (type == EFI_BOOT_SERVICES_DATA)
>                 efi_mem_reserve(esrt_data, esrt_data_size);
>
>         pr_debug("esrt-init: loaded.\n");
> diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c
> index d1ff2186ebb48a7c0981ecb6d4afcbbb25ffcea0..b313f213822f0fd5ba6448f6f6f453cfda4c7e23 100644
> --- a/drivers/xen/efi.c
> +++ b/drivers/xen/efi.c
> @@ -26,6 +26,7 @@
>
>  #include <xen/interface/xen.h>
>  #include <xen/interface/platform.h>
> +#include <xen/page.h>
>  #include <xen/xen.h>
>  #include <xen/xen-ops.h>
>
> @@ -40,6 +41,37 @@
>
>  #define efi_data(op)   (op.u.efi_runtime_call)
>
> +static_assert(XEN_PAGE_SHIFT == EFI_PAGE_SHIFT,
> +              "Mismatch between EFI_PAGE_SHIFT and XEN_PAGE_SHIFT");
> +
> +bool xen_efi_mem_info_query(u64 phys_addr, struct xen_efi_mem_info *md)
> +{
> +       struct xen_platform_op op = {
> +               .cmd = XENPF_firmware_info,
> +               .u.firmware_info = {
> +                       .type = XEN_FW_EFI_INFO,
> +                       .index = XEN_FW_EFI_MEM_INFO,
> +                       .u.efi_info.mem.addr = phys_addr,
> +                       .u.efi_info.mem.size = ((u64)-1ULL) - phys_addr,
> +               }
> +       };
> +       union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
> +       int rc;
> +
> +       memset(md, 0, sizeof(*md)); /* initialize md even on failure */
> +       rc = HYPERVISOR_platform_op(&op);
> +       if (rc) {
> +               pr_warn("Could not obtain information on address %llu from Xen: "
> +                       "error %d\n", phys_addr, rc);
> +               return false;
> +       }
> +       md->addr = info->mem.addr;
> +       md->size = info->mem.size;
> +       md->attr = info->mem.attr;
> +       md->type = info->mem.type;
> +       return true;
> +}
> +
>  static efi_status_t xen_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc)
>  {
>         struct xen_platform_op op = INIT_EFI_OP(get_time);
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index d2b84c2fec39f0268324d1a38a73ed67786973c9..0598869cdc924aef0e2b9cacc4450b728e1a98c7 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1327,1 +1327,19 @@ struct linux_efi_coco_secret_area {
> +/* Result of a XEN_FW_EFI_MEM_INFO query */
> +struct xen_efi_mem_info {
> +    uint64_t addr; /* address queried */
> +    uint64_t size; /* remaining bytes in memory region */
> +    uint64_t attr; /* attributes */
> +    uint32_t type; /* type */
> +};
> +
> +#if IS_ENABLED(CONFIG_XEN_EFI)
> +extern bool xen_efi_mem_info_query(u64 phys_addr, struct xen_efi_mem_info *out_md);
> +#else
> +static inline bool xen_efi_mem_info_query(u64 phys_addr, struct xen_efi_mem_info *out_md)
> +{
> +       BUILD_BUG();
> +       return false;
> +}
> +#endif
> +
>  #endif /* _LINUX_EFI_H */
> --
> Sincerely,
> Demi Marie Obenour (she/her/hers)
> Invisible Things Lab

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@invisiblethingslab.com> 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@invisiblethingslab.com>
> > ---
> > 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.