| Message ID | 1319133298-30249-1-git-send-email-dianders@chromium.org |
|---|---|
| State | Changes Requested |
| Headers | show |
Dear Doug Anderson, In message <1319133298-30249-1-git-send-email-dianders@chromium.org> you wrote: > 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. ... > static void fixup_silent_linux(void) > { > - char buf[256], *start, *end; Are you sure that the kernel's buffer is long enough? For example on PowerPC, there is a current hard limit on 512 characters: arch/powerpc/boot/ops.h:#define COMMAND_LINE_SIZE 512 arch/powerpc/kernel/setup-common.c:char cmd_line[COMMAND_LINE_SIZE]; On SPARC, we have 256 bytes hard limit, see arch/sparc/prom/bootstr_64.c: #define BARG_LEN 256 ... prom_getstring(prom_chosen_node, "bootargs", bootstr_info.bootstr_buf, BARG_LEN); And so on for other architectures, for example: arch/score/include/asm/setup.h:#define COMMAND_LINE_SIZE 256 arch/m68k/include/asm/setup.h:#define COMMAND_LINE_SIZE 256 arch/avr32/include/asm/setup.h:#define COMMAND_LINE_SIZE 256 arch/microblaze/include/asm/setup.h:#define COMMAND_LINE_SIZE 256 arch/mn10300/include/asm/param.h:#define COMMAND_LINE_SIZE 256 arch/sparc/include/asm/setup.h:# define COMMAND_LINE_SIZE 256 arch/cris/include/asm/setup.h:#define COMMAND_LINE_SIZE 256 arch/xtensa/include/asm/setup.h:#define COMMAND_LINE_SIZE 256 arch/alpha/include/asm/setup.h:#define COMMAND_LINE_SIZE 256 I think your patch is likely to break all these architectures? Best regards, Wolfgang Denk
Dear Wolfgang Denk, On Tue, Jan 10, 2012 at 2:28 PM, Wolfgang Denk <wd@denx.de> wrote: >> 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. > ... >> static void fixup_silent_linux(void) >> { >> - char buf[256], *start, *end; > > Are you sure that the kernel's buffer is long enough? The kernel's buffer might be big enough, depending on the architecture. For ARM (which is what I'm on), I see that the command line is 1024 bytes. Here's where I'm looking: <http://git.kernel.org/?p=linux/kernel/git/next/linux-next.git;a=blob;f=arch/arm/include/asm/setup.h;h=23ebc0c82a3975ae5c455dd39598e93ab33922e7;hb=refs/heads/master#l19> > I think your patch is likely to break all these architectures? I'm not sure how my patch would break these architectures. My patch removes a buffer overrun--it doesn't actually increase any particular board's command line length. I need this because my board uses a command line that is ~300 bytes--under the kernel limit but currently over u-boot's. I agree completely that this patch doesn't remove all limits on Linux command line length. However, it does allow you to use the full Linux command line length on kernels that have a #define with something greater than 256. In addition, a buffer overrun is a particularly gnarly failure case (opens you up to all sorts of security attacks if someone can figure out how to manipulate the command line), so is generally good to fix. If you'd rather, I'd be happy to rework the patch to change the hardcoded 256 to a CONFIG_COMMAND_LINE_SIZE, then add overflow checking to the function. That would allow my use case and also prevent future buffer overruns. -Doug
On Tuesday 10 January 2012 17:28:05 Wolfgang Denk wrote: > Doug Anderson wrote: > > 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. > > ... > > > static void fixup_silent_linux(void) > > { > > > > - char buf[256], *start, *end; > > Are you sure that the kernel's buffer is long enough? > > For example on PowerPC, there is a current hard limit on 512 > characters: > > arch/powerpc/boot/ops.h:#define COMMAND_LINE_SIZE 512 > arch/powerpc/kernel/setup-common.c:char cmd_line[COMMAND_LINE_SIZE]; > > On SPARC, we have 256 bytes hard limit, see arch/sparc/prom/bootstr_64.c: > > #define BARG_LEN 256 > ... > prom_getstring(prom_chosen_node, "bootargs", > bootstr_info.bootstr_buf, BARG_LEN); i think this does len checking ... > I think your patch is likely to break all these architectures? i don't know about others, but on Blackfin, we don't care. we just copy the first COMMAND_LINE_SIZE bytes out and ignore the rest. -mike
On Tuesday 10 January 2012 17:51:15 Doug Anderson wrote: > On Tue, Jan 10, 2012 at 2:28 PM, Wolfgang Denk wrote: > > I think your patch is likely to break all these architectures? > > I'm not sure how my patch would break these architectures. if the kernel doesn't do len checking on the input string and only looks for trailing NUL byte, you could trigger a buffer overflow in the kernel. personally, i'd say that's poor behavior on the part of the kernel, but we should still be nice if possible ... -mike
diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c index ece1b9a..5bddea4 100644 --- a/common/cmd_bootm.c +++ b/common/cmd_bootm.c @@ -1200,9 +1200,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 */ @@ -1210,25 +1214,45 @@ 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="); if (start) { - end = strchr(start, ' '); - strncpy(buf, cmdline, (start - cmdline + 8)); + char *end = strchr(start, ' '); + int num_start_bytes = start - cmdline + CONSOLE_ARG_LEN; + + /* We know cmdline bytes will be more than enough. */ + buf = malloc(strlen(cmdline) + 1); + if (!buf) { + debug("WARNING: %s failed to alloc cmdline\n", + __func__); + return; + } + + 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="); + buf = malloc(strlen(cmdline) + 1 + CONSOLE_ARG_LEN + 1); + if (!buf) { + debug("WARNING: %s failed to alloc cmdline\n", + __func__); + return; + } + sprintf(buf, "%s %s", cmdline, CONSOLE_ARG); } } else { - strcpy(buf, "console="); + buf = strdup("console="); + if (!buf) { + debug("WARNING: strdup failed in fixup_silent_linux\n"); + return; + } } setenv("bootargs", buf); debug("after silent fix-up: %s\n", buf); + free(buf); } #endif /* CONFIG_SILENT_CONSOLE */
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. Signed-off-by: Doug Anderson <dianders@chromium.org> --- v2: This is a simpler version of patch 3/4 in my previous patchset that just uses malloc() without using the general command line munging funcs. We can separately continue to discuss about the general command func if desired. common/cmd_bootm.c | 44 ++++++++++++++++++++++++++++++++++---------- 1 files changed, 34 insertions(+), 10 deletions(-)