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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160422074953.GC7352@gmail.com>
Date:	Fri, 22 Apr 2016 09:49:53 +0200
From:	Ingo Molnar <mingo@...nel.org>
To:	Kees Cook <keescook@...omium.org>
Cc:	Yinghai Lu <yinghai@...nel.org>, Baoquan He <bhe@...hat.com>,
	Ingo Molnar <mingo@...hat.com>, x86@...nel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Andrey Ryabinin <aryabinin@...tuozzo.com>,
	Dmitry Vyukov <dvyukov@...gle.com>,
	"H.J. Lu" <hjl.tools@...il.com>,
	Josh Poimboeuf <jpoimboe@...hat.com>,
	Borislav Petkov <bp@...e.de>,
	Andy Lutomirski <luto@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/5] x86, boot: Make memcpy handle overlaps

* Kees Cook <keescook@...omium.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@...nel.org>
> Suggested-by: Lasse Collin <lasse.collin@...aani.org>
> Signed-off-by: Kees Cook <keescook@...omium.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