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

[PATCH] 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>
---
For tip:x86/boot, improves "x86/boot: Make memcpy() handle overlaps"
---
 arch/x86/boot/compressed/misc.c   |  6 ++++--
 arch/x86/boot/compressed/string.c | 18 ++++++++++--------
 2 files changed, 14 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..29b23cfabf7b 100644
--- a/arch/x86/boot/compressed/string.c
+++ b/arch/x86/boot/compressed/string.c
@@ -1,7 +1,13 @@
+/*
+ * This provides an optimized implementation of memcpy, and a simplified
+ * implementation of memset and memmove, to avoid problems with the
+ * built-in implementations when running in the restricted decompression
+ * stub 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 +21,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 +46,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] x86/boot: Rename overlapping memcpy() to memmove()

[Ingo Molnar]

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

> --- a/arch/x86/boot/compressed/string.c
> +++ b/arch/x86/boot/compressed/string.c
> @@ -1,7 +1,13 @@
> +/*
> + * This provides an optimized implementation of memcpy, and a simplified
> + * implementation of memset and memmove, to avoid problems with the
> + * built-in implementations when running in the restricted decompression
> + * stub environment.
> + */

Does 'built in' here mean the compiler's implementation?

We cannot call kernel built-in functions yet, so we have to duplicate everything 
we might need, right?

Thanks,

	Ingo

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

[Kees Cook]

On Sat, Apr 23, 2016 at 4:08 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Kees Cook <keescook@chromium.org> wrote:
>
>> --- a/arch/x86/boot/compressed/string.c
>> +++ b/arch/x86/boot/compressed/string.c
>> @@ -1,7 +1,13 @@
>> +/*
>> + * This provides an optimized implementation of memcpy, and a simplified
>> + * implementation of memset and memmove, to avoid problems with the
>> + * built-in implementations when running in the restricted decompression
>> + * stub environment.
>> + */
>
> Does 'built in' here mean the compiler's implementation?
>
> We cannot call kernel built-in functions yet, so we have to duplicate everything
> we might need, right?

Right, I actually mean both: we can use neither gcc nor kernel
built-ins. (I am fuzzy on why the gcc built-ins aren't available -- I
think because they're not available for standalone builds.)

-Kees

>
> Thanks,
>
>         Ingo



-- 
Kees Cook
Chrome OS & Brillo Security

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

[Ingo Molnar]

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

> On Sat, Apr 23, 2016 at 4:08 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * Kees Cook <keescook@chromium.org> wrote:
> >
> >> --- a/arch/x86/boot/compressed/string.c
> >> +++ b/arch/x86/boot/compressed/string.c
> >> @@ -1,7 +1,13 @@
> >> +/*
> >> + * This provides an optimized implementation of memcpy, and a simplified
> >> + * implementation of memset and memmove, to avoid problems with the
> >> + * built-in implementations when running in the restricted decompression
> >> + * stub environment.
> >> + */
> >
> > Does 'built in' here mean the compiler's implementation?
> >
> > We cannot call kernel built-in functions yet, so we have to duplicate everything
> > we might need, right?
> 
> Right, I actually mean both: we can use neither gcc nor kernel
> built-ins. (I am fuzzy on why the gcc built-ins aren't available -- I
> think because they're not available for standalone builds.)

I think part of it is that we simply don't trust libgcc: it might be using FPU ops 
or it might start doing something silly from a kernel perspective while 
language-lawyering their way out of the regression with some sort of 'we never 
promised to keep that kind of detail stable'.

The smaller the cross-surface to a historically compatibility-breakage-happy 
compiler like GCC the better.

Thanks,

	Ingo