From mboxrd@z Thu Jan  1 00:00:00 1970
From: Takahiro Akashi <takahiro.akashi@linaro.org>
Date: Wed, 20 Mar 2019 09:22:44 +0900
Subject: [U-Boot] [PATCH 1/1] efi_loader: correct parameter size in
 efi_allocate_pool
In-Reply-To: <e235427f-154e-b114-f0fd-74e56028550e@gmx.de>
References: <20190318193223.1725-1-xypron.glpk@gmx.de>
 <20190319001906.GD9937@linaro.org>
 <e235427f-154e-b114-f0fd-74e56028550e@gmx.de>
Message-ID: <20190320002243.GI9937@linaro.org>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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