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 <[email protected]>
> >> ---
> >> 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 mailing list
[email protected]
https://lists.denx.de/listinfo/u-boot
