Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753940AbcDOT22 (ORCPT ); Fri, 15 Apr 2016 15:28:28 -0400 Received: from mail-wm0-f49.google.com ([74.125.82.49]:37933 "EHLO mail-wm0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753909AbcDOT2H (ORCPT ); Fri, 15 Apr 2016 15:28:07 -0400 MIME-Version: 1.0 In-Reply-To: <20160415174231.7425bc7a@tukaani.org> References: <1460672954-32567-1-git-send-email-keescook@chromium.org> <1460672954-32567-14-git-send-email-keescook@chromium.org> <20160415174231.7425bc7a@tukaani.org> Date: Fri, 15 Apr 2016 12:28:05 -0700 X-Google-Sender-Auth: eUoAzTMfIef0NQ63wEwbz_8_3pQ Message-ID: Subject: Re: [PATCH v5 13/21] x86, boot: Report overlap failures in memcpy From: Kees Cook To: Lasse Collin Cc: Ingo Molnar , Yinghai Lu , Baoquan He , Ard Biesheuvel , Matt Redfearn , "x86@kernel.org" , "H. Peter Anvin" , Ingo Molnar , Borislav Petkov , Vivek Goyal , Andy Lutomirski , Andrew Morton , Dave Young , "kernel-hardening@lists.openwall.com" , 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: 1634 Lines: 49 On Fri, Apr 15, 2016 at 7:42 AM, Lasse Collin wrote: > On 2016-04-14 Kees Cook wrote: >> From: Yinghai Lu >> >> parse_elf is using a local memcpy to move sections to their final >> running position. However, this memcpy only supports non-overlapping >> arguments (or dest < src). > > The same copy of memcpy is used by the decompressors too. > >> To avoid future hard-to-debug surprises, this adds checking in memcpy >> to detect the unhandled condition (which should not be happening >> currently). > > It's already a minor surprise that memcpy is expected to work for > overlapping buffers at all. It could be good to have a comment about > it because "scroll" and parse_elf seem to rely on it. > > On the other hand, the new code and error message take quite a few bytes > of space, so a complete memmove can be smaller: > > void *memmove(void *dest, const void *src, size_t n) > { > unsigned char *d = dest; > const unsigned char *s = src; > > if (d <= s || d - s >= n) > return __memcpy(dest, src, n); > > while (n-- > 0) > d[n] = s[n]; > > return dest; > } > > Note that memmove is needed by lib/decompress_unxz.c. It contains its > own small version inside a "#ifndef memmove" block. That #ifndef should > be taken into account when adding a memmove symbol. Changing > decompress_unxz.c is fine but then one needs to think about other > archs too. Awesome, thanks! I'd much prefer to fully fix this instead of just throwing a warning. I'll get this added. -Kees -- Kees Cook Chrome OS & Brillo Security