[PATCH 0/2] lmb: consider EFI memory map

[PATCH 0/2] lmb: consider EFI memory map

[Heinrich Schuchardt]

As reported in Debian bug #1027176 relocation of the initrd may lead to
overwriting memory used by the EFI sub-system.

Currently the available memory for images is determined via the lmb
library functions. The lmb library has several shortcomings:

* It does not protect against overwriting one image with another.
* The same routines to find reserved memory are called again and
  again.

In the long run we should move to allocating memory for images.

As an intermediate solutions let's add lmb-reservations for all EFI memory
areas that are not EFI_CONVENTIONAL_MEMORY.

The variable $loadaddr of at least the vexpress_ca9x4 board collids with
memory used by the EFI sub-system. Adjust $loadaddr for the vexpress
boards to a sane value.

Heinrich Schuchardt (2):
  vexpress: adjust loadaddr
  lmb: consider EFI memory map

 include/configs/vexpress_common.h |  1 +
 lib/lmb.c                         | 45 +++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)

-- 
2.37.2

[PATCH 1/2] vexpress: adjust loadaddr

[Heinrich Schuchardt]

On the vexpress_ca9x4 $loadaddr points to a memory area used by the EFI
sub-system. Use the same value as $kernel_addr_r which is safe.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 include/configs/vexpress_common.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/configs/vexpress_common.h b/include/configs/vexpress_common.h
index 5d773060d8..aac96d29ba 100644
--- a/include/configs/vexpress_common.h
+++ b/include/configs/vexpress_common.h
@@ -147,6 +147,7 @@
 #include <config_distro_bootcmd.h>
 
 #define CONFIG_EXTRA_ENV_SETTINGS \
+                "loadaddr=0x60100000\0" \
                 "kernel_addr_r=0x60100000\0" \
                 "fdt_addr_r=0x60000000\0" \
                 "bootargs=console=tty0 console=ttyAMA0,38400n8\0" \
-- 
2.37.2

[PATCH 2/2] lmb: consider EFI memory map

[Heinrich Schuchardt]

Add reservations for all EFI memory areas that are not
EFI_CONVENTIONAL_MEMORY.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 lib/lmb.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/lib/lmb.c b/lib/lmb.c
index c599608fa3..aee2617593 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -7,7 +7,9 @@
  */
 
 #include <common.h>
+#include <efi_loader.h>
 #include <image.h>
+#include <mapmem.h>
 #include <lmb.h>
 #include <log.h>
 #include <malloc.h>
@@ -153,6 +155,46 @@ void arch_lmb_reserve_generic(struct lmb *lmb, ulong sp, ulong end, ulong align)
 	}
 }
 
+/**
+ * efi_lmb_reserve() - add reservations for EFI memory
+ *
+ * Add reservations for all EFI memory areas that are not
+ * EFI_CONVENTIONAL_MEMORY.
+ *
+ * @lmb:	lmb environment
+ * Return:	0 on success, 1 on failure
+ */
+static __maybe_unused int efi_lmb_reserve(struct lmb *lmb)
+{
+	struct efi_mem_desc *memmap = NULL, *map;
+	efi_uintn_t i, map_size = 0;
+	efi_status_t ret;
+
+	ret = efi_get_memory_map(&map_size, memmap, NULL, NULL, NULL);
+	if (ret == EFI_BUFFER_TOO_SMALL) {
+		map_size += sizeof(struct efi_mem_desc); /* for my own */
+		ret = efi_allocate_pool(EFI_BOOT_SERVICES_DATA, map_size,
+					(void *)&memmap);
+		if (ret != EFI_SUCCESS)
+			return 1;
+		ret = efi_get_memory_map(&map_size, memmap, NULL, NULL, NULL);
+	}
+	if (ret != EFI_SUCCESS) {
+		efi_free_pool(memmap);
+		return 1;
+	}
+
+	for (i = 0, map = memmap; i < map_size / sizeof(*map); ++map, ++i) {
+		if (map->type != EFI_CONVENTIONAL_MEMORY)
+			lmb_reserve(lmb,
+				    map_to_sysmem((void *)(uintptr_t)
+						  map->physical_start),
+				    map->num_pages * EFI_PAGE_SIZE);
+	}
+	efi_free_pool(memmap);
+	return 0;
+}
+
 static void lmb_reserve_common(struct lmb *lmb, void *fdt_blob)
 {
 	arch_lmb_reserve(lmb);
@@ -160,6 +202,9 @@ static void lmb_reserve_common(struct lmb *lmb, void *fdt_blob)
 
 	if (CONFIG_IS_ENABLED(OF_LIBFDT) && fdt_blob)
 		boot_fdt_add_mem_rsv_regions(lmb, fdt_blob);
+
+	if (CONFIG_IS_ENABLED(EFI_LOADER))
+		efi_lmb_reserve(lmb);
 }
 
 /* Initialize the struct, add memory and call arch/board reserve functions */
