diff mbox series

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

Message ID 20221002095626.484279-6-ardb@kernel.org
State New
Headers show
Series efi/x86: Avoid corrupted config tables under Xen | expand

Commit Message

Ard Biesheuvel Oct. 2, 2022, 9:56 a.m. UTC 
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.

Co-developed-by: Demi Marie Obenour <demi@invisiblethingslab.com>
Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/platform/efi/quirks.c |  3 +
 drivers/firmware/efi/esrt.c    | 61 +++++++++++---------
 2 files changed, 37 insertions(+), 27 deletions(-)

Comments

Demi Marie Obenour Oct. 2, 2022, 4:27 p.m. UTC | #1 
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 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.

> Co-developed-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  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.
Ard Biesheuvel Oct. 2, 2022, 9:43 p.m. UTC | #2 
On Sun, 2 Oct 2022 at 18:28, Demi Marie Obenour
<demi@invisiblethingslab.com> 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@invisiblethingslab.com>
> > Signed-off-by: Demi Marie Obenour <demi@invisiblethingslab.com>
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  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
Ard Biesheuvel Oct. 3, 2022, 8:41 a.m. UTC | #3 
On Sun, 2 Oct 2022 at 23:43, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Sun, 2 Oct 2022 at 18:28, Demi Marie Obenour
> <demi@invisiblethingslab.com> 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.
>

Actually, the obvious answer here is to implement
efi_mem_desc_lookup() for the EFI_PARAVIRT / !EFI_MEMMAP case. I'll
have a go at that and send a v2 shortly.
diff mbox series

Patch

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;
+
 	if (efi_mem_desc_lookup(addr, &md) ||
 	    md.type != EFI_BOOT_SERVICES_DATA) {
 		pr_err("Failed to lookup EFI memory descriptor for %pa\n", &addr);
diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
index 2a2f52b017e7..adb31fba45ae 100644
--- a/drivers/firmware/efi/esrt.c
+++ b/drivers/firmware/efi/esrt.c
@@ -243,40 +243,45 @@  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;
+	bool reserve_esrt;
 	phys_addr_t end;
 
-	if (!efi_enabled(EFI_MEMMAP))
-		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;
-	}
+	size = sizeof(*esrt);
+	if (efi_enabled(EFI_MEMMAP)) {
+		efi_memory_desc_t md;
+		int rc;
+
+		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;
+		}
 
-	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);
-		return;
-	}
+		reserve_esrt = (md.type == EFI_BOOT_SERVICES_DATA);
+		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);
+			return;
+		}
 
-	size = sizeof(*esrt);
-	max -= efi.esrt;
+		max -= efi.esrt;
 
-	if (max < size) {
-		pr_err("ESRT header doesn't fit on single memory map entry. (size: %zu max: %zu)\n",
-		       size, max);
-		return;
+		if (max < size) {
+			pr_err("ESRT header doesn't fit on single memory map entry. (size: %zu max: %zu)\n",
+			       size, max);
+			return;
+		}
+	} else {
+		reserve_esrt = true;
+		max = SIZE_MAX;
 	}
 
 	va = early_memremap(efi.esrt, size);
@@ -332,9 +337,11 @@  void __init efi_esrt_init(void)
 	esrt_data_size = size;
 
 	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 (reserve_esrt) {
+		pr_info("Reserving ESRT space from %pa to %pa.\n", &esrt_data,
+			&end);
 		efi_mem_reserve(esrt_data, esrt_data_size);
+	}
 
 	pr_debug("esrt-init: loaded.\n");
 }