diff mbox series

[v4,2/2] Support ESRT in Xen dom0

Message ID 5649176eacda434267f68676f1733d06c572d19e.1664298147.git.demi@invisiblethingslab.com
State New
Headers show
Series EFI improvements for Xen dom0 | expand

Commit Message

Demi Marie Obenour Sept. 29, 2022, 11:02 p.m. UTC
fwupd requires access to the EFI System Resource Table (ESRT) to
discover which firmware can be updated by the OS.  Currently, Linux does
not expose the ESRT when running as a Xen dom0.  Therefore, it is not
possible to use fwupd in a Xen dom0, which is a serious problem for e.g.
Qubes OS.

Before Xen 4.17, this was not fixable due to hypervisor limitations.
The UEFI specification requires the ESRT to be in EfiBootServicesData
memory, which Xen will use for whatever purposes it likes.  Therefore,
Linux cannot safely access the ESRT, as Xen may have overwritten it.

Starting with Xen 4.17, Xen checks if the ESRT is in EfiBootServicesData
or EfiRuntimeServicesData memory.  If the ESRT is in EfiBootServicesData
memory, Xen replaces the ESRT with a copy in memory that it has
reserved.  Such memory is currently of type EFI_RUNTIME_SERVICES_DATA,
but in the future it will be of type EFI_ACPI_RECLAIM_MEMORY.  This
ensures that the ESRT can safely be accessed by the OS.

When running as a Xen dom0, use the new
xen_config_table_memory_region_max() function to determine if Xen has
reserved the ESRT and, if so, find the end of the memory region
containing it.  This allows programs such as fwupd which require the
ESRT to run under Xen, and so makes fwupd support in Qubes OS possible.

Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
---
 drivers/firmware/efi/esrt.c | 43 ++++++++++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 13 deletions(-)

Comments

Ard Biesheuvel Sept. 30, 2022, 4:36 p.m. UTC | #1
On Fri, 30 Sept 2022 at 01:02, Demi Marie Obenour
<demi@invisiblethingslab.com> wrote:
>
> fwupd requires access to the EFI System Resource Table (ESRT) to
> discover which firmware can be updated by the OS.  Currently, Linux does
> not expose the ESRT when running as a Xen dom0.  Therefore, it is not
> possible to use fwupd in a Xen dom0, which is a serious problem for e.g.
> Qubes OS.
>
> Before Xen 4.17, this was not fixable due to hypervisor limitations.
> The UEFI specification requires the ESRT to be in EfiBootServicesData
> memory, which Xen will use for whatever purposes it likes.  Therefore,
> Linux cannot safely access the ESRT, as Xen may have overwritten it.
>
> Starting with Xen 4.17, Xen checks if the ESRT is in EfiBootServicesData
> or EfiRuntimeServicesData memory.  If the ESRT is in EfiBootServicesData
> memory, Xen replaces the ESRT with a copy in memory that it has
> reserved.  Such memory is currently of type EFI_RUNTIME_SERVICES_DATA,
> but in the future it will be of type EFI_ACPI_RECLAIM_MEMORY.  This
> ensures that the ESRT can safely be accessed by the OS.
>
> When running as a Xen dom0, use the new
> xen_config_table_memory_region_max() function to determine if Xen has
> reserved the ESRT and, if so, find the end of the memory region
> containing it.  This allows programs such as fwupd which require the
> ESRT to run under Xen, and so makes fwupd support in Qubes OS possible.
>
> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>

Why do we need this patch? I'd expect esrt_table_exists() to return
false when patch 1/2 is applied.



