[U-Boot] [PATCH v3 1/7] efi_loader: Update description of internal efi_mem_carve_out

[U-Boot] [PATCH v3 1/7] 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 v3 1/7] efi_loader: Update description of internal efi_mem_carve_out

[Alexander Graf]

On 01.10.16 23:32, Stefan Br?ns wrote:
> In 74c16acce30bb882ad5951829d8dafef8eea564c the return values where
> changed, but the description was kept.
> 
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>

Reviewed-by: Alexander Graf <agraf@suse.de>


Alex

[U-Boot] [U-Boot, v3, 1/7] efi_loader: Update description of internal efi_mem_carve_out

[Alexander Graf]

> In 74c16acce30bb882ad5951829d8dafef8eea564c the return values where
> changed, but the description was kept.
> 
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
> Reviewed-by: Alexander Graf <agraf@suse.de>

Thanks, applied to 

[U-Boot] [PATCH v3 2/7] 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..5d71fdf 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -342,16 +342,18 @@ efi_status_t efi_get_memory_map(unsigned long *memory_map_size,
 
 	map_size = map_entries * sizeof(struct efi_mem_desc);
 
-	*memory_map_size = map_size;
-
 	if (descriptor_size)
 		*descriptor_size = sizeof(struct efi_mem_desc);
 
 	if (descriptor_version)
 		*descriptor_version = EFI_MEMORY_DESCRIPTOR_VERSION;
 
-	if (*memory_map_size < map_size)
+	if (*memory_map_size < map_size) {
+		*memory_map_size = map_size;
 		return EFI_BUFFER_TOO_SMALL;
+	}
+
+	*memory_map_size = map_size;
 
 	/* Copy list into array */
 	if (memory_map) {
-- 
2.10.0

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

[Alexander Graf]

On 01.10.16 23:32, 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>

Usually the pattern I like for commit messages is:

  * current state
  * why current state is bad
  * what this patch does to change it
  * what bugs/features this patch enables

So in your case, that would be along the lines of:

---

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.

---

> ---
>  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..5d71fdf 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -342,16 +342,18 @@ efi_status_t efi_get_memory_map(unsigned long *memory_map_size,
>  
>  	map_size = map_entries * sizeof(struct efi_mem_desc);
>  
> -	*memory_map_size = map_size;
> -
>  	if (descriptor_size)
>  		*descriptor_size = sizeof(struct efi_mem_desc);
>  
>  	if (descriptor_version)
>  		*descriptor_version = EFI_MEMORY_DESCRIPTOR_VERSION;
>  
> -	if (*memory_map_size < map_size)
> +	if (*memory_map_size < map_size) {
> +		*memory_map_size = map_size;
>  		return EFI_BUFFER_TOO_SMALL;
> +	}
> +
> +	*memory_map_size = map_size;

Sorry for making you go in circles, but I'm afraid that while the code
flow is pretty obvious for us two now, it feels prone to break by anyone
who touches it next.

Could we instead make the whole thing explicit? Something like this:

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index 80e4e26..eb3cfd1 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -328,6 +328,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 max_memory_map_size = *memory_map_size;

 	list_for_each(lhandle, &efi_mem)
 		map_entries++;
@@ -342,7 +343,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 (max_memory_map_size < map_size)
 		return EFI_BUFFER_TOO_SMALL;

 	/* Copy list into array */


Since most of the other patches can stay basically untouched, you can
also send only an updated v4 of this particular patch. For that, check
the mail header of v3 2/7 for a "message id":

  <aeea463376c84449ae8568a974d71dc8@rwthex-w2-b.rwth-ad.de>

Then use that as reply-to field in the email send

  git format-patch -n --subject-prefix="PATCH v4" -o v4 origin/master
  git send-email
--in-reply-to="<aeea463376c84449ae8568a974d71dc8@rwthex-w2-b.rwth-ad.de>"
--to u-boot at lists.denx.de --cc agraf at suse.de v4/0002*

Unless of course you need to rewrite half the patch set. Then you're
better off resending the whole thing :)

Also your threading is somehow broken for the cover letter. How do you
send that one?


Alex

[U-Boot] [PATCH v3 3/7] efi_loader: Move efi_allocate_pool implementation to efi_memory.c

[Stefan Brüns]

Implementation essentially unchanged, but use EFI_PAGE_MASK/SHIFT
instead of numeric constants.

Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
---
 include/efi_loader.h          |  3 +++
 lib/efi_loader/efi_boottime.c | 11 +++++------
 lib/efi_loader/efi_memory.c   | 11 +++++++++++
 3 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 9738835..40e7beb 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -119,6 +119,9 @@ efi_status_t efi_allocate_pages(int type, int memory_type, unsigned long pages,
 				uint64_t *memory);
 /* EFI memory free function. Not implemented today */
 efi_status_t efi_free_pages(uint64_t memory, unsigned long pages);
+/* EFI memory allocator for small allocations, called by EFI payloads */
+efi_status_t efi_allocate_pool(int pool_type, unsigned long size,
+			       void **buffer);
 /* Returns the EFI memory map */
 efi_status_t efi_get_memory_map(unsigned long *memory_map_size,
 				struct efi_mem_desc *memory_map,
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 784891b..eb74cb0 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -130,15 +130,14 @@ efi_status_t EFIAPI efi_get_memory_map_ext(unsigned long *memory_map_size,
 	return EFI_EXIT(r);
 }
 
-static efi_status_t EFIAPI efi_allocate_pool(int pool_type, unsigned long size,
-					     void **buffer)
+static efi_status_t EFIAPI efi_allocate_pool_ext(int pool_type,
+						 unsigned long size,
+						 void **buffer)
 {
 	efi_status_t r;
-	efi_physical_addr_t t;
 
 	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;
+	r = efi_allocate_pool(pool_type, size, buffer);
 	return EFI_EXIT(r);
 }
 
@@ -736,7 +735,7 @@ static const struct efi_boot_services efi_boot_services = {
 	.allocate_pages = efi_allocate_pages_ext,
 	.free_pages = efi_free_pages_ext,
 	.get_memory_map = efi_get_memory_map_ext,
-	.allocate_pool = efi_allocate_pool,
+	.allocate_pool = efi_allocate_pool_ext,
 	.free_pool = efi_free_pool,
 	.create_event = efi_create_event,
 	.set_timer = efi_set_timer,
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index 5d71fdf..045558d 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -327,6 +327,17 @@ 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);
+	return EFI_EXIT(r);
+}
+
 efi_status_t efi_get_memory_map(unsigned long *memory_map_size,
 			       struct efi_mem_desc *memory_map,
 			       unsigned long *map_key,
-- 
2.10.0

[U-Boot] [PATCH v3 3/7] efi_loader: Move efi_allocate_pool implementation to efi_memory.c

[Alexander Graf]

On 01.10.16 23:32, Stefan Br?ns wrote:
> Implementation essentially unchanged, but use EFI_PAGE_MASK/SHIFT
> instead of numeric constants.

Sorry for being nitpicky about the commit message again. Imagine you're
a distribution maintainer for U-Boot and you need to read the commit
messages of all commits in the tree to figure out whether you need to
cherry-pick a patch or not. Or you git bisect down to this commit and
need to figure out what the original intention of this patch was.

How about something like

---

We currently handle efi_allocate_pool() inside of 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 helper efi_allocate_pool_ext() for use by payloads and an
internal function 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.

---


Alex

> 
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
> ---
>  include/efi_loader.h          |  3 +++
>  lib/efi_loader/efi_boottime.c | 11 +++++------
>  lib/efi_loader/efi_memory.c   | 11 +++++++++++
>  3 files changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 9738835..40e7beb 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -119,6 +119,9 @@ efi_status_t efi_allocate_pages(int type, int memory_type, unsigned long pages,
>  				uint64_t *memory);
>  /* EFI memory free function. Not implemented today */
>  efi_status_t efi_free_pages(uint64_t memory, unsigned long pages);
> +/* EFI memory allocator for small allocations, called by EFI payloads */

The function that gets called by EFI payloads is actually the _ext one,
as that's the one that swizzles the gd pointer as well. Just remove the
last bit of the sentence.

> +efi_status_t efi_allocate_pool(int pool_type, unsigned long size,
> +			       void **buffer);
>  /* Returns the EFI memory map */
>  efi_status_t efi_get_memory_map(unsigned long *memory_map_size,
>  				struct efi_mem_desc *memory_map,
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 784891b..eb74cb0 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -130,15 +130,14 @@ efi_status_t EFIAPI efi_get_memory_map_ext(unsigned long *memory_map_size,
>  	return EFI_EXIT(r);
>  }
>  
> -static efi_status_t EFIAPI efi_allocate_pool(int pool_type, unsigned long size,
> -					     void **buffer)
> +static efi_status_t EFIAPI efi_allocate_pool_ext(int pool_type,
> +						 unsigned long size,
> +						 void **buffer)
>  {
>  	efi_status_t r;
> -	efi_physical_addr_t t;
>  
>  	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;
> +	r = efi_allocate_pool(pool_type, size, buffer);
>  	return EFI_EXIT(r);
>  }
>  
> @@ -736,7 +735,7 @@ static const struct efi_boot_services efi_boot_services = {
>  	.allocate_pages = efi_allocate_pages_ext,
>  	.free_pages = efi_free_pages_ext,
>  	.get_memory_map = efi_get_memory_map_ext,
> -	.allocate_pool = efi_allocate_pool,
> +	.allocate_pool = efi_allocate_pool_ext,
>  	.free_pool = efi_free_pool,
>  	.create_event = efi_create_event,
>  	.set_timer = efi_set_timer,
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index 5d71fdf..045558d 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -327,6 +327,17 @@ 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);

This is missing the *buffer update, no?

> +	return EFI_EXIT(r);

Double EFI_EXIT is fatal. Please just return r here.


Alex

> +}
> +
>  efi_status_t efi_get_memory_map(unsigned long *memory_map_size,
>  			       struct efi_mem_desc *memory_map,
>  			       unsigned long *map_key,
> 

[U-Boot] [PATCH v3 4/7] 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>
---
 include/efi_loader.h          |  2 ++
 lib/efi_loader/efi_boottime.c |  6 +++---
 lib/efi_loader/efi_memory.c   | 40 +++++++++++++++++++++++++++++++++++++++-
 3 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 40e7beb..341d4a4 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -122,6 +122,8 @@ efi_status_t efi_free_pages(uint64_t memory, unsigned long pages);
 /* EFI memory allocator for small allocations, called by EFI payloads */
 efi_status_t efi_allocate_pool(int pool_type, unsigned long size,
 			       void **buffer);
+/* EFI pool memory free function. */
+efi_status_t efi_free_pool(void *buffer);
 /* Returns the EFI memory map */
 efi_status_t efi_get_memory_map(unsigned long *memory_map_size,
 				struct efi_mem_desc *memory_map,
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index eb74cb0..8274d8e 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -141,12 +141,12 @@ static efi_status_t EFIAPI efi_allocate_pool_ext(int pool_type,
 	return EFI_EXIT(r);
 }
 
-static efi_status_t EFIAPI efi_free_pool(void *buffer)
+static efi_status_t EFIAPI efi_free_pool_ext(void *buffer)
 {
 	efi_status_t r;
 
 	EFI_ENTRY("%p", buffer);
-	r = efi_free_pages((ulong)buffer, 0);
+	r = efi_free_pool(buffer);
 	return EFI_EXIT(r);
 }
 
@@ -736,7 +736,7 @@ static const struct efi_boot_services efi_boot_services = {
 	.free_pages = efi_free_pages_ext,
 	.get_memory_map = efi_get_memory_map_ext,
 	.allocate_pool = efi_allocate_pool_ext,
-	.free_pool = efi_free_pool,
+	.free_pool = efi_free_pool_ext,
 	.create_event = efi_create_event,
 	.set_timer = efi_set_timer,
 	.wait_for_event = efi_wait_for_event,
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index 045558d..fa5c639 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -34,6 +34,18 @@ 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 alignement for pool allocations, so it is
+ * possible to reserve some headroom and serve the remainder.
+ */
+struct efi_pool_allocation {
+	u64 num_pages;
+	char data[];
+};
+
+/*
  * Sorts the memory list from highest address to lowest address
  *
  * When allocating memory we should always start from the highest
@@ -332,9 +344,35 @@ 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_EXIT(EFI_SUCCESS);
+	}
 
 	r = efi_allocate_pages(0, pool_type, num_pages, &t);
+
+	if (r == EFI_SUCCESS) {
+		struct efi_pool_allocation *alloc = (void *)(uintptr_t)t;
+		alloc->num_pages = num_pages;
+		*buffer = &(alloc->data);
+		assert(((uintptr_t)(*buffer) & 0x7) == 0);
+	}
+
+	return EFI_EXIT(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);
+	assert(((uintptr_t)alloc & EFI_PAGE_MASK) == 0);
+
+	r = efi_free_pages((uintptr_t)alloc, alloc->num_pages);
+
 	return EFI_EXIT(r);
 }
 
-- 
2.10.0

[U-Boot] [PATCH v3 4/7] efi_loader: Track size of pool allocations to allow freeing

[Alexander Graf]

On 01.10.16 23:32, 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.

The description doesn't say why you're doing what you're doing :).
Basically you want to say

  * We want to free
  * to free we need the size
  * to get the size we need to include it with the allocation
  * move allocations into their own struct for that
  * it's legal because allocations have to be 64bit aligned
  * fixes free

> 
> Signed-off-by: Stefan Br?ns <stefan.bruens@rwth-aachen.de>
> ---
>  include/efi_loader.h          |  2 ++
>  lib/efi_loader/efi_boottime.c |  6 +++---
>  lib/efi_loader/efi_memory.c   | 40 +++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 44 insertions(+), 4 deletions(-)
> 
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 40e7beb..341d4a4 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -122,6 +122,8 @@ efi_status_t efi_free_pages(uint64_t memory, unsigned long pages);
>  /* EFI memory allocator for small allocations, called by EFI payloads */
>  efi_status_t efi_allocate_pool(int pool_type, unsigned long size,
>  			       void **buffer);
> +/* EFI pool memory free function. */
> +efi_status_t efi_free_pool(void *buffer);
>  /* Returns the EFI memory map */
>  efi_status_t efi_get_memory_map(unsigned long *memory_map_size,
>  				struct efi_mem_desc *memory_map,
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index eb74cb0..8274d8e 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -141,12 +141,12 @@ static efi_status_t EFIAPI efi_allocate_pool_ext(int pool_type,
>  	return EFI_EXIT(r);
>  }
>  
> -static efi_status_t EFIAPI efi_free_pool(void *buffer)
> +static efi_status_t EFIAPI efi_free_pool_ext(void *buffer)
>  {
>  	efi_status_t r;
>  
>  	EFI_ENTRY("%p", buffer);
> -	r = efi_free_pages((ulong)buffer, 0);
> +	r = efi_free_pool(buffer);
>  	return EFI_EXIT(r);
>  }
>  
> @@ -736,7 +736,7 @@ static const struct efi_boot_services efi_boot_services = {
>  	.free_pages = efi_free_pages_ext,
>  	.get_memory_map = efi_get_memory_map_ext,
>  	.allocate_pool = efi_allocate_pool_ext,
> -	.free_pool = efi_free_pool,
> +	.free_pool = efi_free_pool_ext,
>  	.create_event = efi_create_event,
>  	.set_timer = efi_set_timer,
>  	.wait_for_event = efi_wait_for_event,
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index 045558d..fa5c639 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -34,6 +34,18 @@ 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 alignement for pool allocations, so it is

alignment

> + * possible to reserve some headroom and serve the remainder.

Any way you could write this in a way that makes it more obvious for
future readers what's going on? Maybe something like

EFI requires 8 byte alignment for pool allocations, so we can extend
every allocation by an internal 64bit variable that holds the size of
the allocation. That way we know how much to free later on.

> + */
> +struct efi_pool_allocation {
> +	u64 num_pages;
> +	char data[];
> +};
> +
> +/*
>   * Sorts the memory list from highest address to lowest address
>   *
>   * When allocating memory we should always start from the highest
> @@ -332,9 +344,35 @@ 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_EXIT(EFI_SUCCESS);

Please don't call EFI_EXIT() inside functions that are not also calling
EFI_ENTRY.

> +	}
>  
>  	r = efi_allocate_pages(0, pool_type, num_pages, &t);
> +
> +	if (r == EFI_SUCCESS) {
> +		struct efi_pool_allocation *alloc = (void *)(uintptr_t)t;
> +		alloc->num_pages = num_pages;
> +		*buffer = &(alloc->data);

This should work without the &, as you're referring to an array (which
is a pointer).

> +		assert(((uintptr_t)(*buffer) & 0x7) == 0);

This should be a given if t is 64bit aligned, as the compiler takes care
of the alignment for you here.

> +	}
> +
> +	return EFI_EXIT(r);

See above

> +}
> +
> +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);

The assert below wants a comment here :)

> +	assert(((uintptr_t)alloc & EFI_PAGE_MASK) == 0);
> +
> +	r = efi_free_pages((uintptr_t)alloc, alloc->num_pages);
> +
>  	return EFI_EXIT(r);

See above.


Alex

>  }
>  
> 

[U-Boot] [PATCH v3 5/7] 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>
---
 include/efi_loader.h        |  2 +-
 lib/efi_loader/efi_memory.c | 11 +++++++++--
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 341d4a4..6d64f4b 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -117,7 +117,7 @@ void *efi_alloc(uint64_t len, int memory_type);
 /* More specific EFI memory allocator, called by EFI payloads */
 efi_status_t efi_allocate_pages(int type, int memory_type, unsigned long pages,
 				uint64_t *memory);
-/* EFI memory free function. Not implemented today */
+/* EFI memory free function. */
 efi_status_t efi_free_pages(uint64_t memory, unsigned long pages);
 /* EFI memory allocator for small allocations, called by EFI payloads */
 efi_status_t efi_allocate_pool(int pool_type, unsigned long size,
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index fa5c639..7051948 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -335,8 +335,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,
-- 
2.10.0

[U-Boot] [U-Boot, v3, 5/7] efi_loader: Readd freed pages to memory pool

[Alexander Graf]

> 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>

Thanks, applied to 

[U-Boot] [PATCH v3 6/7] 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 7051948..2bbf8eb 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -134,7 +134,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] [U-Boot, v3, 6/7] efi_loader: Keep memory mapping sorted when splitting an entry

[Alexander Graf]

> 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>

Thanks, applied to 

[U-Boot] [PATCH v3 7/7] 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 2bbf8eb..7e4ee01 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -115,10 +115,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] [U-Boot, v3, 7/7] efi_loader: Do not leak memory when unlinking a mapping

[Alexander Graf]

> 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>

Thanks, applied to