[U-Boot] [PATCH] malloc: handle free() before gd is set

[U-Boot] [PATCH] malloc: handle free() before gd is set

[Stephen Warren]

On at least Ubuntu Xenial, free() can be called before main(). In this
case, U-Boot won't have set gd, so dereferencing it will crash. Check
whether gd is set before using it.

While at it, apply the same fix to other functions.

Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
---
 common/dlmalloc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/common/dlmalloc.c b/common/dlmalloc.c
index 5ea37dfb6e4c..7453e63d6bf4 100644
--- a/common/dlmalloc.c
+++ b/common/dlmalloc.c
@@ -2453,7 +2453,7 @@ void fREe(mem) Void_t* mem;
 
 #ifdef CONFIG_SYS_MALLOC_F_LEN
 	/* free() is a no-op - all the memory will be freed on relocation */
-	if (!(gd->flags & GD_FLG_FULL_MALLOC_INIT))
+	if (gd && !(gd->flags & GD_FLG_FULL_MALLOC_INIT))
 		return;
 #endif
 
@@ -2609,7 +2609,7 @@ Void_t* rEALLOc(oldmem, bytes) Void_t* oldmem; size_t bytes;
   if (oldmem == NULL) return mALLOc(bytes);
 
 #ifdef CONFIG_SYS_MALLOC_F_LEN
