[U-Boot] [PATCH] memmove_wd: Allow overlapping memory area

[U-Boot] [PATCH] memmove_wd: Allow overlapping memory area

[Alexander Stein]

Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
---
 common/image.c |   11 ++++++++---
 1 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/common/image.c b/common/image.c
index 6d8833e..da0fdd5 100644
--- a/common/image.c
+++ b/common/image.c
@@ -456,9 +456,14 @@ void memmove_wd (void *to, void *from, size_t len, ulong chunksz)
 	while (len > 0) {
 		size_t tail = (len > chunksz) ? chunksz : len;
 		WATCHDOG_RESET ();
-		memmove (to, from, tail);
-		to += tail;
-		from += tail;
+		if (to <= from)
+		{
+			memmove (to, from, tail);
+			to += tail;
+			from += tail;
+		} else {
+			memmove (to + len - tail, from + len - tail, tail);
+		}
 		len -= tail;
 	}
 #else	/* !(CONFIG_HW_WATCHDOG || CONFIG_WATCHDOG) */
-- 
1.7.1

[U-Boot] [PATCH] memmove_wd: Allow overlapping memory area

[Wolfgang Denk]

Dear Alexander Stein,

In message <1279701826-20083-1-git-send-email-alexander.stein@systec-electronic.com> you wrote:
> Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
> ---
>  common/image.c |   11 ++++++++---
>  1 files changed, 8 insertions(+), 3 deletions(-)

Why would this be needed?  Do you have an error scenario?