> ---
>  drivers/firmware/efi/esrt.c | 43 ++++++++++++++++++++++++++-----------
>  1 file changed, 30 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
> index 2a2f52b017e736dd995c69e8aeb5fbd7761732e5..a0642bc161b4b1f94f818b8c9f46511fe2424bb2 100644
> --- a/drivers/firmware/efi/esrt.c
> +++ b/drivers/firmware/efi/esrt.c
> @@ -243,27 +243,44 @@ 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;
> +       u32 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);
> +#ifdef CONFIG_XEN_EFI
> +       } else if (efi_enabled(EFI_PARAVIRT)) {
> +               max = xen_config_table_memory_region_max(efi.esrt);
> +               /*
> +                * This might be wrong, but it doesn't matter.
> +                * xen_config_table_memory_region_max() checks the type
> +                * of the memory region, and if it returns 0, the code
> +                * below will fail without looking at the type.  Choose
> +                * a value that will not cause * subsequent code to try
> +                * to reserve the memory containing the ESRT, as either
> +                * Xen or the firmware has done so already.
> +                */
> +               type = EFI_RUNTIME_SERVICES_DATA;
> +#endif
> +       } 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 +350,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");
> --
> Sincerely,
> Demi Marie Obenour (she/her/hers)
> Invisible Things Lab
>
Demi Marie Obenour Sept. 30, 2022, 6:21 p.m. UTC | #2
On Fri, Sep 30, 2022 at 06:36:11PM +0200, Ard Biesheuvel wrote:
> On Fri, 30 Sept 2022 at 01:02, Demi Marie Obenour
> <demi@invisiblethingslab.com> wrote:
> >
> > fwupd requires access to the EFI System Resource Table (ESRT) to
> > discover which firmware can be updated by the OS.  Currently, Linux does
> > not expose the ESRT when running as a Xen dom0.  Therefore, it is not
> > possible to use fwupd in a Xen dom0, which is a serious problem for e.g.
> > Qubes OS.
> >
> > Before Xen 4.17, this was not fixable due to hypervisor limitations.
> > The UEFI specification requires the ESRT to be in EfiBootServicesData
> > memory, which Xen will use for whatever purposes it likes.  Therefore,
> > Linux cannot safely access the ESRT, as Xen may have overwritten it.
> >
> > Starting with Xen 4.17, Xen checks if the ESRT is in EfiBootServicesData
> > or EfiRuntimeServicesData memory.  If the ESRT is in EfiBootServicesData
> > memory, Xen replaces the ESRT with a copy in memory that it has
> > reserved.  Such memory is currently of type EFI_RUNTIME_SERVICES_DATA,
> > but in the future it will be of type EFI_ACPI_RECLAIM_MEMORY.  This
> > ensures that the ESRT can safely be accessed by the OS.
> >
> > When running as a Xen dom0, use the new
> > xen_config_table_memory_region_max() function to determine if Xen has
> > reserved the ESRT and, if so, find the end of the memory region
> > containing it.  This allows programs such as fwupd which require the
> > ESRT to run under Xen, and so makes fwupd support in Qubes OS possible.
> >
> > Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> 
> Why do we need this patch? I'd expect esrt_table_exists() to return
> false when patch 1/2 is applied.

efi_enabled(EFI_MEMMAP) is false under Xen, so there needs to be an
alternative way to get the end of the memory region containing the ESRT.
That is what this patch provides.
Ard Biesheuvel Sept. 30, 2022, 7:11 p.m. UTC | #3
On Fri, 30 Sept 2022 at 20:21, Demi Marie Obenour
<demi@invisiblethingslab.com> wrote:
>
> On Fri, Sep 30, 2022 at 06:36:11PM +0200, Ard Biesheuvel wrote:
> > On Fri, 30 Sept 2022 at 01:02, Demi Marie Obenour
> > <demi@invisiblethingslab.com> wrote:
> > >
> > > fwupd requires access to the EFI System Resource Table (ESRT) to
> > > discover which firmware can be updated by the OS.  Currently, Linux does
> > > not expose the ESRT when running as a Xen dom0.  Therefore, it is not
> > > possible to use fwupd in a Xen dom0, which is a serious problem for e.g.
> > > Qubes OS.
> > >
> > > Before Xen 4.17, this was not fixable due to hypervisor limitations.
> > > The UEFI specification requires the ESRT to be in EfiBootServicesData
> > > memory, which Xen will use for whatever purposes it likes.  Therefore,
> > > Linux cannot safely access the ESRT, as Xen may have overwritten it.
> > >
> > > Starting with Xen 4.17, Xen checks if the ESRT is in EfiBootServicesData
> > > or EfiRuntimeServicesData memory.  If the ESRT is in EfiBootServicesData
> > > memory, Xen replaces the ESRT with a copy in memory that it has
> > > reserved.  Such memory is currently of type EFI_RUNTIME_SERVICES_DATA,
> > > but in the future it will be of type EFI_ACPI_RECLAIM_MEMORY.  This
> > > ensures that the ESRT can safely be accessed by the OS.
> > >
> > > When running as a Xen dom0, use the new
> > > xen_config_table_memory_region_max() function to determine if Xen has
> > > reserved the ESRT and, if so, find the end of the memory region
> > > containing it.  This allows programs such as fwupd which require the
> > > ESRT to run under Xen, and so makes fwupd support in Qubes OS possible.
> > >
> > > Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> >
> > Why do we need this patch? I'd expect esrt_table_exists() to return
> > false when patch 1/2 is applied.
>
> efi_enabled(EFI_MEMMAP) is false under Xen, so there needs to be an
> alternative way to get the end of the memory region containing the ESRT.
> That is what this patch provides.

