From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755674AbdDFTIU (ORCPT ); Thu, 6 Apr 2017 15:08:20 -0400 Received: from pandora.armlinux.org.uk ([78.32.30.218]:58508 "EHLO pandora.armlinux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933062AbdDFTIC (ORCPT ); Thu, 6 Apr 2017 15:08:02 -0400 Date: Thu, 6 Apr 2017 20:07:47 +0100 From: Russell King - ARM Linux To: Dave Gerlach Cc: Greg Kroah-Hartman , Arnd Bergmann , Tony Lindgren , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-omap@vger.kernel.org, Shawn Guo , Alexandre Belloni , Keerthy J Subject: Re: [PATCH] misc: sram-exec: Use aligned fncpy instead of memcpy Message-ID: <20170406190747.GP23750@n2100.armlinux.org.uk> References: <20170405192120.1009-1-d-gerlach@ti.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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 On Wed, Apr 05, 2017 at 02:22:33PM -0500, Dave Gerlach wrote: > Russell, > On 04/05/2017 02:21 PM, Dave Gerlach wrote: > >Currently the sram-exec functionality, which allows allocation of > >executable memory and provides an API to move code to it, is only > >selected in configs for the ARM architecture. Based on commit > >5756e9dd0de6 ("ARM: 6640/1: Thumb-2: Symbol manipulation macros for > >function body copying") simply copying a C function pointer address > >using memcpy without consideration of alignment and Thumb is unsafe on > >ARM platforms. > > > >The aforementioned patch introduces the fncpy macro which is a safe way > >to copy executable code on ARM platforms, so let's make use of that here > >rather than the unsafe plain memcpy that was previously used by > >sram_exec_copy. > > > >In the future, architectures hoping to make use of the sram-exec > >functionality must define an fncpy macro just as ARM has done to > >guarantee or check for safe copying to executable memory before allowing > >the arch to select CONFIG_SRAM_EXEC. > > > >Signed-off-by: Dave Gerlach > >--- > > drivers/misc/sram-exec.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > >diff --git a/drivers/misc/sram-exec.c b/drivers/misc/sram-exec.c > >index ac522417c462..0057eabe5c03 100644 > >--- a/drivers/misc/sram-exec.c > >+++ b/drivers/misc/sram-exec.c > >@@ -19,6 +19,7 @@ > > #include > > > > #include > >+#include > > > > #include "sram.h" > > > >@@ -93,7 +94,7 @@ int sram_exec_copy(struct gen_pool *pool, void *dst, void *src, > > set_memory_nx((unsigned long)base, pages); > > set_memory_rw((unsigned long)base, pages); > > > >- memcpy(dst, src, size); > >+ fncpy(dst, src, size); > > > > set_memory_ro((unsigned long)base, pages); > > set_memory_x((unsigned long)base, pages); > > > > Does this address your concerns from here [1]? Because the only user of this > code is ARM right now I already only build the sram-exec code in if > CONFIG_ARM is selected. Sorry, it does not. Please read the comments in asm/fncpy.h. Deviating from the proscribed usage means your code is, quite simply, buggy. There's no two ways about that. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.