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

[Graeme Russ]

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)

Regards,

Graeme