Change in coreboot[master]: libpayload: memmove: Don't make expectations of architecture memcpy
Hello Aaron Durbin, Yu-Ping Wu,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/44691
to review the following change.
Change subject: libpayload: memmove: Don't make expectations of architecture memcpy ......................................................................
libpayload: memmove: Don't make expectations of architecture memcpy
default_memmove() calls memcpy() when (src > dst). This is safe for the default_memcpy() implementation, but just calling memcpy() may invoke an architecture-specific implementation. Architectures are free to implement memcpy() however they want and may assume that buffers don't overlap in either direction. So while this happens to work for all current architecture implementations of memcpy(), it's safer not to rely on that and only rely on the known implementation of default_memcpy() for the forwards-overlapping case.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I7ece4ce9e6622a36612bfade3deb62f351877789 --- M payloads/libpayload/libc/memory.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/44691/1
diff --git a/payloads/libpayload/libc/memory.c b/payloads/libpayload/libc/memory.c index daa53f1..cc33eab 100644 --- a/payloads/libpayload/libc/memory.c +++ b/payloads/libpayload/libc/memory.c @@ -90,7 +90,7 @@ ssize_t i;
if (src > dst) - return memcpy(dst, src, n); + return default_memcpy(dst, src, n);
if (!IS_ALIGNED((uintptr_t)dst, sizeof(unsigned long)) || !IS_ALIGNED((uintptr_t)src, sizeof(unsigned long))) {
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44691 )
Change subject: libpayload: memmove: Don't make expectations of architecture memcpy ......................................................................
Patch Set 1: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44691 )
Change subject: libpayload: memmove: Don't make expectations of architecture memcpy ......................................................................
libpayload: memmove: Don't make expectations of architecture memcpy
default_memmove() calls memcpy() when (src > dst). This is safe for the default_memcpy() implementation, but just calling memcpy() may invoke an architecture-specific implementation. Architectures are free to implement memcpy() however they want and may assume that buffers don't overlap in either direction. So while this happens to work for all current architecture implementations of memcpy(), it's safer not to rely on that and only rely on the known implementation of default_memcpy() for the forwards-overlapping case.
Signed-off-by: Julius Werner jwerner@chromium.org Change-Id: I7ece4ce9e6622a36612bfade3deb62f351877789 Reviewed-on: https://review.coreboot.org/c/coreboot/+/44691 Reviewed-by: Aaron Durbin adurbin@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M payloads/libpayload/libc/memory.c 1 file changed, 1 insertion(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved
diff --git a/payloads/libpayload/libc/memory.c b/payloads/libpayload/libc/memory.c index daa53f1..cc33eab 100644 --- a/payloads/libpayload/libc/memory.c +++ b/payloads/libpayload/libc/memory.c @@ -90,7 +90,7 @@ ssize_t i;
if (src > dst) - return memcpy(dst, src, n); + return default_memcpy(dst, src, n);
if (!IS_ALIGNED((uintptr_t)dst, sizeof(unsigned long)) || !IS_ALIGNED((uintptr_t)src, sizeof(unsigned long))) {
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44691 )
Change subject: libpayload: memmove: Don't make expectations of architecture memcpy ......................................................................
Patch Set 2:
Automatic boot test returned (PASS/FAIL/TOTAL): 5/2/7 "QEMU x86 q35/ich9" (x86_32) using payload TianoCore : SUCCESS : https://lava.9esec.io/r/16455 "QEMU x86 q35/ich9" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/16454 "QEMU x86 i440fx/piix4" (x86_64) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/16453 "QEMU x86 i440fx/piix4" (x86_32) using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/16452 "QEMU AArch64" using payload LinuxBoot_u-root_kexec : SUCCESS : https://lava.9esec.io/r/16451 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload TianoCore : FAIL : https://lava.9esec.io/r/16457 "HP Compaq 8200 Elite SFF PC" (x86_32) using payload SeaBIOS : FAIL : https://lava.9esec.io/r/16456
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Aaron Durbin (Code Review)
-
Julius Werner (Code Review)
-
Patrick Georgi (Code Review)