From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933730AbdDETXb (ORCPT ); Wed, 5 Apr 2017 15:23:31 -0400 Received: from fllnx210.ext.ti.com ([198.47.19.17]:13895 "EHLO fllnx210.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751069AbdDETXV (ORCPT ); Wed, 5 Apr 2017 15:23:21 -0400 Subject: Re: [PATCH] misc: sram-exec: Use aligned fncpy instead of memcpy To: Greg Kroah-Hartman , Arnd Bergmann , Tony Lindgren , Russell King References: <20170405192120.1009-1-d-gerlach@ti.com> CC: , , , Shawn Guo , Alexandre Belloni , Keerthy J From: Dave Gerlach Message-ID: Date: Wed, 5 Apr 2017 14:22:33 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: <20170405192120.1009-1-d-gerlach@ti.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [128.247.83.19] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. I originally split the sram-exec code into its own file as it already depends on the changes you made to set_memory_* APIs for ARM which we have a hard dependency on here, and not all platforms support this. So this allowed me to constrain the sram_exec code to platforms with the proper set_memory_* APIs defined, but also now this lets us directly use the fncpy macro in this driver. For future platforms that want to make use of sram_exec we set the constraint that an arch must: * Support the required set_memory_* APIs * Define a fncpy macro that guarantees safe movement of a function. This seems reasonable to me and gives support for ARM right away with a path forward for additional architectures to support sram_exec. Regards, Dave [1] https://www.spinics.net/lists/arm-kernel/msg574481.html From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dave Gerlach Subject: Re: [PATCH] misc: sram-exec: Use aligned fncpy instead of memcpy Date: Wed, 5 Apr 2017 14:22:33 -0500 Message-ID: References: <20170405192120.1009-1-d-gerlach@ti.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20170405192120.1009-1-d-gerlach@ti.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Greg Kroah-Hartman , Arnd Bergmann , Tony Lindgren , Russell King Cc: Keerthy J , linux-kernel@vger.kernel.org, Alexandre Belloni , linux-omap@vger.kernel.org, Shawn Guo , linux-arm-kernel@lists.infradead.org List-Id: linux-omap@vger.kernel.org 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. I originally split the sram-exec code into its own file as it already depends on the changes you made to set_memory_* APIs for ARM which we have a hard dependency on here, and not all platforms support this. So this allowed me to constrain the sram_exec code to platforms with the proper set_memory_* APIs defined, but also now this lets us directly use the fncpy macro in this driver. For future platforms that want to make use of sram_exec we set the constraint that an arch must: * Support the required set_memory_* APIs * Define a fncpy macro that guarantees safe movement of a function. This seems reasonable to me and gives support for ARM right away with a path forward for additional architectures to support sram_exec. Regards, Dave [1] https://www.spinics.net/lists/arm-kernel/msg574481.html From mboxrd@z Thu Jan 1 00:00:00 1970 From: d-gerlach@ti.com (Dave Gerlach) Date: Wed, 5 Apr 2017 14:22:33 -0500 Subject: [PATCH] misc: sram-exec: Use aligned fncpy instead of memcpy In-Reply-To: <20170405192120.1009-1-d-gerlach@ti.com> References: <20170405192120.1009-1-d-gerlach@ti.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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. I originally split the sram-exec code into its own file as it already depends on the changes you made to set_memory_* APIs for ARM which we have a hard dependency on here, and not all platforms support this. So this allowed me to constrain the sram_exec code to platforms with the proper set_memory_* APIs defined, but also now this lets us directly use the fncpy macro in this driver. For future platforms that want to make use of sram_exec we set the constraint that an arch must: * Support the required set_memory_* APIs * Define a fncpy macro that guarantees safe movement of a function. This seems reasonable to me and gives support for ARM right away with a path forward for additional architectures to support sram_exec. Regards, Dave [1] https://www.spinics.net/lists/arm-kernel/msg574481.html