[U-Boot] [PATCH 0/4] Fix fixup_silent_linux() buffer overrun

[U-Boot] [PATCH 0/4] Fix fixup_silent_linux() buffer overrun

[Doug Anderson]

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

-- 
1.7.3.1

[U-Boot] [PATCH 1/4] cmdline: Add linux command line munging tools

[Doug Anderson]

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@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_ */
-- 
1.7.3.1

[U-Boot] [PATCH 1/4] cmdline: Add linux command line munging tools

[Mike Frysinger]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111019/bf949527/attachment.pgp 

[U-Boot] [PATCH 1/4] cmdline: Add linux command line munging tools

[Doug Anderson]

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

[U-Boot] [PATCH 1/4] cmdline: Add linux command line munging tools

[Mike Frysinger]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111019/fc3fd3b6/attachment.pgp 

[U-Boot] [PATCH 1/4] cmdline: Add linux command line munging tools

[Doug Anderson]

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>, a suggested name is
"count_active_users()", not "active_users_count()".

-Doug

[U-Boot] [PATCH 1/4] cmdline: Add linux command line munging tools

[Mike Frysinger]

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>, 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111019/dc51691e/attachment.pgp 

[U-Boot] [PATCH 1/4] cmdline: Add linux command line munging tools

[Wolfgang Denk]

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

-- 
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
As in certain cults it is possible to kill a process if you know  its
true name.                      -- Ken Thompson and Dennis M. Ritchie

[U-Boot] [PATCH 1/4] cmdline: Add linux command line munging tools

[Doug Anderson]

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

[U-Boot] [PATCH 1/4] cmdline: Add linux command line munging tools

[Mike Frysinger]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111020/84a22a62/attachment.pgp 

[U-Boot] [PATCH 1/4] cmdline: Add linux command line munging tools

[Doug Anderson]

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

[U-Boot] [PATCH 1/4] cmdline: Add linux command line munging tools

[Wolfgang Denk]

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

-- 
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
Comparing information  and  knowledge  is  like  asking  whether  the
fatness  of  a  pig  is more or less green than the designated hitter
rule."                                               - David Guaspari

[U-Boot] [PATCH 1/4] cmdline: Add linux command line munging tools

[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

-- 
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
A quarrel is quickly settled when deserted by one party; there is  no
battle unless there be two.                                  - Seneca

[U-Boot] [PATCH 1/4] cmdline: Add linux command line munging tools

[Doug Anderson]

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

[U-Boot] [PATCH 2/4] cosmetic: Fixup fixup_silent_linux() for checkpatch

[Doug Anderson]

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 */
 
-- 
1.7.3.1

[U-Boot] [PATCH 2/4] cosmetic: Fixup fixup_silent_linux() for checkpatch

[Wolfgang Denk]

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

-- 
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
"You shouldn't make my toaster angry." - Household security explained
in "Johnny Quest"

[U-Boot] [PATCH 3/4] bootm: Avoid 256-byte overflow in fixup_silent_linux()

[Doug Anderson]

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 */
 
 
-- 
1.7.3.1

[U-Boot] [PATCH 3/4] bootm: Avoid 256-byte overflow in fixup_silent_linux()

[Mike Frysinger]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111019/d74723ed/attachment.pgp 

[U-Boot] [PATCH 3/4] bootm: Avoid 256-byte overflow in fixup_silent_linux()

[Wolfgang Denk]

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

-- 
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
The IQ of the group is the lowest IQ of a member of the group divided
by the number of people in the group.

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

[Doug Anderson]

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 */
 
-- 
1.7.2.3

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

[Wolfgang Denk]

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

-- 
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
"The good Christian should beware of mathematicians and all those who
make empty prophecies. The danger already exists that  mathematicians
have  made a covenant with the devil to darken the spirit and confine
man in the bonds of Hell."                          - Saint Augustine

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

[Doug Anderson]

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

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

[Mike Frysinger]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120110/6989d5e8/attachment.pgp>

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

[Mike Frysinger]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120110/f530cff5/attachment.pgp>

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

[Doug Anderson]

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 */
 
-- 
1.7.3.1

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

[Mike Frysinger]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120114/1fb0c1e1/attachment.pgp>

[U-Boot] [PATCH v3] bootm: Avoid 256-byte overflow in fixup_silent_linux()

[Doug Anderson]

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 */
 
-- 
1.7.7.3

[U-Boot] [PATCH v3] bootm: Avoid 256-byte overflow in fixup_silent_linux()

[Mike Frysinger]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120117/60ba1f41/attachment.pgp>

[U-Boot] [PATCH v3] bootm: Avoid 256-byte overflow in fixup_silent_linux()

[Doug Anderson]

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

[U-Boot] [PATCH v4] bootm: Avoid 256-byte overflow in fixup_silent_linux()

[Doug Anderson]

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 */
 
-- 
1.7.7.3

[U-Boot] [PATCH v4] bootm: Avoid 256-byte overflow in fixup_silent_linux()

[Mike Frysinger]

Acked-by: Mike Frysinger <vapier@gentoo.org>
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120117/4954ca8d/attachment.pgp>

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

[Tom Rini]

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!

-- 
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/20130522/cb2d8ab0/attachment.pgp>

[U-Boot] [PATCH 4/4] bootm: Add earlyprintk to fixup_silent_linux

[Doug Anderson]

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 */
-- 
1.7.3.1

[U-Boot] [PATCH 4/4] bootm: Add earlyprintk to fixup_silent_linux

[Mike Frysinger]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111019/a76319dc/attachment.pgp 

[U-Boot] [PATCH 4/4] bootm: Add earlyprintk to fixup_silent_linux

[Doug Anderson]

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/mmcblk0p3
> earlyprintk";
> > +     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

[U-Boot] [PATCH 4/4] bootm: Add earlyprintk to fixup_silent_linux

[Mike Frysinger]

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/mmcblk0p3
> > 
> > earlyprintk";
> > 
> > > +     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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111019/83085857/attachment.pgp 

[U-Boot] [PATCH 4/4] bootm: Add earlyprintk to fixup_silent_linux

[Wolfgang Denk]

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

-- 
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
On my planet, to rest is to rest -- to cease using energy. To me,  it
is  quite  illogical to run up and down on green grass, using energy,
instead of saving it.
	-- Spock, "Shore Leave", stardate 3025.2

[U-Boot] [PATCH 4/4] bootm: Add earlyprintk to fixup_silent_linux

[Doug Anderson]

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

[U-Boot] [PATCH 4/4] bootm: Add earlyprintk to fixup_silent_linux

[Wolfgang Denk]

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

-- 
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
CONSUMER NOTICE:  Because  of  the  "Uncertainty  Principle,"  It  Is
Impossible  for  the  Consumer  to  Find  Out  at  the Same Time Both
Precisely Where This Product Is and How Fast It Is Moving.