OK. I don't think we need that to be honest. When running under Xen,
we should be able to assume that the ESRT does not span multiple
memory regions arbitrarily, so we can just omit this check if
!efi_enabled(EFI_MEMMAP)

IIRC (and Peter would know), we are trying to filter out descriptors
that are completely bogus here: zero lenght, zero address, etc etc. I
don't think we need that for Xen.
Demi Marie Obenour Sept. 30, 2022, 8:20 p.m. UTC | #4
On Fri, Sep 30, 2022 at 09:11:19PM +0200, Ard Biesheuvel wrote:
> On Fri, 30 Sept 2022 at 20:21, Demi Marie Obenour
> <demi@invisiblethingslab.com> wrote:
> >
> > On Fri, Sep 30, 2022 at 06:36:11PM +0200, Ard Biesheuvel wrote:
> > > On Fri, 30 Sept 2022 at 01:02, Demi Marie Obenour
> > > <demi@invisiblethingslab.com> wrote:
> > > >
> > > > fwupd requires access to the EFI System Resource Table (ESRT) to
> > > > discover which firmware can be updated by the OS.  Currently, Linux does
> > > > not expose the ESRT when running as a Xen dom0.  Therefore, it is not
> > > > possible to use fwupd in a Xen dom0, which is a serious problem for e.g.
> > > > Qubes OS.
> > > >
> > > > Before Xen 4.17, this was not fixable due to hypervisor limitations.
> > > > The UEFI specification requires the ESRT to be in EfiBootServicesData
> > > > memory, which Xen will use for whatever purposes it likes.  Therefore,
> > > > Linux cannot safely access the ESRT, as Xen may have overwritten it.
> > > >
> > > > Starting with Xen 4.17, Xen checks if the ESRT is in EfiBootServicesData
> > > > or EfiRuntimeServicesData memory.  If the ESRT is in EfiBootServicesData
> > > > memory, Xen replaces the ESRT with a copy in memory that it has
> > > > reserved.  Such memory is currently of type EFI_RUNTIME_SERVICES_DATA,
> > > > but in the future it will be of type EFI_ACPI_RECLAIM_MEMORY.  This
> > > > ensures that the ESRT can safely be accessed by the OS.
> > > >
> > > > When running as a Xen dom0, use the new
> > > > xen_config_table_memory_region_max() function to determine if Xen has
> > > > reserved the ESRT and, if so, find the end of the memory region
> > > > containing it.  This allows programs such as fwupd which require the
> > > > ESRT to run under Xen, and so makes fwupd support in Qubes OS possible.
> > > >
> > > > Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> > >
> > > Why do we need this patch? I'd expect esrt_table_exists() to return
> > > false when patch 1/2 is applied.
> >
> > efi_enabled(EFI_MEMMAP) is false under Xen, so there needs to be an
> > alternative way to get the end of the memory region containing the ESRT.
> > That is what this patch provides.
> 
> OK. I don't think we need that to be honest. When running under Xen,
> we should be able to assume that the ESRT does not span multiple
> memory regions arbitrarily, so we can just omit this check if
> !efi_enabled(EFI_MEMMAP)
> 
> IIRC (and Peter would know), we are trying to filter out descriptors
> that are completely bogus here: zero lenght, zero address, etc etc. I
> don't think we need that for Xen.

