[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.schuchardt at 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.apalodimas at linaro.org>
>