-	if (!(gd->flags & GD_FLG_FULL_MALLOC_INIT)) {
+	if (gd && !(gd->flags & GD_FLG_FULL_MALLOC_INIT)) {
 		/* This is harder to support and should not be needed */
 		panic("pre-reloc realloc() is not supported");
 	}
@@ -2985,7 +2985,7 @@ Void_t* cALLOc(n, elem_size) size_t n; size_t elem_size;
   else
   {
 #ifdef CONFIG_SYS_MALLOC_F_LEN
-	if (!(gd->flags & GD_FLG_FULL_MALLOC_INIT)) {
+	if (gd && !(gd->flags & GD_FLG_FULL_MALLOC_INIT)) {
 		MALLOC_ZERO(mem, sz);
 		return mem;
 	}
-- 
2.7.0

[U-Boot] [PATCH] malloc: handle free() before gd is set

[Hans de Goede]

Hi,

On 04-03-16 09:19, Stephen Warren wrote:
> On at least Ubuntu Xenial, free() can be called before main(). In this
> case, U-Boot won't have set gd, so dereferencing it will crash. Check
> whether gd is set before using it.
>
> While at it, apply the same fix to other functions.
>
> Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
> ---
>   common/dlmalloc.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/common/dlmalloc.c b/common/dlmalloc.c
> index 5ea37dfb6e4c..7453e63d6bf4 100644
> --- a/common/dlmalloc.c
> +++ b/common/dlmalloc.c
> @@ -2453,7 +2453,7 @@ void fREe(mem) Void_t* mem;
>
>   #ifdef CONFIG_SYS_MALLOC_F_LEN
>   	/* free() is a no-op - all the memory will be freed on relocation */
> -	if (!(gd->flags & GD_FLG_FULL_MALLOC_INIT))
> +	if (gd && !(gd->flags & GD_FLG_FULL_MALLOC_INIT))
>   		return;
>   #endif
>

I believe you want:

+	if (!gd || !(gd->flags & GD_FLG_FULL_MALLOC_INIT))

Instead, so that you actually go into the return; path when there is no gd.

Regards,

Hans



> @@ -2609,7 +2609,7 @@ Void_t* rEALLOc(oldmem, bytes) Void_t* oldmem; size_t bytes;
>     if (oldmem == NULL) return mALLOc(bytes);
>
>   #ifdef CONFIG_SYS_MALLOC_F_LEN
> -	if (!(gd->flags & GD_FLG_FULL_MALLOC_INIT)) {
> +	if (gd && !(gd->flags & GD_FLG_FULL_MALLOC_INIT)) {
>   		/* This is harder to support and should not be needed */
>   		panic("pre-reloc realloc() is not supported");
>   	}
> @@ -2985,7 +2985,7 @@ Void_t* cALLOc(n, elem_size) size_t n; size_t elem_size;
>     else
>     {
>   #ifdef CONFIG_SYS_MALLOC_F_LEN
> -	if (!(gd->flags & GD_FLG_FULL_MALLOC_INIT)) {
> +	if (gd && !(gd->flags & GD_FLG_FULL_MALLOC_INIT)) {
>   		MALLOC_ZERO(mem, sz);
>   		return mem;
>   	}
>

[U-Boot] [PATCH] malloc: handle free() before gd is set

[Stephen Warren]

On 03/04/2016 01:45 AM, Hans de Goede wrote:
> Hi,
>
> On 04-03-16 09:19, Stephen Warren wrote:
>> On at least Ubuntu Xenial, free() can be called before main(). In this
>> case, U-Boot won't have set gd, so dereferencing it will crash. Check
>> whether gd is set before using it.
>>
>> While at it, apply the same fix to other functions.
>>
>> Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
>> ---
>>   common/dlmalloc.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/common/dlmalloc.c b/common/dlmalloc.c
>> index 5ea37dfb6e4c..7453e63d6bf4 100644
>> --- a/common/dlmalloc.c
>> +++ b/common/dlmalloc.c
>> @@ -2453,7 +2453,7 @@ void fREe(mem) Void_t* mem;
>>
>>   #ifdef CONFIG_SYS_MALLOC_F_LEN
>>       /* free() is a no-op - all the memory will be freed on
>> relocation */
>> -    if (!(gd->flags & GD_FLG_FULL_MALLOC_INIT))
>> +    if (gd && !(gd->flags & GD_FLG_FULL_MALLOC_INIT))
>>           return;
>>   #endif
>>
>
> I believe you want:
>
> +    if (!gd || !(gd->flags & GD_FLG_FULL_MALLOC_INIT))
>
> Instead, so that you actually go into the return; path when there is no gd.

Hmm. Is the existing logic at the start of malloc() (which I copied) 
incorrect too then? Perhaps so...

#ifdef CONFIG_SYS_MALLOC_F_LEN
   if (gd && !(gd->flags & GD_FLG_FULL_MALLOC_INIT))
     return malloc_simple(bytes);
#endif

   /* check if mem_malloc_init() was run */
   if ((mem_malloc_start == 0) && (mem_malloc_end == 0)) {
     /* not initialized yet */
     return NULL;
   }

I guess that works because "if (gd && ..." prevents gd from being 
dereferenced, but doesn't actually return, and then presumably 
"(mem_malloc_start == 0) && (mem_malloc_end == 0)" is true at that 
point, so the function returns NULL immediately anyway.

For free() after my change:

#ifdef CONFIG_SYS_MALLOC_F_LEN
   /* free() is a no-op - all the memory will be freed on relocation */
   if (!(gd->flags & GD_FLG_FULL_MALLOC_INIT))
     return;
#endif

   if (mem == NULL) /* free(0) has no effect */
     return;

I guess that "mem == NULL" is always true, since malloc() always 
returned NULL, so everything works out somewhat accidentally in a 
similar way. Still, as you say it's probably better to be a bit more 
direct and add an explicit guard in malloc on gd leaving it:

+ if (!gd)
+   return NULL;
   #ifdef CONFIG_SYS_MALLOC_F_LEN
-   if (gd && !(gd->flags & GD_FLG_FULL_MALLOC_INIT))
+   if (!(gd->flags & GD_FLG_FULL_MALLOC_INIT)
       return malloc_simple(bytes);
   #endif

and free:

+ if (!gd)
+   return;

Or perhaps actually using malloc_simple() if (!gd) is the better option, 
since obviously something[1] is actually trying to allocate memory?

[1] IIRC something in the dynamic loader, but I forget the complete 
backtrace right now.

[U-Boot] [PATCH] malloc: handle free() before gd is set

[Hans de Goede]

Hi,

On 04-03-16 18:38, Stephen Warren wrote:
> On 03/04/2016 01:45 AM, Hans de Goede wrote:
>> Hi,
>>
>> On 04-03-16 09:19, Stephen Warren wrote:
>>> On at least Ubuntu Xenial, free() can be called before main(). In this
>>> case, U-Boot won't have set gd, so dereferencing it will crash. Check
>>> whether gd is set before using it.
>>>
>>> While at it, apply the same fix to other functions.
>>>
>>> Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
>>> ---
>>>   common/dlmalloc.c | 6 +++---
>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/common/dlmalloc.c b/common/dlmalloc.c
>>> index 5ea37dfb6e4c..7453e63d6bf4 100644
>>> --- a/common/dlmalloc.c
>>> +++ b/common/dlmalloc.c
>>> @@ -2453,7 +2453,7 @@ void fREe(mem) Void_t* mem;
>>>
>>>   #ifdef CONFIG_SYS_MALLOC_F_LEN
>>>       /* free() is a no-op - all the memory will be freed on
>>> relocation */
>>> -    if (!(gd->flags & GD_FLG_FULL_MALLOC_INIT))
>>> +    if (gd && !(gd->flags & GD_FLG_FULL_MALLOC_INIT))
>>>           return;
>>>   #endif
>>>
>>
>> I believe you want:
>>
>> +    if (!gd || !(gd->flags & GD_FLG_FULL_MALLOC_INIT))
>>
>> Instead, so that you actually go into the return; path when there is no gd.
>
> Hmm. Is the existing logic at the start of malloc() (which I copied) incorrect too then? Perhaps so...
>
> #ifdef CONFIG_SYS_MALLOC_F_LEN
>    if (gd && !(gd->flags & GD_FLG_FULL_MALLOC_INIT))
>      return malloc_simple(bytes);
> #endif
>
>    /* check if mem_malloc_init() was run */
>    if ((mem_malloc_start == 0) && (mem_malloc_end == 0)) {
>      /* not initialized yet */
>      return NULL;
>    }
>
> I guess that works because "if (gd && ..." prevents gd from being dereferenced, but doesn't actually return, and then presumably "(mem_malloc_start == 0) && (mem_malloc_end == 0)" is true at that point, so the function returns NULL immediately anyway.

You're right, since simple_malloc depends on gd being set
we should not call it when gd is not set, so the above code
is correct.

> For free() after my change:
>
> #ifdef CONFIG_SYS_MALLOC_F_LEN
>    /* free() is a no-op - all the memory will be freed on relocation */
>    if (!(gd->flags & GD_FLG_FULL_MALLOC_INIT))
>      return;
> #endif
>
>    if (mem == NULL) /* free(0) has no effect */
>      return;
>
> I guess that "mem == NULL" is always true, since malloc() always returned NULL, so everything works out somewhat accidentally in a similar way. Still, as you say it's probably better to be a bit more direct and add an explicit guard in malloc on gd leaving it:
>
> + if (!gd)
> +   return NULL;
>    #ifdef CONFIG_SYS_MALLOC_F_LEN
> -   if (gd && !(gd->flags & GD_FLG_FULL_MALLOC_INIT))
> +   if (!(gd->flags & GD_FLG_FULL_MALLOC_INIT)
>        return malloc_simple(bytes);
>    #endif
>
> and free:
>
> + if (!gd)
> +   return;
>

I was thinking along the same lines, except that I wonder if
the non-simple malloc may work without gd, or in other words
if there are platforms which call mem_malloc_init() before
setting the gd because they need it early?

> Or perhaps actually using malloc_simple() if (!gd) is the better option, since obviously something[1] is actually trying to allocate memory?

malloc_simple depends on gd being set unlike the dlmalloc code itself,
which depends on mem_malloc_init() being called.

So in hindsight I believe that your original patch is correct,
since the malloc() simple_malloc check is correct, and we should
mirror it free(), and then indeed trust that if we get past this
check because gd == NULL, free is being called with a NULL ptr.

> [1] IIRC something in the dynamic loader, but I forget the complete backtrace right now.

It could be that it has some cleanup-code which also gets called
on init which unconditionally does: free(foo), even if foo was never
set, since free(NULL) is a nop. And your patch makes it a nop again
even when building with malloc_simple and gd == NULL.

But if there is a matching malloc which gets called before gd gets
set then indeed there is something fishy here.

Regards,

Hans