Xen doesn’t uninstall bogus ESRTs, so there is no less reason to worry
under Xen than on bare hardware.
Ard Biesheuvel Sept. 30, 2022, 8:59 p.m. UTC | #5
On Fri, 30 Sept 2022 at 22:21, Demi Marie Obenour
<demi@invisiblethingslab.com> wrote:
>
> On Fri, Sep 30, 2022 at 09:11:19PM +0200, Ard Biesheuvel wrote:
> > On Fri, 30 Sept 2022 at 20:21, Demi Marie Obenour
> > <demi@invisiblethingslab.com> wrote:
> > >
> > > On Fri, Sep 30, 2022 at 06:36:11PM +0200, Ard Biesheuvel wrote:
> > > > On Fri, 30 Sept 2022 at 01:02, Demi Marie Obenour
> > > > <demi@invisiblethingslab.com> wrote:
> > > > >
> > > > > fwupd requires access to the EFI System Resource Table (ESRT) to
> > > > > discover which firmware can be updated by the OS.  Currently, Linux does
> > > > > not expose the ESRT when running as a Xen dom0.  Therefore, it is not
> > > > > possible to use fwupd in a Xen dom0, which is a serious problem for e.g.
> > > > > Qubes OS.
> > > > >
> > > > > Before Xen 4.17, this was not fixable due to hypervisor limitations.
> > > > > The UEFI specification requires the ESRT to be in EfiBootServicesData
> > > > > memory, which Xen will use for whatever purposes it likes.  Therefore,
> > > > > Linux cannot safely access the ESRT, as Xen may have overwritten it.
> > > > >
> > > > > Starting with Xen 4.17, Xen checks if the ESRT is in EfiBootServicesData
> > > > > or EfiRuntimeServicesData memory.  If the ESRT is in EfiBootServicesData
> > > > > memory, Xen replaces the ESRT with a copy in memory that it has
> > > > > reserved.  Such memory is currently of type EFI_RUNTIME_SERVICES_DATA,
> > > > > but in the future it will be of type EFI_ACPI_RECLAIM_MEMORY.  This
> > > > > ensures that the ESRT can safely be accessed by the OS.
> > > > >
> > > > > When running as a Xen dom0, use the new
> > > > > xen_config_table_memory_region_max() function to determine if Xen has
> > > > > reserved the ESRT and, if so, find the end of the memory region
> > > > > containing it.  This allows programs such as fwupd which require the
> > > > > ESRT to run under Xen, and so makes fwupd support in Qubes OS possible.
> > > > >
> > > > > Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> > > >
> > > > Why do we need this patch? I'd expect esrt_table_exists() to return
> > > > false when patch 1/2 is applied.
> > >
> > > efi_enabled(EFI_MEMMAP) is false under Xen, so there needs to be an
> > > alternative way to get the end of the memory region containing the ESRT.
> > > That is what this patch provides.
> >
> > OK. I don't think we need that to be honest. When running under Xen,
> > we should be able to assume that the ESRT does not span multiple
> > memory regions arbitrarily, so we can just omit this check if
> > !efi_enabled(EFI_MEMMAP)
> >
> > IIRC (and Peter would know), we are trying to filter out descriptors
> > that are completely bogus here: zero lenght, zero address, etc etc. I
> > don't think we need that for Xen.
>
> Xen doesn’t uninstall bogus ESRTs, so there is no less reason to worry
> under Xen than on bare hardware.

