[PATCH] bootinfo_load.c: stop parsing CHRP boot script when NULL is reached

[Mark Cave-Ayland]

The Mac OS 9 CHRP boot script consists of a null-terminated Forth string
followed by a large binary payload. Make sure we correctly determine the size
of the bootscript at this point instead of trying to allocate memory for the
entire binary blob which fails due to insufficient memory.

Signed-off-by: Mark Cave-Ayland <[e-mail]>
---
 openbios-devel/libopenbios/bootinfo_load.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/openbios-devel/libopenbios/bootinfo_load.c b/openbios-devel/libopenbios/bootinfo_load.c
index fa9e36b..c70203e 100644
--- a/openbios-devel/libopenbios/bootinfo_load.c
+++ b/openbios-devel/libopenbios/bootinfo_load.c
@@ -161,6 +161,12 @@ bootinfo_init_program(void)
 	feval("load-size");
 	size = POP();
 
+	/* Some bootinfo scripts contain a binary payload after the
+	   NULL-terminated Forth string such as OS 9. Restrict our
+	   size to just the Forth section, otherwise we end up trying
+	   to allocate memory for the entire binary which will fail. */
+	size = MIN(strlen(base), size);
+
 	bootscript = malloc(size);
 	if (bootscript == NULL) {
 		DPRINTF("Can't malloc %d bytes\n", size);
-- 
1.7.10.4

[PATCH] bootinfo_load.c: stop parsing CHRP boot script when NULL is reached

[Segher Boessenkool]

On Sat, Jun 20, 2015 at 11:07:01PM +0100, Mark Cave-Ayland wrote:
> +	/* Some bootinfo scripts contain a binary payload after the
> +	   NULL-terminated Forth string such as OS 9. Restrict our
> +	   size to just the Forth section, otherwise we end up trying
> +	   to allocate memory for the entire binary which will fail. */

"Might fail"?
> +	size = MIN(strlen(base), size);

"MIN" doesn't make terribly much sense here -- if "size" is the smaller
of the two, strlen() will have done out-of-bounds accesses, and if not,
you don't need MIN.  Use strnlen() isntead?  I.e.

	size = strnlen(base, size);


Segher

[PATCH] bootinfo_load.c: stop parsing CHRP boot script when NULL is reached

[Mark Cave-Ayland]

On 21/06/15 00:44, Segher Boessenkool wrote:
> On Sat, Jun 20, 2015 at 11:07:01PM +0100, Mark Cave-Ayland wrote:
> > +	/* Some bootinfo scripts contain a binary payload after the
> > +	   NULL-terminated Forth string such as OS 9. Restrict our
> > +	   size to just the Forth section, otherwise we end up trying
> > +	   to allocate memory for the entire binary which will fail. */
>
> "Might fail"?

It depends on how big your binary is. A few 10s of K will be fine,
something that is 2MB+ like OS 9 obviously isn't.
> > +	size = MIN(strlen(base), size);
>
> "MIN" doesn't make terribly much sense here -- if "size" is the smaller
> of the two, strlen() will have done out-of-bounds accesses, and if not,
> you don't need MIN.  Use strnlen() isntead?  I.e.
>
> size = strnlen(base, size);

For some reason the PPC OFMEM bindings will automatically allocate a
phys == virt mapping for any unknown address (that's a whole different
story though). Hence since the loader has already loaded a binary of
size bytes at load-base, this entire region is guaranteed to be accessible.

At some point though, I would like to change this behaviour to map a
fixed (8M) area at load-base instead as per the official specification.
Fortunately it appears that OpenBIOS does have a strnlen() in its mini
libc so I'll use that instead.


ATB,

Mark.