Re: [PATCH v2 2/3] efi_loader: carve out efi_get_memory_map_alloc()
Hi Heinrich
[...]
> >>
> >> +/**
> >> + * 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_sizesize 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?
Nop, just misread the patch!
Regards
/Ilias
>
> 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
> >
>
Re: [PATCH v2 2/3] efi_loader: carve out efi_get_memory_map_alloc()
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
---
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
Re: [PATCH v2 2/3] efi_loader: carve out efi_get_memory_map_alloc()
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
> ---
> 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?
> + 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
Re: [PATCH v2 2/3] efi_loader: carve out efi_get_memory_map_alloc()
On 2023-01-05, Heinrich Schuchardt wrote:
> Carve out code from efidebug command used to read the memory map.
>
> Signed-off-by: Heinrich Schuchardt
Tested on odroid-c2, fixes booting from extlinux.conf and boot.scr using
booti, and still works using EFI boot as well.
Thanks!
Tested-by: Vagrant Cascadian
live well,
vagrant
> ---
> 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);
> + 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
> *
signature.asc
Description: PGP signature
[PATCH v2 2/3] efi_loader: carve out efi_get_memory_map_alloc()
Carve out code from efidebug command used to read the memory map.
Signed-off-by: Heinrich Schuchardt
---
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);
+ 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
