linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] ARM: zImage: Fix stack overflow in merge_fdt_bootargs()
@ 2017-07-16 21:43 Rask Ingemann Lambertsen
  2017-07-18  7:39 ` Richard Genoud
  2017-07-19  8:15 ` [v4.13 regression] " Pavel Machek
  0 siblings, 2 replies; 6+ messages in thread
From: Rask Ingemann Lambertsen @ 2017-07-16 21:43 UTC (permalink / raw)
  To: Russell King; +Cc: Richard Genoud, linux-arm-kernel, linux-kernel

This function is called very early on from head.S and currently sets up a
stack frame of more than 1024 bytes:

atags_to_fdt.c: In function ‘merge_fdt_bootargs’:
atags_to_fdt.c:98:1: warning: the frame size of 1032 bytes is larger than 1024 bytes [-Wframe-larger-than=]

This causes a crash and failure to boot with some combinations of kernel
version, gcc version and dtb, such as kernel version 4.1-rc1 of 4.1.0,
gcc version 5.4.1 20161019 (Debian 5.4.1-3) and tegra20-trimslice.dtb.

With this patch, merge_fdt_bootargs() is rewritten to not use a large buffer,
thereby solving the problem of the stack overflow.

As a side effect of this rewrite, you no longer get a space added in front
of the kernel command line when no bootargs property was found in the fdt.

Signed-off-by: Rask Ingemann Lambertsen <rask@formelder.dk>
Fixes: d0f34a11ddab ("ARM: 7437/1: zImage: Allow DTB command line concatenation with ATAG_CMDLINE")
---

I have tested that this works properly when
a) only the FDT bootargs are provided,
b) only the ATAG command line is provided, and
c) both the FDT bootargs and the ATAG command line are provided.

 arch/arm/boot/compressed/atags_to_fdt.c | 59 +++++++++++++++++++--------------
 1 file changed, 35 insertions(+), 24 deletions(-)

diff --git a/arch/arm/boot/compressed/atags_to_fdt.c b/arch/arm/boot/compressed/atags_to_fdt.c
index 9448aa0c6686..ede23ef2e889 100644
--- a/arch/arm/boot/compressed/atags_to_fdt.c
+++ b/arch/arm/boot/compressed/atags_to_fdt.c
@@ -55,6 +55,27 @@ static const void *getprop(const void *fdt, const char *node_path,
 	return fdt_getprop(fdt, offset, property, len);
 }
 
+static void *getprop_w(void *fdt, const char *node_path,
+		       const char *property, int *len)
+{
+	int offset = fdt_path_offset(fdt, node_path);
+
+	if (offset == -FDT_ERR_NOTFOUND)
+		return NULL;
+
+	return fdt_getprop_w(fdt, offset, property, len);
+}
+
+static int appendprop_string(void *fdt, const char *node_path,
+			     const char *property, const char *string)
+{
+	int offset = node_offset(fdt, node_path);
+
+	if (offset < 0)
+		return offset;
+	return fdt_appendprop_string(fdt, offset, property, string);
+}
+
 static uint32_t get_cell_size(const void *fdt)
 {
 	int len;
@@ -66,35 +87,25 @@ static uint32_t get_cell_size(const void *fdt)
 	return cell_size;
 }
 
