[U-Boot] [PATCH 1/1] efi_loader: correct parameter size in efi_allocate_pool

[Heinrich Schuchardt]

On 3/19/19 1:19 AM, Takahiro Akashi wrote:
> On Mon, Mar 18, 2019 at 08:32:23PM +0100, Heinrich Schuchardt wrote:
>> efi_allocate_pages() expects a (uint64_t *) pointer to pass the address of
>> the assigned memory. If we pass the address of a pointer here, an illegal
>> memory access occurs on 32bit systems.
>>
>> Fixes: 282a06cbcae8 ("efi_loader: Expose U-Boot addresses in memory map
>> for sandbox")
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
>> ---
>>  lib/efi_loader/efi_memory.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
>> index ebd2b36c03..55622d2fb4 100644
>> --- a/lib/efi_loader/efi_memory.c
>> +++ b/lib/efi_loader/efi_memory.c
>> @@ -440,6 +440,7 @@ efi_status_t efi_free_pages(uint64_t memory, efi_uintn_t pages)
>>  efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void **buffer)
>>  {
>>  	efi_status_t r;
>> +	u64 addr;
>>  	struct efi_pool_allocation *alloc;
>>  	u64 num_pages = efi_size_in_pages(size +
>>  					  sizeof(struct efi_pool_allocation));
>> @@ -453,9 +454,9 @@ efi_status_t efi_allocate_pool(int pool_type, efi_uintn_t size, void **buffer)
>>  	}
>>  
>>  	r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, pool_type, num_pages,
>> -			       (uint64_t *)&alloc);
> 
> I wonder why efi_allocate_pages() doesn't expect (void **) for the fourth
> argument.

efi_allocate_pages implements the AllocatePages() boot time service. The
UEFI spec mandates that the parameter is of type EFI_PHYSICAL_ADDRESS *
i.e. u64 *.

> If this is because the type of the argument is a pointer to "physical address,"
> 
>> -
>> +			       &addr);
>>  	if (r == EFI_SUCCESS) {
>> +		alloc = (struct efi_pool_allocation *)(uintptr_t)addr;
> 
> we should use map_sysmem() here.

This would create a bug on the sandbox.

map_sysmem() converts an address from the sandbox virtual address space
to the address space that can be used by EFI binaries.

AllocatePool() and AllocatePages() both refer to the same address space.

Cf. commit 49759743bf09 ("efi_loader: eliminate sandbox addresses")

Best regards

Heinrich

> 
> Thanks,
> -Takahiro Akashi
> 
>>  		alloc->num_pages = num_pages;
>>  		*buffer = alloc->data;
>>  	}
>> -- 
>> 2.20.1
>>
>