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

[U-Boot] [PATCH 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 2/6] efi_loader: Fix memory map size check to avoid out-of-bounds access

[Stefan Brüns]

memory_map_size as IN parameter specifies the size of the provided buffer.
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 2/6] efi_loader: Fix memory map size check to avoid out-of-bounds access

[Alexander Graf]

On 30.09.16 02:03, Stefan Br?ns wrote:
> memory_map_size as IN parameter specifies the size of the provided buffer.
> 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>

This patch doesn't actually change anything, does it?


Alex

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

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

[Brüns, Stefan]

On Freitag, 30. September 2016 14:25:40 CEST Alexander Graf wrote:
> On 30.09.16 02:03, Stefan Br?ns wrote:
> > memory_map_size as IN parameter specifies the size of the provided buffer.
> > 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>
> 
> This patch doesn't actually change anything, does it?

It does ...

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

The caller provided buffer size was changed here

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

-> this check was always false

> > +	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) {

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

[Alexander Graf]

On 30.09.16 16:15, Br?ns, Stefan wrote:
> On Freitag, 30. September 2016 14:25:40 CEST Alexander Graf wrote:
>> On 30.09.16 02:03, Stefan Br?ns wrote:
>>> memory_map_size as IN parameter specifies the size of the provided buffer.
>>> 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>
>>
>> This patch doesn't actually change anything, does it?
> 
> It does ...
> 
>>
>> Alex
>>
>>> ---
>>>
>>>  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;
> 
> The caller provided buffer size was changed here
> 
>>> -
>>>
>>>  	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)
> 
> -> this check was always false

Ah, I see it now. Could you please adjust the commit message to point
that out properly? :)


Thanks!

Alex

[U-Boot] [PATCH 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 | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index be6f5e8..3e526eb 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -134,18 +134,35 @@ static efi_status_t EFIAPI efi_allocate_pool(int pool_type, unsigned long size,
 					     void **buffer)
 {
 	efi_status_t r;
+	u64 num_pages = (size + 8 + 0xfff) >> 12;
 
 	EFI_ENTRY("%d, %ld, %p", pool_type, size, buffer);
-	r = efi_allocate_pages(0, pool_type, (size + 0xfff) >> 12, (void*)buffer);
+
+	if (size == 0) {
+		*buffer = NULL;
+		return EFI_EXIT(EFI_SUCCESS);
+	}
+
+	r = efi_allocate_pages(0, pool_type, num_pages, (void *)buffer);
+	if (r == EFI_SUCCESS) {
+		*(u64 *)(*buffer) = num_pages;
+		*buffer = (char *)(*buffer) + 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;
+	num_pages = *(u64 *)buffer;
+
+	r = efi_free_pages((ulong)buffer, num_pages);
 	return EFI_EXIT(r);
 }
 
-- 
2.10.0

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

[Alexander Graf]

On 30.09.16 02:03, 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 | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index be6f5e8..3e526eb 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -134,18 +134,35 @@ static efi_status_t EFIAPI efi_allocate_pool(int pool_type, unsigned long size,
>  					     void **buffer)
>  {
>  	efi_status_t r;
> +	u64 num_pages = (size + 8 + 0xfff) >> 12;
>  
>  	EFI_ENTRY("%d, %ld, %p", pool_type, size, buffer);
> -	r = efi_allocate_pages(0, pool_type, (size + 0xfff) >> 12, (void*)buffer);

Please base on top of this patch to avoid conflicts:

  http://lists.denx.de/pipermail/u-boot/2016-September/266580.html

> +
> +	if (size == 0) {
> +		*buffer = NULL;
> +		return EFI_EXIT(EFI_SUCCESS);
> +	}
> +
> +	r = efi_allocate_pages(0, pool_type, num_pages, (void *)buffer);
> +	if (r == EFI_SUCCESS) {
> +		*(u64 *)(*buffer) = num_pages;
> +		*buffer = (char *)(*buffer) + 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;

Please add a sanity assert here that verifies that (ulong)buffer &
EFI_PAGE_MASK == 8.


Alex

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

[U-Boot] [PATCH 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>
---
 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 5d71fdf..763b79f 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 4/6] efi_loader: Readd freed pages to memory pool

[Alexander Graf]

On 30.09.16 02:03, Stefan Br?ns wrote:
> 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>

Alex

[U-Boot] [PATCH 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>
---
 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 763b79f..0fcfb7a 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 5/6] efi_loader: Keep memory mapping sorted when splitting an entry

[Alexander Graf]

On 30.09.16 02:03, Stefan Br?ns wrote:
> 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>

I wonder why I didn't do that. Hm. Oh well.

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

[U-Boot] [PATCH 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>
---
 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 0fcfb7a..6d8fae4 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 6/6] efi_loader: Do not leak memory when unlinking a mapping

[Alexander Graf]

On 30.09.16 02:04, Stefan Br?ns wrote:
> 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>


Alex