[PATCH] lmb: Correctly unmap and free memory on errors

[Heinrich Schuchardt]

On 10/24/24 12:46, Ilias Apalodimas wrote:
> We never free and unmap the memory on errors and we never unmap it when
> freeing it. This won't cause any problems on actual hardware but it will
> on sandbox
>
> Fixes: commit 22f2c9ed9f53 ("efi: memory: use the lmb API's for allocating and freeing memory")
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> ---
>   lib/efi_loader/efi_memory.c | 15 ++++++++++-----
>   1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index 9f3f08769977..84be5532a655 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -451,7 +451,7 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
>   				enum efi_memory_type memory_type,
>   				efi_uintn_t pages, uint64_t *memory)
>   {
> -	u64 len;
> +	u64 efi_addr, len;
>   	uint flags;
>   	efi_status_t ret;
>   	phys_addr_t addr;
> @@ -499,14 +499,17 @@ efi_status_t efi_allocate_pages(enum efi_allocate_type type,
>   		return EFI_INVALID_PARAMETER;
>   	}
>
> -	addr = (u64)(uintptr_t)map_sysmem(addr, 0);
> +	efi_addr = (u64)(uintptr_t)map_sysmem(addr, 0);
>   	/* Reserve that map in our memory maps */
> -	ret = efi_add_memory_map_pg(addr, pages, memory_type, true);
> -	if (ret != EFI_SUCCESS)
> +	ret = efi_add_memory_map_pg(efi_addr, pages, memory_type, true);
> +	if (ret != EFI_SUCCESS) {
>   		/* Map would overlap, bail out */
> +		lmb_free_flags(addr, (u64)pages << EFI_PAGE_SHIFT, flags);
> +		unmap_sysmem((void *)(uintptr_t)efi_addr);
>   		return  EFI_OUT_OF_RESOURCES;
> +	}
>
> -	*memory = addr;
> +	*memory = efi_addr;
>
>   	return EFI_SUCCESS;
>   }
> @@ -548,6 +551,8 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages)
>   	if (ret != EFI_SUCCESS)
>   		return EFI_NOT_FOUND;
>
> +	unmap_sysmem((void *)(uintptr_t)memory);
> +
>   	return ret;
>   }
>
> --
> 2.45.2
>

AllocatePages() only provides access to EfiConventionalMemory.

We can safely call map_sysmem() with len = 0 and not use unmap_sysmem().

Best regards

Heinrich