Re: [PATCH 4/5] x86, boot: Make memcpy handle overlaps

From: Ingo Molnar
Date: Fri Apr 22 2016 - 03:50:01 EST

* Kees Cook <keescook@xxxxxxxxxxxx> wrote:

> Two uses of memcpy (screen scrolling and ELF parsing) were handling
> overlapping memory areas. While there were no explicitly noticed bugs
> here (yet), it is best to fix this so that the copying will always be
> safe.
> Instead of making a new memmove function that might collide with other
> memmove definitions in the decompressors, this just makes the compressed
> boot's copy of memcpy overlap safe.
> Reported-by: Yinghai Lu <yinghai@xxxxxxxxxx>
> Suggested-by: Lasse Collin <lasse.collin@xxxxxxxxxxx>
> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
> ---
> arch/x86/boot/compressed/misc.c | 4 +---
> arch/x86/boot/compressed/string.c | 22 ++++++++++++++++++++--
> 2 files changed, 21 insertions(+), 5 deletions(-)
> diff --git a/arch/x86/boot/compressed/string.c b/arch/x86/boot/compressed/string.c
> index 00e788be1db9..1e10e40f49dd 100644
> --- a/arch/x86/boot/compressed/string.c
> +++ b/arch/x86/boot/compressed/string.c
> @@ -1,7 +1,7 @@
> #include "../string.c"
> #ifdef CONFIG_X86_32

I've applied this patch, but could you please also do another patch that adds a
comment block to the top of this special version of compressed/string.c, which
explains why this file exists and what its purpose is?


+ * 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)

I'd not name it memcpy() if its semantics are not the same as the regular kernel
memcpy() - that will only cause confusion later on.

I'd try to name it memmove() and would fix the memmove() hacks in decompressors:

lib/decompress_unxz.c:#ifndef memmove
lib/decompress_unxz.c:void *memmove(void *dest, const void *src, size_t size)
lib/decompress_unxz.c: * Since we need memmove anyway, would use it as memcpy too.
lib/decompress_unxz.c:# define memcpy memmove

any strong reason this cannot be done?

Some other decompressors seem to avoid memmove() intentionally:

lib/decompress_bunzip2.c: *by 256 in any case, using memmove here would
lib/decompress_unlzo.c: * of the buffer. This way memmove() isn't needed which
lib/decompress_unlzo.c: * Use a loop to avoid memmove() dependency.