[U-Boot] [PATCH 0/4] Fix fixup_silent_linux() buffer overrun
This set of patches together fixes a buffer overrun in the fixup_silent_linux() function when we've got a long linux command line. It also adds the removal of "earlyprintk" when we are silencing the linux console.
One thing you will notice is that some of these changes have unit tests assocated with them. I wasn't sure exactly where these should live. If people would prefer them elsewhere (or simply removed), just let me know and I'll resubmit.
Doug Anderson (4): cmdline: Add linux command line munging tools cosmetic: Fixup fixup_silent_linux() for checkpatch bootm: Avoid 256-byte overflow in fixup_silent_linux() bootm: Add earlyprintk to fixup_silent_linux
common/Makefile | 1 + common/cmd_bootm.c | 136 +++++++++++++++++++---- common/cmdline.c | 318 ++++++++++++++++++++++++++++++++++++++++++++++++++++ include/cmdline.h | 30 +++++ 4 files changed, 465 insertions(+), 20 deletions(-) create mode 100644 common/cmdline.c create mode 100644 include/cmdline.h
It appears that there are a handful of places in u-boot that we want to munge the linux command line. This adds some helper functions that make that easier.
Signed-off-by: Doug Anderson dianders@chromium.org --- common/Makefile | 1 + common/cmdline.c | 318 +++++++++++++++++++++++++++++++++++++++++++++++++++++ include/cmdline.h | 30 +++++ 3 files changed, 349 insertions(+), 0 deletions(-) create mode 100644 common/cmdline.c create mode 100644 include/cmdline.h
diff --git a/common/Makefile b/common/Makefile index ae795e0..90d2ff0 100644 --- a/common/Makefile +++ b/common/Makefile @@ -186,6 +186,7 @@ COBJS-$(CONFIG_UPDATE_TFTP) += update.o COBJS-$(CONFIG_USB_KEYBOARD) += usb_kbd.o endif
+COBJS-y += cmdline.o COBJS-y += console.o COBJS-y += memsize.o COBJS-y += stdio.o diff --git a/common/cmdline.c b/common/cmdline.c new file mode 100644 index 0000000..0862838 --- /dev/null +++ b/common/cmdline.c @@ -0,0 +1,318 @@ +/* + * Copyright (c) 2011 The Chromium OS Authors. + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +/* Functions for munging the Linux command line */ + +/* + * To run unit tests in this file: + * gcc -DRUN_UNITTESTS -Wall -Werror common/cmdline.c -o cmdline && ./cmdline + */ +#ifdef RUN_UNITTESTS + +#define CONFIG_SILENT_CONSOLE + +#include <assert.h> +#include <ctype.h> +#include <stdio.h> +#include <stdlib.h> +#include <string.h> + +#else + +#include <common.h> +#include <malloc.h> + +#include <cmdline.h> + +#include <linux/ctype.h> + +#endif + + +/** + * Modify the command line to remove a parameter. + * + * This can either remove standalone parameters or ones with arguments. For + * instance you can remove the param "console=/dev/ttyS0" by passing in + * "console" and you can remove the param "earlyprintk" by passing in + * "earlyprintk". + * + * WARNING: + * - The current code doesn't handle removing parameters with spaces in + * them. Specifically, you can't use it to remove xyz if you have + * something like xyz="abc def". + * + * Notes: + * - It is _not_ considered an error to remove a parameter that doesn't exist. + * - If the parameter exists more than once, this just removes the first. + * You can loop to get them all. + * - The parameter must match exactly. AKA "onsole" doesn't match "console". + * + * @param cmdline The command line to modify. + * @param param_name The name of the parameter to remove. + * @return 1 if the param was removed; 0 otherwise. + */ +int remove_cmdline_param(char *cmdline, const char *param_name) +{ + char *start; + char *end; + int param_name_len = strlen(param_name); + + assert(param_name_len != 0); + + /* + * Try to find the param; if we find it, start will point to the + * beginning of the param and end to the character after the param + * (could be '\0', '=', or ' '). If we fail, we return 0 in the loop. + */ + start = cmdline; + while (1) { + start = strstr(start, param_name); + if (!start) + return 0; + end = start + param_name_len; + + /* + * Loop break condition is space (or nothing) before param and + * space or equals (or nothing) after param. + */ + if (((start == cmdline) || isspace(start[-1])) && + ((*end == '\0') || (*end == '=') || isspace(*end))) + break; + + start = end; + } + + /* + * Skip so end points to the start of the next param; note that we don't + * handle quoting here (!), so we'll get confused with abc="def ghi" + */ + while ((*end != '\0') && !isspace(*end)) + end++; + while (isspace(*end)) + end++; + + /* + * Move start backwards to clean up any extra spaces. After this runs, + * start will point to the place to move end onto. + */ + if (start != cmdline) { + start--; + while ((start != cmdline) && isspace(*start)) + start--; + + /* + * Two cases: + * - nothing at end: move fwd one char so we don't clobber the + * last char of the previous cmd. + * - more stuff at end: add exactly one ' ' to separate the + * chunks. + */ + start++; + if (*end != '\0') { + *start = ' '; + start++; + } + } + + /* Kill the parameter */ + memmove(start, end, strlen(end)+1); + + return 1; +} + +/** + * Add a parameter to the command line. + * + * This is much like a glorified strncat(), but handles adding a space between + * the old cmdline and the new one if needed and takes the whole bufsize + * instead of the number of characters to copy. + * + * @param cmdline The command line to modify. + * @param toappend The parameter to append, like "console=/dev/ttyS0". + * @param bufsize The number of bytes that were allocated to cmdline, so + * we know not to overrun. + */ +void add_cmdline_param(char *cmdline, const char *toappend, int bufsize) +{ + int cmdline_len = strlen(cmdline); + + if (cmdline_len == 0) + strncat(cmdline, toappend, bufsize-1); + else { + int bytes_avail = bufsize - cmdline_len; + + if (bytes_avail <= 0) { + assert(bytes_avail == 0); + return; + } + cmdline[bufsize-1] = '\0'; + cmdline[cmdline_len] = ' '; + strncpy(&cmdline[cmdline_len+1], toappend, + bytes_avail-2); + } +} + + +#ifdef RUN_UNITTESTS + +/** + * Unit tests for remove_cmdline_param(). + */ +void remove_cmdline_param_unittest(void) +{ + char *original_str; + char *expected_str; + char *result; + int retval; + + /* Try removing each bit of a reasonable sample */ + original_str = "console=ttyS0,115200n8 root=/dev/mmcblk0p3 rootwait ro"; + expected_str = "root=/dev/mmcblk0p3 rootwait ro"; + result = strdup(original_str); + retval = remove_cmdline_param(result, "console"); + assert(strcmp(result, expected_str) == 0); + assert(retval == 1); + free(result); + + original_str = "console=ttyS0,115200n8 root=/dev/mmcblk0p3 rootwait ro"; + expected_str = "console=ttyS0,115200n8 rootwait ro"; + result = strdup(original_str); + retval = remove_cmdline_param(result, "root"); + assert(strcmp(result, expected_str) == 0); + assert(retval == 1); + free(result); + + original_str = "console=ttyS0,115200n8 root=/dev/mmcblk0p3 rootwait ro"; + expected_str = "console=ttyS0,115200n8 root=/dev/mmcblk0p3 ro"; + result = strdup(original_str); + retval = remove_cmdline_param(result, "rootwait"); + assert(strcmp(result, expected_str) == 0); + assert(retval == 1); + free(result); + + original_str = "console=ttyS0,115200n8 root=/dev/mmcblk0p3 rootwait ro"; + expected_str = "console=ttyS0,115200n8 root=/dev/mmcblk0p3 rootwait"; + result = strdup(original_str); + retval = remove_cmdline_param(result, "ro"); + assert(strcmp(result, expected_str) == 0); + assert(retval == 1); + free(result); + + /* Remove something that's not there */ + original_str = "console=ttyS0,115200n8 root=/dev/mmcblk0p3 rootwait ro"; + expected_str = "console=ttyS0,115200n8 root=/dev/mmcblk0p3 rootwait ro"; + result = strdup(original_str); + retval = remove_cmdline_param(result, "oogabooga"); + assert(strcmp(result, expected_str) == 0); + assert(retval == 0); + free(result); + + /* Remove from a NULL string */ + original_str = ""; + expected_str = ""; + result = strdup(original_str); + retval = remove_cmdline_param(result, "oogabooga"); + assert(strcmp(result, expected_str) == 0); + assert(retval == 0); + free(result); + + /* Remove with an '=' based param at the end */ + original_str = "root=/dev/mmcblk0p3 rootwait ro console=ttyS0,115200n8"; + expected_str = "root=/dev/mmcblk0p3 rootwait ro"; + result = strdup(original_str); + retval = remove_cmdline_param(result, "console"); + assert(strcmp(result, expected_str) == 0); + assert(retval == 1); + free(result); + + /* Remove with a non-'=' based param at the beginning */ + original_str = "ro root=/dev/mmcblk0p3 rootwait console=ttyS0,115200n8"; + expected_str = "root=/dev/mmcblk0p3 rootwait console=ttyS0,115200n8"; + result = strdup(original_str); + retval = remove_cmdline_param(result, "ro"); + assert(strcmp(result, expected_str) == 0); + assert(retval == 1); + free(result); + + /* Add a few extra spaces and see how it deals with it */ + original_str = "console=ttyS0,115200n8\t root=/dev/mmcblk0p3 \tro"; + expected_str = "console=ttyS0,115200n8 ro"; + result = strdup(original_str); + retval = remove_cmdline_param(result, "root"); + assert(strcmp(result, expected_str) == 0); + assert(retval == 1); + free(result); + + printf("remove_cmdline_param_unittest: pass\n"); +} + +/** + * Unit tests for add_cmdline_param(). + */ +void add_cmdline_param_unittest(void) +{ + char *original_str; + char *expected_str; + char *result; + int extra_chars = strlen(" console="); + int bufsize; + + /* Simple case first; try adding "console=" */ + original_str = "root=/dev/mmcblk0p3 rootwait ro"; + expected_str = "root=/dev/mmcblk0p3 rootwait ro console="; + bufsize = strlen(original_str) + 1 + extra_chars; + result = malloc(bufsize); + strcpy(result, original_str); + add_cmdline_param(result, "console=", bufsize); + assert(strcmp(result, expected_str) == 0); + free(result); + + /* Add to an empty string; should see no ' ' before... */ + original_str = ""; + expected_str = "console="; + bufsize = strlen(original_str) + 1 + extra_chars - 1; + result = malloc(bufsize); + strcpy(result, original_str); + add_cmdline_param(result, "console=", bufsize); + assert(strcmp(result, expected_str) == 0); + free(result); + + /* Shrink down bufsize and see loss of = */ + original_str = "root=/dev/mmcblk0p3 rootwait ro"; + expected_str = "root=/dev/mmcblk0p3 rootwait ro console"; + bufsize = strlen(original_str) + 1 + extra_chars - 1; + result = malloc(bufsize); + strcpy(result, original_str); + add_cmdline_param(result, "console=", bufsize); + assert(strcmp(result, expected_str) == 0); + free(result); + + printf("add_cmdline_param_unittest: pass\n"); +} + +int main(int argc, char **argv) +{ + remove_cmdline_param_unittest(); + add_cmdline_param_unittest(); + return 0; +} +#endif diff --git a/include/cmdline.h b/include/cmdline.h new file mode 100644 index 0000000..65b415c --- /dev/null +++ b/include/cmdline.h @@ -0,0 +1,30 @@ +/* + * Copyright (c) 2011 The Chromium OS Authors. + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +/* Functions for munging the Linux command line */ + +#ifndef _CMDLINE_H_ +#define _CMDLINE_H_ + +int remove_cmdline_param(char *cmdline, const char *param_name); +void add_cmdline_param(char *cmdline, const char *toappend, int bufsize); + +#endif /*_CMDLINE_H_ */
On Wednesday 19 October 2011 18:30:56 Doug Anderson wrote:
--- /dev/null +++ b/common/cmdline.c
+/*
- To run unit tests in this file:
- gcc -DRUN_UNITTESTS -Wall -Werror common/cmdline.c -o cmdline &&
./cmdline
- */
+#ifdef RUN_UNITTESTS
i'm not sure this part should be merged ... better to keep on the back burner while Simon sorts out testing framework with new sandbox arch.
+void add_cmdline_param(char *cmdline, const char *toappend, int bufsize)
bufsize should be size_t
+{
- int cmdline_len = strlen(cmdline);
cmdline_len should be size_t
- if (cmdline_len == 0)
strncat(cmdline, toappend, bufsize-1);- else {
int bytes_avail = bufsize - cmdline_len;if (bytes_avail <= 0) {assert(bytes_avail == 0);return;}cmdline[bufsize-1] = '\0';cmdline[cmdline_len] = ' ';strncpy(&cmdline[cmdline_len+1], toappend,bytes_avail-2);- }
hmm, i feel like this should be simpler and not need that branch
if (cmdline_len) cmdline[cmdline_len++] = ' '; if (bufsize <= cmdline_len) return; memcpy(&cmdline[cmdline_len], toappend, bufsize - cmdline_len - 1); cmdline[bufsize] = '\0'; -mike
On Wed, Oct 19, 2011 at 3:46 PM, Mike Frysinger vapier@gentoo.org wrote:
On Wednesday 19 October 2011 18:30:56 Doug Anderson wrote:
--- /dev/null +++ b/common/cmdline.c
+/*
- To run unit tests in this file:
- gcc -DRUN_UNITTESTS -Wall -Werror common/cmdline.c -o cmdline &&
./cmdline
- */
+#ifdef RUN_UNITTESTS
i'm not sure this part should be merged ... better to keep on the back burner while Simon sorts out testing framework with new sandbox arch.
Fair enough. I will take out for the next version.
bufsize should be size_t cmdline_len should be size_t
Will do in all cases. Thanks for catching!
hmm, i feel like this should be simpler and not need that branch
if (cmdline_len) cmdline[cmdline_len++] = ' '; if (bufsize <= cmdline_len) return; memcpy(&cmdline[cmdline_len], toappend, bufsize - cmdline_len - 1); cmdline[bufsize] = '\0';
There is one case that this doesn't catch I think: where cmdline_len was exactly the same size as the original string (so we shouldn't append anything).
...but then, as I look at this, I realize that I somehow sent you 1 rev back of my patch. Aack! I will resend a new one tomorrow morning.
Thanks!
-Doug
On Wednesday 19 October 2011 18:30:56 Doug Anderson wrote:
--- /dev/null +++ b/include/cmdline.h
+int remove_cmdline_param(char *cmdline, const char *param_name); +void add_cmdline_param(char *cmdline, const char *toappend, int bufsize);
i'm not particularly enamored with this naming style. i find the style of "<section>_<operation>" to be easier on the eyes rather than this RPN.
i.e. cmdline_param_remove() and cmdline_param_add() -mike
On Wed, Oct 19, 2011 at 3:52 PM, Mike Frysinger vapier@gentoo.org wrote:
i'm not particularly enamored with this naming style. i find the style of "<section>_<operation>" to be easier on the eyes rather than this RPN.
i.e. cmdline_param_remove() and cmdline_param_add()
I'm happy to name it whatever you'd like. My next patch will use your naming suggestion unless someone says otherwise.
I will note, however, that the "verb_noun" style of naming appears to be favored by the Linux kernel. In the section of "Naming" in < http://www.kernel.org/doc/Documentation/CodingStyle%3E, a suggested name is "count_active_users()", not "active_users_count()".
-Doug
On Wednesday 19 October 2011 21:07:11 Doug Anderson wrote:
On Wed, Oct 19, 2011 at 3:52 PM, Mike Frysinger vapier@gentoo.org wrote:
i'm not particularly enamored with this naming style. i find the style of "<section>_<operation>" to be easier on the eyes rather than this RPN.
i.e. cmdline_param_remove() and cmdline_param_add()
I'm happy to name it whatever you'd like. My next patch will use your naming suggestion unless someone says otherwise.
I will note, however, that the "verb_noun" style of naming appears to be favored by the Linux kernel. In the section of "Naming" in < http://www.kernel.org/doc/Documentation/CodingStyle%3E, a suggested name is "count_active_users()", not "active_users_count()".
to be fair, that file says: ... you should call that "count_active_users()" or similar, you should _not_ call it "cntusr()". it doesn't seem to take a position on the "verb_noun" vs "noun_verb" style.
in my experience, the common string tends to be more used at the start rather than the end (spinlocks, mutexes, interrupts, gpios, spi, i2c). but i've certainly not done a comprehensive survey to back up my position :). -mike
Dear Doug Anderson,
In message 1319063459-4804-2-git-send-email-dianders@chromium.org you wrote:
It appears that there are a handful of places in u-boot that we want to munge the linux command line. This adds some helper functions that make that easier.
Signed-off-by: Doug Anderson dianders@chromium.org
common/Makefile | 1 + common/cmdline.c | 318 +++++++++++++++++++++++++++++++++++++++++++++++++++++ include/cmdline.h | 30 +++++ 3 files changed, 349 insertions(+), 0 deletions(-) create mode 100644 common/cmdline.c create mode 100644 include/cmdline.h
Sorry, but could you please explain why anybody would need this?
Instead of adding tons of code to process one environment variable that happens to be passed to Linux, but otherwise is not different form any of the other variables, you can as well just use plain simple shell scripting capabilities to build your Linux command line from pieces. This is way more flexible and much less resource consuming.
Unless you have a really good explanation why such coude is needed I tend to NAK it.
Best regards,
Wolfgang Denk
Wolfgang,
On Thu, Oct 20, 2011 at 7:36 AM, Wolfgang Denk wd@denx.de wrote:
Sorry, but could you please explain why anybody would need this?
I'm happy to explain. :) In our setup, the Linux command line is constructed (in part) by reading from the disk. When we load the kernel, we also load the kernel command line. It is convenient for us if this kernel command line on disk continues to have things like "console=/dev/ttyS0" and "earlyprintk" in it so that we can swap between release (no console) and development (with console) by just tweaking a settings in u-boot.
Certainly we could change our setup, but the prior existence of fixup_silent_linux() indicated that this might be a common way to do things.
In the case of Chromium OS, we also do some additional programmatic munging of the command line for verified boot purposes, but since that is not upstream it's not really a good argument for making the more general function.
In any case, I already have the more direct (and less generalized) version of fixup_silent_linux() written and will send out a patch with just that shortly.
Based on your comments, I'll assume that you're not interested in the more general command line munging tools and will abandon them for now until there is a clear need for them.
Thanks for your review!
-Doug
On Thursday 20 October 2011 13:06:23 Doug Anderson wrote:
Based on your comments, I'll assume that you're not interested in the more general command line munging tools and will abandon them for now until there is a clear need for them.
what is the difference in compiled sizes ? if the abstracted funcs add negligible overhead, i think merging these locally in the bootm code might make sense in a pure clean up sense ... -mike
Mike,
On Thu, Oct 20, 2011 at 10:15 AM, Mike Frysinger vapier@gentoo.org wrote:
what is the difference in compiled sizes ? if the abstracted funcs add negligible overhead, i think merging these locally in the bootm code might make sense in a pure clean up sense ...
Compared to the simple version I just posted and my latest attempt to address your review comments (and pulling in the newest version of my patches):
Simple version: 168820 bytes With munging functions abstracted: 169184 bytes
...so 364 bytes. It might be smaller if I actually inlined my functions into bootm.
How about this for a plan?
Wolfgang can see if he wants to apply my "simple" fix to use malloc(). If so, great! ...at least the bug will be fixed. :)
...then, we can decide if we want to add the abstract munging tools and where to add them (either a separate lib/cmdline.c file or direct into bootm). If you want them, I'll submit a patch with all of your review feedback addressed and a second patch to change fixup_silent_linux() to use them (with a better description).
...we can think about the earlyprintk and looping questions after the above have been addressed.
-Doug
Dear Doug Anderson,
In message CAD=FV=Vki9xXZG0uoHnMgR9=XXv5p=XCbbwhr-xAu9A0WoxdkA@mail.gmail.com you wrote:
...then, we can decide if we want to add the abstract munging tools and where to add them (either a separate lib/cmdline.c file or direct into bootm). If you want them, I'll submit a patch with all of your review feedback addressed and a second patch to change fixup_silent_linux() to use them (with a better description).
As I wrote before: if such a feature should be added, then please in a generally useful and usable way, not special-cased for specific operations on a single special variable only.
Best regards,
Wolfgang Denk
Dear Doug Anderson,
In message CAD=FV=WEk1To=hyOTLBC+htq=7hxrTqtyckajJyLByqs3DBFYA@mail.gmail.com you wrote:
I'm happy to explain. :) In our setup, the Linux command line is constructed (in part) by reading from the disk. When we load the kernel, we also load the kernel command line. It is convenient for us if this kernel command line on disk continues to have things like "console=/dev/ttyS0" and "earlyprintk" in it so that we can swap between release (no console) and development (with console) by just tweaking a settings in u-boot.
Certainly we could change our setup, but the prior existence of fixup_silent_linux() indicated that this might be a common way to do things.
Please check where this is actually used, and how. It serves a very special purpose, and by todays level of reviews I seriously doubt such code would be accepted any more.
Yes, please change your setup. Just because you are doing things your (highly unefficient) way does not mean I'm going to accept such code for mainline.
In the case of Chromium OS, we also do some additional programmatic munging of the command line for verified boot purposes, but since that is not upstream it's not really a good argument for making the more general function.
May I ask why you don;t use the powerful capabilities of incrementally building the command line from environment variable settings?
[These can also be read from disk.]
In any case, I already have the more direct (and less generalized) version of fixup_silent_linux() written and will send out a patch with just that shortly.
Please consider it NAKed.
Based on your comments, I'll assume that you're not interested in the more general command line munging tools and will abandon them for now until there is a clear need for them.
"Not interested" is an understatement. I consider them a highly unefficient and conceptually broken approach.
Best regards,
Wolfgang Denk
Wolfgang,
On Thu, Oct 20, 2011 at 12:03 PM, Wolfgang Denk wd@denx.de wrote:
In any case, I already have the more direct (and less generalized)
version
of fixup_silent_linux() written and will send out a patch with just that shortly.
Please consider it NAKed.
One side effect of not committing any patch for fixup_silent_linux() is that it means that anyone who defines CONFIG_SILENT_CONSOLE and is booting Linux with "bootm" is subject to the current buffer overrun (regardless of whether they have a "console=" clause in their Linux command line).
I understand your concerns and will plan to change the way our boot scripts work so that fixup_silent_linux() isn't needed in our case. However, since we still want to define CONFIG_SILENT_CONSOLE we will need something to keep the buffer overrun away.
If you will not accept a patch to fix the buffer overrun, will you accept a patch that adds a new config option like CONFIG_DONT_FIXUP_SILENT_LINUX that prevents fixup_silent_linux() from being called? That way, any existing code that relies on the current behavior will continue to work as it does today, but anyone who wants to avoid the buffer overrun can.
...or, an alternate would be to add CONFIG_FIXUP_SILENT_LINUX to all existing configs with CONFIG_SILENT_CONSOLE and use that.
Thanks!
-Doug
Signed-off-by: Doug Anderson dianders@chromium.org Reviewed-by: Anton Staaf robotboy@chromium.org --- common/cmd_bootm.c | 25 +++++++++++++------------ 1 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c index bb9b698..ece1b9a 100644 --- a/common/cmd_bootm.c +++ b/common/cmd_bootm.c @@ -1200,34 +1200,35 @@ U_BOOT_CMD( /* helper routines */ /*******************************************************************/ #ifdef CONFIG_SILENT_CONSOLE -static void fixup_silent_linux () +static void fixup_silent_linux(void) { char buf[256], *start, *end; - char *cmdline = getenv ("bootargs"); + char *cmdline = getenv("bootargs");
/* Only fix cmdline when requested */ if (!(gd->flags & GD_FLG_SILENT)) return;
- debug ("before silent fix-up: %s\n", cmdline); + debug("before silent fix-up: %s\n", cmdline); if (cmdline) { - if ((start = strstr (cmdline, "console=")) != NULL) { - end = strchr (start, ' '); - strncpy (buf, cmdline, (start - cmdline + 8)); + start = strstr(cmdline, "console="); + if (start) { + end = strchr(start, ' '); + strncpy(buf, cmdline, (start - cmdline + 8)); if (end) - strcpy (buf + (start - cmdline + 8), end); + strcpy(buf + (start - cmdline + 8), end); else buf[start - cmdline + 8] = '\0'; } else { - strcpy (buf, cmdline); - strcat (buf, " console="); + strcpy(buf, cmdline); + strcat(buf, " console="); } } else { - strcpy (buf, "console="); + strcpy(buf, "console="); }
- setenv ("bootargs", buf); - debug ("after silent fix-up: %s\n", buf); + setenv("bootargs", buf); + debug("after silent fix-up: %s\n", buf); } #endif /* CONFIG_SILENT_CONSOLE */
Dear Doug Anderson,
In message 1319063459-4804-3-git-send-email-dianders@chromium.org you wrote:
Signed-off-by: Doug Anderson dianders@chromium.org Reviewed-by: Anton Staaf robotboy@chromium.org
common/cmd_bootm.c | 25 +++++++++++++------------ 1 files changed, 13 insertions(+), 12 deletions(-)
Applied, thanks.
Best regards,
Wolfgang Denk
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 --- common/cmd_bootm.c | 125 ++++++++++++++++++++++++++++++++++++++++++++-------- 1 files changed, 106 insertions(+), 19 deletions(-)
diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c index ece1b9a..f426e2f 100644 --- a/common/cmd_bootm.c +++ b/common/cmd_bootm.c @@ -26,6 +26,7 @@ * Boot support */ #include <common.h> +#include <cmdline.h> #include <watchdog.h> #include <command.h> #include <image.h> @@ -1200,36 +1201,122 @@ U_BOOT_CMD( /* helper routines */ /*******************************************************************/ #ifdef CONFIG_SILENT_CONSOLE + +/** + * Remove "console=blah" and from cmdline, replace w/ "console=". + * + * This has the effect of telling Linux that we'd like it to have a silent + * console. + * + * @param cmdline The original commanjd line. + * @return The new command line, which has been allocated with malloc(). + * Might be NULL if we ran out of memory. + */ +static char *do_fixup_silent_linux(const char *cmdline) +{ + char *buf; + int bufsize; + int did_remove; + + if (!cmdline) + cmdline = ""; + + /* + * Allocate enough space for: + * - a copy of the command line + * - a space + * - a blank "console=" argument + * - the '\0' + * + * ...we might not need all this space, but it's OK to overallocate a + * little. + */ + bufsize = strlen(cmdline) + 1 + sizeof("console="); + buf = malloc(bufsize); + if (!buf) { + debug("WARNING: malloc failed in fixup_silent_linux\n"); + return NULL; + } + + strcpy(buf, cmdline); + do { + did_remove = remove_cmdline_param(buf, "console"); + } while (did_remove); + add_cmdline_param(buf, "console=", bufsize); + + return buf; +} + static void fixup_silent_linux(void) { - char buf[256], *start, *end; - char *cmdline = getenv("bootargs"); + char *buf; + const char *cmdline = getenv("bootargs");
/* Only fix cmdline when requested */ if (!(gd->flags & GD_FLG_SILENT)) return;
debug("before silent fix-up: %s\n", cmdline); - if (cmdline) { - start = strstr(cmdline, "console="); - if (start) { - end = strchr(start, ' '); - strncpy(buf, cmdline, (start - cmdline + 8)); - if (end) - strcpy(buf + (start - cmdline + 8), end); - else - buf[start - cmdline + 8] = '\0'; - } else { - strcpy(buf, cmdline); - strcat(buf, " console="); - } - } else { - strcpy(buf, "console="); + + buf = do_fixup_silent_linux(cmdline); + if (buf) { + setenv("bootargs", buf); + debug("after silent fix-up: %s\n", buf); + free(buf); } +}
- setenv("bootargs", buf); - debug("after silent fix-up: %s\n", buf); +/** + * Unit tests for do_fixup_silent_linux(). + * + * At the moment, there's no easy way to run this other than to copy it (and + * do_fixup_silent_linux) to another file. + */ +#ifdef RUN_UNITTESTS +void do_fixup_silent_linux_unittest(void) +{ + char *original_str; + char *expected_str; + char *result; + + /* Simple case first, as an example */ + original_str = "console=ttyS0,115200n8 root=/dev/mmcblk0p3 rootwait ro"; + expected_str = "root=/dev/mmcblk0p3 rootwait ro console="; + result = do_fixup_silent_linux(original_str); + assert(strcmp(result, expected_str) == 0); + free(result); + + /* Null cases next */ + original_str = NULL; + expected_str = "console="; + result = do_fixup_silent_linux(original_str); + assert(strcmp(result, expected_str) == 0); + free(result); + + original_str = ""; + expected_str = "console="; + result = do_fixup_silent_linux(original_str); + assert(strcmp(result, expected_str) == 0); + free(result); + + /* Throw console= at the end */ + original_str = "root=/dev/mmcblk0p3 rootwait ro console=ttyS0,115200n8"; + expected_str = "root=/dev/mmcblk0p3 rootwait ro console="; + result = do_fixup_silent_linux(original_str); + assert(strcmp(result, expected_str) == 0); + free(result); + + /* Something non-NULL with no "console=" */ + original_str = "root=/dev/mmcblk0p3 rootwait ro"; + expected_str = "root=/dev/mmcblk0p3 rootwait ro console="; + result = do_fixup_silent_linux(original_str); + assert(strcmp(result, expected_str) == 0); + free(result); + + debug("do_fixup_silent_linux_unittest: pass\n"); } +#endif /* RUN_UNITTESTS */ + #endif /* CONFIG_SILENT_CONSOLE */
On Wednesday 19 October 2011 18:30:58 Doug Anderson wrote:
--- a/common/cmd_bootm.c +++ b/common/cmd_bootm.c
+static char *do_fixup_silent_linux(const char *cmdline) +{
- int bufsize;
size_t
- /*
* Allocate enough space for:* - a copy of the command line* - a space* - a blank "console=" argument* - the '\0'** ...we might not need all this space, but it's OK to overallocate a* little.*/- bufsize = strlen(cmdline) + 1 + sizeof("console=");
relying on the sizeof() to include the NUL byte calculation seems like it could confuse some. how about: strlen(cmdline) + 1 + strlen("console=") + 1; gcc should optimize that into a constant anyways.
- strcpy(buf, cmdline);
- do {
did_remove = remove_cmdline_param(buf, "console");- } while (did_remove);
- add_cmdline_param(buf, "console=", bufsize);
this is different behavior from what was there before. the previous code only removed the first console= and not all of them. i've relied on this behavior in the past, so i'm not sure you should change it. at least not without a dedicated commit rather than merging it with a commit that's supposed to just change the code to use the new remove_cmdline_param() helper. -mike
Dear Doug Anderson,
In message 1319063459-4804-4-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.
Signed-off-by: Doug Anderson dianders@chromium.org
common/cmd_bootm.c | 125 ++++++++++++++++++++++++++++++++++++++++++++-------- 1 files changed, 106 insertions(+), 19 deletions(-)
Code and description have very little to do with each other. Yoiur patch contains at least one other, big change.
Please split into separate patches.
Expect that the change to use malloc() will be applied, and the rest will probably be rejected (see previous message).
Best regards,
Wolfgang Denk
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(-)
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 */
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: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
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
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(-)
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 */
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';
strcpy(buf, cmdline);strcat(buf, " console=");
sprintf(buf, "%s %s", cmdline, CONSOLE_ARG);
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
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.
Changes in v3: - Took Mike Frysinger's suggestion of removing strdup()
common/cmd_bootm.c | 41 +++++++++++++++++++++++++++++------------ 1 files changed, 29 insertions(+), 12 deletions(-)
diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c index d5745b1..95ac2d9 100644 --- a/common/cmd_bootm.c +++ b/common/cmd_bootm.c @@ -1229,9 +1229,14 @@ 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 *env_val; char *cmdline = getenv("bootargs");
/* Only fix cmdline when requested */ @@ -1239,25 +1244,37 @@ 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); } + env_val = buf; } else { - strcpy(buf, "console="); + buf = NULL; + env_val = CONSOLE_ARG; }
- setenv("bootargs", buf); - debug("after silent fix-up: %s\n", buf); + setenv("bootargs", env_val); + debug("after silent fix-up: %s\n", env_val); + free(buf); } #endif /* CONFIG_SILENT_CONSOLE */
On Tuesday 17 January 2012 14:16:53 Doug Anderson wrote:
--- a/common/cmd_bootm.c +++ b/common/cmd_bootm.c
- char *env_val;
did marking this const not work out ?
otherwise, seems to look OK ... -mike
Mike,
On Tue, Jan 17, 2012 at 11:27 AM, Mike Frysinger vapier@gentoo.org wrote:
On Tuesday 17 January 2012 14:16:53 Doug Anderson wrote:
--- a/common/cmd_bootm.c +++ b/common/cmd_bootm.c
- char *env_val;
did marking this const not work out ?
I coded the patch an old "-v2011.06" branch then tested on ToT. On "-v2011.06" setenv wasn't const. I'll resubmit with the "const".
Thanks!
-Doug
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.
Changes in v3: - Took Mike Frysinger's suggestion of removing strdup()
Changes in v4: - Added in const
common/cmd_bootm.c | 41 +++++++++++++++++++++++++++++------------ 1 files changed, 29 insertions(+), 12 deletions(-)
diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c index d5745b1..0a5ac81 100644 --- a/common/cmd_bootm.c +++ b/common/cmd_bootm.c @@ -1229,9 +1229,14 @@ 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; + const char *env_val; char *cmdline = getenv("bootargs");
/* Only fix cmdline when requested */ @@ -1239,25 +1244,37 @@ 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); } + env_val = buf; } else { - strcpy(buf, "console="); + buf = NULL; + env_val = CONSOLE_ARG; }
- setenv("bootargs", buf); - debug("after silent fix-up: %s\n", buf); + setenv("bootargs", env_val); + debug("after silent fix-up: %s\n", env_val); + free(buf); } #endif /* CONFIG_SILENT_CONSOLE */
Acked-by: Mike Frysinger vapier@gentoo.org -mike
On Tue, Jan 17, 2012 at 09:37:41AM -0000, 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.
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 Acked-by: Mike Frysinger vapier@gentoo.org
Applied to u-boot/master, thanks!
Signed-off-by: Doug Anderson dianders@chromium.org --- common/cmd_bootm.c | 10 +++++++++- 1 files changed, 9 insertions(+), 1 deletions(-)
diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c index f426e2f..c259bfb 100644 --- a/common/cmd_bootm.c +++ b/common/cmd_bootm.c @@ -1203,7 +1203,7 @@ U_BOOT_CMD( #ifdef CONFIG_SILENT_CONSOLE
/** - * Remove "console=blah" and from cmdline, replace w/ "console=". + * Remove "console=blah" and "earlyprintk" from cmdline, replace w/ "console=". * * This has the effect of telling Linux that we'd like it to have a silent * console. @@ -1241,6 +1241,7 @@ static char *do_fixup_silent_linux(const char *cmdline) strcpy(buf, cmdline); do { did_remove = remove_cmdline_param(buf, "console"); + did_remove |= remove_cmdline_param(buf, "earlyprintk"); } while (did_remove); add_cmdline_param(buf, "console=", bufsize);
@@ -1313,6 +1314,13 @@ void do_fixup_silent_linux_unittest(void) assert(strcmp(result, expected_str) == 0); free(result);
+ /* Add in earlyprintk */ + original_str = "console=ttyS0,115200n8 root=/dev/mmcblk0p3 earlyprintk"; + expected_str = "root=/dev/mmcblk0p3 console="; + result = do_fixup_silent_linux(original_str); + assert(strcmp(result, expected_str) == 0); + free(result); + debug("do_fixup_silent_linux_unittest: pass\n"); } #endif /* RUN_UNITTESTS */
On Wednesday 19 October 2011 18:30:59 Doug Anderson wrote:
- /* Add in earlyprintk */
- original_str = "console=ttyS0,115200n8 root=/dev/mmcblk0p3 earlyprintk";
- expected_str = "root=/dev/mmcblk0p3 console=";
*choke* wtf just happened here ? -mike
On Wed, Oct 19, 2011 at 3:35 PM, Mike Frysinger vapier@gentoo.org wrote:
On Wednesday 19 October 2011 18:30:59 Doug Anderson wrote:
/* Add in earlyprintk */original_str = "console=ttyS0,115200n8 root=/dev/mmcblk0p3earlyprintk";
expected_str = "root=/dev/mmcblk0p3 console=";*choke* wtf just happened here ?
Can you clarify what you're asking?
Are you asking about fixup_silent_linux()? That's a function that already exists in u-boot. Its goal is to modify the command line to change the "console=blah" argument to a "console=", effectively telling Linux not to have a console.
This particular patchset is extending fixup_silent_linux() to also remove "earlyprintk" from the command line, since the console isn't really silent unless you do that.
...or are you asking about the bit of unittest code to test that the stripping of "earlyprintk" really works? I was definitely not certain about whether to add the unittests in here (since there's no way to run them). My hope was that at some point in time there'd be a unit test infrastructure in u-boot and my unit tests could then be incorporated. If you'd rather not see the unit tests at all, I'm happy to strip them out.
Maybe a GUI diff showing this change would help? If so, you can see it here: http://gerrit.chromium.org/gerrit/8822
...that is where I got early reviews of this change before sending it to the list.
Thanks!
-Doug
On Wednesday 19 October 2011 18:46:20 Doug Anderson wrote:
On Wed, Oct 19, 2011 at 3:35 PM, Mike Frysinger vapier@gentoo.org wrote:
On Wednesday 19 October 2011 18:30:59 Doug Anderson wrote:
/* Add in earlyprintk */original_str = "console=ttyS0,115200n8 root=/dev/mmcblk0p3earlyprintk";
expected_str = "root=/dev/mmcblk0p3 console=";*choke* wtf just happened here ?
Can you clarify what you're asking?
oh, you're updating the internal test code. i missed that this existed, so i thought you were blindly adding these strings for everyone. -mike
Dear Doug Anderson,
In message 1319063459-4804-5-git-send-email-dianders@chromium.org you wrote:
Signed-off-by: Doug Anderson dianders@chromium.org
common/cmd_bootm.c | 10 +++++++++- 1 files changed, 9 insertions(+), 1 deletions(-)
As before, I see very little (actually, none at all) need for such function. Things like that should be handled usign environment variables in a clever way.
Please explain why you think this code is needed.
Best regards,
Wolfgang Denk
Wolfgang,
On Thu, Oct 20, 2011 at 7:42 AM, Wolfgang Denk wd@denx.de wrote:
As before, I see very little (actually, none at all) need for such function. Things like that should be handled usign environment variables in a clever way.
Please explain why you think this code is needed.
I'm not sure I understand your comment. It sounds to me like you're saying that fixup_silent_linux() (which already exists in u-boot code) shouldn't be needed anymore in u-boot. ...and maybe you're considering it deprecated? Would you like me to submit a patch to remove it?
It appears that fixup_silent_linux() was originally added by you in 2003 (f72da3406bf6f1c1bce9aa03b07d070413a916af).
...or are you saying that you don't see the need to change fixup_silent_linux() to also remove "earlyprintk"?
Thanks!
-Doug
Dear Doug Anderson,
In message CAD=FV=Ueawx_8Pw8bdni2BPbHP1p-XjsoURmRZr-1QvQ3YXd-A@mail.gmail.com you wrote:
I'm not sure I understand your comment. It sounds to me like you're saying that fixup_silent_linux() (which already exists in u-boot code) shouldn't be needed anymore in u-boot. ...and maybe you're considering it deprecated?
I consider at least the way how it was done deprecated.
Would you like me to submit a patch to remove it?
There are boards that use it. If you remove it, you must provide some replacement so these boards don;t break.
It appears that fixup_silent_linux() was originally added by you in 2003 (f72da3406bf6f1c1bce9aa03b07d070413a916af).
2003... heh. By then, life was pretty much unregulated, and you could get about any code in ;-)
...or are you saying that you don't see the need to change fixup_silent_linux() to also remove "earlyprintk"?
I think all this code is more or less dead (however I don't know who might actually actively use or even depend on it). fixup_silent_linux() was intended to remove only the "console=" argument from the kernel command line, and actually only the firrst of it if there should be several. At the time this code was written, that was only used on PPC, and the 256 byte buffer size was also the hardwired limit for the cmdline in that time's Linux kernels - so when written everything was correct (though bound to break as soon as Linux allows for longer cmdline args).
If you think it would be nice to be able to perform special operations (like general substitution) on U-Boot environment variables, this should be written as a separate command that can be run from the command line, and that can be applied to all variables - not a hardwired special-cased construct for bootargs only. We can then make this command optional, and then we can even remove fixup_silent_linux() and replace it by calls to that code for the boards that need it.
Best regards,
Wolfgang Denk
participants (4)
-
Doug Anderson -
Mike Frysinger -
Tom Rini -
Wolfgang Denk