x86/boot: Warn on future overlapping memcpy() use

PATCH Applied Kees Cook 2 participants 6 messages Apr 29, 2016 x86

Share & tools

Kees Cook

author

2016-04-29 00:18 UTC #0 lore raw

PATCH v4 1/1 x86/boot: Warn on future overlapping memcpy() use

If an overlapping memcpy() is ever attempted, we should at least report it, in case it might lead to problems, so it could be changed to a memmove() call instead.

Signed-off-by Kees Cook <[email protected]>

---
v4:
- use __memcpy not memcpy since we've already done the check. v3:
- call memmove in addition to doing the warning
v2:
- warn about overlapping region

---
 arch/x86/boot/compressed/string.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

arch/x86/boot/compressed/string.c +13 -3

index 2befeca1aada..5ac3acb5699f 100644
@@ -8,7 +8,7 @@
 #include "../string.c"
 
 #ifdef CONFIG_X86_32
-void *memcpy(void *dest, const void *src, size_t n)
+static void *__memcpy(void *dest, const void *src, size_t n)
 {
 	int d0, d1, d2;
 	asm volatile(

@@ -22,7 +22,7 @@ void *memcpy(void *dest, const void *src, size_t n)
 	return dest;
 }
 #else
-void *memcpy(void *dest, const void *src, size_t n)
+static void *__memcpy(void *dest, const void *src, size_t n)
 {
 	long d0, d1, d2;
 	asm volatile(

@@ -53,10 +53,20 @@ void *memmove(void *dest, const void *src, size_t n)
 	const unsigned char *s = src;
 
 	if (d <= s || d - s >= n)
-		return memcpy(dest, src, n);
+		return __memcpy(dest, src, n);
 
 	while (n-- > 0)
 		d[n] = s[n];
 
 	return dest;
 }
+
+/* Detect and warn about potential overlaps, but handle them with memmove. */
+void *memcpy(void *dest, const void *src, size_t n)
+{
+	if (dest > src && dest - src < n) {
+		warn("Avoiding potentially unsafe overlapping memcpy()!");
+		return memmove(dest, src, n);
+	}
+	return __memcpy(dest, src, n);
+}

Signature · 6 lines

2.6.3


-- 
Kees Cook
Chrome OS & Brillo Security

Ingo Molnar

Applied

2016-04-29 06:43 UTC #1 lore raw

Quoted 13 lines · Kees Cook

If an overlapping memcpy() is ever attempted, we should at least report it, in case it might lead to problems, so it could be changed to a memmove() call instead.
Suggested-by Ingo Molnar <[email protected]>
Signed-off-by Kees Cook <[email protected]>
---
v4:
- use __memcpy not memcpy since we've already done the check. v3:
- call memmove in addition to doing the warning
v2:
- warn about overlapping region

Quoted 3 lines

---
 arch/x86/boot/compressed/string.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

Applied, thanks Kees!

Btw., can we now also remove the memmove() hack from lib/decompress_unxz.c?

Thanks,

Ingo

Kees Cook

author

2016-04-29 06:56 UTC #2 lore raw

Quoted 20 lines · Ingo Molnar

Quoted 13 lines · Kees Cook
If an overlapping memcpy() is ever attempted, we should at least report it, in case it might lead to problems, so it could be changed to a memmove() call instead.
Suggested-by Ingo Molnar <[email protected]>
Signed-off-by Kees Cook <[email protected]>
---
v4:
- use __memcpy not memcpy since we've already done the check. v3:
- call memmove in addition to doing the warning
v2:
- warn about overlapping region
Quoted 3 lines
---
 arch/x86/boot/compressed/string.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

Quoted 3 lines

Applied, thanks Kees!
Btw., can we now also remove the memmove() hack from lib/decompress_unxz.c?

I'll let Lasse answer for sure, but I don't think so. The original commit says:

The XZ decompressor needs memmove(), memeq() (memcmp() == 0), and memzero() (memset(ptr, 0, size)), which aren't available in all arch-specific pre-boot environments. I'm including simple versions in decompress_unxz.c, but a cleaner solution would naturally be nicer.

-Kees

Signature · 2 lines

Kees Cook
Chrome OS & Brillo Security

Ingo Molnar

2016-04-29 07:20 UTC #3 lore raw

Quoted 23 lines · Kees Cook

Quoted 22 lines · Ingo Molnar
Quoted 13 lines · Kees Cook
If an overlapping memcpy() is ever attempted, we should at least report it, in case it might lead to problems, so it could be changed to a memmove() call instead.
Suggested-by Ingo Molnar <[email protected]>
Signed-off-by Kees Cook <[email protected]>
---
v4:
- use __memcpy not memcpy since we've already done the check. v3:
- call memmove in addition to doing the warning
v2:
- warn about overlapping region
Quoted 3 lines
---
 arch/x86/boot/compressed/string.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)
Applied, thanks Kees!

Quoted 8 lines

Quoted 1 line
Btw., can we now also remove the memmove() hack from lib/decompress_unxz.c?
I'll let Lasse answer for sure, but I don't think so. The original commit says:
The XZ decompressor needs memmove(), memeq() (memcmp() == 0), and memzero() (memset(ptr, 0, size)), which aren't available in all arch-specific pre-boot environments. I'm including simple versions in decompress_unxz.c, but a cleaner solution would naturally be nicer.

I see, so non-x86 architectures might not have the proper runtime environment implemented, right?

Fair enough!

Thanks,

Ingo

Ingo Molnar

2016-04-29 07:57 UTC #4 lore raw

In reply to Kees Cook

Quoted 13 lines · Kees Cook

If an overlapping memcpy() is ever attempted, we should at least report it, in case it might lead to problems, so it could be changed to a memmove() call instead.
Suggested-by Ingo Molnar <[email protected]>
Signed-off-by Kees Cook <[email protected]>
---
v4:
- use __memcpy not memcpy since we've already done the check. v3:
- call memmove in addition to doing the warning
v2:
- warn about overlapping region

Quoted 3 lines

---
 arch/x86/boot/compressed/string.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

I think you'll hate this patch some more:

arch/x86/boot/compressed/string.c:68:3: warning: implicit declaration of function ‘warn’ [-Wimplicit-function-declaration]

:-)