-- 
2.37.2

Re: [PATCH 2/2] lmb: consider EFI memory map

[Ilias Apalodimas]

Hi Heinrich, 

This looks reasonable to me as a short term solution, but I'd feel safer if
someone else had a look at it

On Wed, Jan 04, 2023 at 05:26:06AM +0100, Heinrich Schuchardt wrote:
> Add reservations for all EFI memory areas that are not
> EFI_CONVENTIONAL_MEMORY.
> 
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  lib/lmb.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/lib/lmb.c b/lib/lmb.c
> index c599608fa3..aee2617593 100644
> --- a/lib/lmb.c
> +++ b/lib/lmb.c
> @@ -7,7 +7,9 @@
>   */
>  
>  #include <common.h>
> +#include <efi_loader.h>
>  #include <image.h>
> +#include <mapmem.h>
>  #include <lmb.h>
>  #include <log.h>
>  #include <malloc.h>
> @@ -153,6 +155,46 @@ void arch_lmb_reserve_generic(struct lmb *lmb, ulong sp, ulong end, ulong align)
>  	}
>  }
>  
> +/**
> + * efi_lmb_reserve() - add reservations for EFI memory
> + *
> + * Add reservations for all EFI memory areas that are not
> + * EFI_CONVENTIONAL_MEMORY.
> + *
> + * @lmb:	lmb environment
> + * Return:	0 on success, 1 on failure
> + */
> +static __maybe_unused int efi_lmb_reserve(struct lmb *lmb)
> +{
> +	struct efi_mem_desc *memmap = NULL, *map;
> +	efi_uintn_t i, map_size = 0;
> +	efi_status_t ret;
> +
> +	ret = efi_get_memory_map(&map_size, memmap, NULL, NULL, NULL);
> +	if (ret == EFI_BUFFER_TOO_SMALL) {
> +		map_size += sizeof(struct efi_mem_desc); /* for my own */
> +		ret = efi_allocate_pool(EFI_BOOT_SERVICES_DATA, map_size,
> +					(void *)&memmap);
> +		if (ret != EFI_SUCCESS)
> +			return 1;
> +		ret = efi_get_memory_map(&map_size, memmap, NULL, NULL, NULL);
> +	}
> +	if (ret != EFI_SUCCESS) {
> +		efi_free_pool(memmap);
> +		return 1;
> +	}

We got a very similar piece of code in cmd/efidebug.c.  I think we are
better off adding a function that just calls efi_get_memory_map() and
returns the map + map_size, instead of duplicating that 

> +
> +	for (i = 0, map = memmap; i < map_size / sizeof(*map); ++map, ++i) {
> +		if (map->type != EFI_CONVENTIONAL_MEMORY)
> +			lmb_reserve(lmb,
> +				    map_to_sysmem((void *)(uintptr_t)
> +						  map->physical_start),
> +				    map->num_pages * EFI_PAGE_SIZE);
> +	}
> +	efi_free_pool(memmap);
> +	return 0;
> +}
> +
>  static void lmb_reserve_common(struct lmb *lmb, void *fdt_blob)
>  {
>  	arch_lmb_reserve(lmb);
> @@ -160,6 +202,9 @@ static void lmb_reserve_common(struct lmb *lmb, void *fdt_blob)
>  
>  	if (CONFIG_IS_ENABLED(OF_LIBFDT) && fdt_blob)
>  		boot_fdt_add_mem_rsv_regions(lmb, fdt_blob);
> +
> +	if (CONFIG_IS_ENABLED(EFI_LOADER))
> +		efi_lmb_reserve(lmb);
>  }
>  
>  /* Initialize the struct, add memory and call arch/board reserve functions */
> -- 
> 2.37.2
> 

Other than that this looks fine

Cheers
/Ilias