Patchwork powerpc/boot: compare _start against ei.loadsize instead ei.memsize

login
register
mail settings
Submitter Sebastian Siewior
Date Oct. 13, 2008, 9:15 a.m.
Message ID <20081013091526.GA9925@www.tglx.de>
Download mbox | patch
Permalink /patch/4150/
State Accepted
Commit c10c178a92b032ea3dd7259dcbbd1b9331c05c41
Delegated to: Benjamin Herrenschmidt
Headers show

Comments

Sebastian Siewior - Oct. 13, 2008, 9:15 a.m.
If the vmlinux binary in memory is larger than 4 MiB than it collides
with the initial boot code which is linked at 4 MiB in case of cuBoot.
If the the uncompressed image size (on disk size) is less than 4 MiB
then it would fit. The difference between those two sizes is the bss
section. In cuBoot we have the dtb embedded right after the data
section so it is very likely that the reset of the bss section (in
kernel's start up code) will overwrite the dtb blob. Therefore we
reallocate the dtb. Something similar is allready done to the initrd.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 arch/powerpc/boot/libfdt-wrapper.c |   16 ++++++++--------
 arch/powerpc/boot/main.c           |   14 ++++++++++++--
 2 files changed, 20 insertions(+), 10 deletions(-)
David Gibson - Oct. 14, 2008, 1:57 a.m.
On Mon, Oct 13, 2008 at 11:15:26AM +0200, Sebastian Andrzej Siewior wrote:
> If the vmlinux binary in memory is larger than 4 MiB than it collides
> with the initial boot code which is linked at 4 MiB in case of cuBoot.
> If the the uncompressed image size (on disk size) is less than 4 MiB
> then it would fit. The difference between those two sizes is the bss
> section. In cuBoot we have the dtb embedded right after the data
> section so it is very likely that the reset of the bss section (in
> kernel's start up code) will overwrite the dtb blob. Therefore we
> reallocate the dtb. Something similar is allready done to the initrd.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

This patch looks sound to me.  It does strike me as only a stopgap
until we come up with some better way of managing the early memory,
preferably avoiding the magical fixed 4MiB (or whatever) offset.

Still, it looks safe - since it does check that the wrapper's malloc()
region can't collide with the kernel's BSS.  So, if this little slice
of extra BSS room is useful for the time being, I see no reason not to
apply the patch.

Acked-by: David Gibson <david@gibson.dropbear.id.au>
David Gibson - Oct. 14, 2008, 2 a.m.
On Tue, Oct 14, 2008 at 12:57:53PM +1100, David Gibson wrote:
> On Mon, Oct 13, 2008 at 11:15:26AM +0200, Sebastian Andrzej Siewior wrote:
> > If the vmlinux binary in memory is larger than 4 MiB than it collides
> > with the initial boot code which is linked at 4 MiB in case of cuBoot.
> > If the the uncompressed image size (on disk size) is less than 4 MiB
> > then it would fit. The difference between those two sizes is the bss
> > section. In cuBoot we have the dtb embedded right after the data
> > section so it is very likely that the reset of the bss section (in
> > kernel's start up code) will overwrite the dtb blob. Therefore we
> > reallocate the dtb. Something similar is allready done to the initrd.
> > 
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> 
> This patch looks sound to me.  It does strike me as only a stopgap
> until we come up with some better way of managing the early memory,
> preferably avoiding the magical fixed 4MiB (or whatever) offset.
> 
> Still, it looks safe - since it does check that the wrapper's malloc()
> region can't collide with the kernel's BSS.  So, if this little slice
> of extra BSS room is useful for the time being, I see no reason not to
> apply the patch.
> 
> Acked-by: David Gibson <david@gibson.dropbear.id.au>

Oh, one other thing.  Since we are now unconditionally copying the dtb
into a malloc()ed buffer, possibly it would be sensible to add a
little padding to the buffer at that point, so that further device
tree manipulations won't need to reallocate it.

Patch

diff --git a/arch/powerpc/boot/libfdt-wrapper.c b/arch/powerpc/boot/libfdt-wrapper.c
index c541fd8..1daa73f 100644
--- a/arch/powerpc/boot/libfdt-wrapper.c
+++ b/arch/powerpc/boot/libfdt-wrapper.c
@@ -165,6 +165,7 @@  static unsigned long fdt_wrapper_finalize(void)
 void fdt_init(void *blob)
 {
 	int err;
+	int bufsize;
 
 	dt_ops.finddevice = fdt_wrapper_finddevice;
 	dt_ops.getprop = fdt_wrapper_getprop;
@@ -178,16 +179,15 @@  void fdt_init(void *blob)
 
 	/* Make sure the dt blob is the right version and so forth */
 	fdt = blob;
-	err = fdt_open_into(fdt, fdt, fdt_totalsize(blob));
-	if (err == -FDT_ERR_NOSPACE) {
-		int bufsize = fdt_totalsize(fdt) + 4;
-		buf = malloc(bufsize);
-		err = fdt_open_into(fdt, buf, bufsize);
-	}
+	bufsize = fdt_totalsize(fdt) + 4;
+	buf = malloc(bufsize);
+	if(!buf)
+		fatal("malloc failed. can't relocate the device tree\n\r");
+
+	err = fdt_open_into(fdt, buf, bufsize);
 
 	if (err != 0)
 		fatal("fdt_init(): %s\n\r", fdt_strerror(err));
 
-	if (buf)
-		fdt = buf;
+	fdt = buf;
 }
diff --git a/arch/powerpc/boot/main.c b/arch/powerpc/boot/main.c
index 9e7f3dd..ae32801 100644
--- a/arch/powerpc/boot/main.c
+++ b/arch/powerpc/boot/main.c
@@ -56,9 +56,19 @@  static struct addr_range prep_kernel(void)
 	if (platform_ops.vmlinux_alloc) {
 		addr = platform_ops.vmlinux_alloc(ei.memsize);
 	} else {
-		if ((unsigned long)_start < ei.memsize)
+		/*
+		 * Check if the kernel image (without bss) would overwrite the
+		 * bootwrapper. The device tree has been moved in fdt_init()
+		 * to an area allocated with malloc() (somewhere past _end).
+		 */
+		if ((unsigned long)_start < ei.loadsize)
 			fatal("Insufficient memory for kernel at address 0!"
-			       " (_start=%p)\n\r", _start);
+			       " (_start=%p, uncomressed size=%08x)\n\r",
+			       _start, ei.loadsize);
+
+		if ((unsigned long)_end < ei.memsize)
+			fatal("The final kernel image would overwrite the "
+					"device tree\n\r");
 	}
 
 	/* Finally, gunzip the kernel */