[PATCH v2] x86/boot: Rename overlapping memcpy() to memmove()

[PATCH v2] x86/boot: Rename overlapping memcpy() to memmove()

[Kees Cook]

Instead of having non-standard memcpy() behavior, explicitly call the new
function memmove(), make it available to the decompressors, and switch
the two overlap cases (screen scrolling and ELF parsing) to use memmove().
Additionally documents the purpose of compressed/string.c.

Suggested-by: Lasse Collin <lasse.collin@tukaani.org>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
Applies on top of tip:x86/boot
---
 arch/x86/boot/compressed/misc.c   |  6 ++++--
 arch/x86/boot/compressed/string.c | 19 +++++++++++--------
 2 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index c57d785ff955..6dde6ccdf00e 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -32,9 +32,11 @@
 #undef memcpy
 #undef memset
 #define memzero(s, n)	memset((s), 0, (n))
+#define memmove		memmove
 
 /* Functions used by the included decompressor code below. */
 static void error(char *m);
+void *memmove(void *dest, const void *src, size_t n);
 
 /*
  * This is set up by the setup-routine at boot-time
@@ -80,7 +82,7 @@ static void scroll(void)
 {
 	int i;
 
-	memcpy(vidmem, vidmem + cols * 2, (lines - 1) * cols * 2);
+	memmove(vidmem, vidmem + cols * 2, (lines - 1) * cols * 2);
 	for (i = (lines - 1) * cols * 2; i < lines * cols * 2; i += 2)
 		vidmem[i] = ' ';
 }
@@ -307,7 +309,7 @@ static void parse_elf(void *output)
 #else
 			dest = (void *)(phdr->p_paddr);
 #endif
-			memcpy(dest, output + phdr->p_offset, phdr->p_filesz);
+			memmove(dest, output + phdr->p_offset, phdr->p_filesz);
 			break;
 		default: /* Ignore other PT_* */ break;
 		}
diff --git a/arch/x86/boot/compressed/string.c b/arch/x86/boot/compressed/string.c
index 1e10e40f49dd..2befeca1aada 100644
--- a/arch/x86/boot/compressed/string.c
+++ b/arch/x86/boot/compressed/string.c
@@ -1,7 +1,14 @@
+/*
+ * This provides an optimized implementation of memcpy, and a simplified
+ * implementation of memset and memmove. These are used here because the
+ * standard kernel runtime versions are not yet available and we don't
+ * trust the gcc built-in implementations as they may do unexpected things
+ * (e.g. FPU ops) in the minimal decompression stub execution environment.
+ */
 #include "../string.c"
 
 #ifdef CONFIG_X86_32