That may be true. But if Xen needs dom0 to be able to cross reference
the EFI memory map, it should provide one (and set EFI_MEMMAP to
enabled).
Ard Biesheuvel Sept. 30, 2022, 9:24 p.m. UTC | #6
On Fri, 30 Sept 2022 at 22:59, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Fri, 30 Sept 2022 at 22:21, Demi Marie Obenour
> <demi@invisiblethingslab.com> wrote:
> >
> > On Fri, Sep 30, 2022 at 09:11:19PM +0200, Ard Biesheuvel wrote:
> > > On Fri, 30 Sept 2022 at 20:21, Demi Marie Obenour
> > > <demi@invisiblethingslab.com> wrote:
> > > >
> > > > On Fri, Sep 30, 2022 at 06:36:11PM +0200, Ard Biesheuvel wrote:
> > > > > On Fri, 30 Sept 2022 at 01:02, Demi Marie Obenour
> > > > > <demi@invisiblethingslab.com> wrote:
> > > > > >
> > > > > > fwupd requires access to the EFI System Resource Table (ESRT) to
> > > > > > discover which firmware can be updated by the OS.  Currently, Linux does
> > > > > > not expose the ESRT when running as a Xen dom0.  Therefore, it is not
> > > > > > possible to use fwupd in a Xen dom0, which is a serious problem for e.g.
> > > > > > Qubes OS.
> > > > > >
> > > > > > Before Xen 4.17, this was not fixable due to hypervisor limitations.
> > > > > > The UEFI specification requires the ESRT to be in EfiBootServicesData
> > > > > > memory, which Xen will use for whatever purposes it likes.  Therefore,
> > > > > > Linux cannot safely access the ESRT, as Xen may have overwritten it.
> > > > > >
> > > > > > Starting with Xen 4.17, Xen checks if the ESRT is in EfiBootServicesData
> > > > > > or EfiRuntimeServicesData memory.  If the ESRT is in EfiBootServicesData
> > > > > > memory, Xen replaces the ESRT with a copy in memory that it has
> > > > > > reserved.  Such memory is currently of type EFI_RUNTIME_SERVICES_DATA,
> > > > > > but in the future it will be of type EFI_ACPI_RECLAIM_MEMORY.  This
> > > > > > ensures that the ESRT can safely be accessed by the OS.
> > > > > >
> > > > > > When running as a Xen dom0, use the new
> > > > > > xen_config_table_memory_region_max() function to determine if Xen has
> > > > > > reserved the ESRT and, if so, find the end of the memory region
> > > > > > containing it.  This allows programs such as fwupd which require the
> > > > > > ESRT to run under Xen, and so makes fwupd support in Qubes OS possible.
> > > > > >
> > > > > > Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> > > > >
> > > > > Why do we need this patch? I'd expect esrt_table_exists() to return
> > > > > false when patch 1/2 is applied.
> > > >
> > > > efi_enabled(EFI_MEMMAP) is false under Xen, so there needs to be an
> > > > alternative way to get the end of the memory region containing the ESRT.
> > > > That is what this patch provides.
> > >
> > > OK. I don't think we need that to be honest. When running under Xen,
> > > we should be able to assume that the ESRT does not span multiple
> > > memory regions arbitrarily, so we can just omit this check if
> > > !efi_enabled(EFI_MEMMAP)
> > >
> > > IIRC (and Peter would know), we are trying to filter out descriptors
> > > that are completely bogus here: zero lenght, zero address, etc etc. I
> > > don't think we need that for Xen.
> >
> > Xen doesn’t uninstall bogus ESRTs, so there is no less reason to worry
> > under Xen than on bare hardware.
>
> That may be true. But if Xen needs dom0 to be able to cross reference
> the EFI memory map, it should provide one (and set EFI_MEMMAP to
> enabled).

