Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751582AbdGZOEA (ORCPT ); Wed, 26 Jul 2017 10:04:00 -0400 Received: from mail-oi0-f66.google.com ([209.85.218.66]:36191 "EHLO mail-oi0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751421AbdGZOD4 (ORCPT ); Wed, 26 Jul 2017 10:03:56 -0400 MIME-Version: 1.0 In-Reply-To: References: From: Richard Genoud Date: Wed, 26 Jul 2017 16:03:34 +0200 Message-ID: Subject: Re: [PATCH v2] ARM: zImage: Fix stack overflow in merge_fdt_bootargs() To: Rask Ingemann Lambertsen Cc: Russell King , Pavel Machek , Sebastian Reichel , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by nfs id v6QE44E5031566 Content-Length: 5283 Lines: 134 2017-07-22 15:02 GMT+02:00 Rask Ingemann Lambertsen : > 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 or 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 > Tested-by: Pavel Machek > Tested-by: Sebastian Reichel > Fixes: d0f34a11ddab ("ARM: 7437/1: zImage: Allow DTB command line concatenation with ATAG_CMDLINE") seems good to me ! Reviewed-by: Richard Genoud > --- > > 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. > > Changes from v1 to v2: > The original "bootargs" NUL terminator is changed into a space only when > appenprop_string() succeeds. Commented by Richard Genoud. > > arch/arm/boot/compressed/atags_to_fdt.c | 59 +++++++++++++++++++++------------ > 1 file changed, 37 insertions(+), 22 deletions(-) > > diff --git a/arch/arm/boot/compressed/atags_to_fdt.c b/arch/arm/boot/compressed/atags_to_fdt.c > index 9448aa0c6686..87a5fba5a28e 100644 > --- a/arch/arm/boot/compressed/atags_to_fdt.c > +++ b/arch/arm/boot/compressed/atags_to_fdt.c > @@ -55,6 +55,29 @@ static const void *getprop(const void *fdt, const char *node_path, > return fdt_getprop(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 int setprop_inplace_partial(void *fdt, const char *node_path, > + const char *property, unsigned int idx, > + const void *val, int len) > +{ > + int offset = node_offset(fdt, node_path); > + > + if (offset < 0) > + return offset; > + return fdt_setprop_inplace_namelen_partial(fdt, offset, property, > + strlen(property), idx, > + val, len); > +} > + > static uint32_t get_cell_size(const void *fdt) > { > int len; > @@ -66,35 +89,27 @@ 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) > +{ > + const 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(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; > + } > + if (appendprop_string(fdt, "/chosen", "bootargs", atag_cmdline) < 0) > + return; > + /* Replace the previous NUL terminator with a space. */ > + setprop_inplace_partial(fdt, "/chosen", "bootargs", len - 1, " ", 1); > } > > /* > -- > 2.13.2 > Thanks !