| Message ID | 20190318193223.1725-1-xypron.glpk@gmx.de |
|---|---|
| State | Accepted, archived |
| Commit | 306b16718edddd660b84bf3c6627ce5d41b53ce7 |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | [U-Boot,1/1] efi_loader: correct parameter size in efi_allocate_pool | expand |
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 >
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 >> >
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 > >> > > >
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 >>>> >>> >> >
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; }
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(-)