[U-Boot] memory corruption on nios2 due to overlap of gbl data and malloc

[Albert ARIBAUD]

Le 29/02/2012 00:20, Graeme Russ a écrit :
> Hi Albert,
>
> On Wed, Feb 29, 2012 at 9:55 AM, Albert ARIBAUD
> <albert.u.boot at aribaud.net>  wrote:
>> Hi Graeme,
>>
>> Le 28/02/2012 23:39, Graeme Russ a écrit :
>>
>>> Hi Albert,
>>>
>>> On Wed, Feb 29, 2012 at 9:29 AM, Albert ARIBAUD
>>> <albert.u.boot at aribaud.net>    wrote:
>>>>
>>>> Hi Alex,
>>>>
>>>> Le 21/02/2012 00:24, Alex Hornung a écrit :
>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>> I've run into some memory corruption due to an error in the logic used
>>>>> to allocate the bd (and gd) during board_init of the nios2.
>>>>>
>>>>>
>>>>> #define CONFIG_SYS_GBL_DATA_OFFSET      (CONFIG_SYS_MALLOC_BASE - \
>>>>>                                           GENERATED_GBL_DATA_SIZE)
>>>>> [...]
>>>>>
>>>>>          gd = (gd_t *)CONFIG_SYS_GBL_DATA_OFFSET;
>>>>> [...]
>>>>>          gd->bd = (bd_t *)(gd+1);        /* At end of global data */
>>>>> [...]
>>>>>          mem_malloc_init(CONFIG_SYS_MALLOC_BASE, CONFIG_SYS_MALLOC_LEN);
>>>>>
>>>>> The relevant points here are that CONFIG_SYS_GBL_DATA_OFFSET is
>>>>> GENERATED_GBL_DATA_SIZE (80) bytes below the CONFIG_SYS_MALLOC_BASE.
>>>>>
>>>>> Given that gd is 68 bytes big, now the start of bd is only 12 bytes from
>>>>> the beginning of the malloc base - but the size of bd is 36 bytes!
>>>>
>>>>
>>>>
>>>> So GENERATED_GBL_DATA_SIZE is wrong if it was supposed to contain both gd
>>>> and bd, which I suspect is not the case; but if it is supposed to only
>>>> contain a gd, then the definition of CONFIG_SYS_GBL_DATA_OFFSET is wrong
>>>> in
>>>> that it does not account for gd and bd as it should.
>>>
>>>
>>> The global data struct only contains a pointer to the board data struct.
>>>
>>> IMHO I think the approach taken (but almost all arches) is very errror
>>> prone
>>> as it relies on manually laying out gd and bd in memory with bd sitting
>>> immediately above or below gd. In theory, this layout should never be
>>> tampered with, but I still don't like it.
>>>
>>> For x86, gd and bd are in BSS after relocation, so there is no need to
>>> hack around them when calculating the heap or stack, but I have a sneaking
>>> suspicion that this could make debugging harder as there is no way to
>>> reliably find the relocation offset as gd is never located at a known
>>> location in memory...
>>
>>
>> Duh. I had misread the code... Time for me to go to sleep. :/
>>
>> For ARM we have gd in r8, which makes things simpler.
>
> Of course :) - x86 now has it in FS so it should be easy to find
>
>> Anyway -- this does not affect the fact that GENERATED_GBL_DATA_SIZE should
>> be equal to or greater than sizeof(gd_t)+sizeof(bd-t), right?
>
> No - GENERATED_GBL_DATA_SIZE should be sizeof(gd_t)
>
> The space reserved between U-Boot and the heap needs to be sizeof(gd_t) +
> sizeof(bd-t) (on the delicate proviso that only gd and bd live there, and
> that gd and bd are immediately next to each other)

Ok, so :

1. do you know why here gd = 68 bytes and GENERATED_GBL_DATA_SIZE is 80?

2. luckily for my ego, my proposal was actually correct when I suggested 
the following, right?

#define CONFIG_SYS_GBL_DATA_OFFSET      (CONFIG_SYS_MALLOC_BASE - \
                                             sizeof(bd_t) - \
                                             GENERATED_GBL_DATA_SIZE)

> Regards,
>
> Graeme

Amicalement,
-- 
Albert.