-static void merge_fdt_bootargs(void *fdt, const char *fdt_cmdline)
-{
-	char cmdline[COMMAND_LINE_SIZE];
-	const char *fdt_bootargs;
-	char *ptr = cmdline;
-	int len = 0;
-
-	/* copy the fdt command line into the buffer */
-	fdt_bootargs = getprop(fdt, "/chosen", "bootargs", &len);
-	if (fdt_bootargs)
-		if (len < COMMAND_LINE_SIZE) {
-			memcpy(ptr, fdt_bootargs, len);
-			/* len is the length of the string
-			 * including the NULL terminator */
-			ptr += len - 1;
-		}
-
-	/* and append the ATAG_CMDLINE */
-	if (fdt_cmdline) {
-		len = strlen(fdt_cmdline);
-		if (ptr - cmdline + len + 2 < COMMAND_LINE_SIZE) {
-			*ptr++ = ' ';
-			memcpy(ptr, fdt_cmdline, len);
-			ptr += len;
-		}
-	}
-	*ptr = '\0';
-
-	setprop_string(fdt, "/chosen", "bootargs", cmdline);
+/* This is called early on from head.S, so it can't use much stack. */
+static void merge_fdt_bootargs(void *fdt, const char *atag_cmdline)
+{
+	char *fdt_bootargs;
+	int len = 0;
+
+	/* With no ATAG command line or an empty one, there is nothing to do. */
+	if (!atag_cmdline || strlen(atag_cmdline) == 0)
+		return;
+
+	fdt_bootargs = getprop_w(fdt, "/chosen", "bootargs", &len);
+
+	/* With no FDT command line or an empty one, just use the ATAG one. */
+	if (!fdt_bootargs || len <= 1) {
+		setprop_string(fdt, "/chosen", "bootargs", atag_cmdline);
+		return;
+	}
+	fdt_bootargs[len - 1] = ' ';
+	appendprop_string(fdt, "/chosen", "bootargs", atag_cmdline);
 }
 
 /*
-- 
2.13.2

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] ARM: zImage: Fix stack overflow in merge_fdt_bootargs()
  2017-07-16 21:43 [PATCH] ARM: zImage: Fix stack overflow in merge_fdt_bootargs() Rask Ingemann Lambertsen
@ 2017-07-18  7:39 ` Richard Genoud
  2017-07-18 21:40   ` Rask Ingemann Lambertsen
  2017-07-19  8:15 ` [v4.13 regression] " Pavel Machek
  1 sibling, 1 reply; 6+ messages in thread
From: Richard Genoud @ 2017-07-18  7:39 UTC (permalink / raw)
  To: Rask Ingemann Lambertsen, Russell King; +Cc: linux-arm-kernel, linux-kernel

On 16/07/2017 23:43, Rask Ingemann Lambertsen wrote:
> This function is called very early on from head.S and currently sets up a
> stack frame of more than 1024 bytes:
> 
> atags_to_fdt.c: In function ‘merge_fdt_bootargs’:
> atags_to_fdt.c:98:1: warning: the frame size of 1032 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> 
> This causes a crash and failure to boot with some combinations of kernel
> version, gcc version and dtb, such as kernel version 4.1-rc1 of 4.1.0,
> gcc version 5.4.1 20161019 (Debian 5.4.1-3) and tegra20-trimslice.dtb.
> 
> With this patch, merge_fdt_bootargs() is rewritten to not use a large buffer,
> thereby solving the problem of the stack overflow.
> 
> As a side effect of this rewrite, you no longer get a space added in front
> of the kernel command line when no bootargs property was found in the fdt.
> 
> Signed-off-by: Rask Ingemann Lambertsen <rask@formelder.dk>
> Fixes: d0f34a11ddab ("ARM: 7437/1: zImage: Allow DTB command line concatenation with ATAG_CMDLINE")
> ---
> 
> I have tested that this works properly when
> a) only the FDT bootargs are provided,
> b) only the ATAG command line is provided, and
> c) both the FDT bootargs and the ATAG command line are provided.
> 
>  arch/arm/boot/compressed/atags_to_fdt.c | 59 +++++++++++++++++++--------------
>  1 file changed, 35 insertions(+), 24 deletions(-)

Thanks for fixing that !

> 
> diff --git a/arch/arm/boot/compressed/atags_to_fdt.c b/arch/arm/boot/compressed/atags_to_fdt.c
> index 9448aa0c6686..ede23ef2e889 100644
> --- a/arch/arm/boot/compressed/atags_to_fdt.c
> +++ b/arch/arm/boot/compressed/atags_to_fdt.c
> @@ -55,6 +55,27 @@ static const void *getprop(const void *fdt, const char *node_path,
>  	return fdt_getprop(fdt, offset, property, len);
>  }
>  
> +static void *getprop_w(void *fdt, const char *node_path,
> +		       const char *property, int *len)
> +{
> +	int offset = fdt_path_offset(fdt, node_path);
> +
> +	if (offset == -FDT_ERR_NOTFOUND)
> +		return NULL;
> +
> +	return fdt_getprop_w(fdt, offset, property, len);
> +}
> +
> +static int appendprop_string(void *fdt, const char *node_path,
> +			     const char *property, const char *string)
> +{
> +	int offset = node_offset(fdt, node_path);
> +
> +	if (offset < 0)
> +		return offset;
> +	return fdt_appendprop_string(fdt, offset, property, string);
> +}
> +
>  static uint32_t get_cell_size(const void *fdt)
>  {
>  	int len;
> @@ -66,35 +87,25 @@ static uint32_t get_cell_size(const void *fdt)
>  	return cell_size;
>  }
>  
> -static void merge_fdt_bootargs(void *fdt, const char *fdt_cmdline)
> -{
> -	char cmdline[COMMAND_LINE_SIZE];
> -	const char *fdt_bootargs;
> -	char *ptr = cmdline;
> -	int len = 0;
> -
> -	/* copy the fdt command line into the buffer */
> -	fdt_bootargs = getprop(fdt, "/chosen", "bootargs", &len);
> -	if (fdt_bootargs)
> -		if (len < COMMAND_LINE_SIZE) {
> -			memcpy(ptr, fdt_bootargs, len);
> -			/* len is the length of the string
> -			 * including the NULL terminator */
> -			ptr += len - 1;
> -		}
> -
> -	/* and append the ATAG_CMDLINE */
> -	if (fdt_cmdline) {
> -		len = strlen(fdt_cmdline);
> -		if (ptr - cmdline + len + 2 < COMMAND_LINE_SIZE) {
> -			*ptr++ = ' ';
> -			memcpy(ptr, fdt_cmdline, len);
> -			ptr += len;
> -		}
> -	}
> -	*ptr = '\0';
> -
> -	setprop_string(fdt, "/chosen", "bootargs", cmdline);
> +/* This is called early on from head.S, so it can't use much stack. */
> +static void merge_fdt_bootargs(void *fdt, const char *atag_cmdline)
> +{
> +	char *fdt_bootargs;
> +	int len = 0;
> +
> +	/* With no ATAG command line or an empty one, there is nothing to do. */
> +	if (!atag_cmdline || strlen(atag_cmdline) == 0)
> +		return;
> +
> +	fdt_bootargs = getprop_w(fdt, "/chosen", "bootargs", &len);
> +
> +	/* With no FDT command line or an empty one, just use the ATAG one. */
> +	if (!fdt_bootargs || len <= 1) {
> +		setprop_string(fdt, "/chosen", "bootargs", atag_cmdline);
> +		return;
> +	}
> +	fdt_bootargs[len - 1] = ' ';
> +	appendprop_string(fdt, "/chosen", "bootargs", atag_cmdline);
Let's say appendprop_string() fails for whatever reason, the /chosen
string won't be null terminated anymore.
Won't it cause some problems ?

>  }
>  
>  /*
> 

Richard.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] ARM: zImage: Fix stack overflow in merge_fdt_bootargs()
  2017-07-18  7:39 ` Richard Genoud
@ 2017-07-18 21:40   ` Rask Ingemann Lambertsen
  0 siblings, 0 replies; 6+ messages in thread
From: Rask Ingemann Lambertsen @ 2017-07-18 21:40 UTC (permalink / raw)
  To: Richard Genoud; +Cc: Russell King, linux-kernel, linux-arm-kernel

On Tue, Jul 18, 2017 at 09:39:10AM +0200, Richard Genoud wrote:
> On 16/07/2017 23:43, Rask Ingemann Lambertsen wrote:
[snip]
> > +/* This is called early on from head.S, so it can't use much stack. */
> > +static void merge_fdt_bootargs(void *fdt, const char *atag_cmdline)
> > +{
> > +	char *fdt_bootargs;
> > +	int len = 0;
> > +
> > +	/* With no ATAG command line or an empty one, there is nothing to do. */
> > +	if (!atag_cmdline || strlen(atag_cmdline) == 0)
> > +		return;
> > +
> > +	fdt_bootargs = getprop_w(fdt, "/chosen", "bootargs", &len);
> > +
> > +	/* With no FDT command line or an empty one, just use the ATAG one. */
> > +	if (!fdt_bootargs || len <= 1) {
> > +		setprop_string(fdt, "/chosen", "bootargs", atag_cmdline);
> > +		return;
> > +	}
> > +	fdt_bootargs[len - 1] = ' ';
> > +	appendprop_string(fdt, "/chosen", "bootargs", atag_cmdline);
> Let's say appendprop_string() fails for whatever reason, the /chosen
> string won't be null terminated anymore.

Good catch, I will fix that. Thank you for your review.

> Won't it cause some problems ?

It's OK at the moment because the last character of the "bootargs" property
is not used. This is the code from early_init_dt_scan_chosen() in
drivers/of/fdt.c:

	/* Retrieve command line */
	p = of_get_flat_dt_prop(node, "bootargs", &l);
	if (p != NULL && l > 0)
		strlcpy(data, p, min((int)l, COMMAND_LINE_SIZE));

But not all accesses are that careful. I found get_cmdline() in
arch/arm64/kernel/kaslr.c using

		prop = fdt_getprop(fdt, node, "bootargs", NULL);

so there is an expectation that the string is NUL terminated.

-- 
Rask Ingemann Lambertsen

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [v4.13 regression] ARM: zImage: Fix stack overflow in merge_fdt_bootargs()
  2017-07-16 21:43 [PATCH] ARM: zImage: Fix stack overflow in merge_fdt_bootargs() Rask Ingemann Lambertsen
  2017-07-18  7:39 ` Richard Genoud
@ 2017-07-19  8:15 ` Pavel Machek
  2017-07-19  8:47   ` Sebastian Reichel
  1 sibling, 1 reply; 6+ messages in thread
From: Pavel Machek @ 2017-07-19  8:15 UTC (permalink / raw)
  To: Rask Ingemann Lambertsen
  Cc: Russell King, Richard Genoud, linux-arm-kernel, linux-kernel,
	nico, gregory.clement, Linus Torvalds

[-- Attachment #1: Type: text/plain, Size: 4255 bytes --]

Hi!

> This function is called very early on from head.S and currently sets up a
> stack frame of more than 1024 bytes:
> 
> atags_to_fdt.c: In function ‘merge_fdt_bootargs’:
> atags_to_fdt.c:98:1: warning: the frame size of 1032 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> 
> This causes a crash and failure to boot with some combinations of kernel
> version, gcc version and dtb, such as kernel version 4.1-rc1 of 4.1.0,
> gcc version 5.4.1 20161019 (Debian 5.4.1-3) and tegra20-trimslice.dtb.

> 
> Signed-off-by: Rask Ingemann Lambertsen <rask@formelder.dk>
> Fixes: d0f34a11ddab ("ARM: 7437/1: zImage: Allow DTB command line concatenation with ATAG_CMDLINE")

I tested that it does not break boot on N900. I hoped that it would
fix boot on N950 (but no luck there so far).

Tested-by: Pavel Machek <pavel@ucw.cz>

AFAICT this is regression in v4.13-rc1. Thus it is quite surprising
that there are no comments here.

Genoud? Russell?

								Pavel


> 
> I have tested that this works properly when
> a) only the FDT bootargs are provided,
> b) only the ATAG command line is provided, and
> c) both the FDT bootargs and the ATAG command line are provided.
> 
>  arch/arm/boot/compressed/atags_to_fdt.c | 59 +++++++++++++++++++--------------
>  1 file changed, 35 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/arm/boot/compressed/atags_to_fdt.c b/arch/arm/boot/compressed/atags_to_fdt.c
> index 9448aa0c6686..ede23ef2e889 100644
> --- a/arch/arm/boot/compressed/atags_to_fdt.c
> +++ b/arch/arm/boot/compressed/atags_to_fdt.c
> @@ -55,6 +55,27 @@ static const void *getprop(const void *fdt, const char *node_path,
>  	return fdt_getprop(fdt, offset, property, len);
>  }
>  
> +static void *getprop_w(void *fdt, const char *node_path,
> +		       const char *property, int *len)
> +{
> +	int offset = fdt_path_offset(fdt, node_path);
> +
> +	if (offset == -FDT_ERR_NOTFOUND)
> +		return NULL;
> +
> +	return fdt_getprop_w(fdt, offset, property, len);
> +}
> +
> +static int appendprop_string(void *fdt, const char *node_path,
> +			     const char *property, const char *string)
> +{
> +	int offset = node_offset(fdt, node_path);
> +
> +	if (offset < 0)
> +		return offset;
> +	return fdt_appendprop_string(fdt, offset, property, string);
> +}
> +
>  static uint32_t get_cell_size(const void *fdt)
>  {
>  	int len;
> @@ -66,35 +87,25 @@ static uint32_t get_cell_size(const void *fdt)
>  	return cell_size;
>  }
>  
> -static void merge_fdt_bootargs(void *fdt, const char *fdt_cmdline)
> -{
> -	char cmdline[COMMAND_LINE_SIZE];
> -	const char *fdt_bootargs;
> -	char *ptr = cmdline;
> -	int len = 0;
> -
> -	/* copy the fdt command line into the buffer */
> -	fdt_bootargs = getprop(fdt, "/chosen", "bootargs", &len);
> -	if (fdt_bootargs)
> -		if (len < COMMAND_LINE_SIZE) {
> -			memcpy(ptr, fdt_bootargs, len);
> -			/* len is the length of the string
> -			 * including the NULL terminator */
> -			ptr += len - 1;
> -		}
> -
> -	/* and append the ATAG_CMDLINE */
> -	if (fdt_cmdline) {
> -		len = strlen(fdt_cmdline);
> -		if (ptr - cmdline + len + 2 < COMMAND_LINE_SIZE) {
> -			*ptr++ = ' ';
> -			memcpy(ptr, fdt_cmdline, len);
> -			ptr += len;
> -		}
> -	}
> -	*ptr = '\0';
> -
> -	setprop_string(fdt, "/chosen", "bootargs", cmdline);
> +/* This is called early on from head.S, so it can't use much stack. */
> +static void merge_fdt_bootargs(void *fdt, const char *atag_cmdline)
> +{
> +	char *fdt_bootargs;
> +	int len = 0;
> +
> +	/* With no ATAG command line or an empty one, there is nothing to do. */
> +	if (!atag_cmdline || strlen(atag_cmdline) == 0)
> +		return;
> +
> +	fdt_bootargs = getprop_w(fdt, "/chosen", "bootargs", &len);
> +
> +	/* With no FDT command line or an empty one, just use the ATAG one. */
> +	if (!fdt_bootargs || len <= 1) {
> +		setprop_string(fdt, "/chosen", "bootargs", atag_cmdline);
> +		return;
> +	}
> +	fdt_bootargs[len - 1] = ' ';
> +	appendprop_string(fdt, "/chosen", "bootargs", atag_cmdline);
>  }
>  
>  /*

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [v4.13 regression] ARM: zImage: Fix stack overflow in merge_fdt_bootargs()
  2017-07-19  8:15 ` [v4.13 regression] " Pavel Machek
@ 2017-07-19  8:47   ` Sebastian Reichel
  2017-07-19  9:12     ` Sebastian Reichel
  0 siblings, 1 reply; 6+ messages in thread
From: Sebastian Reichel @ 2017-07-19  8:47 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Rask Ingemann Lambertsen, Russell King, Richard Genoud,
	linux-arm-kernel, linux-kernel, nico, gregory.clement,
	Linus Torvalds

[-- Attachment #1: Type: text/plain, Size: 4873 bytes --]

Hi,

On Wed, Jul 19, 2017 at 10:15:36AM +0200, Pavel Machek wrote:
> > This function is called very early on from head.S and currently sets up a
> > stack frame of more than 1024 bytes:
> > 
> > atags_to_fdt.c: In function ‘merge_fdt_bootargs’:
> > atags_to_fdt.c:98:1: warning: the frame size of 1032 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> > 
> > This causes a crash and failure to boot with some combinations of kernel
> > version, gcc version and dtb, such as kernel version 4.1-rc1 of 4.1.0,
> > gcc version 5.4.1 20161019 (Debian 5.4.1-3) and tegra20-trimslice.dtb.
> 
> > 
> > Signed-off-by: Rask Ingemann Lambertsen <rask@formelder.dk>
> > Fixes: d0f34a11ddab ("ARM: 7437/1: zImage: Allow DTB command line concatenation with ATAG_CMDLINE")
> 
> I tested that it does not break boot on N900. I hoped that it would
> fix boot on N950 (but no luck there so far).
> 
> Tested-by: Pavel Machek <pavel@ucw.cz>
> 
> AFAICT this is regression in v4.13-rc1. Thus it is quite surprising
> that there are no comments here.

No, it's not. I definitely saw the warning before (and see it now)
and images can be booted on all of the phones I'm working on (i.e.
N900, N950, Droid 4).

Anyawys thanks for fixing this properly. I will add the patch to my
dev branch and give it a test during the day.

-- Sebastian

> 
> Genoud? Russell?
> 
> 								Pavel
> 
> 
> > 
> > I have tested that this works properly when
> > a) only the FDT bootargs are provided,
> > b) only the ATAG command line is provided, and
> > c) both the FDT bootargs and the ATAG command line are provided.
> > 
> >  arch/arm/boot/compressed/atags_to_fdt.c | 59 +++++++++++++++++++--------------
> >  1 file changed, 35 insertions(+), 24 deletions(-)
> > 
> > diff --git a/arch/arm/boot/compressed/atags_to_fdt.c b/arch/arm/boot/compressed/atags_to_fdt.c
> > index 9448aa0c6686..ede23ef2e889 100644
> > --- a/arch/arm/boot/compressed/atags_to_fdt.c
> > +++ b/arch/arm/boot/compressed/atags_to_fdt.c
> > @@ -55,6 +55,27 @@ static const void *getprop(const void *fdt, const char *node_path,
> >  	return fdt_getprop(fdt, offset, property, len);
> >  }
> >  
> > +static void *getprop_w(void *fdt, const char *node_path,
> > +		       const char *property, int *len)
> > +{
> > +	int offset = fdt_path_offset(fdt, node_path);
> > +
> > +	if (offset == -FDT_ERR_NOTFOUND)
> > +		return NULL;
> > +
> > +	return fdt_getprop_w(fdt, offset, property, len);
> > +}
> > +
> > +static int appendprop_string(void *fdt, const char *node_path,
> > +			     const char *property, const char *string)
> > +{
> > +	int offset = node_offset(fdt, node_path);
> > +
> > +	if (offset < 0)
> > +		return offset;
> > +	return fdt_appendprop_string(fdt, offset, property, string);
> > +}
> > +
> >  static uint32_t get_cell_size(const void *fdt)
> >  {
> >  	int len;
> > @@ -66,35 +87,25 @@ static uint32_t get_cell_size(const void *fdt)
> >  	return cell_size;
> >  }
> >  
> > -static void merge_fdt_bootargs(void *fdt, const char *fdt_cmdline)
> > -{
> > -	char cmdline[COMMAND_LINE_SIZE];
> > -	const char *fdt_bootargs;
> > -	char *ptr = cmdline;
> > -	int len = 0;
> > -
> > -	/* copy the fdt command line into the buffer */
> > -	fdt_bootargs = getprop(fdt, "/chosen", "bootargs", &len);
> > -	if (fdt_bootargs)
> > -		if (len < COMMAND_LINE_SIZE) {
> > -			memcpy(ptr, fdt_bootargs, len);
> > -			/* len is the length of the string
> > -			 * including the NULL terminator */
> > -			ptr += len - 1;
> > -		}
> > -
> > -	/* and append the ATAG_CMDLINE */
> > -	if (fdt_cmdline) {
> > -		len = strlen(fdt_cmdline);
> > -		if (ptr - cmdline + len + 2 < COMMAND_LINE_SIZE) {
> > -			*ptr++ = ' ';
> > -			memcpy(ptr, fdt_cmdline, len);
> > -			ptr += len;
> > -		}
> > -	}
> > -	*ptr = '\0';
> > -
> > -	setprop_string(fdt, "/chosen", "bootargs", cmdline);
> > +/* This is called early on from head.S, so it can't use much stack. */
> > +static void merge_fdt_bootargs(void *fdt, const char *atag_cmdline)
> > +{
> > +	char *fdt_bootargs;
> > +	int len = 0;
> > +
> > +	/* With no ATAG command line or an empty one, there is nothing to do. */
> > +	if (!atag_cmdline || strlen(atag_cmdline) == 0)
> > +		return;
> > +
> > +	fdt_bootargs = getprop_w(fdt, "/chosen", "bootargs", &len);
> > +
> > +	/* With no FDT command line or an empty one, just use the ATAG one. */
> > +	if (!fdt_bootargs || len <= 1) {
> > +		setprop_string(fdt, "/chosen", "bootargs", atag_cmdline);
> > +		return;
> > +	}
> > +	fdt_bootargs[len - 1] = ' ';
> > +	appendprop_string(fdt, "/chosen", "bootargs", atag_cmdline);
> >  }
> >  
> >  /*
> 
> -- 
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [v4.13 regression] ARM: zImage: Fix stack overflow in merge_fdt_bootargs()
  2017-07-19  8:47   ` Sebastian Reichel
@ 2017-07-19  9:12     ` Sebastian Reichel
  0 siblings, 0 replies; 6+ messages in thread
From: Sebastian Reichel @ 2017-07-19  9:12 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Rask Ingemann Lambertsen, Russell King, Richard Genoud,
	linux-arm-kernel, linux-kernel, nico, gregory.clement,
	Linus Torvalds

[-- Attachment #1: Type: text/plain, Size: 1578 bytes --]

Hi,

On Wed, Jul 19, 2017 at 10:47:27AM +0200, Sebastian Reichel wrote:
> On Wed, Jul 19, 2017 at 10:15:36AM +0200, Pavel Machek wrote:
> > > This function is called very early on from head.S and currently sets up a
> > > stack frame of more than 1024 bytes:
> > > 
> > > atags_to_fdt.c: In function ‘merge_fdt_bootargs’:
> > > atags_to_fdt.c:98:1: warning: the frame size of 1032 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> > > 
> > > This causes a crash and failure to boot with some combinations of kernel
> > > version, gcc version and dtb, such as kernel version 4.1-rc1 of 4.1.0,
> > > gcc version 5.4.1 20161019 (Debian 5.4.1-3) and tegra20-trimslice.dtb.
> > 
> > > 
> > > Signed-off-by: Rask Ingemann Lambertsen <rask@formelder.dk>
> > > Fixes: d0f34a11ddab ("ARM: 7437/1: zImage: Allow DTB command line concatenation with ATAG_CMDLINE")
> > 
> > I tested that it does not break boot on N900. I hoped that it would
> > fix boot on N950 (but no luck there so far).
> > 
> > Tested-by: Pavel Machek <pavel@ucw.cz>
> > 
> > AFAICT this is regression in v4.13-rc1. Thus it is quite surprising
> > that there are no comments here.
> 
> No, it's not. I definitely saw the warning before (and see it now)
> and images can be booted on all of the phones I'm working on (i.e.
> N900, N950, Droid 4).
> 
> Anyawys thanks for fixing this properly. I will add the patch to my
> dev branch and give it a test during the day.

It does work for me.

Tested-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2017-07-19  9:12 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-16 21:43 [PATCH] ARM: zImage: Fix stack overflow in merge_fdt_bootargs() Rask Ingemann Lambertsen
2017-07-18  7:39 ` Richard Genoud
2017-07-18 21:40   ` Rask Ingemann Lambertsen
2017-07-19  8:15 ` [v4.13 regression] " Pavel Machek
2017-07-19  8:47   ` Sebastian Reichel
2017-07-19  9:12     ` Sebastian Reichel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).