Can we do the trick below? Because misc.h also includes the regular kernel memcpy functions, we can remove the decompressor specific __memcpy() - but the question is, is it safe to do?

If it's not safe to do, we are playing with fire already I suspect:

arch/x86/boot/compressed/cmdline.c:#include "misc.h" arch/x86/boot/compressed/early_serial_console.c:#include "misc.h" arch/x86/boot/compressed/kaslr.c:#include "misc.h" arch/x86/boot/compressed/misc.c:#include "misc.h"

Thanks,

Ingo

 arch/x86/boot/compressed/string.c | 31 +------------------------------
 1 file changed, 1 insertion(+), 30 deletions(-)

arch/x86/boot/compressed/string.c +1 ~~-30~~

index 952510976732..f4b95ed4e7a2 100644
@@ -6,37 +6,8 @@
  * (e.g. FPU ops) in the minimal decompression stub execution environment.
  */
 #include "../string.c"
-#include "misc.h"
-
-#ifdef CONFIG_X86_32
-static void *__memcpy(void *dest, const void *src, size_t n)
-{
-	int d0, d1, d2;
-	asm volatile(
-		"rep ; movsl\n\t"
-		"movl %4,%%ecx\n\t"
-		"rep ; movsb\n\t"
-		: "=&c" (d0), "=&D" (d1), "=&S" (d2)
-		: "0" (n >> 2), "g" (n & 3), "1" (dest), "2" (src)
-		: "memory");
-
-	return dest;
-}
-#else
-static void *__memcpy(void *dest, const void *src, size_t n)
-{
-	long d0, d1, d2;
-	asm volatile(
-		"rep ; movsq\n\t"
-		"movq %4,%%rcx\n\t"
-		"rep ; movsb\n\t"
-		: "=&c" (d0), "=&D" (d1), "=&S" (d2)
-		: "0" (n >> 3), "g" (n & 7), "1" (dest), "2" (src)
-		: "memory");
 
-	return dest;
-}
-#endif
+#include "misc.h"
 
 void *memset(void *s, int c, size_t n)
 {

Kees Cook

author

2016-04-29 14:17 UTC #5 lore raw

Quoted 20 lines · Ingo Molnar

Quoted 13 lines · Kees Cook
If an overlapping memcpy() is ever attempted, we should at least report it, in case it might lead to problems, so it could be changed to a memmove() call instead.
Suggested-by Ingo Molnar <[email protected]>
Signed-off-by Kees Cook <[email protected]>
---
v4:
- use __memcpy not memcpy since we've already done the check. v3:
- call memmove in addition to doing the warning
v2:
- warn about overlapping region
Quoted 3 lines
---
 arch/x86/boot/compressed/string.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

Quoted 5 lines

I think you'll hate this patch some more:
arch/x86/boot/compressed/string.c:68:3: warning: implicit declaration of function ‘warn’ [-Wimplicit-function-declaration]
:-)

