Re: [PATCH] ARM: zImage: Fix stack overflow in merge_fdt_bootargs()

[Rask Ingemann Lambertsen <rask@formelder.dk>]

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