[U-Boot] env: fix potential stack overflow in environment functions

[Rob Herring]

On 04/05/2013 06:17 AM, Wolfgang Denk wrote:
> Dear Tom Rini,
> 
> In message <20130403153014.GF7035 at bill-the-cat> you wrote:
>>
>> On Fri, Mar 22, 2013 at 11:26:21AM -0000, Rob Herring wrote:
>>
>>> From: Rob Herring <rob.herring at calxeda.com>
>>>
>>> Most of the various environment functions create CONFIG_ENV_SIZE buffers on
>>> the stack. At least on ARM and PPC which have 4KB stacks, this can overflow
>>> the stack if we have large environment sizes. So move all the buffers off
>>> the stack to static buffers.
>>>
>>> Signed-off-by: Rob Herring <rob.herring at calxeda.com>
>>
>> Applied to u-boot/master, thanks!
> 
> I'm unhappy about this.
> 
> The patch makes no sense to me, and in addition it causes build
> warnings for some boards (PPC: debris, kvme080):
> 
> env_nvram.c: In function 'env_init':
> env_nvram.c:124:16: warning: pointer targets in initialization differ
> in signedness [-Wpointer-sign]
> 
> 
> I tried to explain this before, but eventually you missed my remarks,
> so here they go again:
> 
> * The functiuons we are talking about run after relocation, i. e. when
>   we have a full standard C runtime environment.  In this situation we
>   can assume to have virtually unlimited stack space available -
>   actually limited only by the RAM size.
> 
> * Moving the buffers from the stack into BSS is bad, as this way we
>   permanently lose the space for these buffers, nut we need them only
>   for a very short time, so we are wasting lots of memory.
> 
> * If some board have for some reasons unreasonable small stack sizes,
>   these need to be fixed.   Rob refers to LMB stack space in his
>   previous message - if there are indeed such small stack sizes used
>   there, this is a problem that needs to be fixed elsewhere, but NOT
>   by adapting all the rest of the U-Boot code to an artifical small
>   stack.

The stack size limit only comes into play when bootm runs and starts
moving initrd and dtb to high addresses below the stack. At that point,
the stack size does become limited because only 4KB (recently increase
from 1KB) of space is reserved. So I had this in mind when I started
debugging my environment getting corrupted and saw large buffers on the
stack. My problem was ultimately that blank lines in mvenvimage input
files are not handled correctly giving intermittent issues with the env
import. I'm still not clear why the issue was intermittent, but I think
mkenvimage should skip/remove blank lines. I still need to make a fix
for that.

> I hereby request to revert that commit.

That's fine with me.

Rob

> 
> Best regards,
> 
> Wolfgang Denk
>