Argh, I didn't get that warning when I built. But yes, this patch hates me too, it seems.

Quoted 11 lines

Can we do the trick below? Because misc.h also includes the regular kernel memcpy functions, we can remove the decompressor specific __memcpy() - but the question is, is it safe to do?
If it's not safe to do, we are playing with fire already I suspect:
arch/x86/boot/compressed/cmdline.c:#include "misc.h" arch/x86/boot/compressed/early_serial_console.c:#include "misc.h" arch/x86/boot/compressed/kaslr.c:#include "misc.h" arch/x86/boot/compressed/misc.c:#include "misc.h"

Hrm, let me poke at it. I think the better thing to do would be to split up header files. I would rather have the warn/error/etc functions in a separate header than to put the memcpy code in a header.

-Kees

Quoted 53 lines

Thanks,

Ingo

 arch/x86/boot/compressed/string.c | 31 +------------------------------
 1 file changed, 1 insertion(+), 30 deletions(-)

arch/x86/boot/compressed/string.c +1 ~~-30~~

index 952510976732..f4b95ed4e7a2 100644
@@ -6,37 +6,8 @@
  * (e.g. FPU ops) in the minimal decompression stub execution environment.
  */
 #include "../string.c"
-#include "misc.h"
-
-#ifdef CONFIG_X86_32
-static void *__memcpy(void *dest, const void *src, size_t n)
-{
-       int d0, d1, d2;
-       asm volatile(
-               "rep ; movsl\n\t"
-               "movl %4,%%ecx\n\t"
-               "rep ; movsb\n\t"
-               : "=&c" (d0), "=&D" (d1), "=&S" (d2)
-               : "0" (n >> 2), "g" (n & 3), "1" (dest), "2" (src)
-               : "memory");
-
-       return dest;
-}
-#else
-static void *__memcpy(void *dest, const void *src, size_t n)
-{
-       long d0, d1, d2;
-       asm volatile(
-               "rep ; movsq\n\t"
-               "movq %4,%%rcx\n\t"
-               "rep ; movsb\n\t"
-               : "=&c" (d0), "=&D" (d1), "=&S" (d2)
-               : "0" (n >> 3), "g" (n & 7), "1" (dest), "2" (src)
-               : "memory");
 
-       return dest;
-}
-#endif
+#include "misc.h"
 
 void *memset(void *s, int c, size_t n)
 {

Signature · 2 lines

Kees Cook
Chrome OS & Brillo Security