From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751846AbcDOOuI (ORCPT ); Fri, 15 Apr 2016 10:50:08 -0400 Received: from mailscanner02.zoner.fi ([84.34.166.11]:52801 "EHLO mailscanner02.zoner.fi" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750933AbcDOOuF (ORCPT ); Fri, 15 Apr 2016 10:50:05 -0400 X-Greylist: delayed 444 seconds by postgrey-1.27 at vger.kernel.org; Fri, 15 Apr 2016 10:50:05 EDT Date: Fri, 15 Apr 2016 17:42:31 +0300 From: Lasse Collin To: Kees Cook 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 Subject: Re: [PATCH v5 13/21] x86, boot: Report overlap failures in memcpy Message-ID: <20160415174231.7425bc7a@tukaani.org> In-Reply-To: <1460672954-32567-14-git-send-email-keescook@chromium.org> References: <1460672954-32567-1-git-send-email-keescook@chromium.org> <1460672954-32567-14-git-send-email-keescook@chromium.org> X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; x86_64-unknown-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Antivirus-Scanner: Clean mail though you should still use an Antivirus Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. -- Lasse Collin | IRC: Larhzu @ IRCnet & Freenode From mboxrd@z Thu Jan 1 00:00:00 1970 Reply-To: kernel-hardening@lists.openwall.com Date: Fri, 15 Apr 2016 17:42:31 +0300 From: Lasse Collin Message-ID: <20160415174231.7425bc7a@tukaani.org> In-Reply-To: <1460672954-32567-14-git-send-email-keescook@chromium.org> References: <1460672954-32567-1-git-send-email-keescook@chromium.org> <1460672954-32567-14-git-send-email-keescook@chromium.org> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: [kernel-hardening] Re: [PATCH v5 13/21] x86, boot: Report overlap failures in memcpy To: Kees Cook 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 List-ID: 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. -- Lasse Collin | IRC: Larhzu @ IRCnet & Freenode