Patchwork [U-Boot,v2] bootm: Avoid 256-byte overflow in fixup_silent_linux()

login
register
mail settings
Submitter Doug Anderson
Date Jan. 11, 2012, 6:19 p.m.
Message ID <1326305992-27939-1-git-send-email-dianders@chromium.org>
Download mbox | patch
Permalink /patch/135460/
State Superseded
Headers show

Comments

Doug Anderson - Jan. 11, 2012, 6:19 p.m.
This makes fixup_silent_linux() use malloc() to allocate its
working space, meaning that our maximum kernel command line
should only be limited by malloc().  Previously it was silently
overflowing the stack.

Note that nothing about this change increases the kernel's maximum
command line length.  If you have a command line that is >256
bytes it's up to you to make sure that kernel can handle it.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
Changes in v2:
- Tried to trim down to just the minimum changes needed with
no extra helper code.

 common/cmd_bootm.c |   38 ++++++++++++++++++++++++++++----------
 1 files changed, 28 insertions(+), 10 deletions(-)
Mike Frysinger - Jan. 15, 2012, 1:32 a.m.
On Wednesday 11 January 2012 13:19:52 Doug Anderson wrote:
> +	if (cmdline && (cmdline[0] != '\0')) {
> +		char *start = strstr(cmdline, CONSOLE_ARG);
> +
>  		if (start) {
> -			end = strchr(start, ' ');
> -			strncpy(buf, cmdline, (start - cmdline + 8));
> +			char *end = strchr(start, ' ');
> +			int num_start_bytes = start - cmdline + CONSOLE_ARG_LEN;
> +
> +			strncpy(buf, cmdline, num_start_bytes);
>  			if (end)
> -				strcpy(buf + (start - cmdline + 8), end);
> +				strcpy(buf + num_start_bytes, end);
>  			else
> -				buf[start - cmdline + 8] = '\0';
> +				buf[num_start_bytes] = '\0';
>  		} else {
> -			strcpy(buf, cmdline);
> -			strcat(buf, " console=");
> +			sprintf(buf, "%s %s", cmdline, CONSOLE_ARG);
>  		}
>  	} else {
> -		strcpy(buf, "console=");
> +		buf = strdup(CONSOLE_ARG);
> +		if (!buf) {
> +			debug("%s: strdup failed\n", __func__);
> +			return;
> +		}
>  	}
> 
>  	setenv("bootargs", buf);
>  	debug("after silent fix-up: %s\n", buf);
> +	free(buf);

seems like the strdup() in the else branch is unnecessary.
const char *env_val;
...
if (cmdline && (cmdline[0] != '\0')) {
	...
	env_val = buf;
} else {
	buf = NULL;
	env_val = "console=";
}

setenv("bootargs", env_val);
debug("after silent fix-up: %s\n", env_val);
free(buf);
-mike

Patch

diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
index d5745b1..9a0c08d 100644
--- a/common/cmd_bootm.c
+++ b/common/cmd_bootm.c
@@ -1229,9 +1229,13 @@  U_BOOT_CMD(
 /* helper routines */
 /*******************************************************************/
 #ifdef CONFIG_SILENT_CONSOLE
+
+#define CONSOLE_ARG     "console="
+#define CONSOLE_ARG_LEN (sizeof(CONSOLE_ARG) - 1)
+
 static void fixup_silent_linux(void)
 {
-	char buf[256], *start, *end;
+	char *buf;
 	char *cmdline = getenv("bootargs");
 
 	/* Only fix cmdline when requested */
@@ -1239,25 +1243,39 @@  static void fixup_silent_linux(void)
 		return;
 
 	debug("before silent fix-up: %s\n", cmdline);
-	if (cmdline) {
-		start = strstr(cmdline, "console=");
+	if (cmdline && (cmdline[0] != '\0')) {
+		char *start = strstr(cmdline, CONSOLE_ARG);
+
+		/* Allocate space for maximum possible new command line */
+		buf = malloc(strlen(cmdline) + 1 + CONSOLE_ARG_LEN + 1);
+		if (!buf) {
+			debug("%s: out of memory\n", __func__);
+			return;
+		}
+
 		if (start) {
-			end = strchr(start, ' ');
-			strncpy(buf, cmdline, (start - cmdline + 8));
+			char *end = strchr(start, ' ');
+			int num_start_bytes = start - cmdline + CONSOLE_ARG_LEN;
+
+			strncpy(buf, cmdline, num_start_bytes);
 			if (end)
-				strcpy(buf + (start - cmdline + 8), end);
+				strcpy(buf + num_start_bytes, end);
 			else
-				buf[start - cmdline + 8] = '\0';
+				buf[num_start_bytes] = '\0';
 		} else {
-			strcpy(buf, cmdline);
-			strcat(buf, " console=");
+			sprintf(buf, "%s %s", cmdline, CONSOLE_ARG);
 		}
 	} else {
-		strcpy(buf, "console=");
+		buf = strdup(CONSOLE_ARG);
+		if (!buf) {
+			debug("%s: strdup failed\n", __func__);
+			return;
+		}
 	}
 
 	setenv("bootargs", buf);
 	debug("after silent fix-up: %s\n", buf);
+	free(buf);
 }
 #endif /* CONFIG_SILENT_CONSOLE */