Re: [PATCH v2 2/3] efi_loader: carve out efi_get_memory_map_alloc()

[Heinrich Schuchardt]

On 1/9/23 08:18, Ilias Apalodimas wrote:
On Thu, Jan 05, 2023 at 09:25:35PM +0100, Heinrich Schuchardt wrote:
Carve out code from efidebug command used to read the memory map.
Signed-off-by: Heinrich Schuchardt <heinrich.schucha...@canonical.com>
---
v2:
        new patch
---
  cmd/efidebug.c              | 18 ++++--------------
  include/efi_loader.h        |  3 +++
  lib/efi_loader/efi_memory.c | 34 ++++++++++++++++++++++++++++++++++
  3 files changed, 41 insertions(+), 14 deletions(-)

diff --git a/cmd/efidebug.c b/cmd/efidebug.c
index 569003ae2e..e6959ede93 100644
--- a/cmd/efidebug.c
+++ b/cmd/efidebug.c
@@ -591,25 +591,15 @@ static void print_memory_attributes(u64 attributes)
  static int do_efi_show_memmap(struct cmd_tbl *cmdtp, int flag,
                              int argc, char *const argv[])
  {
-       struct efi_mem_desc *memmap = NULL, *map;
-       efi_uintn_t map_size = 0;
+       struct efi_mem_desc *memmap, *map;
+       efi_uintn_t map_size;
        const char *type;
        int i;
        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 CMD_RET_FAILURE;
-               ret = efi_get_memory_map(&map_size, memmap, NULL, NULL, NULL);
-       }
-       if (ret != EFI_SUCCESS) {
-               efi_free_pool(memmap);
+       ret = efi_get_memory_map_alloc(&map_size, &memmap);
+       if (ret != EFI_SUCCESS)
                return CMD_RET_FAILURE;
-       }
  
  	printf("Type             Start%.*s End%.*s Attributes\n",
               EFI_PHYS_ADDR_WIDTH - 5, spc, EFI_PHYS_ADDR_WIDTH - 3, spc);
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 0899e293e5..02d151b715 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -734,6 +734,9 @@ efi_status_t efi_allocate_pool(enum efi_memory_type 
pool_type,
                               efi_uintn_t size, void **buffer);
  /* EFI pool memory free function. */
  efi_status_t efi_free_pool(void *buffer);
+/* Allocate and retrieve EFI memory map */
+efi_status_t efi_get_memory_map_alloc(efi_uintn_t *map_size,
+                                     struct efi_mem_desc **memory_map);
  /* Returns the EFI memory map */
  efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size,
                                struct efi_mem_desc *memory_map,
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index 8d347f101f..32254d2433 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -736,6 +736,40 @@ efi_status_t efi_get_memory_map(efi_uintn_t 
*memory_map_size,
        return EFI_SUCCESS;
  }
  
+/**
+ * efi_get_memory_map_alloc() - allocate map describing memory usage
+ *
+ * The caller is responsible for calling FreePool() if the call succeeds.
+ *
+ * @memory_map         buffer to which the memory map is written
+ * @map_size           size of the memory map
+ * Return:             status code
+ */
+efi_status_t efi_get_memory_map_alloc(efi_uintn_t *map_size,
+                                     struct efi_mem_desc **memory_map)
+{
+       efi_status_t ret;
+
+       *memory_map = NULL;
+       *map_size = 0;
+       ret = efi_get_memory_map(map_size, *memory_map, NULL, NULL, NULL);

Although this is correct and efi_get_memory_map() will only return
EFI_BUFFER_TOO_SMALL, since we initialize the map_size to 0,  I don't know
if code analysis tools are smart enough to understand this.  Perhaps we
should initialize ret?

After an assignment ret cannot be uninitialized.

Did you find a path through efi_get_memory_map() returning an undefined 
value?

Best regards

Heinrich



+       if (ret == EFI_BUFFER_TOO_SMALL) {
+               *map_size += sizeof(struct efi_mem_desc); /* for the map */
+               ret = efi_allocate_pool(EFI_BOOT_SERVICES_DATA, *map_size,
+                                       (void **)memory_map);
+               if (ret != EFI_SUCCESS)
+                       return ret;
+               ret = efi_get_memory_map(map_size, *memory_map,
+                                        NULL, NULL, NULL);
+               if (ret != EFI_SUCCESS) {
+                       efi_free_pool(*memory_map);
+                       *memory_map = NULL;
+               }
+       }
+
+       return ret;
+}
+
  /**
   * efi_add_conventional_memory_map() - add a RAM memory area to the map
   *
--
2.37.2


Reviewed-by: Ilias Apalodimas <ilias.apalodi...@linaro.org>