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

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

[Heinrich Schuchardt]

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@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);
-
+			       &addr);
 	if (r == EFI_SUCCESS) {
+		alloc = (struct efi_pool_allocation *)(uintptr_t)addr;
 		alloc->num_pages = num_pages;
 		*buffer = alloc->data;
 	}
-- 
2.20.1

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

[Takahiro Akashi]

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@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.
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.

Thanks,
-Takahiro Akashi

>  		alloc->num_pages = num_pages;
>  		*buffer = alloc->data;
>  	}
> -- 
> 2.20.1
> 

[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@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
>>
> 

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

[Takahiro Akashi]

On Tue, Mar 19, 2019 at 07:59:37AM +0100, Heinrich Schuchardt wrote:
> 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@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 *.

But allocate_pool() takes "void **" for the third argument. Why?

> > 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")

I don't know the case of sandbox, but What I have in mind is
LPAE, that is, the system have >32-bit physical address space,
but cpu can only access it via 32-bit pointer.

Thanks,
-Takahiro Akashi

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

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

[Heinrich Schuchardt]

On 3/20/19 1:22 AM, Takahiro Akashi wrote:
> On Tue, Mar 19, 2019 at 07:59:37AM +0100, Heinrich Schuchardt wrote:
>> 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@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 *.
>
> But allocate_pool() takes "void **" for the third argument. Why?

efi_allocate_pool() implements the AllocatePool() boot service which is
defined in the UEFI spec this way.

>
>>> 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")
>
> I don't know the case of sandbox, but What I have in mind is
> LPAE, that is, the system have >32-bit physical address space,
> but cpu can only access it via 32-bit pointer.

The UEFI spec requires "if paging is enabled and SetVirtualAddressMap()
has not been called, any memory space defined by the UEFI memory map is
identity mapped (virtual address equals physical address)".

It is only after calling SetVirtualAddressMap() that virtual addresses
and physical ones may differ. But at that time boottime services like
AllocatePool may not be called anymore.

I guess it is with LPAE in mind that AllocatePages() uses 64bit physical
addresses while AllocatePool() uses virtual addresses with system
dependent size.

Best regards

Heinrich

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