> +		if (to <= from)
> +		{
> +			memmove (to, from, tail);
> +			to += tail;
> +			from += tail;
> +		} else {
> +			memmove (to + len - tail, from + len - tail, tail);

In which way is this supposed to allow overlapping memory areas?

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 think there's a world market for about five computers.
         -- attr. Thomas J. Watson (Chairman of the Board, IBM), 1943

[U-Boot] [PATCH] memmove_wd: Allow overlapping memory area

[Wolfgang Denk]

Dear Alexander Stein,

In message <1279701826-20083-1-git-send-email-alexander.stein@systec-electronic.com> you wrote:
> Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
> ---
>  common/image.c |   11 ++++++++---
>  1 files changed, 8 insertions(+), 3 deletions(-)

And BTW:

> +		if (to <= from)
> +		{

Incorrect brace style.

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
This is now.  Later is later.

[U-Boot] [PATCH] memmove_wd: Allow overlapping memory area

[Alexander Stein]

Dear Wolfgang,

Am Montag, 9. August 2010, 00:18:19 schrieb Wolfgang Denk:
> In message <1279701826-20083-1-git-send-email-alexander.stein@systec-
electronic.com> you wrote:
> > Signed-off-by: Alexander Stein <alexander.stein@systec-electronic.com>
> > ---
> > 
> >  common/image.c |   11 ++++++++---
> >  1 files changed, 8 insertions(+), 3 deletions(-)
> 
> Why would this be needed?  Do you have an error scenario?

IIRC the problem arose when i downloaded my image to 0x20000000 (SD-RAM) using 
TFTP and my Linux kernel entry point was at 0x20008000. So when 
CONFIG_HW_WATCHDOG is defined u-boot starts copying from the beginning thus 
overriding the source. Finally i got decompression errors upon Linux kernel 
start.
Without CONFIG_HW_WATCHDOG memmove_wd is simply memmove which handled this 
cased correctly.

> > +		if (to <= from)
> > +		{
> > +			memmove (to, from, tail);
> > +			to += tail;
> > +			from += tail;
> > +		} else {
> > +			memmove (to + len - tail, from + len - tail, tail);
> 
> In which way is this supposed to allow overlapping memory areas?

With this change u-boot starts to copy from the end to the beginning thus 
preventing overriding the source. This change was adopted from memmove 
(lib/string.c) which handles this case correctly.

Best regards,
Alexander

[U-Boot] [PATCH] memmove_wd: Allow overlapping memory area

[Wolfgang Denk]

Dear Alexander Stein,

In message <201008090857.32705.alexander.stein@systec-electronic.com> you wrote:
> 
> > >  common/image.c |   11 ++++++++---
> > >  1 files changed, 8 insertions(+), 3 deletions(-)
> > 
> > Why would this be needed?  Do you have an error scenario?
> 
> IIRC the problem arose when i downloaded my image to 0x20000000 (SD-RAM) using 
> TFTP and my Linux kernel entry point was at 0x20008000. So when 

Don;t do this, then. Such kind of overlap has never been supported.
Even if it would work in your case of uncompressed images, it is bound
to fail for compressed ones where the uncompressed code grows faster
then compressed data get consumed.

> > > +		if (to <= from)
> > > +		{
> > > +			memmove (to, from, tail);
> > > +			to += tail;
> > > +			from += tail;
> > > +		} else {
> > > +			memmove (to + len - tail, from + len - tail, tail);
> > 
> > In which way is this supposed to allow overlapping memory areas?
> 
> With this change u-boot starts to copy from the end to the beginning thus 
> preventing overriding the source. This change was adopted from memmove 
> (lib/string.c) which handles this case correctly.

You are just shifting the problem to another area, but you are not
solving it. Without your patch there is a problem when the initial
area overlaps, with your problem there is one when the tail overlaps.

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
"The two most common things in the universe are hydrogen  and  stupi-
dity."

[U-Boot] [PATCH] memmove_wd: Allow overlapping memory area

[Alexander Stein]

Dear Wolfgang,

Am Montag, 9. August 2010, 11:26:56 schrieb Wolfgang Denk:
> > IIRC the problem arose when i downloaded my image to 0x20000000 (SD-RAM)
> > using TFTP and my Linux kernel entry point was at 0x20008000. So when
> 
> Don;t do this, then. Such kind of overlap has never been supported.
> Even if it would work in your case of uncompressed images, it is bound
> to fail for compressed ones where the uncompressed code grows faster
> then compressed data get consumed.

Well, that's at least one possibility but it is very annoying that something 
like memmove that works fine so far suddenly stops working when watchdog 
support is enabled.

> > > > +		if (to <= from)
> > > > +		{
> > > > +			memmove (to, from, tail);
> > > > +			to += tail;
> > > > +			from += tail;
> > > > +		} else {
> > > > +			memmove (to + len - tail, from + len - tail, tail);
> > > 
> > > In which way is this supposed to allow overlapping memory areas?
> > 
> > With this change u-boot starts to copy from the end to the beginning thus
> > preventing overriding the source. This change was adopted from memmove
> > (lib/string.c) which handles this case correctly.
> 
> You are just shifting the problem to another area, but you are not
> solving it. Without your patch there is a problem when the initial
> area overlaps, with your problem there is one when the tail overlaps.

Well, why is this situation then handled by memmove?

Best regards,
Alexander

[U-Boot] [PATCH] memmove_wd: Allow overlapping memory area

[Wolfgang Denk]

Dear Alexander Stein,

In message <201008091310.37634.alexander.stein@systec-electronic.com> you wrote:
> 
> > Even if it would work in your case of uncompressed images, it is bound
> > to fail for compressed ones where the uncompressed code grows faster
> > then compressed data get consumed.
> 
> Well, that's at least one possibility but it is very annoying that something 
> like memmove that works fine so far suddenly stops working when watchdog 
> support is enabled.

Yes, I understand this. It's also annoying when you've been using
compressed images just fine for years and suddenly they stop working
because the kernel grows beyond some magic (and undefined) limit.

Better do not build on undefined behaviour.

> > You are just shifting the problem to another area, but you are not
> > solving it. Without your patch there is a problem when the initial
> > area overlaps, with your problem there is one when the tail overlaps.
> 
> Well, why is this situation then handled by memmove?

memmove() is used in a log of places.


Please note that I'm not against fixing this problem, but if we
address it, it should be fixed correctly, i. e. in sich a way that it
will always work, not only for certain cases.

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
"Free markets select for winning solutions."        - Eric S. Raymond

[U-Boot] [PATCH] memmove_wd: Allow overlapping memory area

[Alexander Stein]

Dear Wolfgang,

August 2010, 13:43:32 schrieb Wolfgang Denk:
> > Well, that's at least one possibility but it is very annoying that
> > something like memmove that works fine so far suddenly stops working
> > when watchdog support is enabled.
> 
> Yes, I understand this. It's also annoying when you've been using
> compressed images just fine for years and suddenly they stop working
> because the kernel grows beyond some magic (and undefined) limit.
> 
> Better do not build on undefined behaviour.

Yes, of course. But it seems difficult to recognise thing like a simple 
memmove with dest < src as an undefined behavior.


> Please note that I'm not against fixing this problem, but if we
> address it, it should be fixed correctly, i. e. in sich a way that it
> will always work, not only for certain cases.

So how to fix this then? I'm totally inexperienced to compressed images.

Best regards,
Alexander

[U-Boot] [PATCH] memmove_wd: Allow overlapping memory area

[Wolfgang Denk]

Dear Alexander Stein,

In message <201008091351.39800.alexander.stein@systec-electronic.com> you wrote:
> 
> > Better do not build on undefined behaviour.
> 
> Yes, of course. But it seems difficult to recognise thing like a simple 
> memmove with dest < src as an undefined behavior.

See the FAQ: http://www.denx.de/wiki/view/DULG/LinuxUncompressingError

It has been explained before (and often) that the download address of
a kernel image must be sufficiently far away from the load address in
the image header.

> > Please note that I'm not against fixing this problem, but if we
> > address it, it should be fixed correctly, i. e. in sich a way that it
> > will always work, not only for certain cases.
> 
> So how to fix this then? I'm totally inexperienced to compressed images.

Well, I don't an easy fix for the compression case.

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
Only a fool fights in a burning house.
	-- Kank the Klingon, "Day of the Dove", stardate unknown