[U-Boot] [PATCH] env: fix potential stack overflow in environment functions
From: Rob Herring rob.herring@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@calxeda.com --- common/env_dataflash.c | 15 +++++++-------- common/env_eeprom.c | 13 +++++++------ common/env_fat.c | 11 ++++++----- common/env_mmc.c | 13 +++++++------ common/env_nand.c | 23 ++++++++++++----------- common/env_nvram.c | 26 ++++++++++++++++---------- common/env_onenand.c | 13 +++++++------ common/env_sf.c | 23 ++++++++++++----------- 8 files changed, 74 insertions(+), 63 deletions(-)
diff --git a/common/env_dataflash.c b/common/env_dataflash.c index 38c9615..0591b99 100644 --- a/common/env_dataflash.c +++ b/common/env_dataflash.c @@ -30,6 +30,7 @@ DECLARE_GLOBAL_DATA_PTR; env_t *env_ptr;
char *env_name_spec = "dataflash"; +static char env_buf[CONFIG_ENV_SIZE];
uchar env_get_char_spec(int index) { @@ -42,11 +43,9 @@ uchar env_get_char_spec(int index)
void env_relocate_spec(void) { - char buf[CONFIG_ENV_SIZE]; + read_dataflash(CONFIG_ENV_ADDR, CONFIG_ENV_SIZE, env_buf);
- read_dataflash(CONFIG_ENV_ADDR, CONFIG_ENV_SIZE, buf); - - env_import(buf, 1); + env_import(env_buf, 1); }
#ifdef CONFIG_ENV_OFFSET_REDUND @@ -55,20 +54,20 @@ void env_relocate_spec(void)
int saveenv(void) { - env_t env_new; + env_t *env_new = (env_t *)env_buf; ssize_t len; char *res;
- res = (char *)&env_new.data; + res = (char *)env_new->data; len = hexport_r(&env_htab, '\0', 0, &res, ENV_SIZE, 0, NULL); if (len < 0) { error("Cannot export environment: errno = %d\n", errno); return 1; } - env_new.crc = crc32(0, env_new.data, ENV_SIZE); + env_new->crc = crc32(0, env_new->data, ENV_SIZE);
return write_dataflash(CONFIG_ENV_ADDR, - (unsigned long)&env_new, + (unsigned long)env_new, CONFIG_ENV_SIZE); }
diff --git a/common/env_eeprom.c b/common/env_eeprom.c index 45c935b..b136f04 100644 --- a/common/env_eeprom.c +++ b/common/env_eeprom.c @@ -38,6 +38,7 @@ DECLARE_GLOBAL_DATA_PTR;
env_t *env_ptr; +static char env_buf[CONFIG_ENV_SIZE];
char *env_name_spec = "EEPROM"; int env_eeprom_bus = -1; @@ -111,7 +112,7 @@ uchar env_get_char_spec(int index)
void env_relocate_spec(void) { - char buf[CONFIG_ENV_SIZE]; + char *buf = env_buf; unsigned int off = CONFIG_ENV_OFFSET;
#ifdef CONFIG_ENV_OFFSET_REDUND @@ -126,7 +127,7 @@ void env_relocate_spec(void)
int saveenv(void) { - env_t env_new; + env_t *env_new = (env_t *)env_buf; ssize_t len; char *res; int rc; @@ -138,13 +139,13 @@ int saveenv(void)
BUG_ON(env_ptr != NULL);
- res = (char *)&env_new.data; + res = (char *)env_new->data; len = hexport_r(&env_htab, '\0', 0, &res, ENV_SIZE, 0, NULL); if (len < 0) { error("Cannot export environment: errno = %d\n", errno); return 1; } - env_new.crc = crc32(0, env_new.data, ENV_SIZE); + env_new->crc = crc32(0, env_new->data, ENV_SIZE);
#ifdef CONFIG_ENV_OFFSET_REDUND if (gd->env_valid == 1) { @@ -152,11 +153,11 @@ int saveenv(void) off_red = CONFIG_ENV_OFFSET; }
- env_new.flags = ACTIVE_FLAG; + env_new->flags = ACTIVE_FLAG; #endif
rc = eeprom_bus_write(CONFIG_SYS_DEF_EEPROM_ADDR, - off, (uchar *)&env_new, CONFIG_ENV_SIZE); + off, (uchar *)env_new, CONFIG_ENV_SIZE);
#ifdef CONFIG_ENV_OFFSET_REDUND if (rc == 0) { diff --git a/common/env_fat.c b/common/env_fat.c index c0f18ab..dd7139d 100644 --- a/common/env_fat.c +++ b/common/env_fat.c @@ -37,6 +37,7 @@ char *env_name_spec = "FAT";
env_t *env_ptr; +static char env_buf[CONFIG_ENV_SIZE];
DECLARE_GLOBAL_DATA_PTR;
@@ -52,7 +53,7 @@ int env_init(void) #ifdef CONFIG_CMD_SAVEENV int saveenv(void) { - env_t env_new; + env_t *env_new = env_buf; ssize_t len; char *res; block_dev_desc_t *dev_desc = NULL; @@ -60,7 +61,7 @@ int saveenv(void) int part = FAT_ENV_PART; int err;
- res = (char *)&env_new.data; + res = (char *)env_new->data; len = hexport_r(&env_htab, '\0', 0, &res, ENV_SIZE, 0, NULL); if (len < 0) { error("Cannot export environment: errno = %d\n", errno); @@ -95,8 +96,8 @@ int saveenv(void) return 1; }
- env_new.crc = crc32(0, env_new.data, ENV_SIZE); - err = file_fat_write(FAT_ENV_FILE, (void *)&env_new, sizeof(env_t)); + env_new->crc = crc32(0, env_new->data, ENV_SIZE); + err = file_fat_write(FAT_ENV_FILE, (void *)env_new, sizeof(env_t)); if (err == -1) { printf("\n** Unable to write "%s" from %s%d:%d **\n", FAT_ENV_FILE, FAT_ENV_INTERFACE, dev, part); @@ -110,7 +111,7 @@ int saveenv(void)
void env_relocate_spec(void) { - char buf[CONFIG_ENV_SIZE]; + char *buf = env_buf; block_dev_desc_t *dev_desc = NULL; int dev = FAT_ENV_DEVICE; int part = FAT_ENV_PART; diff --git a/common/env_mmc.c b/common/env_mmc.c index 02bd5ae..f568013 100644 --- a/common/env_mmc.c +++ b/common/env_mmc.c @@ -40,6 +40,8 @@ env_t *env_ptr = &environment; env_t *env_ptr; #endif /* ENV_IS_EMBEDDED */
+DEFINE_CACHE_ALIGN_BUFFER(char, env_buf, CONFIG_ENV_SIZE); + DECLARE_GLOBAL_DATA_PTR;
#if !defined(CONFIG_ENV_OFFSET) @@ -112,7 +114,7 @@ static inline int write_env(struct mmc *mmc, unsigned long size,
int saveenv(void) { - ALLOC_CACHE_ALIGN_BUFFER(env_t, env_new, 1); + env_t *env_new = (env_t *)env_buf; ssize_t len; char *res; struct mmc *mmc = find_mmc_device(CONFIG_SYS_MMC_ENV_DEV); @@ -127,7 +129,7 @@ int saveenv(void) goto fini; }
- res = (char *)&env_new->data; + res = (char *)env_new->data; len = hexport_r(&env_htab, '\0', 0, &res, ENV_SIZE, 0, NULL); if (len < 0) { error("Cannot export environment: errno = %d\n", errno); @@ -135,7 +137,7 @@ int saveenv(void) goto fini; }
- env_new->crc = crc32(0, &env_new->data[0], ENV_SIZE); + env_new->crc = crc32(0, env_new->data, ENV_SIZE); printf("Writing to MMC(%d)... ", CONFIG_SYS_MMC_ENV_DEV); if (write_env(mmc, CONFIG_ENV_SIZE, offset, (u_char *)env_new)) { puts("failed\n"); @@ -169,7 +171,6 @@ static inline int read_env(struct mmc *mmc, unsigned long size, void env_relocate_spec(void) { #if !defined(ENV_IS_EMBEDDED) - ALLOC_CACHE_ALIGN_BUFFER(char, buf, CONFIG_ENV_SIZE); struct mmc *mmc = find_mmc_device(CONFIG_SYS_MMC_ENV_DEV); u32 offset; int ret; @@ -184,12 +185,12 @@ void env_relocate_spec(void) goto fini; }
- if (read_env(mmc, CONFIG_ENV_SIZE, offset, buf)) { + if (read_env(mmc, CONFIG_ENV_SIZE, offset, env_buf)) { ret = 1; goto fini; }
- env_import(buf, 1); + env_import(env_buf, 1); ret = 0;
fini: diff --git a/common/env_nand.c b/common/env_nand.c index 5b69889..8cc2055 100644 --- a/common/env_nand.c +++ b/common/env_nand.c @@ -64,6 +64,8 @@ env_t *env_ptr = (env_t *)CONFIG_NAND_ENV_DST; env_t *env_ptr; #endif /* ENV_IS_EMBEDDED */
+DEFINE_CACHE_ALIGN_BUFFER(char, env_buf, CONFIG_ENV_SIZE); + DECLARE_GLOBAL_DATA_PTR;
/* @@ -173,7 +175,7 @@ static unsigned char env_flags;
int saveenv(void) { - env_t env_new; + env_t *env_new = (env_t *)env_buf; ssize_t len; char *res; int ret = 0; @@ -185,14 +187,14 @@ int saveenv(void) if (CONFIG_ENV_RANGE < CONFIG_ENV_SIZE) return 1;
- res = (char *)&env_new.data; + res = (char *)env_new->data; len = hexport_r(&env_htab, '\0', 0, &res, ENV_SIZE, 0, NULL); if (len < 0) { error("Cannot export environment: errno = %d\n", errno); return 1; } - env_new.crc = crc32(0, env_new.data, ENV_SIZE); - env_new.flags = ++env_flags; /* increase the serial */ + env_new->crc = crc32(0, env_new->data, ENV_SIZE); + env_new->flags = ++env_flags; /* increase the serial */
if (gd->env_valid == 1) { puts("Erasing redundant NAND...\n"); @@ -201,7 +203,7 @@ int saveenv(void) return 1;
puts("Writing to redundant NAND... "); - ret = writeenv(CONFIG_ENV_OFFSET_REDUND, (u_char *)&env_new); + ret = writeenv(CONFIG_ENV_OFFSET_REDUND, (u_char *)env_new); } else { puts("Erasing NAND...\n"); nand_erase_options.offset = CONFIG_ENV_OFFSET; @@ -209,7 +211,7 @@ int saveenv(void) return 1;
puts("Writing to NAND... "); - ret = writeenv(CONFIG_ENV_OFFSET, (u_char *)&env_new); + ret = writeenv(CONFIG_ENV_OFFSET, (u_char *)env_new); } if (ret) { puts("FAILED!\n"); @@ -226,7 +228,7 @@ int saveenv(void) int saveenv(void) { int ret = 0; - ALLOC_CACHE_ALIGN_BUFFER(env_t, env_new, 1); + env_t *env_new = (env_t *)env_buf; ssize_t len; char *res; nand_erase_options_t nand_erase_options; @@ -238,7 +240,7 @@ int saveenv(void) if (CONFIG_ENV_RANGE < CONFIG_ENV_SIZE) return 1;
- res = (char *)&env_new->data; + res = (char *)env_new->data; len = hexport_r(&env_htab, '\0', 0, &res, ENV_SIZE, 0, NULL); if (len < 0) { error("Cannot export environment: errno = %d\n", errno); @@ -404,7 +406,6 @@ void env_relocate_spec(void) { #if !defined(ENV_IS_EMBEDDED) int ret; - ALLOC_CACHE_ALIGN_BUFFER(char, buf, CONFIG_ENV_SIZE);
#if defined(CONFIG_ENV_OFFSET_OOB) ret = get_nand_env_oob(&nand_info[0], &nand_env_oob_offset); @@ -420,13 +421,13 @@ void env_relocate_spec(void) } #endif
- ret = readenv(CONFIG_ENV_OFFSET, (u_char *)buf); + ret = readenv(CONFIG_ENV_OFFSET, (u_char *)env_buf); if (ret) { set_default_env("!readenv() failed"); return; }
- env_import(buf, 1); + env_import(env_buf, 1); #endif /* ! ENV_IS_EMBEDDED */ } #endif /* CONFIG_ENV_OFFSET_REDUND */ diff --git a/common/env_nvram.c b/common/env_nvram.c index eab0e7b..ff74a6c 100644 --- a/common/env_nvram.c +++ b/common/env_nvram.c @@ -60,6 +60,10 @@ env_t *env_ptr = (env_t *)CONFIG_ENV_ADDR; char *env_name_spec = "NVRAM";
#ifdef CONFIG_SYS_NVRAM_ACCESS_ROUTINE +static char env_buf[CONFIG_ENV_SIZE]; +#endif + +#ifdef CONFIG_SYS_NVRAM_ACCESS_ROUTINE uchar env_get_char_spec(int index) { uchar c; @@ -72,36 +76,38 @@ uchar env_get_char_spec(int index)
void env_relocate_spec(void) { - char buf[CONFIG_ENV_SIZE]; + char *buf;
#if defined(CONFIG_SYS_NVRAM_ACCESS_ROUTINE) + buf = env_buf; nvram_read(buf, CONFIG_ENV_ADDR, CONFIG_ENV_SIZE); #else - memcpy(buf, (void *)CONFIG_ENV_ADDR, CONFIG_ENV_SIZE); + buf = (void *)CONFIG_ENV_ADDR; #endif env_import(buf, 1); }
int saveenv(void) { - env_t env_new; +#ifdef CONFIG_SYS_NVRAM_ACCESS_ROUTINE + env_t *env_new = (env_t *)env_buf; +#else + env_t *env_new = (env_t *)CONFIG_ENV_ADDR; +#endif ssize_t len; char *res; int rcode = 0;
- res = (char *)&env_new.data; + res = (char *)env_new->data; len = hexport_r(&env_htab, '\0', 0, &res, ENV_SIZE, 0, NULL); if (len < 0) { error("Cannot export environment: errno = %d\n", errno); return 1; } - env_new.crc = crc32(0, env_new.data, ENV_SIZE); + env_new->crc = crc32(0, env_new->data, ENV_SIZE);
#ifdef CONFIG_SYS_NVRAM_ACCESS_ROUTINE - nvram_write(CONFIG_ENV_ADDR, &env_new, CONFIG_ENV_SIZE); -#else - if (memcpy((char *)CONFIG_ENV_ADDR, &env_new, CONFIG_ENV_SIZE) == NULL) - rcode = 1; + nvram_write(CONFIG_ENV_ADDR, env_new, CONFIG_ENV_SIZE); #endif return rcode; } @@ -115,7 +121,7 @@ int env_init(void) { #if defined(CONFIG_SYS_NVRAM_ACCESS_ROUTINE) ulong crc; - uchar data[ENV_SIZE]; + uchar *data = env_buf;
nvram_read(&crc, CONFIG_ENV_ADDR, sizeof(ulong)); nvram_read(data, CONFIG_ENV_ADDR + sizeof(ulong), ENV_SIZE); diff --git a/common/env_onenand.c b/common/env_onenand.c index faa903d..6fd5613 100644 --- a/common/env_onenand.c +++ b/common/env_onenand.c @@ -42,6 +42,8 @@ char *env_name_spec = "OneNAND"; #define ONENAND_MAX_ENV_SIZE CONFIG_ENV_SIZE #define ONENAND_ENV_SIZE(mtd) (ONENAND_MAX_ENV_SIZE - ENV_HEADER_SIZE)
+static char env_buf[CONFIG_ENV_SIZE]; + DECLARE_GLOBAL_DATA_PTR;
void env_relocate_spec(void) @@ -56,8 +58,7 @@ void env_relocate_spec(void) char *buf = (char *)&environment; #else loff_t env_addr = CONFIG_ENV_ADDR; - char onenand_env[ONENAND_MAX_ENV_SIZE]; - char *buf = (char *)&onenand_env[0]; + char *buf = env_buf; #endif /* ENV_IS_EMBEDDED */
#ifndef ENV_IS_EMBEDDED @@ -81,7 +82,7 @@ void env_relocate_spec(void)
int saveenv(void) { - env_t env_new; + env_t *env_new = env_buf; ssize_t len; char *res; struct mtd_info *mtd = &onenand_mtd; @@ -94,13 +95,13 @@ int saveenv(void) .callback = NULL, };
- res = (char *)&env_new.data; + res = (char *)env_new->data; len = hexport_r(&env_htab, '\0', 0, &res, ENV_SIZE, 0, NULL); if (len < 0) { error("Cannot export environment: errno = %d\n", errno); return 1; } - env_new.crc = crc32(0, env_new.data, ENV_SIZE); + env_new->crc = crc32(0, env_new->data, ENV_SIZE);
instr.len = CONFIG_ENV_SIZE; #ifdef CONFIG_ENV_ADDR_FLEX @@ -119,7 +120,7 @@ int saveenv(void) }
if (mtd->write(mtd, env_addr, ONENAND_MAX_ENV_SIZE, &retlen, - (u_char *)&env_new)) { + (u_char *)env_new)) { printf("OneNAND: write failed at 0x%llx\n", instr.addr); return 2; } diff --git a/common/env_sf.c b/common/env_sf.c index d9e9085..9a592ba 100644 --- a/common/env_sf.c +++ b/common/env_sf.c @@ -58,11 +58,12 @@ DECLARE_GLOBAL_DATA_PTR; char *env_name_spec = "SPI Flash";
static struct spi_flash *env_flash; +static char env_buf[CONFIG_ENV_SIZE];
#if defined(CONFIG_ENV_OFFSET_REDUND) int saveenv(void) { - env_t env_new; + env_t *env_new = (env_t *)env_buf; ssize_t len; char *res, *saved_buffer = NULL, flag = OBSOLETE_FLAG; u32 saved_size, saved_offset, sector = 1; @@ -78,14 +79,14 @@ int saveenv(void) } }
- res = (char *)&env_new.data; + res = (char *)env_new->data; len = hexport_r(&env_htab, '\0', 0, &res, ENV_SIZE, 0, NULL); if (len < 0) { error("Cannot export environment: errno = %d\n", errno); return 1; } - env_new.crc = crc32(0, env_new.data, ENV_SIZE); - env_new.flags = ACTIVE_FLAG; + env_new->crc = crc32(0, env_new->data, ENV_SIZE); + env_new->flags = ACTIVE_FLAG;
if (gd->env_valid == 1) { env_new_offset = CONFIG_ENV_OFFSET_REDUND; @@ -125,7 +126,7 @@ int saveenv(void) puts("Writing to SPI flash...");
ret = spi_flash_write(env_flash, env_new_offset, - CONFIG_ENV_SIZE, &env_new); + CONFIG_ENV_SIZE, env_new); if (ret) goto done;
@@ -137,7 +138,7 @@ int saveenv(void) }
ret = spi_flash_write(env_flash, env_offset + offsetof(env_t, flags), - sizeof(env_new.flags), &flag); + sizeof(env_new->flags), &flag); if (ret) goto done;
@@ -243,7 +244,7 @@ int saveenv(void) u32 saved_size, saved_offset, sector = 1; char *res, *saved_buffer = NULL; int ret = 1; - env_t env_new; + env_t *env_new = (env_t *)env_buf; ssize_t len;
if (!env_flash) { @@ -276,13 +277,13 @@ int saveenv(void) sector++; }
- res = (char *)&env_new.data; + res = (char *)env_new->data; len = hexport_r(&env_htab, '\0', 0, &res, ENV_SIZE, 0, NULL); if (len < 0) { error("Cannot export environment: errno = %d\n", errno); goto done; } - env_new.crc = crc32(0, env_new.data, ENV_SIZE); + env_new->crc = crc32(0, env_new->data, ENV_SIZE);
puts("Erasing SPI flash..."); ret = spi_flash_erase(env_flash, CONFIG_ENV_OFFSET, @@ -292,7 +293,7 @@ int saveenv(void)
puts("Writing to SPI flash..."); ret = spi_flash_write(env_flash, CONFIG_ENV_OFFSET, - CONFIG_ENV_SIZE, &env_new); + CONFIG_ENV_SIZE, env_new); if (ret) goto done;
@@ -315,7 +316,7 @@ int saveenv(void)
void env_relocate_spec(void) { - char buf[CONFIG_ENV_SIZE]; + char *buf = env_buf; int ret;
env_flash = spi_flash_probe(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS,
Dear Rob Herring,
In message 1363987581-28050-1-git-send-email-robherring2@gmail.com you wrote:
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.
Could you please explain what exactly you mean with this "have 4KB stacks" statement?
Also, why do you think there is more space for data then for stack?
Has this patch been tested on any actual hardware?
I would expect problems, as this code is running before relocation, i. e. when the data segment is not writable?
Best regards,
Wolfgang Denk
On 03/22/2013 05:04 PM, Wolfgang Denk wrote:
Dear Rob Herring,
In message 1363987581-28050-1-git-send-email-robherring2@gmail.com you wrote:
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.
Could you please explain what exactly you mean with this "have 4KB stacks" statement?
Well, that's what ARM and PPC have for their stack size. This is not the first stack overflow I've fixed. The last was caused by having a large printf buffer size:
commit fcfa696b3a354ab1e16601683c61f671487aded7 Author: Rob Herring rob.herring@calxeda.com Date: Thu Jun 28 03:54:11 2012 +0000
ARM: increase lmb stack space reservation to 4KB
The bootm initrd image copy to ram can collide with the stack in cases where the print buffer size is large (i.e. 1K). The result is intermittent initrd decompression errors depending on the initrd size MOD 4KB since the initrd start address is 4KB aligned.
Cc: Albert ARIBAUD albert.u.boot@aribaud.net Signed-off-by: Rob Herring rob.herring@calxeda.com
Also, why do you think there is more space for data then for stack?
Because in "normal" systems that is the case. It is reasonable to expect that a 1MB allocation would be possible for static data or malloc, but not for a stack. I could have just increased the stack size, but that is a fragile solution.
Has this patch been tested on any actual hardware?
Yes. This fixes a problem on actual h/w (highbank). The problem was introduced with only an env change.
I would expect problems, as this code is running before relocation, i. e. when the data segment is not writable?
env_relocate is called by board_init_r at least on ARM which is after relocation, right?
Rob
Dear Rob Herring,
In message 514CE1A0.7080907@gmail.com you wrote:
Could you please explain what exactly you mean with this "have 4KB stacks" statement?
Well, that's what ARM and PPC have for their stack size. This is not the first stack overflow I've fixed. The last was caused by having a large printf buffer size:
Please be more precise with what you are talking about...
Normally, the stack is just limited by the amount of available RAM, i. e. we usually habe tens of megabytes (or more) of stack space.
commit fcfa696b3a354ab1e16601683c61f671487aded7 Author: Rob Herring rob.herring@calxeda.com Date: Thu Jun 28 03:54:11 2012 +0000
ARM: increase lmb stack space reservation to 4KB
What has LMB to do with this, then?
Also, why do you think there is more space for data then for stack?
Because in "normal" systems that is the case. It is reasonable to expect
Please define (exactly!) what you consider "normal".
that a 1MB allocation would be possible for static data or malloc, but not for a stack. I could have just increased the stack size, but that is a fragile solution.
Usually, on "normal" systems, we have tens or evern hundreds of megabytes of stack space available.
Has this patch been tested on any actual hardware?
Yes. This fixes a problem on actual h/w (highbank). The problem was introduced with only an env change.
I would expect problems, as this code is running before relocation, i. e. when the data segment is not writable?
env_relocate is called by board_init_r at least on ARM which is after relocation, right?
This makes no sense to me. After relocation you have basically unlimited stack space.
Best regards,
Wolfgang Denk
On Fri, Mar 22, 2013 at 11:26:21AM -0000, Rob Herring wrote:
From: Rob Herring rob.herring@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@calxeda.com
Applied to u-boot/master, thanks!
Dear Tom Rini,
In message 20130403153014.GF7035@bill-the-cat you wrote:
On Fri, Mar 22, 2013 at 11:26:21AM -0000, Rob Herring wrote:
From: Rob Herring rob.herring@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@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.
I hereby request to revert that commit.
Best regards,
Wolfgang Denk
On 04/05/2013 06:17 AM, Wolfgang Denk wrote:
Dear Tom Rini,
In message 20130403153014.GF7035@bill-the-cat you wrote:
On Fri, Mar 22, 2013 at 11:26:21AM -0000, Rob Herring wrote:
From: Rob Herring rob.herring@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@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
Dear Rob Herring,
In message 515EED36.9090305@gmail.com you wrote:
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
This looks to be conceptually broken to me. You cannot just lmb_reserve() arbitrary amounts of memory, when the documented, pubished memory map clearly states that this memory is "free", and in use for the downward growing stack. If you need memory, you must reserve it in a clearly documented way.
A part of the problem appears to be that it's actually very difficult to even understand how the mnemory concept of LMB has been designed - it it was designed at all. Is there any related documentation?
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.
No, it should not. It is supposed to keep the very formatting chosen by the implementor.
The core of the problem is the illegal, and totally undocumented assumptions LMB appears to be making.
_This_ is what needs to be fixed.
Best regards,
Wolfgang Denk
Dear Rob Herring,
In message 515EED36.9090305@gmail.com you wrote:
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
BTW - the ARM code is simply broken - see "arch/arm/lib/bootm.c":
74 lmb_reserve(lmb, sp, 75 gd->bd->bi_dram[0].start + gd->bd->bi_dram[0].size - sp);
What if we have more than one memory bank? Then the computation above is pretty much random...
Best regards,
Wolfgang Denk
Dear Tom, dear Albert,
In message 20130405111710.8C04C200589@gemini.denx.de I wrote:
I hereby request to revert that commit.
In addition to commit 60d7d5a "env: fix potential stack overflow in environment functions" discussed here, I think we should also revert commit fcfa696 "ARM: increase lmb stack space reservation to 4KB" because it is conceptually broken and just papers over the real problems.
Best regards,
Wolfgang Denk
On 04/05/2013 11:24 AM, Wolfgang Denk wrote:
Dear Tom, dear Albert,
In message 20130405111710.8C04C200589@gemini.denx.de I wrote:
I hereby request to revert that commit.
In addition to commit 60d7d5a "env: fix potential stack overflow in environment functions" discussed here, I think we should also revert commit fcfa696 "ARM: increase lmb stack space reservation to 4KB" because it is conceptually broken and just papers over the real problems.
Doing so will randomly break any system with a large command or print buffer. For extra fun, it is dependent on the initrd or dtb image size in terms of remainder of 4KB multiple.
It is exactly the same code as PPC. It you look at the git history, PPC made exactly the same change (1 to 4KB increase) around the same time all the FDT boot code got copied from PPC to ARM. So ARM missed this change.
If the stack is all of RAM, then what address should the initrd and dtb be copied to?
Rob
Dear Rob,
In message 515EFE6F.1020609@gmail.com you wrote:
In addition to commit 60d7d5a "env: fix potential stack overflow in environment functions" discussed here, I think we should also revert commit fcfa696 "ARM: increase lmb stack space reservation to 4KB" because it is conceptually broken and just papers over the real problems.
Doing so will randomly break any system with a large command or print buffer. For extra fun, it is dependent on the initrd or dtb image size in terms of remainder of 4KB multiple.
Well, yes, but that's because the LMB code makes unjustified assumptions about the memory usage, so it needs to be fixed there.
It is exactly the same code as PPC. It you look at the git history, PPC made exactly the same change (1 to 4KB increase) around the same time all the FDT boot code got copied from PPC to ARM. So ARM missed this change.
Thanks for pointing out. This adds commit 3882d7a "ppc: unused memory region too close to current stack pointer" to the list of patches that should bne reverted.
If the stack is all of RAM, then what address should the initrd and dtb be copied to?
Why do they have to be copied at all? Why cannot they remain where they have been loaded in the firtst place? The memcpy just costs time, which is a precious resource. Leave it to the user to find a reasonable location in RAM where he loads the data, and don't mess with it.
Best regards,
Wolfgang Denk
On 04/05/2013 12:13 PM, Wolfgang Denk wrote:
Dear Rob,
In message 515EFE6F.1020609@gmail.com you wrote:
In addition to commit 60d7d5a "env: fix potential stack overflow in environment functions" discussed here, I think we should also revert commit fcfa696 "ARM: increase lmb stack space reservation to 4KB" because it is conceptually broken and just papers over the real problems.
Doing so will randomly break any system with a large command or print buffer. For extra fun, it is dependent on the initrd or dtb image size in terms of remainder of 4KB multiple.
Well, yes, but that's because the LMB code makes unjustified assumptions about the memory usage, so it needs to be fixed there.
It is exactly the same code as PPC. It you look at the git history, PPC made exactly the same change (1 to 4KB increase) around the same time all the FDT boot code got copied from PPC to ARM. So ARM missed this change.
Thanks for pointing out. This adds commit 3882d7a "ppc: unused memory region too close to current stack pointer" to the list of patches that should bne reverted.
If the stack is all of RAM, then what address should the initrd and dtb be copied to?
Why do they have to be copied at all? Why cannot they remain where they have been loaded in the firtst place? The memcpy just costs time, which is a precious resource. Leave it to the user to find a reasonable location in RAM where he loads the data, and don't mess with it.
I've got no freaking idea! I do turn that crap off in my environment with initrd_high=0xffffffff. But the default operation is to copy it.
Rob
Dear Rob,
In message 515F1504.4090705@gmail.com you wrote:
If the stack is all of RAM, then what address should the initrd and dtb be copied to?
Why do they have to be copied at all? Why cannot they remain where they have been loaded in the firtst place? The memcpy just costs time, which is a precious resource. Leave it to the user to find a reasonable location in RAM where he loads the data, and don't mess with it.
I've got no freaking idea! I do turn that crap off in my environment with initrd_high=0xffffffff. But the default operation is to copy it.
Scott, Andy: I think I remember that some architectures really _need_ LMB - can you please shed a bit ligh on which these are, and why? And why it is enabled everywhere?
Also, any information about the underlying design, intended memory map etc. would be highly welcome.
Best regards,
Wolfgang Denk
On 04/05/2013 01:47:12 PM, Wolfgang Denk wrote:
Dear Rob,
In message 515F1504.4090705@gmail.com you wrote:
If the stack is all of RAM, then what address should the initrd
and dtb
be copied to?
Why do they have to be copied at all? Why cannot they remain
where
they have been loaded in the firtst place? The memcpy just costs
time,
which is a precious resource. Leave it to the user to find a reasonable location in RAM where he loads the data, and don't mess with it.
I've got no freaking idea! I do turn that crap off in my environment with initrd_high=0xffffffff. But the default operation is to copy
it.
Scott, Andy: I think I remember that some architectures really _need_ LMB - can you please shed a bit ligh on which these are, and why? And why it is enabled everywhere?
Also, any information about the underlying design, intended memory map etc. would be highly welcome.
CCing Kumar, who added a lot of the lmb stuff -- but it looks like ramdisk copying predated lmb.
-Scott
Dear Scott,
In message 1365203857.17535.16@snotra you wrote:
Scott, Andy: I think I remember that some architectures really _need_ LMB - can you please shed a bit ligh on which these are, and why? And why it is enabled everywhere?
Also, any information about the underlying design, intended memory map etc. would be highly welcome.
CCing Kumar, who added a lot of the lmb stuff -- but it looks like ramdisk copying predated lmb.
Indeed it does. It even predates U-Boot. The question is what the actual design of LMB is, i. e. why it is implemented the way it is.
Best regards,
Wolfgang Denk
On Fri, Apr 05, 2013 at 01:16:36PM -0500, Rob Herring wrote:
On 04/05/2013 12:13 PM, Wolfgang Denk wrote:
Dear Rob,
In message 515EFE6F.1020609@gmail.com you wrote:
In addition to commit 60d7d5a "env: fix potential stack overflow in environment functions" discussed here, I think we should also revert commit fcfa696 "ARM: increase lmb stack space reservation to 4KB" because it is conceptually broken and just papers over the real problems.
Doing so will randomly break any system with a large command or print buffer. For extra fun, it is dependent on the initrd or dtb image size in terms of remainder of 4KB multiple.
Well, yes, but that's because the LMB code makes unjustified assumptions about the memory usage, so it needs to be fixed there.
It is exactly the same code as PPC. It you look at the git history, PPC made exactly the same change (1 to 4KB increase) around the same time all the FDT boot code got copied from PPC to ARM. So ARM missed this change.
Thanks for pointing out. This adds commit 3882d7a "ppc: unused memory region too close to current stack pointer" to the list of patches that should bne reverted.
If the stack is all of RAM, then what address should the initrd and dtb be copied to?
Why do they have to be copied at all? Why cannot they remain where they have been loaded in the firtst place? The memcpy just costs time, which is a precious resource. Leave it to the user to find a reasonable location in RAM where he loads the data, and don't mess with it.
I've got no freaking idea! I do turn that crap off in my environment with initrd_high=0xffffffff. But the default operation is to copy it.
There's also fdt_high=0xffffffff which a number of platforms do to keep from FDT being thrown into highmem and unavailable to Linux.
So, I shall revert the first commit in question today. For after v2013.04 we should: - Revert the other two commits Wolfgang found - See if there looks to be a real good reason for defaulting to relocating initrd/fdt, specifically up into the top of memory where we also know that U-Boot has / is alive and kicking.
participants (4)
-
Rob Herring -
Scott Wood -
Tom Rini -
Wolfgang Denk