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

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

[Rob Herring]

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,
-- 
1.7.10.4

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

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
F u cn rd ths u cnt spl wrth a dm!

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

[Rob Herring]

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

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

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Nothing ever becomes real until it is experienced.       - John Keats

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

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130403/a3f8609c/attachment.pgp>

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

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
How long does it take a  DEC  field  service  engineer  to  change  a
lightbulb?       It depends on how many bad ones he brought with him.

[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@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
> 

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

[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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
ATTENTION: Despite Any Other Listing of Product Contents Found  Here-
on, the Consumer is Advised That, in Actuality, This Product Consists
Of 99.9999999999% Empty Space.

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

[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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
It is more rational to sacrifice one life than six.
	-- Spock, "The Galileo Seven", stardate 2822.3

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

[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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
They say a little knowledge is a dangerous thing,  but it is not  one
half so bad as a lot of ignorance.   - Terry Pratchett, _Equal Rites_

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

[Rob Herring]

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

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

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"We shall reach greater and greater platitudes of achievement."
- Richard J. Daley

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

[Rob Herring]

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

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

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
1 1 was a race-horse, 2 2 was 1 2. When 1 1 1 1 race, 2 2 1 1 2.

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

[Scott Wood]

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

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

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"I find this a nice feature but it is not according to  the  documen-
tation. Or is it a BUG?"   "Let's call it an accidental feature. :-)"
                       - Larry Wall in <6909@jpl-devvax.JPL.NASA.GOV>

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

[Tom Rini]

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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130405/881773c3/attachment.pgp>