From 852efbf5bd3047b12c1926564d792a7a1cea9eac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stefan=20Br=C3=BCns?= Date: Sat, 1 Oct 2016 23:32:23 +0200 Subject: efi_loader: Update description of internal efi_mem_carve_out MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In 74c16acce30bb882ad5951829d8dafef8eea564c the return values where changed, but the description was kept. Signed-off-by: Stefan Brüns Reviewed-by: Alexander Graf Signed-off-by: Alexander Graf --- lib/efi_loader/efi_memory.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) (limited to 'lib/efi_loader/efi_memory.c') diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 80e4e26e05e..ebe8e94c834 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -62,9 +62,17 @@ static void efi_mem_sort(void) * Unmaps all memory occupied by the carve_desc region from the * list entry pointed to by map. * - * Returns 1 if carving was performed or 0 if the regions don't overlap. - * Returns -1 if it would affect non-RAM regions but overlap_only_ram is set. - * Carving is only guaranteed to complete when all regions return 0. + * Returns EFI_CARVE_NO_OVERLAP if the regions don't overlap. + * Returns EFI_CARVE_OVERLAPS_NONRAM if the carve and map overlap, + * and the map contains anything but free ram. + * (only when overlap_only_ram is true) + * Returns EFI_CARVE_LOOP_AGAIN if the mapping list should be traversed + * again, as it has been altered + * Returns the number of overlapping pages. The pages are removed from + * the mapping list. + * + * In case of EFI_CARVE_OVERLAPS_NONRAM it is the callers responsibility + * to readd the already carved out pages to the mapping. */ static int efi_mem_carve_out(struct efi_mem_list *map, struct efi_mem_desc *carve_desc, -- cgit v1.2.3 From bdf5c1b3607bd6384ac5319caad2d8107130ace1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stefan=20Br=C3=BCns?= Date: Sun, 9 Oct 2016 22:17:07 +0200 Subject: efi_loader: Fix memory map size check to avoid out-of-bounds access MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The current efi_get_memory_map() function overwrites the map_size property before reading its value. That way the sanity check whether our memory map fits into the given array always succeeds, potentially overwriting arbitrary payload memory. This patch moves the property update write after its sanity check, so that the check actually verifies the correct value. So far this has not triggered any known bugs, but we're better off safe than sorry. If the buffer is to small, the returned memory_map_size indicates the required size to the caller. Signed-off-by: Stefan Brüns Reviewed-by: Alexander Graf Signed-off-by: Alexander Graf --- lib/efi_loader/efi_memory.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'lib/efi_loader/efi_memory.c') diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index ebe8e94c834..1d237830202 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -336,6 +336,7 @@ efi_status_t efi_get_memory_map(unsigned long *memory_map_size, ulong map_size = 0; int map_entries = 0; struct list_head *lhandle; + unsigned long provided_map_size = *memory_map_size; list_for_each(lhandle, &efi_mem) map_entries++; @@ -350,7 +351,7 @@ efi_status_t efi_get_memory_map(unsigned long *memory_map_size, if (descriptor_version) *descriptor_version = EFI_MEMORY_DESCRIPTOR_VERSION; - if (*memory_map_size < map_size) + if (provided_map_size < map_size) return EFI_BUFFER_TOO_SMALL; /* Copy list into array */ -- cgit v1.2.3 From ead1274b7f9578e346b3cdcb3d9e2002ef8f0e75 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stefan=20Br=C3=BCns?= Date: Sun, 9 Oct 2016 22:17:18 +0200 Subject: efi_loader: Move efi_allocate_pool implementation to efi_memory.c MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We currently handle efi_allocate_pool() in our boot time service file. In the following patch, pool allocation will receive additional internal semantics that we should preserve inside efi_memory.c instead. As foundation for those changes, split the function into an externally facing efi_allocate_pool_ext() for use by payloads and an internal helper efi_allocate_pool() in efi_memory.c that handles the actual allocation. While at it, change the magic 0xfff / 12 constants to the more obvious EFI_PAGE_MASK/SHIFT defines. Signed-off-by: Stefan Brüns Reviewed-by: Alexander Graf Signed-off-by: Alexander Graf --- lib/efi_loader/efi_memory.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) (limited to 'lib/efi_loader/efi_memory.c') diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 1d237830202..be642f12a92 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -327,6 +327,20 @@ efi_status_t efi_free_pages(uint64_t memory, unsigned long pages) return EFI_SUCCESS; } +efi_status_t efi_allocate_pool(int pool_type, unsigned long size, + void **buffer) +{ + efi_status_t r; + efi_physical_addr_t t; + u64 num_pages = (size + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT; + + r = efi_allocate_pages(0, pool_type, num_pages, &t); + if (r == EFI_SUCCESS) + *buffer = (void *)(uintptr_t)t; + + return r; +} + efi_status_t efi_get_memory_map(unsigned long *memory_map_size, struct efi_mem_desc *memory_map, unsigned long *map_key, -- cgit v1.2.3 From 42417bc84d1d56f739d43d562773d4821cf1bf47 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stefan=20Br=C3=BCns?= Date: Sun, 9 Oct 2016 22:17:26 +0200 Subject: efi_loader: Track size of pool allocations to allow freeing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We need a functional free_pool implementation, as otherwise each allocate_pool causes growth of the memory descriptor table. Different to free_pages, free_pool does not provide the size for the to be freed allocation, thus we have to track the size ourselves. As the only EFI requirement for pool allocation is an alignment of 8 bytes, we can keep allocating a range using the page allocator, reserve the first 8 bytes for our bookkeeping and hand out the remainder to the caller. This saves us from having to use any independent data structures for tracking. To simplify the conversion between pool allocations and the corresponding page allocation, we create an auxiliary struct efi_pool_allocation. Given the allocation size free_pool size can handoff freeing the page range, which was indirectly allocated by a call to allocate_pool, to free_pages. Signed-off-by: Stefan Brüns Reviewed-by: Alexander Graf Signed-off-by: Alexander Graf --- lib/efi_loader/efi_memory.c | 42 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 39 insertions(+), 3 deletions(-) (limited to 'lib/efi_loader/efi_memory.c') diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index be642f12a92..de28db6e44b 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -33,6 +33,19 @@ LIST_HEAD(efi_mem); void *efi_bounce_buffer; #endif +/* + * U-Boot services each EFI AllocatePool request as a separate + * (multiple) page allocation. We have to track the number of pages + * to be able to free the correct amount later. + * EFI requires 8 byte alignment for pool allocations, so we can + * prepend each allocation with an 64 bit header tracking the + * allocation size, and hand out the remainder to the caller. + */ +struct efi_pool_allocation { + u64 num_pages; + char data[]; +}; + /* * Sorts the memory list from highest address to lowest address * @@ -332,11 +345,34 @@ efi_status_t efi_allocate_pool(int pool_type, unsigned long size, { efi_status_t r; efi_physical_addr_t t; - u64 num_pages = (size + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT; + u64 num_pages = (size + sizeof(u64) + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT; + + if (size == 0) { + *buffer = NULL; + return EFI_SUCCESS; + } r = efi_allocate_pages(0, pool_type, num_pages, &t); - if (r == EFI_SUCCESS) - *buffer = (void *)(uintptr_t)t; + + if (r == EFI_SUCCESS) { + struct efi_pool_allocation *alloc = (void *)(uintptr_t)t; + alloc->num_pages = num_pages; + *buffer = alloc->data; + } + + return r; +} + +efi_status_t efi_free_pool(void *buffer) +{ + efi_status_t r; + struct efi_pool_allocation *alloc; + + 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); + + r = efi_free_pages((uintptr_t)alloc, alloc->num_pages); return r; } -- cgit v1.2.3 From b61d857b2ff3b0b099ef187d7ceebe26ea788578 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stefan=20Br=C3=BCns?= Date: Sat, 1 Oct 2016 23:32:27 +0200 Subject: efi_loader: Readd freed pages to memory pool MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently each allocation creates a new mapping. Readding the mapping as free memory (EFI_CONVENTIONAL_MEMORY) potentially allows to hand out an existing mapping, thus limiting the number of mapping descriptors in the memory map. Mitigates a problem with current (4.8rc7) linux kernels when doing an efi_get_memory map, resulting in an infinite loop. Space for the memory map is reserved with allocate_pool (implicitly creating a new mapping) and filled. If there is insufficient slack space (8 entries) in the map, the space is freed and a new round is started, with space for one more entry. As each round increases requirement and allocation by exactly one, there is never enough slack space. (At least 32 entries are allocated, so as long as there are less than 24 entries, there is enough slack). Earlier kernels reserved no slack, and did less allocations, so this problem was not visible. Signed-off-by: Stefan Brüns Reviewed-by: Alexander Graf Signed-off-by: Alexander Graf --- lib/efi_loader/efi_memory.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) (limited to 'lib/efi_loader/efi_memory.c') diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index de28db6e44b..d3a2ffdac68 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -336,8 +336,15 @@ void *efi_alloc(uint64_t len, int memory_type) efi_status_t efi_free_pages(uint64_t memory, unsigned long pages) { - /* We don't free, let's cross our fingers we have plenty RAM */ - return EFI_SUCCESS; + uint64_t r = 0; + + r = efi_add_memory_map(memory, pages, EFI_CONVENTIONAL_MEMORY, false); + /* Merging of adjacent free regions is missing */ + + if (r == memory) + return EFI_SUCCESS; + + return EFI_NOT_FOUND; } efi_status_t efi_allocate_pool(int pool_type, unsigned long size, -- cgit v1.2.3 From b6a951727504d4159467ac98434849f81aaf9ffa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stefan=20Br=C3=BCns?= Date: Sat, 1 Oct 2016 23:32:28 +0200 Subject: efi_loader: Keep memory mapping sorted when splitting an entry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The code assumes sorted mappings in descending address order. When splitting a mapping, insert the new part next to the current mapping. Signed-off-by: Stefan Brüns Reviewed-by: Alexander Graf Signed-off-by: Alexander Graf --- lib/efi_loader/efi_memory.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'lib/efi_loader/efi_memory.c') diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index d3a2ffdac68..742bc9084ff 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -135,7 +135,8 @@ static int efi_mem_carve_out(struct efi_mem_list *map, newmap->desc = map->desc; newmap->desc.physical_start = carve_start; newmap->desc.num_pages = (map_end - carve_start) >> EFI_PAGE_SHIFT; - list_add_tail(&newmap->link, &efi_mem); + /* Insert before current entry (descending address order) */ + list_add_tail(&newmap->link, &map->link); /* Shrink the map to [ map_start ... carve_start ] */ map_desc->num_pages = (carve_start - map_start) >> EFI_PAGE_SHIFT; -- cgit v1.2.3 From 511d0b97ef709d13da4922fb694d55ef9a5ef641 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stefan=20Br=C3=BCns?= Date: Sat, 1 Oct 2016 23:32:29 +0200 Subject: efi_loader: Do not leak memory when unlinking a mapping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As soon as a mapping is unlinked from the list, there are no further references to it, so it should be freed. If it not unlinked, update the start address and length. Signed-off-by: Stefan Brüns Reviewed-by: Alexander Graf Signed-off-by: Alexander Graf --- lib/efi_loader/efi_memory.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) (limited to 'lib/efi_loader/efi_memory.c') diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index 742bc9084ff..95aa590c8af 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -116,10 +116,13 @@ static int efi_mem_carve_out(struct efi_mem_list *map, if (map_end == carve_end) { /* Full overlap, just remove map */ list_del(&map->link); + free(map); + } else { + map->desc.physical_start = carve_end; + map->desc.num_pages = (map_end - carve_end) + >> EFI_PAGE_SHIFT; } - map_desc->physical_start = carve_end; - map_desc->num_pages = (map_end - carve_end) >> EFI_PAGE_SHIFT; return (carve_end - carve_start) >> EFI_PAGE_SHIFT; } -- cgit v1.2.3