Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753735AbcDVWSM (ORCPT ); Fri, 22 Apr 2016 18:18:12 -0400 Received: from mail-wm0-f50.google.com ([74.125.82.50]:38234 "EHLO mail-wm0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752623AbcDVWSI (ORCPT ); Fri, 22 Apr 2016 18:18:08 -0400 MIME-Version: 1.0 In-Reply-To: <20160422074953.GC7352@gmail.com> References: <1461185746-8017-1-git-send-email-keescook@chromium.org> <1461185746-8017-5-git-send-email-keescook@chromium.org> <20160422074953.GC7352@gmail.com> Date: Fri, 22 Apr 2016 15:18:07 -0700 X-Google-Sender-Auth: WUd8dSxJyoIgixjVuDJDsEfOC_E Message-ID: Subject: Re: [PATCH 4/5] x86, boot: Make memcpy handle overlaps From: Kees Cook To: Ingo Molnar 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 , LKML Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2987 Lines: 76 On Fri, Apr 22, 2016 at 12:49 AM, Ingo Molnar wrote: > > * 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? So... this isn't exactly clearly to me. I assume that something about the builtin memcpy doesn't work during compressed boot, so compressed/string.c needed to explicitly overload them. If that matches your understanding, I can add a comment to that effect. > 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? Lasse asked for this too, but I'm going to avoid poking at the decompressor code and just use the interface it already defines: the "memmove" define. > 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. Yeah, seems like it's not hard to add memmove! :) -Kees -- Kees Cook Chrome OS & Brillo Security