Btw the efi_mem_reserve() for the ESRT is also redundant if it is
guaranteed to be in RT services data or ACPI reclaim memory.
Demi Marie Obenour Sept. 30, 2022, 10:22 p.m. UTC | #7
On Fri, Sep 30, 2022 at 11:24:37PM +0200, Ard Biesheuvel wrote:
> On Fri, 30 Sept 2022 at 22:59, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Fri, 30 Sept 2022 at 22:21, Demi Marie Obenour
> > <demi@invisiblethingslab.com> wrote:
> > >
> > > On Fri, Sep 30, 2022 at 09:11:19PM +0200, Ard Biesheuvel wrote:
> > > > On Fri, 30 Sept 2022 at 20:21, Demi Marie Obenour
> > > > <demi@invisiblethingslab.com> wrote:
> > > > >
> > > > > On Fri, Sep 30, 2022 at 06:36:11PM +0200, Ard Biesheuvel wrote:
> > > > > > On Fri, 30 Sept 2022 at 01:02, Demi Marie Obenour
> > > > > > <demi@invisiblethingslab.com> wrote:
> > > > > > >
> > > > > > > fwupd requires access to the EFI System Resource Table (ESRT) to
> > > > > > > discover which firmware can be updated by the OS.  Currently, Linux does
> > > > > > > not expose the ESRT when running as a Xen dom0.  Therefore, it is not
> > > > > > > possible to use fwupd in a Xen dom0, which is a serious problem for e.g.
> > > > > > > Qubes OS.
> > > > > > >
> > > > > > > Before Xen 4.17, this was not fixable due to hypervisor limitations.
> > > > > > > The UEFI specification requires the ESRT to be in EfiBootServicesData
> > > > > > > memory, which Xen will use for whatever purposes it likes.  Therefore,
> > > > > > > Linux cannot safely access the ESRT, as Xen may have overwritten it.
> > > > > > >
> > > > > > > Starting with Xen 4.17, Xen checks if the ESRT is in EfiBootServicesData
> > > > > > > or EfiRuntimeServicesData memory.  If the ESRT is in EfiBootServicesData
> > > > > > > memory, Xen replaces the ESRT with a copy in memory that it has
> > > > > > > reserved.  Such memory is currently of type EFI_RUNTIME_SERVICES_DATA,
> > > > > > > but in the future it will be of type EFI_ACPI_RECLAIM_MEMORY.  This
> > > > > > > ensures that the ESRT can safely be accessed by the OS.
> > > > > > >
> > > > > > > When running as a Xen dom0, use the new
> > > > > > > xen_config_table_memory_region_max() function to determine if Xen has
> > > > > > > reserved the ESRT and, if so, find the end of the memory region
> > > > > > > containing it.  This allows programs such as fwupd which require the
> > > > > > > ESRT to run under Xen, and so makes fwupd support in Qubes OS possible.
> > > > > > >
> > > > > > > Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> > > > > >
> > > > > > Why do we need this patch? I'd expect esrt_table_exists() to return
> > > > > > false when patch 1/2 is applied.
> > > > >
> > > > > efi_enabled(EFI_MEMMAP) is false under Xen, so there needs to be an
> > > > > alternative way to get the end of the memory region containing the ESRT.
> > > > > That is what this patch provides.
> > > >
> > > > OK. I don't think we need that to be honest. When running under Xen,
> > > > we should be able to assume that the ESRT does not span multiple
> > > > memory regions arbitrarily, so we can just omit this check if
> > > > !efi_enabled(EFI_MEMMAP)
> > > >
> > > > IIRC (and Peter would know), we are trying to filter out descriptors
> > > > that are completely bogus here: zero lenght, zero address, etc etc. I
> > > > don't think we need that for Xen.
> > >
> > > Xen doesn’t uninstall bogus ESRTs, so there is no less reason to worry
> > > under Xen than on bare hardware.
> >
> > That may be true. But if Xen needs dom0 to be able to cross reference
> > the EFI memory map, it should provide one (and set EFI_MEMMAP to
> > enabled).
> 
> Btw the efi_mem_reserve() for the ESRT is also redundant if it is
> guaranteed to be in RT services data or ACPI reclaim memory.

