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