Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1390661 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v4] x86/boot: Warn on future overlapping memcpy() use Kees Cook <keescook@chromium.org> - 2016-04-29 02:20 +0200
    Re: [PATCH v4] x86/boot: Warn on future overlapping memcpy() use Ingo Molnar <mingo@kernel.org> - 2016-04-29 08:50 +0200
      Re: [PATCH v4] x86/boot: Warn on future overlapping memcpy() use Kees Cook <keescook@chromium.org> - 2016-04-29 09:00 +0200
        Re: [PATCH v4] x86/boot: Warn on future overlapping memcpy() use Ingo Molnar <mingo@kernel.org> - 2016-04-29 09:30 +0200
    Re: [PATCH v4] x86/boot: Warn on future overlapping memcpy() use Ingo Molnar <mingo@kernel.org> - 2016-04-29 10:00 +0200
      Re: [PATCH v4] x86/boot: Warn on future overlapping memcpy() use Kees Cook <keescook@chromium.org> - 2016-04-29 16:20 +0200

#1390661 — [PATCH v4] 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.

Suggested-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
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(-)

diff --git a/arch/x86/boot/compressed/string.c b/arch/x86/boot/compressed/string.c
index 2befeca1aada..5ac3acb5699f 100644
--- a/arch/x86/boot/compressed/string.c
+++ b/arch/x86/boot/compressed/string.c
@@ -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);
+}
-- 
2.6.3


-- 
Kees Cook
Chrome OS & Brillo Security

[toc] | [next] | [standalone]

#1390804

* Kees Cook <keescook@chromium.org> wrote:

> 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 <mingo@kernel.org>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> 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(-)

Applied, thanks Kees!

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

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1390809

On Thu, Apr 28, 2016 at 11:43 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Kees Cook <keescook@chromium.org> wrote:
>
>> 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 <mingo@kernel.org>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>> 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(-)
>
> 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


-- 
Kees Cook
Chrome OS & Brillo Security

[toc] | [prev] | [next] | [standalone]

#1390823

* Kees Cook <keescook@chromium.org> wrote:

> On Thu, Apr 28, 2016 at 11:43 PM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * Kees Cook <keescook@chromium.org> wrote:
> >
> >> 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 <mingo@kernel.org>
> >> Signed-off-by: Kees Cook <keescook@chromium.org>
> >> ---
> >> 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(-)
> >
> > 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.

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

Fair enough!

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1390845

* Kees Cook <keescook@chromium.org> wrote:

> 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 <mingo@kernel.org>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> 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(-)

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(-)

diff --git a/arch/x86/boot/compressed/string.c b/arch/x86/boot/compressed/string.c
index 952510976732..f4b95ed4e7a2 100644
--- a/arch/x86/boot/compressed/string.c
+++ b/arch/x86/boot/compressed/string.c
@@ -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)
 {

[toc] | [prev] | [next] | [standalone]

#1391185

On Fri, Apr 29, 2016 at 12:57 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Kees Cook <keescook@chromium.org> wrote:
>
>> 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 <mingo@kernel.org>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>> 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(-)
>
> 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.

>
> 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

>
> ?
>
> Thanks,
>
>         Ingo
>
>  arch/x86/boot/compressed/string.c | 31 +------------------------------
>  1 file changed, 1 insertion(+), 30 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/string.c b/arch/x86/boot/compressed/string.c
> index 952510976732..f4b95ed4e7a2 100644
> --- a/arch/x86/boot/compressed/string.c
> +++ b/arch/x86/boot/compressed/string.c
> @@ -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
Chrome OS & Brillo Security

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web