From mboxrd@z Thu Jan 1 00:00:00 1970 From: Albert ARIBAUD Date: Wed, 29 Feb 2012 00:24:15 +0100 Subject: [U-Boot] memory corruption on nios2 due to overlap of gbl data and malloc In-Reply-To: References: <4F42D61C.6080201@alexhornung.com> <4F4D5550.7010701@aribaud.net> <4F4D5B4F.2030806@aribaud.net> Message-ID: <4F4D621F.6000301@aribaud.net> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Le 29/02/2012 00:20, Graeme Russ a ?crit : > Hi Albert, > > On Wed, Feb 29, 2012 at 9:55 AM, Albert ARIBAUD > 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 >>> 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.