[U-Boot] [RFC] efi_loader: Handle 32-bit memory overflows

[U-Boot] [RFC] efi_loader: Handle 32-bit memory overflows

[Andreas Färber]

jetson-tk1 has 2 GB of RAM at 0x80000000, causing gd->ram_top to be zero.
Handle this by replacing it with 0x100000000 in that case.

Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Andreas F?rber <afaerber@suse.de>
---
 lib/efi_loader/efi_memory.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index 138736f..7b87108 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -225,7 +225,7 @@ efi_status_t efi_allocate_pages(int type, int memory_type,
 	switch (type) {
 	case 0:
 		/* Any page */
-		addr = efi_find_free_memory(len, gd->ram_top);
+		addr = efi_find_free_memory(len, gd->ram_top == 0 ? 0x100000000ull : gd->ram_top);
 		if (!addr) {
 			r = EFI_NOT_FOUND;
 			break;
@@ -343,7 +343,7 @@ int efi_memory_init(void)
 
 	/* Add U-Boot */
 	uboot_start = (gd->start_addr_sp - uboot_stack_size) & ~EFI_PAGE_MASK;
-	uboot_pages = (gd->ram_top - uboot_start) >> EFI_PAGE_SHIFT;
+	uboot_pages = ((gd->ram_top == 0 ? 0x100000000ull : gd->ram_top) - uboot_start) >> EFI_PAGE_SHIFT;
 	efi_add_memory_map(uboot_start, uboot_pages, EFI_LOADER_DATA, false);
 
 	/* Add Runtime Services */
-- 
2.6.6

[U-Boot] [RFC] efi_loader: Handle 32-bit memory overflows

[Alexander Graf]

> Am 13.04.2016 um 05:24 schrieb Andreas F?rber <afaerber@suse.de>:
> 
> jetson-tk1 has 2 GB of RAM at 0x80000000, causing gd->ram_top to be zero.
> Handle this by replacing it with 0x100000000 in that case.

Nice catch!

> 
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Andreas F?rber <afaerber@suse.de>
> ---
> lib/efi_loader/efi_memory.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
> index 138736f..7b87108 100644
> --- a/lib/efi_loader/efi_memory.c
> +++ b/lib/efi_loader/efi_memory.c
> @@ -225,7 +225,7 @@ efi_status_t efi_allocate_pages(int type, int memory_type,
>    switch (type) {
>    case 0:
>        /* Any page */
> -        addr = efi_find_free_memory(len, gd->ram_top);
> +        addr = efi_find_free_memory(len, gd->ram_top == 0 ? 0x100000000ull : gd->ram_top);

Couldn't we just use gd->ram_top - 1? Then we underflow to 0xffffffff and everything should just work.

>        if (!addr) {
>            r = EFI_NOT_FOUND;
>            break;
> @@ -343,7 +343,7 @@ int efi_memory_init(void)
> 
>    /* Add U-Boot */
>    uboot_start = (gd->start_addr_sp - uboot_stack_size) & ~EFI_PAGE_MASK;
> -    uboot_pages = (gd->ram_top - uboot_start) >> EFI_PAGE_SHIFT;
> +    uboot_pages = ((gd->ram_top == 0 ? 0x100000000ull : gd->ram_top) - uboot_start) >> EFI_PAGE_SHIFT;

Are you sure this hunk is necessary? We should already underflow to the correct value here.

Alex

>    efi_add_memory_map(uboot_start, uboot_pages, EFI_LOADER_DATA, false);
> 
>    /* Add Runtime Services */
> -- 
> 2.6.6
> 

[U-Boot] [RFC] efi_loader: Handle 32-bit memory overflows

[Andreas Färber]

Am 13.04.2016 um 07:50 schrieb Alexander Graf:
>> Am 13.04.2016 um 05:24 schrieb Andreas F?rber <afaerber@suse.de>:
>>
>> jetson-tk1 has 2 GB of RAM at 0x80000000, causing gd->ram_top to be zero.
>> Handle this by replacing it with 0x100000000 in that case.
> 
> Nice catch!
> 
>>
>> Cc: Alexander Graf <agraf@suse.de>
>> Signed-off-by: Andreas F?rber <afaerber@suse.de>
>> ---
>> lib/efi_loader/efi_memory.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
>> index 138736f..7b87108 100644
>> --- a/lib/efi_loader/efi_memory.c
>> +++ b/lib/efi_loader/efi_memory.c
>> @@ -225,7 +225,7 @@ efi_status_t efi_allocate_pages(int type, int memory_type,
>>    switch (type) {
>>    case 0:
>>        /* Any page */
>> -        addr = efi_find_free_memory(len, gd->ram_top);
>> +        addr = efi_find_free_memory(len, gd->ram_top == 0 ? 0x100000000ull : gd->ram_top);

If we do use a constant, we should probably be using
UINT64_C(0x100000000) rather than ull for compatibility.

> Couldn't we just use gd->ram_top - 1? Then we underflow to 0xffffffff and everything should just work.

Hm, that does work in my testing. Is it guaranteed it will handle that
as unsigned long rather than uint64_t? And did you mean to always use it
that way or just in the zero case? I.e., might we be wasting one byte in
the non-zero case or is it guaranteed that the top of RAM is always
reserved?

I was wondering whether we might run into the same overflow problem on
aarch64, in which case my hunk would be wrong, but your -1 should work.

>>        if (!addr) {
>>            r = EFI_NOT_FOUND;
>>            break;
>> @@ -343,7 +343,7 @@ int efi_memory_init(void)
>>
>>    /* Add U-Boot */
>>    uboot_start = (gd->start_addr_sp - uboot_stack_size) & ~EFI_PAGE_MASK;
>> -    uboot_pages = (gd->ram_top - uboot_start) >> EFI_PAGE_SHIFT;
>> +    uboot_pages = ((gd->ram_top == 0 ? 0x100000000ull : gd->ram_top) - uboot_start) >> EFI_PAGE_SHIFT;
> 
> Are you sure this hunk is necessary? We should already underflow to the correct value here.

Positive that _something_ here is necessary for sanity, it was using a
huge number of pages (uboot_start is uint64_t).

If we use an odd number like -1, then we probably should add
EFI_PAGE_MASK as done for the RAM for the non-zero case, shouldn't we?
That might overflow on aarch64 though...

>>    efi_add_memory_map(uboot_start, uboot_pages, EFI_LOADER_DATA, false);
>>
>>    /* Add Runtime Services */
>> -- 
>> 2.6.6

Thanks,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 N?rnberg, Germany
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)

[U-Boot] [RFC] efi_loader: Handle 32-bit memory overflows

[Alexander Graf]

On 04/13/2016 01:26 PM, Andreas F?rber wrote:
> Am 13.04.2016 um 07:50 schrieb Alexander Graf:
>>> Am 13.04.2016 um 05:24 schrieb Andreas F?rber <afaerber@suse.de>:
>>>
>>> jetson-tk1 has 2 GB of RAM at 0x80000000, causing gd->ram_top to be zero.
>>> Handle this by replacing it with 0x100000000 in that case.
>> Nice catch!
>>
>>> Cc: Alexander Graf <agraf@suse.de>
>>> Signed-off-by: Andreas F?rber <afaerber@suse.de>
>>> ---
>>> lib/efi_loader/efi_memory.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
>>> index 138736f..7b87108 100644
>>> --- a/lib/efi_loader/efi_memory.c
>>> +++ b/lib/efi_loader/efi_memory.c
>>> @@ -225,7 +225,7 @@ efi_status_t efi_allocate_pages(int type, int memory_type,
>>>     switch (type) {
>>>     case 0:
>>>         /* Any page */
>>> -        addr = efi_find_free_memory(len, gd->ram_top);
>>> +        addr = efi_find_free_memory(len, gd->ram_top == 0 ? 0x100000000ull : gd->ram_top);
> If we do use a constant, we should probably be using
> UINT64_C(0x100000000) rather than ull for compatibility.
>
>> Couldn't we just use gd->ram_top - 1? Then we underflow to 0xffffffff and everything should just work.
> Hm, that does work in my testing. Is it guaranteed it will handle that
> as unsigned long rather than uint64_t? And did you mean to always use it
> that way or just in the zero case? I.e., might we be wasting one byte in
> the non-zero case or is it guaranteed that the top of RAM is always
> reserved?

Top of RAM is where U-Boot lives, so it's always reserved anyways :).

> I was wondering whether we might run into the same overflow problem on
> aarch64, in which case my hunk would be wrong, but your -1 should work.

I'm not aware of an aarch64 implementation (or even spec) that would 
allow for 64bit physical address space, so we're safe on aarch64. But I 
like the -1 better, since it just completely negates any overflow.

>
>>>         if (!addr) {
>>>             r = EFI_NOT_FOUND;
>>>             break;
>>> @@ -343,7 +343,7 @@ int efi_memory_init(void)
>>>
>>>     /* Add U-Boot */
>>>     uboot_start = (gd->start_addr_sp - uboot_stack_size) & ~EFI_PAGE_MASK;
>>> -    uboot_pages = (gd->ram_top - uboot_start) >> EFI_PAGE_SHIFT;
>>> +    uboot_pages = ((gd->ram_top == 0 ? 0x100000000ull : gd->ram_top) - uboot_start) >> EFI_PAGE_SHIFT;
>> Are you sure this hunk is necessary? We should already underflow to the correct value here.
> Positive that _something_ here is necessary for sanity, it was using a
> huge number of pages (uboot_start is uint64_t).

Ok, so the fix would be to change uboot_start to unsigned long to have 
the same type as gd->ram_top.

>
> If we use an odd number like -1, then we probably should add
> EFI_PAGE_MASK as done for the RAM for the non-zero case, shouldn't we?
> That might overflow on aarch64 though...

Since we shift afterwards we don't need to mask anything. Bits that we 
would mask away are already gone after the shift ;).

Does the patch below work?


Alex

diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c
index df99585..71a3d19 100644
--- a/lib/efi_loader/efi_memory.c
+++ b/lib/efi_loader/efi_memory.c
@@ -220,7 +220,7 @@ efi_status_t efi_allocate_pages(int type, int 
memory_type,
         switch (type) {
         case 0:
                 /* Any page */
-               addr = efi_find_free_memory(len, gd->ram_top);
+               addr = efi_find_free_memory(len, gd->start_addr_sp);
                 if (!addr) {
                         r = EFI_NOT_FOUND;
                         break;
@@ -320,9 +320,9 @@ efi_status_t efi_get_memory_map(unsigned long 
*memory_map_size,

  int efi_memory_init(void)
  {
-       uint64_t runtime_start, runtime_end, runtime_pages;
-       uint64_t uboot_start, uboot_pages;
-       uint64_t uboot_stack_size = 16 * 1024 * 1024;
+       unsigned long runtime_start, runtime_end, runtime_pages;
+       unsigned long uboot_start, uboot_pages;
+       unsigned long uboot_stack_size = 16 * 1024 * 1024;
         int i;

         /* Add RAM */