From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Goldschmidt Date: Sat, 4 May 2019 20:16:38 +0200 Subject: [U-Boot] [PATCH v5 2/2] dlmalloc: fix malloc range at end of ram In-Reply-To: References: <20190425192240.5925-1-simon.k.r.goldschmidt@gmail.com> <20190425192240.5925-2-simon.k.r.goldschmidt@gmail.com> <5fe137d2-4a8e-82a3-58a9-a85e5b520f08@gmail.com> <1bb0e151-274c-8fc1-a1fa-f5844300b228@gmail.com> Message-ID: <71a5ea6e-a001-1f1a-cab7-af503d62f086@gmail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Tom, Am 26.04.2019 um 13:00 schrieb Marek Vasut: > On 4/26/19 12:19 PM, Simon Goldschmidt wrote: >> On Fri, Apr 26, 2019 at 11:56 AM Marek Vasut wrote: >>> >>> On 4/26/19 11:36 AM, Simon Goldschmidt wrote: >>>> On Fri, Apr 26, 2019 at 11:32 AM Marek Vasut wrote: >>>>> >>>>> On 4/26/19 8:19 AM, Simon Goldschmidt wrote: >>>>>> Marek Vasut schrieb am Fr., 26. Apr. 2019, 00:22: >>>>>> >>>>>>> On 4/25/19 9:22 PM, Simon Goldschmidt wrote: >>>>>>>> If the malloc range passed to mem_malloc_init() is at the end of address >>>>>>>> range and 'start + size' overflows to 0, following allocations fail as >>>>>>>> mem_malloc_end is zero (which looks like uninitialized). >>>>>>>> >>>>>>>> Fix this by subtracting 1 of 'start + size' overflows to zero. >>>>>>>> >>>>>>>> Signed-off-by: Simon Goldschmidt Since there's no way this fits without breaking smartweb, I'd rather drop this for now in order to get 1/2 accepted. Regards, Simon >>>>>>>> --- >>>>>>>> >>>>>>>> Changes in v5: >>>>>>>> - this patch was 1/2 in v4 but is now 2/2 as the 2nd patch of v4 has >>>>>>>> already been accepted >>>>>>>> - rearrange the code to make it only 8 bytes plus in code size for arm >>>>>>>> (which fixes smartweb SPL overflowing) >>>>>>>> >>>>>>>> common/dlmalloc.c | 6 +++++- >>>>>>>> 1 file changed, 5 insertions(+), 1 deletion(-) >>>>>>>> >>>>>>>> diff --git a/common/dlmalloc.c b/common/dlmalloc.c >>>>>>>> index 6f12a18d54..38859ecbd4 100644 >>>>>>>> --- a/common/dlmalloc.c >>>>>>>> +++ b/common/dlmalloc.c >>>>>>>> @@ -601,8 +601,12 @@ void *sbrk(ptrdiff_t increment) >>>>>>>> void mem_malloc_init(ulong start, ulong size) >>>>>>>> { >>>>>>>> mem_malloc_start = start; >>>>>>>> - mem_malloc_end = start + size; >>>>>>>> mem_malloc_brk = start; >>>>>>>> + mem_malloc_end = start + size; >>>>>>>> + if (size > mem_malloc_end) { >>>>>>>> + /* overflow: malloc area is at end of address range */ >>>>>>>> + mem_malloc_end--; >>>>>>> >>>>>>> Does this mean a memory wrap-around happened ? >>>>>>> I don't think decrementing malloc area size by 1 is a proper solution. >>>>>>> You can have it overflow by 2 and decrementing by 1 won't help. >>>>>>> >>>>>> >>>>>> No, not a real overflow. Instead, as I tried to described in the commit >>>>>> message, mem_malloc_end gets 0 if the range is at the end of addr range, >>>>>> e.g. malloc start is 0xffff0000 and malloc size is 0x10000. Subtracting 1 >>>>>> will be enough here. It reduces the available mall of aize, but I don't >>>>>> think that should be a problem. >>>>> >>>>> That's a wrap-around . What happens with your example if malloc_size is >>>>> 0x10001 ? Hint: It fails ... >>>> >>>> Yes it fails. But in contrast, that's an invalid configuration, while >>>> my patch makes >>>> a valid configuration work. I don't know if we want to fix all invalid >>>> configurations. >>> >>> Yes ? Should be easy, just clamp() size to (size, (BIT(32) - 1) - >>> mem_malloc_start) or similar for 64bit systems. >> >> I'm not convinced we should. This range is normally generated using >> something like: >> SIZE=2048 >> START=RAM_END - SIZE > > Normally ... on SoCFPGA . Other ARM32 platforms can have OCRAM mapped > somewhere in the middle of the address space. Take R-Car Gen2, which has > it at 0xe6300000 + 64k or something like that. > > And, to make things worse, you cannot detect these overflows at compile > time, since the DRAM layout, which is passed to malloc init can come > from DT. > > Thus, you might want to sanitize the input, properly. > >> I don't want to be overprotective here. I don't think there's much point >> in fixing the out-of-ram-range check if it produces an overflow but not >> fix it if it's in the middle of an address space. >> >> Again, this patch simply fixes the case for something like this: >> RAM_SIZE=0x10000 >> RAM_START=0xFFFF0000 >> so RAM_END=0 >> >> We can use clamp as you suggested, but what would it be good for >> if it only fixes an out-of-range heap if an overflow occurs? > > It's better than nothing. Further refinements welcome. > >>>> You could as well enter a range without RAM, that would fail as well. >>> >>> That info is available in gd , but I wonder whether this is the right >>> place to check for it. >> >> Indeed, that would seem misplaced here. >> >> Regards, >> Simon >> >>> >>>> A different approach to fix my valid end-of-ram configuration would be to set >>>> the end to "start + size - 1" and to change all the checks using it. But that >>>> would probably lead to more code size problems in various SPL... >>>> >>>> Regards, >>>> Simon >>>> >>>>> >>>>>> I got this when experimenting with full heap in socfpga. Due to other >>>>>> patches not being accepted, this is not an issue currebtly, but can easily >>>>>> become one on the future. >>>>>> >>>>>> Regrds, >>>>>> Simon >>>>>> >>>>>> >>>>>>>> + } >>>>>>>> >>>>>>>> debug("using memory %#lx-%#lx for malloc()\n", mem_malloc_start, >>>>>>>> mem_malloc_end); >>>>>>>> >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> Best regards, >>>>>>> Marek Vasut >>>>>>> >>>>>> >>>>> >>>>> >>>>> -- >>>>> Best regards, >>>>> Marek Vasut >>> >>> >>> -- >>> Best regards, >>> Marek Vasut > >