[U-Boot] [PATCH v2 1/6] efi_loader: Update description of internal efi_mem_carve_out

[U-Boot] [PATCH v2 1/6] efi_loader: Update description of internal efi_mem_carve_out

[Stefan Brüns]

In 74c16acce30bb882ad5951829d8dafef8eea564c the return values where
changed, but the description was kept.

Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
---
 lib/efi_loader/efi_memory.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index 80e4e26..ebe8e94 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,
-- 
2.10.0

[U-Boot] [PATCH v2 2/6] efi_loader: Fix memory map size check to avoid out-of-bounds access

[Stefan Brüns]

Do not overwrite the specified size of the provided buffer without
having checked it is sufficient.

If the buffer is to small, memory_map_size is updated to indicate the
required size, and an error code is returned.

Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
---
 lib/efi_loader/efi_memory.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index ebe8e94..72a5870 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -342,6 +342,11 @@ efi_status_t efi_get_memory_map(unsigned long *memory_map_size,
 
 	map_size = map_entries * sizeof(struct efi_mem_desc);
 
+	if (*memory_map_size < map_size) {
+		*memory_map_size = map_size;
+		return EFI_BUFFER_TOO_SMALL;
+	}
+
 	*memory_map_size = map_size;
 
 	if (descriptor_size)
@@ -350,9 +355,6 @@ 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)
-		return EFI_BUFFER_TOO_SMALL;
-
 	/* Copy list into array */
 	if (memory_map) {
 		/* Return the list in ascending order */
-- 
2.10.0

[U-Boot] [PATCH v2 3/6] efi_loader: Track size of pool allocations to allow freeing

[Stefan Brüns]

allocate_pool has to return a buffer which is 8-byte aligned. Shift the
region returned by allocate_pages by 8 byte and store the size in the
headroom. The 8 byte overhead is neglegible, but provides the required
size when freeing the allocation later.

Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
---
 lib/efi_loader/efi_boottime.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 784891b..c413ecb 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -135,19 +135,37 @@ static efi_status_t EFIAPI efi_allocate_pool(int pool_type, unsigned long size,
 {
 	efi_status_t r;
 	efi_physical_addr_t t;
+	u64 num_pages = (size + 8 + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;
 
 	EFI_ENTRY("%d, %ld, %p", pool_type, size, buffer);
-	r = efi_allocate_pages(0, pool_type, (size + 0xfff) >> 12, &t);
-	*buffer = (void *)(uintptr_t)t;
+
+	if (size == 0) {
+		*buffer = NULL;
+		return EFI_EXIT(EFI_SUCCESS);
+	}
+
+	r = efi_allocate_pages(0, pool_type, num_pages, &t);
+
+	if (r == EFI_SUCCESS) {
+		*(u64 *)(uintptr_t)t = num_pages;
+		*buffer = (void *)(uintptr_t)(t + 8);
+	}
+
 	return EFI_EXIT(r);
 }
 
 static efi_status_t EFIAPI efi_free_pool(void *buffer)
 {
 	efi_status_t r;
+	u64 num_pages;
 
 	EFI_ENTRY("%p", buffer);
-	r = efi_free_pages((ulong)buffer, 0);
+
+	buffer = (char *)(buffer) - 8;
+	assert(((ulong)buffer & EFI_PAGE_MASK) == 0);
+	num_pages = *(u64 *)buffer;
+
+	r = efi_free_pages((ulong)buffer, num_pages);
 	return EFI_EXIT(r);
 }
 
-- 
2.10.0

[U-Boot] [PATCH v2 4/6] efi_loader: Readd freed pages to memory pool

[Stefan Brüns]

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 <stefan.bruens@rwth-aachen.de>
Reviewed-by: Alexander Graf <agraf@suse.de>
---
 lib/efi_loader/efi_memory.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index 72a5870..9c785dd 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -323,8 +323,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_get_memory_map(unsigned long *memory_map_size,
-- 
2.10.0

[U-Boot] [PATCH v2 5/6] efi_loader: Keep memory mapping sorted when splitting an entry

[Stefan Brüns]

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 <stefan.bruens@rwth-aachen.de>
Reviewed-by: Alexander Graf <agraf@suse.de>
---
 lib/efi_loader/efi_memory.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index 9c785dd..26a5d50 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -122,7 +122,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;
-- 
2.10.0

[U-Boot] [PATCH v2 6/6] efi_loader: Do not leak memory when unlinking a mapping

[Stefan Brüns]

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 <stefan.bruens@rwth-aachen.de>
Reviewed-by: Alexander Graf <agraf@suse.de>
---
 lib/efi_loader/efi_memory.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index 26a5d50..000e776 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -103,10 +103,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;
 	}
 
-- 
2.10.0

[U-Boot] [PATCH v2 2/6] efi_loader: Fix memory map size check to avoid out-of-bounds access

[Alexander Graf]

On 01.10.16 19:31, Stefan Br?ns wrote:
> Do not overwrite the specified size of the provided buffer without
> having checked it is sufficient.
> 
> If the buffer is to small, memory_map_size is updated to indicate the
> required size, and an error code is returned.
> 
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
> ---
>  lib/efi_loader/efi_memory.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index ebe8e94..72a5870 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -342,6 +342,11 @@ efi_status_t efi_get_memory_map(unsigned long *memory_map_size,
>  
>  	map_size = map_entries * sizeof(struct efi_mem_desc);
>  
> +	if (*memory_map_size < map_size) {
> +		*memory_map_size = map_size;
> +		return EFI_BUFFER_TOO_SMALL;

IIRC I had issues with that approach in the past with payloads that
wanted to determine the descriptor size regardless of the error output.
I liked your previous version better - it really just needed a
description overhaul :)


Alex

[U-Boot] [PATCH v2 3/6] efi_loader: Track size of pool allocations to allow freeing

[Alexander Graf]

On 01.10.16 19:31, Stefan Br?ns wrote:
> allocate_pool has to return a buffer which is 8-byte aligned. Shift the
> region returned by allocate_pages by 8 byte and store the size in the
> headroom. The 8 byte overhead is neglegible, but provides the required
> size when freeing the allocation later.
> 
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
> ---
>  lib/efi_loader/efi_boottime.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 784891b..c413ecb 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -135,19 +135,37 @@ static efi_status_t EFIAPI efi_allocate_pool(int pool_type, unsigned long size,
>  {
>  	efi_status_t r;
>  	efi_physical_addr_t t;
> +	u64 num_pages = (size + 8 + EFI_PAGE_MASK) >> EFI_PAGE_SHIFT;

Would it make things more readable if you used sizeof(u64) here?

Also, it's great to have a nice patch description which described why we
have the 8 byte padding here, but we definitely need it as comment in
the code as well. Otherwise people 2 years from now will have no idea
why we allocate 8 bytes more ;). At least not without reverse
engineering the source.

>  
>  	EFI_ENTRY("%d, %ld, %p", pool_type, size, buffer);
> -	r = efi_allocate_pages(0, pool_type, (size + 0xfff) >> 12, &t);
> -	*buffer = (void *)(uintptr_t)t;
> +
> +	if (size == 0) {
> +		*buffer = NULL;
> +		return EFI_EXIT(EFI_SUCCESS);
> +	}
> +
> +	r = efi_allocate_pages(0, pool_type, num_pages, &t);
> +
> +	if (r == EFI_SUCCESS) {
> +		*(u64 *)(uintptr_t)t = num_pages;

I would prefer if we could make that cast a bit more expressive. How about

  u64 *num_pages_hint = (void *)(uintptr_t)t;

  *num_pages_hint = num_pages;
  *buffer = &num_pages_hint[1];

or maybe even better would be a trivial struct. Something like

  struct efi_pool_allocation {
    u64 num_pages;
    char data[];
  };

Then you could basically write mostly self-documenting code:

  struct efi_pool_alloction *alloc = (void *)(uintptr_t)t;

  alloc->num_pages = num_pages;
  *buffer = alloc->data;

I would still prefer if you could write a comment about what's going on,
but with this it's much more obvious IMHO.

> +		*buffer = (void *)(uintptr_t)(t + 8);
> +	}
> +
>  	return EFI_EXIT(r);
>  }
>  
>  static efi_status_t EFIAPI efi_free_pool(void *buffer)
>  {
>  	efi_status_t r;
> +	u64 num_pages;
>  
>  	EFI_ENTRY("%p", buffer);
> -	r = efi_free_pages((ulong)buffer, 0);
> +
> +	buffer = (char *)(buffer) - 8;
> +	assert(((ulong)buffer & EFI_PAGE_MASK) == 0);
> +	num_pages = *(u64 *)buffer;

With the struct, this could look a lot cleaner too:

  struct efi_pool_alloction *alloc = container_of(buffer, struct
efi_pool_alloction, data);

  /* Pool allocations always happen on page boundaries */
  assert(!((uintptr_t)alloc & EFI_PAGE_MASK));
  r = efi_free_pages((uintptr_t)alloc->data, alloc->num_pages);


Alex

> +
> +	r = efi_free_pages((ulong)buffer, num_pages);
>  	return EFI_EXIT(r);
>  }
>  
>