[U-Boot-Users] [rfc] warning about overlapping regions when booting with bootm

[U-Boot-Users] [rfc] warning about overlapping regions when booting with bootm

[Mike Frysinger]

we semi-frequently get users who try to boot an image on top of itself and
when when things crash, dont realize why.  i put together this quick little
warning, but i'm guessing that Wolfgang's answer is "don't bloat the code for
stupid people" ...

--- common/cmd_bootm.c
+++ common/cmd_bootm.c
@@ -1434,6 +1439,16 @@ int gunzip(void *dst, int dstlen, unsign
 	z_stream s;
 	int r, i, flags;
 
+	/* If memory regions overlap, whine about it.  We cannot check
+	 * the dstlen value as it is usually set to the max possible value
+	 * (CFG_BOOTM_LEN) which can be huge (64M).  Instead, we'll assume
+	 * the decompression size will be at least as big as the compressed
+	 * size so that we can at least check part of the destination.
+	 */
+	if ((dst <= (void *)src && (void *)src < dst + /*dstlen*/ *lenp) ||
+	    (dst <= (void *)(src + *lenp) && (void *)(src + *lenp) < dst + /*dstlen*/ *lenp))
+		puts ("\n   Warning: src and dst regions overlap ... ");
+
 	/* skip header */
 	i = 10;
 	flags = src[3];
-mike

[U-Boot-Users] [rfc] warning about overlapping regions when booting with bootm

[Wolfgang Denk]

In message <200802160259.32371.vapier@gentoo.org> you wrote:
> we semi-frequently get users who try to boot an image on top of itself and
> when when things crash, dont realize why.  i put together this quick little
> warning, but i'm guessing that Wolfgang's answer is "don't bloat the code for
> stupid people" ...

Indeed I reject the patch as is, but not because I think it would be
not useful, but rather because it is IMHO not correct. 

The thing is that images *may* overlap, at least a bit. When the  de-
compressor is running, it starts from the beginning of the compressed
image  and progrsses towards the end. The already preocessed parts of
the image are not neede dany longer - if they later  get  overwritten
by  umcompressed  code  this does no hard. Problems arise only if the
write pointer catches up with the read pointer.

Unfortunaltely I don't know of an  intelligent  way  to  handle  this
situation.  I  think  I  remember that RMK claims that the ARM kernel
uncompressor was really clever in this respect, but never found  time
to check it.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"I haven't lost my mind - it's backed up on tape somewhere."

[U-Boot-Users] [rfc] warning about overlapping regions when booting with bootm

[Mike Frysinger]

On Saturday 16 February 2008, Wolfgang Denk wrote:
> In message <200802160259.32371.vapier@gentoo.org> you wrote:
> > we semi-frequently get users who try to boot an image on top of itself
> > and when when things crash, dont realize why.  i put together this quick
> > little warning, but i'm guessing that Wolfgang's answer is "don't bloat
> > the code for stupid people" ...
>
> Indeed I reject the patch as is, but not because I think it would be
> not useful, but rather because it is IMHO not correct.
>
> The thing is that images *may* overlap, at least a bit. When the  de-
> compressor is running, it starts from the beginning of the compressed
> image  and progrsses towards the end. The already preocessed parts of
> the image are not neede dany longer - if they later  get  overwritten
> by  umcompressed  code  this does no hard. Problems arise only if the
> write pointer catches up with the read pointer.

right ... it isnt a big deal for us on Blackfin as people tend to have a ton 
of RAM compared to many boards, so the decompression areas need not overlap.

> Unfortunaltely I don't know of an  intelligent  way  to  handle  this
> situation.  I  think  I  remember that RMK claims that the ARM kernel
> uncompressor was really clever in this respect, but never found  time
> to check it.

it'd be better moved to the actual decompression routines rather than top of 
bootm.  and either check it before decompressing each chunk, or have a good 
heuristic to estimate things ...
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 827 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20080216/538eb9c7/attachment.pgp 

[U-Boot-Users] [rfc] warning about overlapping regions whenbooting with bootm

[Robin Getz]

On Sat 16 Feb 2008 18:06, Wolfgang Denk pondered:
> In message <200802160259.32371.vapier@gentoo.org> you wrote:
> > we semi-frequently get users who try to boot an image on top of itself
> and
> > when when things crash, dont realize why.  i put together this quick
> little
> > warning, but i'm guessing that Wolfgang's answer is "don't bloat the
> code for
> > stupid people" ...
> 
> Indeed I reject the patch as is, but not because I think it would be
> not useful, but rather because it is IMHO not correct. 
> 
> The thing is that images *may* overlap, at least a bit. When the  de-
> compressor is running, it starts from the beginning of the compressed
> image  and progrsses towards the end. The already preocessed parts of
> the image are not neede dany longer - if they later  get  overwritten
> by  umcompressed  code  this does no hard. Problems arise only if the
> write pointer catches up with the read pointer.

Hmm...

I always thought that when decompressing a uImage, that the entry point was 
stored in the header, (at the beginning of the file) and was read after the 
decompression took place - is that wrong?

-Robin

[U-Boot-Users] [rfc] warning about overlapping regions whenbooting with bootm

[Wolfgang Denk]

In message <200802171529.23434.rgetz@blackfin.uclinux.org> you wrote:
>
> I always thought that when decompressing a uImage, that the entry point was 
> stored in the header, (at the beginning of the file) and was read after the 
> decompression took place - is that wrong?

I'm not sure what all the different architectures do,  but  at  least
for PowerPC, ARM and MIPS I know that one of the very first things we
do in the bootm code is to create a local copy of the image header.

Search for 'memcpy.*header'.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
You can do this in a number of ways.     IBM chose to do all of them.
Why do you find that funny?        -- D. Taylor, Computer Science 350

[U-Boot-Users] [rfc] warning about overlapping regions whenbooting with bootm

[Robin Getz]

On Sun 17 Feb 2008 17:09, Wolfgang Denk pondered:
> In message <200802171529.23434.rgetz@blackfin.uclinux.org> you wrote:
> >
> > I always thought that when decompressing a uImage, that the entry 
> > point was stored in the header, (at the beginning of the file) and
> > was read after the decompression took place - is that wrong?
> 
> I'm not sure what all the different architectures do,  but  at  least
> for PowerPC, ARM and MIPS I know that one of the very first things we
> do in the bootm code is to create a local copy of the image header.
> 
> Search for 'memcpy.*header'.

What I saw that in common/cmd_bootm.c:do_bootm() there is a:

  /* Copy header so we can blank CRC field for re-calculation */
  memmove (&header, (char *)addr, sizeof(image_header_t));

but this seems only to be used in the calc/checking of the CRC.

When I look at 
            do_bootm_linux  (cmdtp, flag, argc, argv,
                             addr, len_ptr, verify);

I saw it not being used, and what I missed was that header is a global (and 
doesn't need to be passed to do_bootm_linux as a parameter).

Thanks & sorry for the noise.

-Robin

[U-Boot-Users] [rfc] warning about overlapping regions whenbooting with bootm

[Marian Balakowicz]

Wolfgang Denk wrote:
> In message <200802171529.23434.rgetz@blackfin.uclinux.org> you wrote:
>> I always thought that when decompressing a uImage, that the entry point was 
>> stored in the header, (at the beginning of the file) and was read after the 
>> decompression took place - is that wrong?
> 
> I'm not sure what all the different architectures do,  but  at  least
> for PowerPC, ARM and MIPS I know that one of the very first things we
> do in the bootm code is to create a local copy of the image header.

The image header copy is created but its not always properly used,
do_bootm_netbsd() and do_bootm_linux() for AVR32 and I386 are the
examples where we access image location directly instead of the header
copy. This might be fixed, but other, serious problem is concerning
MULTI images, where we need to access second or third component. If the
original image gets overwritten with the uncompressed kernel u-boot will
not be able read len_ptr[] table and access components.

New uImage format is even more demanding. As there is no such thing as
image header (data is spread across the new uImage blob as a various
properties) we are unable to create a header copy, thus we may not
accept image overwrites.

I have already posted a patch that adds overwrite checks, see:
http://www.denx.de/cgi-bin/gitweb.cgi?p=u-boot/u-boot-testing.git;a=commit;h=7582438c285bf0cef82909d0f232de64ec567a8a

Overwrite checking is done in bootm code as we may be using gunzip,
bunzip or no compression and need to handle all the cases.

Cheers,
m.