It’s needed on bare hardware.  On Xen it’s unreachable code.
Demi Marie Obenour Sept. 30, 2022, 10:25 p.m. UTC | #8
On Fri, Sep 30, 2022 at 10:59:49PM +0200, Ard Biesheuvel wrote:
> On Fri, 30 Sept 2022 at 22:21, Demi Marie Obenour
> <demi@invisiblethingslab.com> wrote:
> >
> > On Fri, Sep 30, 2022 at 09:11:19PM +0200, Ard Biesheuvel wrote:
> > > On Fri, 30 Sept 2022 at 20:21, Demi Marie Obenour
> > > <demi@invisiblethingslab.com> wrote:
> > > >
> > > > On Fri, Sep 30, 2022 at 06:36:11PM +0200, Ard Biesheuvel wrote:
> > > > > On Fri, 30 Sept 2022 at 01:02, Demi Marie Obenour
> > > > > <demi@invisiblethingslab.com> wrote:
> > > > > >
> > > > > > fwupd requires access to the EFI System Resource Table (ESRT) to
> > > > > > discover which firmware can be updated by the OS.  Currently, Linux does
> > > > > > not expose the ESRT when running as a Xen dom0.  Therefore, it is not
> > > > > > possible to use fwupd in a Xen dom0, which is a serious problem for e.g.
> > > > > > Qubes OS.
> > > > > >
> > > > > > Before Xen 4.17, this was not fixable due to hypervisor limitations.
> > > > > > The UEFI specification requires the ESRT to be in EfiBootServicesData
> > > > > > memory, which Xen will use for whatever purposes it likes.  Therefore,
> > > > > > Linux cannot safely access the ESRT, as Xen may have overwritten it.
> > > > > >
> > > > > > Starting with Xen 4.17, Xen checks if the ESRT is in EfiBootServicesData
> > > > > > or EfiRuntimeServicesData memory.  If the ESRT is in EfiBootServicesData
> > > > > > memory, Xen replaces the ESRT with a copy in memory that it has
> > > > > > reserved.  Such memory is currently of type EFI_RUNTIME_SERVICES_DATA,
> > > > > > but in the future it will be of type EFI_ACPI_RECLAIM_MEMORY.  This
> > > > > > ensures that the ESRT can safely be accessed by the OS.
> > > > > >
> > > > > > When running as a Xen dom0, use the new
> > > > > > xen_config_table_memory_region_max() function to determine if Xen has
> > > > > > reserved the ESRT and, if so, find the end of the memory region
> > > > > > containing it.  This allows programs such as fwupd which require the
> > > > > > ESRT to run under Xen, and so makes fwupd support in Qubes OS possible.
> > > > > >
> > > > > > Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> > > > >
> > > > > Why do we need this patch? I'd expect esrt_table_exists() to return
> > > > > false when patch 1/2 is applied.
> > > >
> > > > efi_enabled(EFI_MEMMAP) is false under Xen, so there needs to be an
> > > > alternative way to get the end of the memory region containing the ESRT.
> > > > That is what this patch provides.
> > >
> > > OK. I don't think we need that to be honest. When running under Xen,
> > > we should be able to assume that the ESRT does not span multiple
> > > memory regions arbitrarily, so we can just omit this check if
> > > !efi_enabled(EFI_MEMMAP)
> > >
> > > IIRC (and Peter would know), we are trying to filter out descriptors
> > > that are completely bogus here: zero lenght, zero address, etc etc. I
> > > don't think we need that for Xen.
> >
> > Xen doesn’t uninstall bogus ESRTs, so there is no less reason to worry
> > under Xen than on bare hardware.
> 
> That may be true. But if Xen needs dom0 to be able to cross reference
> the EFI memory map, it should provide one (and set EFI_MEMMAP to
> enabled).

I agree, but it is also a significant amount of work compared to this
patch.
diff mbox series

Patch

diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
index 2a2f52b017e736dd995c69e8aeb5fbd7761732e5..a0642bc161b4b1f94f818b8c9f46511fe2424bb2 100644
--- a/drivers/firmware/efi/esrt.c
+++ b/drivers/firmware/efi/esrt.c
@@ -243,27 +243,44 @@  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;
+	u32 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);
+#ifdef CONFIG_XEN_EFI
+	} else if (efi_enabled(EFI_PARAVIRT)) {
+		max = xen_config_table_memory_region_max(efi.esrt);
+		/*
+		 * This might be wrong, but it doesn't matter.
+		 * xen_config_table_memory_region_max() checks the type
+		 * of the memory region, and if it returns 0, the code
+		 * below will fail without looking at the type.  Choose
+		 * a value that will not cause * subsequent code to try
+		 * to reserve the memory containing the ESRT, as either
+		 * Xen or the firmware has done so already.
+		 */
+		type = EFI_RUNTIME_SERVICES_DATA;
+#endif
+	} 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 +350,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");