-void *__memcpy(void *dest, const void *src, size_t n)
+void *memcpy(void *dest, const void *src, size_t n)
 {
 	int d0, d1, d2;
 	asm volatile(
@@ -15,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)
+void *memcpy(void *dest, const void *src, size_t n)
 {
 	long d0, d1, d2;
 	asm volatile(
@@ -40,17 +47,13 @@ void *memset(void *s, int c, size_t n)
 	return s;
 }
 
-/*
- * This memcpy is overlap safe (i.e. it is memmove without conflicting
- * with other definitions of memmove from the various decompressors.
- */
-void *memcpy(void *dest, const void *src, size_t n)
+void *memmove(void *dest, const void *src, size_t n)
 {
 	unsigned char *d = dest;
 	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];
-- 
2.6.3


-- 
Kees Cook
Chrome OS & Brillo Security

Re: [PATCH v2] x86/boot: Rename overlapping memcpy() to memmove()

[Ingo Molnar]

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

> Instead of having non-standard memcpy() behavior, explicitly call the new
> function memmove(), make it available to the decompressors, and switch
> the two overlap cases (screen scrolling and ELF parsing) to use memmove().
> Additionally documents the purpose of compressed/string.c.
> 
> Suggested-by: Lasse Collin <lasse.collin@tukaani.org>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> Applies on top of tip:x86/boot
> ---
>  arch/x86/boot/compressed/misc.c   |  6 ++++--
>  arch/x86/boot/compressed/string.c | 19 +++++++++++--------
>  2 files changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
> index c57d785ff955..6dde6ccdf00e 100644
> --- a/arch/x86/boot/compressed/misc.c
> +++ b/arch/x86/boot/compressed/misc.c
> @@ -32,9 +32,11 @@
>  #undef memcpy
>  #undef memset
>  #define memzero(s, n)	memset((s), 0, (n))
> +#define memmove		memmove

Btw., what's the purpose of this define? If it's already defined then we should 
get a build warning. If it's not, we won't.

Anyway, applied it, just curious.

Thanks,

	Ingo

Re: [PATCH v2] x86/boot: Rename overlapping memcpy() to memmove()

[Ingo Molnar]

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

> +void *memcpy(void *dest, const void *src, size_t n)

btw., if there's any doubt about other overlapping uses, we could add this to 
memcpy():

  WARN_ON_ONCE(dest > src && dest-src < n);

or so? Does printk() work so early on?

Thanks,

	Ingo

Re: [PATCH v2] x86/boot: Rename overlapping memcpy() to memmove()

[Kees Cook]

On Thu, Apr 28, 2016 at 2:04 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Kees Cook <keescook@chromium.org> wrote:
> > +#define memmove              memmove
>
> Btw., what's the purpose of this define? If it's already defined then we should
> get a build warning. If it's not, we won't.

It's for the decompressor that checks for memmove existing already via
"ifdef memmove". If this isn't done here, we will end up with two
memmove implementations.

>> +void *memcpy(void *dest, const void *src, size_t n)
>
> btw., if there's any doubt about other overlapping uses, we could add this to
> memcpy():
>
>   WARN_ON_ONCE(dest > src && dest-src < n);
>
> or so? Does printk() work so early on?

It does not, but we could use either "error" or the new "warn". Should
we abort a boot in this case, or just warn about it? (Our
implementations of memcpy, fwiw, currently seem to support overlap, so
I would suggest warn.)

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

Re: [PATCH v2] x86/boot: Rename overlapping memcpy() to memmove()

[Ingo Molnar]

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

> On Thu, Apr 28, 2016 at 2:04 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * Kees Cook <keescook@chromium.org> wrote:
> > > +#define memmove              memmove
> >
> > Btw., what's the purpose of this define? If it's already defined then we should
> > get a build warning. If it's not, we won't.
> 
> It's for the decompressor that checks for memmove existing already via
> "ifdef memmove". If this isn't done here, we will end up with two
> memmove implementations.

So:

 triton:~/tip> git grep memmove | grep -i ifdef
 triton:~/tip> 

what am I missing?

> >> +void *memcpy(void *dest, const void *src, size_t n)
> >
> > btw., if there's any doubt about other overlapping uses, we could add this to
> > memcpy():
> >
> >   WARN_ON_ONCE(dest > src && dest-src < n);
> >
> > or so? Does printk() work so early on?
> 
> It does not, but we could use either "error" or the new "warn". Should
> we abort a boot in this case, or just warn about it? (Our
> implementations of memcpy, fwiw, currently seem to support overlap, so
> I would suggest warn.)

Yeah, I'd definitely not try to crash the bootup for the user, but try to 
continue.

Thanks,

	Ingo

[tip:x86/boot] x86/boot: Rename overlapping memcpy() to memmove()

[tip-bot for Kees Cook]

Commit-ID:  81b785f3e4114ed74fceb48a54e7de2f797a2ba1
Gitweb:     http://git.kernel.org/tip/81b785f3e4114ed74fceb48a54e7de2f797a2ba1
Author:     Kees Cook <keescook@chromium.org>
AuthorDate: Tue, 26 Apr 2016 14:46:06 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 28 Apr 2016 11:02:29 +0200

x86/boot: Rename overlapping memcpy() to memmove()

Instead of having non-standard memcpy() behavior, explicitly call the new
function memmove(), make it available to the decompressors, and switch
the two overlap cases (screen scrolling and ELF parsing) to use memmove().
Additionally documents the purpose of compressed/string.c.

Suggested-by: Lasse Collin <lasse.collin@tukaani.org>
Signed-off-by: Kees Cook <keescook@chromium.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Baoquan He <bhe@redhat.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: H.J. Lu <hjl.tools@gmail.com>
Cc: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Yinghai Lu <yinghai@kernel.org>
Link: http://lkml.kernel.org/r/20160426214606.GA5758@www.outflux.net
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/boot/compressed/misc.c   |  6 ++++--
 arch/x86/boot/compressed/string.c | 19 +++++++++++--------
 2 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c
index c57d785..6dde6cc 100644
--- a/arch/x86/boot/compressed/misc.c
+++ b/arch/x86/boot/compressed/misc.c
@@ -32,9 +32,11 @@
 #undef memcpy
 #undef memset
 #define memzero(s, n)	memset((s), 0, (n))
+#define memmove		memmove
 
 /* Functions used by the included decompressor code below. */
 static void error(char *m);
+void *memmove(void *dest, const void *src, size_t n);
 
 /*
  * This is set up by the setup-routine at boot-time
@@ -80,7 +82,7 @@ static void scroll(void)
 {
 	int i;
 
-	memcpy(vidmem, vidmem + cols * 2, (lines - 1) * cols * 2);
+	memmove(vidmem, vidmem + cols * 2, (lines - 1) * cols * 2);
 	for (i = (lines - 1) * cols * 2; i < lines * cols * 2; i += 2)
 		vidmem[i] = ' ';
 }
@@ -307,7 +309,7 @@ static void parse_elf(void *output)
 #else
 			dest = (void *)(phdr->p_paddr);
 #endif
-			memcpy(dest, output + phdr->p_offset, phdr->p_filesz);
+			memmove(dest, output + phdr->p_offset, phdr->p_filesz);
 			break;
 		default: /* Ignore other PT_* */ break;
 		}
diff --git a/arch/x86/boot/compressed/string.c b/arch/x86/boot/compressed/string.c
index 1e10e40..2befeca 100644
--- a/arch/x86/boot/compressed/string.c
+++ b/arch/x86/boot/compressed/string.c
@@ -1,7 +1,14 @@
+/*
+ * This provides an optimized implementation of memcpy, and a simplified
+ * implementation of memset and memmove. These are used here because the
+ * standard kernel runtime versions are not yet available and we don't
+ * trust the gcc built-in implementations as they may do unexpected things
+ * (e.g. FPU ops) in the minimal decompression stub execution environment.
+ */
 #include "../string.c"
 
 #ifdef CONFIG_X86_32
-void *__memcpy(void *dest, const void *src, size_t n)
+void *memcpy(void *dest, const void *src, size_t n)
 {
 	int d0, d1, d2;
 	asm volatile(
@@ -15,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)
+void *memcpy(void *dest, const void *src, size_t n)
 {
 	long d0, d1, d2;
 	asm volatile(
@@ -40,17 +47,13 @@ void *memset(void *s, int c, size_t n)
 	return s;
 }
 
-/*
- * This memcpy is overlap safe (i.e. it is memmove without conflicting
- * with other definitions of memmove from the various decompressors.
- */
-void *memcpy(void *dest, const void *src, size_t n)
+void *memmove(void *dest, const void *src, size_t n)
 {
 	unsigned char *d = dest;
 	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];

Re: [PATCH v2] x86/boot: Rename overlapping memcpy() to memmove()

[Kees Cook]

On Thu, Apr 28, 2016 at 2:37 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Kees Cook <keescook@chromium.org> wrote:
>
>> On Thu, Apr 28, 2016 at 2:04 AM, Ingo Molnar <mingo@kernel.org> wrote:
>> >
>> > * Kees Cook <keescook@chromium.org> wrote:
>> > > +#define memmove              memmove
>> >
>> > Btw., what's the purpose of this define? If it's already defined then we should
>> > get a build warning. If it's not, we won't.
>>
>> It's for the decompressor that checks for memmove existing already via
>> "ifdef memmove". If this isn't done here, we will end up with two
>> memmove implementations.
>
> So:
>
>  triton:~/tip> git grep memmove | grep -i ifdef
>  triton:~/tip>
>
> what am I missing?

Sorry, I typoed. It's #ifndef:

lib/decompress_unxz.c:

#ifndef memmove
/* Not static to avoid a conflict with the prototype in the Linux headers. */
void *memmove(void *dest, const void *src, size_t size)
{
...

In the regular kernel, each architecture provides a define for memmove, e.g.:

arch/x86/include/asm/string_64.h:#define memmove(dst, src, len)
__memmove(dst, src, len)

So the compressed boot stub does too so the decompressor can work in
both code bases.

-Kees

>> >> +void *memcpy(void *dest, const void *src, size_t n)
>> >
>> > btw., if there's any doubt about other overlapping uses, we could add this to
>> > memcpy():
>> >
>> >   WARN_ON_ONCE(dest > src && dest-src < n);
>> >
>> > or so? Does printk() work so early on?
>>
>> It does not, but we could use either "error" or the new "warn". Should
>> we abort a boot in this case, or just warn about it? (Our
>> implementations of memcpy, fwiw, currently seem to support overlap, so
>> I would suggest warn.)
>
> Yeah, I'd definitely not try to crash the bootup for the user, but try to
> continue.
>
> Thanks,
>
>         Ingo



-- 
Kees Cook
Chrome OS & Brillo Security

Re: [PATCH v2] x86/boot: Rename overlapping memcpy() to memmove()

[Kees Cook]

On Thu, Apr 28, 2016 at 2:37 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Kees Cook <keescook@chromium.org> wrote:
>
>> On Thu, Apr 28, 2016 at 2:04 AM, Ingo Molnar <mingo@kernel.org> wrote:
>> >
>> > * Kees Cook <keescook@chromium.org> wrote:
>> >> +void *memcpy(void *dest, const void *src, size_t n)
>> >
>> > btw., if there's any doubt about other overlapping uses, we could add this to
>> > memcpy():
>> >
>> >   WARN_ON_ONCE(dest > src && dest-src < n);
>> >
>> > or so? Does printk() work so early on?
>>
>> It does not, but we could use either "error" or the new "warn". Should
>> we abort a boot in this case, or just warn about it? (Our
>> implementations of memcpy, fwiw, currently seem to support overlap, so
>> I would suggest warn.)
>
> Yeah, I'd definitely not try to crash the bootup for the user, but try to
> continue.

So, I can do this either with a macro to try to inline every caller
with meaningful reporting (__func__, __line__, etc), but I feel like
that is needless bloat to the stub, given the rare situation. I think
the best thing to do is just complain about it happening at all, and
we should be able to find the problem manually if it is ever reported.

For example, this is what I've got currently:

/* Detect and warn about potential overlaps. */
void *memcpy(void *dest, const void *src, size_t n)
{
        if (dest > src && dest - src < n)
                warn("Potentially unsafe overlapping memcpy detected!");
        return __memcpy(dest, src, n);
}

Does that seem okay? If so, I'll send the patch...

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

Re: [PATCH v2] x86/boot: Rename overlapping memcpy() to memmove()

[One Thousand Gnomes]

O> For example, this is what I've got currently:
> 
> /* Detect and warn about potential overlaps. */
> void *memcpy(void *dest, const void *src, size_t n)
> {
>         if (dest > src && dest - src < n)
>                 warn("Potentially unsafe overlapping memcpy detected!");
>         return __memcpy(dest, src, n);
> }
> 
> Does that seem okay? If so, I'll send the patch...

Probably useful for debug, but instead of relying on __memcpy
happening to handle overlaps - which isn't portable you could instead
debug all platforms by doing

	if (...) {
		warn(...)
		memmove()
	} else
		__memcpy


Alan

Re: [PATCH v2] x86/boot: Rename overlapping memcpy() to memmove()

[Kees Cook]

On Thu, Apr 28, 2016 at 9:47 AM, One Thousand Gnomes
<gnomes@lxorguk.ukuu.org.uk> wrote:
> O> For example, this is what I've got currently:
>>
>> /* Detect and warn about potential overlaps. */
>> void *memcpy(void *dest, const void *src, size_t n)
>> {
>>         if (dest > src && dest - src < n)
>>                 warn("Potentially unsafe overlapping memcpy detected!");
>>         return __memcpy(dest, src, n);
>> }
>>
>> Does that seem okay? If so, I'll send the patch...
>
> Probably useful for debug, but instead of relying on __memcpy
> happening to handle overlaps - which isn't portable you could instead
> debug all platforms by doing
>
>         if (...) {
>                 warn(...)
>                 memmove()
>         } else
>                 __memcpy
>

Yeah, that's kind of where we started (but without the warning). I
prefer this, since we don't run the risk of MAYBE breaking. We warn,
but we remain safe.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

Re: [PATCH v2] x86/boot: Rename overlapping memcpy() to memmove()

[Ingo Molnar]

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

> On Thu, Apr 28, 2016 at 9:47 AM, One Thousand Gnomes
> <gnomes@lxorguk.ukuu.org.uk> wrote:
> > O> For example, this is what I've got currently:
> >>
> >> /* Detect and warn about potential overlaps. */
> >> void *memcpy(void *dest, const void *src, size_t n)
> >> {
> >>         if (dest > src && dest - src < n)
> >>                 warn("Potentially unsafe overlapping memcpy detected!");
> >>         return __memcpy(dest, src, n);
> >> }
> >>
> >> Does that seem okay? If so, I'll send the patch...
> >
> > Probably useful for debug, but instead of relying on __memcpy
> > happening to handle overlaps - which isn't portable you could instead
> > debug all platforms by doing
> >
> >         if (...) {
> >                 warn(...)
> >                 memmove()
> >         } else
> >                 __memcpy
> >
> 
> Yeah, that's kind of where we started (but without the warning). I
> prefer this, since we don't run the risk of MAYBE breaking. We warn,
> but we remain safe.

Works for me!

Thanks,

	Ingo