| Message ID | 20180618140835.195901-25-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Alexander Graf |
| Headers | show |
| Series | efi: Enable sandbox support for EFI loader | expand |
On 06/18/2018 04:08 PM, Simon Glass wrote: > Sandbox does not support direct casts between addresses and pointers, > since it uses an emulated RAM buffer rather than directly using host > RAM. > > The current EFI code uses an integer type for addresses, but treats them > like pointers. > > Update the code to maintain a (reasonably) clear separation between the > two, so that sandbox can work. > > Unfortunately there remains an inconsistency between the arguments of > allocate_pages() and allocate_pool(). One takes an address and one takes > a pointer. Partly this seems to be defined by the boot services API itself > but it would be fairly easy to update the functions in efi_memory.c to be > consistent. However, this is a larger change and needs discussion. > > Signed-off-by: Simon Glass <sjg@chromium.org> This also seems to be fallout of a misinterpretation of the API :) Alex
Hi Alex, On 18 June 2018 at 09:03, Alexander Graf <agraf@suse.de> wrote: > On 06/18/2018 04:08 PM, Simon Glass wrote: >> >> Sandbox does not support direct casts between addresses and pointers, >> since it uses an emulated RAM buffer rather than directly using host >> RAM. >> >> The current EFI code uses an integer type for addresses, but treats them >> like pointers. >> >> Update the code to maintain a (reasonably) clear separation between the >> two, so that sandbox can work. >> >> Unfortunately there remains an inconsistency between the arguments of >> allocate_pages() and allocate_pool(). One takes an address and one takes >> a pointer. Partly this seems to be defined by the boot services API itself >> but it would be fairly easy to update the functions in efi_memory.c to be >> consistent. However, this is a larger change and needs discussion. >> >> Signed-off-by: Simon Glass <sjg@chromium.org> > > > This also seems to be fallout of a misinterpretation of the API :) Well I suppose it's not important. But AllocatePages() returns an address, but AllocatePool() returns a pointer. I'm not sure why? Regards, Simon
On 06/21/2018 04:01 AM, Simon Glass wrote: > Hi Alex, > > On 18 June 2018 at 09:03, Alexander Graf <agraf@suse.de> wrote: >> On 06/18/2018 04:08 PM, Simon Glass wrote: >>> Sandbox does not support direct casts between addresses and pointers, >>> since it uses an emulated RAM buffer rather than directly using host >>> RAM. >>> >>> The current EFI code uses an integer type for addresses, but treats them >>> like pointers. >>> >>> Update the code to maintain a (reasonably) clear separation between the >>> two, so that sandbox can work. >>> >>> Unfortunately there remains an inconsistency between the arguments of >>> allocate_pages() and allocate_pool(). One takes an address and one takes >>> a pointer. Partly this seems to be defined by the boot services API itself >>> but it would be fairly easy to update the functions in efi_memory.c to be >>> consistent. However, this is a larger change and needs discussion. >>> >>> Signed-off-by: Simon Glass <sjg@chromium.org> >> >> This also seems to be fallout of a misinterpretation of the API :) > Well I suppose it's not important. > > But AllocatePages() returns an address, but AllocatePool() returns a > pointer. I'm not sure why? AllocatePages returns a pointer. Alex
Hi Alex, On 21 June 2018 at 04:14, Alexander Graf <agraf@suse.de> wrote: > On 06/21/2018 04:01 AM, Simon Glass wrote: >> >> Hi Alex, >> >> On 18 June 2018 at 09:03, Alexander Graf <agraf@suse.de> wrote: >>> >>> On 06/18/2018 04:08 PM, Simon Glass wrote: >>>> >>>> Sandbox does not support direct casts between addresses and pointers, >>>> since it uses an emulated RAM buffer rather than directly using host >>>> RAM. >>>> >>>> The current EFI code uses an integer type for addresses, but treats them >>>> like pointers. >>>> >>>> Update the code to maintain a (reasonably) clear separation between the >>>> two, so that sandbox can work. >>>> >>>> Unfortunately there remains an inconsistency between the arguments of >>>> allocate_pages() and allocate_pool(). One takes an address and one takes >>>> a pointer. Partly this seems to be defined by the boot services API >>>> itself >>>> but it would be fairly easy to update the functions in efi_memory.c to >>>> be >>>> consistent. However, this is a larger change and needs discussion. >>>> >>>> Signed-off-by: Simon Glass <sjg@chromium.org> >>> >>> >>> This also seems to be fallout of a misinterpretation of the API :) >> >> Well I suppose it's not important. >> >> But AllocatePages() returns an address, but AllocatePool() returns a >> pointer. I'm not sure why? > > > AllocatePages returns a pointer. It is declared: IN OUT EFI_PHYSICAL_ADDRESS *Memory So it returns an address, a EFI_PHYSICAL_ADDRESS, which is a uint64_t, which is an address, not a pointer. This is how it is used in U-Boot too. The fact that it is declared as EFI_PHYSICAL_ADDRESS * is simply so it can return a value. That is how things work in C. You cannot return a value in an arg unless you make that are a pointer. Regards, Simon
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 50d311548e..aefafc3fba 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -14,6 +14,7 @@ #include <u-boot/crc.h> #include <bootm.h> #include <inttypes.h> +#include <mapmem.h> #include <watchdog.h> DECLARE_GLOBAL_DATA_PTR; @@ -305,9 +306,17 @@ static efi_status_t EFIAPI efi_allocate_pages_ext(int type, int memory_type, uint64_t *memory) { efi_status_t r; + uint64_t addr; EFI_ENTRY("%d, %d, 0x%zx, %p", type, memory_type, pages, memory); - r = efi_allocate_pages(type, memory_type, pages, memory); + if (memory_type == EFI_ALLOCATE_MAX_ADDRESS || + memory_type == EFI_ALLOCATE_ADDRESS) + addr = *memory == -1ULL ? *memory : + map_to_sysmem((void *)(uintptr_t)*memory); + else + addr = 0; + r = efi_allocate_pages(type, memory_type, pages, &addr); + *memory = (uintptr_t)map_sysmem(addr, pages << EFI_PAGE_SHIFT); return EFI_EXIT(r); } @@ -328,7 +337,7 @@ static efi_status_t EFIAPI efi_free_pages_ext(uint64_t memory, efi_status_t r; EFI_ENTRY("%" PRIx64 ", 0x%zx", memory, pages); - r = efi_free_pages(memory, pages); + r = efi_free_pages(map_to_sysmem((void *)memory), pages); return EFI_EXIT(r); } diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c index ecdb77e5b6..d5dd8864d7 100644 --- a/lib/efi_loader/efi_image_loader.c +++ b/lib/efi_loader/efi_image_loader.c @@ -9,6 +9,7 @@ #include <common.h> #include <efi_loader.h> +#include <mapmem.h> #include <pe.h> const efi_guid_t efi_global_variable_guid = EFI_GLOBAL_VARIABLE_GUID; @@ -301,7 +302,7 @@ void *efi_load_pe(void *efi, struct efi_loaded_image *loaded_image_info) /* Run through relocations */ if (efi_loader_relocate(rel, rel_size, efi_reloc, (unsigned long)image_base) != EFI_SUCCESS) { - efi_free_pages((uintptr_t) efi_reloc, + efi_free_pages(map_to_sysmem(efi_reloc), (virt_size + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT); return NULL; } diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index c6410613c7..ad61b723e6 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -345,7 +345,7 @@ void *efi_alloc(uint64_t len, int memory_type) r = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES, memory_type, pages, &ret); if (r == EFI_SUCCESS) - return (void*)(uintptr_t)ret; + return map_sysmem(ret, len); return NULL; } @@ -412,15 +412,17 @@ efi_status_t efi_free_pool(void *buffer) { efi_status_t r; struct efi_pool_allocation *alloc; + uintptr_t addr; if (buffer == NULL) return EFI_INVALID_PARAMETER; alloc = container_of(buffer, struct efi_pool_allocation, data); /* Sanity check, was the supplied address returned by allocate_pool */ - assert(((uintptr_t)alloc & EFI_PAGE_MASK) == 0); + addr = map_to_sysmem(alloc); + assert((addr & EFI_PAGE_MASK) == 0); - r = efi_free_pages((uintptr_t)alloc, alloc->num_pages); + r = efi_free_pages(addr, alloc->num_pages); return r; } @@ -471,7 +473,14 @@ efi_status_t efi_get_memory_map(efi_uintn_t *memory_map_size, struct efi_mem_list *lmem; lmem = list_entry(lhandle, struct efi_mem_list, link); - *memory_map = lmem->desc; + memory_map->type = lmem->desc.type; + memory_map->reserved = 0; + memory_map->physical_start = (uintptr_t)map_sysmem( + lmem->desc.physical_start, + memory_map->num_pages << EFI_PAGE_SHIFT); + memory_map->virtual_start = memory_map->physical_start; + memory_map->num_pages = lmem->desc.num_pages; + memory_map->attribute = lmem->desc.attribute; memory_map--; } }
Sandbox does not support direct casts between addresses and pointers, since it uses an emulated RAM buffer rather than directly using host RAM. The current EFI code uses an integer type for addresses, but treats them like pointers. Update the code to maintain a (reasonably) clear separation between the two, so that sandbox can work. Unfortunately there remains an inconsistency between the arguments of allocate_pages() and allocate_pool(). One takes an address and one takes a pointer. Partly this seems to be defined by the boot services API itself but it would be fairly easy to update the functions in efi_memory.c to be consistent. However, this is a larger change and needs discussion. Signed-off-by: Simon Glass <sjg@chromium.org> --- Changes in v8: None Changes in v7: None Changes in v6: None Changes in v5: None Changes in v4: None Changes in v3: None Changes in v2: None lib/efi_loader/efi_boottime.c | 13 +++++++++++-- lib/efi_loader/efi_image_loader.c | 3 ++- lib/efi_loader/efi_memory.c | 17 +++++++++++++---- 3 files changed, 26 insertions(+), 7 deletions(-)