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

[Ingo Molnar <mingo@kernel.org>]

* Kees Cook <keescook@chromium.org> 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@kernel.org>
> Suggested-by: Lasse Collin <lasse.collin@tukaani.org>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  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?

Also:

+/*
+ * 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.

Thanks,

	Ingo