Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752991AbcDVHt7 (ORCPT ); Fri, 22 Apr 2016 03:49:59 -0400 Received: from mail-wm0-f68.google.com ([74.125.82.68]:35472 "EHLO mail-wm0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751972AbcDVHt5 (ORCPT ); Fri, 22 Apr 2016 03:49:57 -0400 Date: Fri, 22 Apr 2016 09:49:53 +0200 From: Ingo Molnar To: Kees Cook Cc: Yinghai Lu , Baoquan He , Ingo Molnar , x86@kernel.org, Andrew Morton , Andrey Ryabinin , Dmitry Vyukov , "H.J. Lu" , Josh Poimboeuf , Borislav Petkov , Andy Lutomirski , linux-kernel@vger.kernel.org Subject: Re: [PATCH 4/5] x86, boot: Make memcpy handle overlaps Message-ID: <20160422074953.GC7352@gmail.com> References: <1461185746-8017-1-git-send-email-keescook@chromium.org> <1461185746-8017-5-git-send-email-keescook@chromium.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1461185746-8017-5-git-send-email-keescook@chromium.org> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2351 Lines: 62 * Kees Cook 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 > Suggested-by: Lasse Collin > Signed-off-by: Kees Cook > --- > 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