From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753584AbcCHFaS (ORCPT ); Tue, 8 Mar 2016 00:30:18 -0500 Received: from mx1.redhat.com ([209.132.183.28]:42186 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751498AbcCHFaO (ORCPT ); Tue, 8 Mar 2016 00:30:14 -0500 Date: Tue, 8 Mar 2016 13:30:05 +0800 From: Baoquan He To: Kees Cook Cc: LKML , Yinghai Lu , "H. Peter Anvin" , Vivek Goyal , Ingo Molnar , Borislav Petkov , Andy Lutomirski , lasse.collin@tukaani.org, Andrew Morton , Dave Young Subject: Re: [PATCH v3 11/19] x86, boot: Add checking for memcpy Message-ID: <20160308053005.GH2481@x1.redhat.com> References: <1457108717-12191-1-git-send-email-bhe@redhat.com> <1457108717-12191-12-git-send-email-bhe@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Tue, 08 Mar 2016 05:30:13 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 03/07/16 at 03:36pm, Kees Cook wrote: > On Fri, Mar 4, 2016 at 8:25 AM, Baoquan He wrote: > > From: Yinghai Lu > > > > parse_elf is using local memcpy to move section to running position. > > That memcpy actually only support no overlapping case or when dest < src. > > > > Add checking in memcpy to find out the wrong case for future use, at > > that time we will need to have backward memcpy for it. > > > > Also add comments in parse_elf about the fact. > > Seems like this would be better to just fix the memcpy to handle the overlap? Yeah, agree. I will remove the code comment. > > -Kees > > > > > Signed-off-by: Yinghai Lu > > --- > > v2->v3: > > Add a declaration for error() since its declaration is in misc.h. > > But it's not included in compressed/string.c. > > > > arch/x86/boot/compressed/misc.c | 14 +++++++------- > > arch/x86/boot/compressed/misc.h | 2 ++ > > arch/x86/boot/compressed/string.c | 29 +++++++++++++++++++++++++++-- > > 3 files changed, 36 insertions(+), 9 deletions(-) > > > > diff --git a/arch/x86/boot/compressed/misc.c b/arch/x86/boot/compressed/misc.c > > index dd7ed8a..4b2cd0c 100644 > > --- a/arch/x86/boot/compressed/misc.c > > +++ b/arch/x86/boot/compressed/misc.c > > @@ -114,9 +114,6 @@ > > #undef memset > > #define memzero(s, n) memset((s), 0, (n)) > > > > - > > -static void error(char *m); > > - > > /* > > * This is set up by the setup-routine at boot-time > > */ > > @@ -243,7 +240,7 @@ void __puthex(unsigned long value) > > } > > } > > > > -static void error(char *x) > > +void error(char *x) > > { > > error_putstr("\n\n"); > > error_putstr(x); > > @@ -378,9 +375,12 @@ static void parse_elf(void *output) > > #else > > dest = (void *)(phdr->p_paddr); > > #endif > > - memcpy(dest, > > - output + phdr->p_offset, > > - phdr->p_filesz); > > + /* > > + * simple version memcpy only can work when dest is > > + * smaller than src or no overlapping. > > + * Here dest is smaller than src always. > > + */ > > + memcpy(dest, output + phdr->p_offset, phdr->p_filesz); > > break; > > default: /* Ignore other PT_* */ break; > > } > > diff --git a/arch/x86/boot/compressed/misc.h b/arch/x86/boot/compressed/misc.h > > index 11736a6..39d0e9a 100644 > > --- a/arch/x86/boot/compressed/misc.h > > +++ b/arch/x86/boot/compressed/misc.h > > @@ -38,6 +38,8 @@ void __puthex(unsigned long value); > > #define error_putstr(__x) __putstr(__x) > > #define error_puthex(__x) __puthex(__x) > > > > +void error(char *x); > > + > > #ifdef CONFIG_X86_VERBOSE_BOOTUP > > > > #define debug_putstr(__x) __putstr(__x) > > diff --git a/arch/x86/boot/compressed/string.c b/arch/x86/boot/compressed/string.c > > index 00e788b..3a935d0 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 > > -void *memcpy(void *dest, const void *src, size_t n) > > +void *__memcpy(void *dest, const void *src, size_t n) > > { > > int d0, d1, d2; > > asm volatile( > > @@ -15,7 +15,7 @@ void *memcpy(void *dest, const void *src, size_t n) > > return dest; > > } > > #else > > -void *memcpy(void *dest, const void *src, size_t n) > > +void *__memcpy(void *dest, const void *src, size_t n) > > { > > long d0, d1, d2; > > asm volatile( > > @@ -30,6 +30,31 @@ void *memcpy(void *dest, const void *src, size_t n) > > } > > #endif > > > > +extern void error(char *x); > > +void *memcpy(void *dest, const void *src, size_t n) > > +{ > > + unsigned long start_dest, end_dest; > > + unsigned long start_src, end_src; > > + unsigned long max_start, min_end; > > + > > + if (dest < src) > > + return __memcpy(dest, src, n); > > + > > + start_dest = (unsigned long)dest; > > + end_dest = (unsigned long)dest + n; > > + start_src = (unsigned long)src; > > + end_src = (unsigned long)src + n; > > + max_start = (start_dest > start_src) ? start_dest : start_src; > > + min_end = (end_dest < end_src) ? end_dest : end_src; > > + > > + if (max_start >= min_end) > > + return __memcpy(dest, src, n); > > + > > + error("memcpy does not support overlapping with dest > src!\n"); > > + > > + return dest; > > +} > > + > > void *memset(void *s, int c, size_t n) > > { > > int i; > > -- > > 2.5.0 > > > > > > -- > Kees Cook > Chrome OS & Brillo Security