[U-Boot] [PATCH] memmove_wd: Allow overlapping memory area
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) */
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
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
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
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
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
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
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
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
participants (2)
-
Alexander Stein -
Wolfgang Denk