Nokia N900: insecure W+X mapping at 0xd0050000

Nokia N900: insecure W+X mapping at 0xd0050000

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2448 bytes --]

Hi!

Insecure W+X mappings, who cares about those? I have 7 pages...

								Pavel

[    0.000000] Booting Linux on physical CPU 0x0
[    0.000000] Linux version 4.16.0-rc3-next-20180302 (pavel@duo) (gcc
version 4.7.2 (GC
C)) #70 Fri Mar 2 10:16:00 CET 2018
[    0.000000] CPU: ARMv7 Processor [411fc083] revision 3 (ARMv7),
cr=10c5387d
[    0.000000] CPU: PIPT / VIPT nonaliasing data cache, VIPT
nonaliasing instruction cac
...
[    1.266693] ------------[ cut here ]------------
[    1.271606] WARNING: CPU: 0 PID: 1 at lib/refcount.c:187
refcount_sub_and_test+0x94/0xa8
[    1.280181] refcount_t: underflow; use-after-free.
[    1.285247] Modules linked in:
...
[    4.557220] devtmpfs: mounted
[    4.569610] Freeing unused kernel memory: 1024K
[    4.581481] ------------[ cut here ]------------
[    4.592376] WARNING: CPU: 0 PID: 1 at arch/arm/mm/dump.c:253
note_page+0x2f8/0x328
[    4.606658] arm/mm: Found insecure W+X mapping at address
0xd0050000
[    4.619781] Modules linked in:
[    4.629272] CPU: 0 PID: 1 Comm: swapper Tainted: G        W
4.16.0-rc3-next-20180302 #70
[    4.645111] Hardware name: Nokia RX-51 board
[    4.655944] [<c010d6cc>] (unwind_backtrace) from [<c010b560>]
(show_stack+0x10/0x14)
[    4.670593] [<c010b560>] (show_stack) from [<c0127dec>]
(__warn+0xe8/0x110)
[    4.684295] [<c0127dec>] (__warn) from [<c0127edc>]
(warn_slowpath_fmt+0x38/0x48)
[    4.698455] [<c0127edc>] (warn_slowpath_fmt) from [<c0113aa8>]
(note_page+0x2f8/0x328)
[    4.713134] [<c0113aa8>] (note_page) from [<c0113b60>]
(walk_pgd+0x88/0x178)
[    4.726837] [<c0113b60>] (walk_pgd) from [<c0113d28>]
(ptdump_check_wx+0x64/0xb8)
[    4.741210] [<c0113d28>] (ptdump_check_wx) from [<c0716428>]
(kernel_init+0x24/0x108)
[    4.755859] [<c0716428>] (kernel_init) from [<c01010e8>]
(ret_from_fork+0x14/0x2c)
[    4.770294] Exception stack(0xce049fb0 to 0xce049ff8)
[    4.781921] 9fa0:                                     00000000
00000000 00000000 00000000
[    4.796966] 9fc0: 00000000 00000000 00000000 00000000 00000000
00000000 00000000 00000000
[    4.812011] 9fe0: 00000000 00000000 00000000 00000000 00000013
00000000
[    4.825378] ---[ end trace dcb3a72772bbfe7c ]---
[    4.837005] Checked W+X mappings: FAILED, 7 W+X pages found

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: Nokia N900: insecure W+X mapping at 0xd0050000

[Tony Lindgren]

* Pavel Machek <pavel@ucw.cz> [180308 06:29]:
> Insecure W+X mappings, who cares about those? I have 7 pages...

Is this with CONFIG_DEBUG_WX=y?

My guess is that it's for mapping the PM assembly to SRAM. This
is already fixed for am335x that is using drivers/misc/sram*.c.
I think omap2 - omap4 still need fixing if this is the culprit.

Regards,

Tony

Re: Nokia N900: insecure W+X mapping at 0xd0050000

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [180308 16:25]:
> * Pavel Machek <pavel@ucw.cz> [180308 06:29]:
> > Insecure W+X mappings, who cares about those? I have 7 pages...
> 
> Is this with CONFIG_DEBUG_WX=y?
> 
> My guess is that it's for mapping the PM assembly to SRAM. This
> is already fixed for am335x that is using drivers/misc/sram*.c.
> I think omap2 - omap4 still need fixing if this is the culprit.

Adding Dave to Cc, here's a quick fix for this one.

Regards,

Tony

8< -------------------
>From tony Mon Sep 17 00:00:00 2001
From: Tony Lindgren <tony@atomide.com>
Date: Thu, 8 Mar 2018 08:41:16 -0800
Subject: [PATCH] ARM: OMAP: Fix SRAM W+X mapping

We are still using custom SRAM code for some SoCs and are not marking
the PM code mapped to SRAM as read-only and executable after we're
done. With CONFIG_DEBUG_WX=y, we will get "Found insecure W+X mapping
at address" warning.

Let's fix this issue the same way as commit 728bbe75c82f ("misc: sram:
Introduce support code for protect-exec sram type") is doing for
drivers/misc/sram-exec.c.

Note that eventually we should be using sram-exec.c for all SoCs.

Cc: Dave Gerlach <d-gerlach@ti.com>
Reported-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 arch/arm/plat-omap/sram.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/arch/arm/plat-omap/sram.c b/arch/arm/plat-omap/sram.c
--- a/arch/arm/plat-omap/sram.c
+++ b/arch/arm/plat-omap/sram.c
@@ -23,6 +23,7 @@
 #include <asm/fncpy.h>
 #include <asm/tlb.h>
 #include <asm/cacheflush.h>
+#include <asm/set_memory.h>
 
 #include <asm/mach/map.h>
 
@@ -96,3 +97,21 @@ void __init omap_map_sram(unsigned long start, unsigned long size,
 	memset_io(omap_sram_base + omap_sram_skip, 0,
 		  omap_sram_size - omap_sram_skip);
 }
+
+static int __init omap_sram_lock(void)
+{
+	unsigned long base;
+	int pages;
+
+	if (!omap_sram_base || !omap_sram_size)
+		return 0;
+
+	base = (unsigned long)omap_sram_base;
+	pages = PAGE_ALIGN(omap_sram_size) / PAGE_SIZE;
+
+	set_memory_ro((unsigned long)base, pages);
+	set_memory_x((unsigned long)base, pages);
+
+	return 0;
+}
+late_initcall(omap_sram_lock);
-- 
2.16.2

Re: Nokia N900: insecure W+X mapping at 0xd0050000

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 698 bytes --]

Hi!

> > * Pavel Machek <pavel@ucw.cz> [180308 06:29]:
> > > Insecure W+X mappings, who cares about those? I have 7 pages...
> > 
> > Is this with CONFIG_DEBUG_WX=y?
> > 
> > My guess is that it's for mapping the PM assembly to SRAM. This
> > is already fixed for am335x that is using drivers/misc/sram*.c.
> > I think omap2 - omap4 still need fixing if this is the culprit.
> 
> Adding Dave to Cc, here's a quick fix for this one.

I did a quick testing and warning is gone. Thanks!

Tested-by: Pavel Machek <pavel@ucw.cz>

								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: Nokia N900: insecure W+X mapping at 0xd0050000

[Tony Lindgren]

* Pavel Machek <pavel@ucw.cz> [180308 18:06]:
> Hi!
> 
> > > * Pavel Machek <pavel@ucw.cz> [180308 06:29]:
> > > > Insecure W+X mappings, who cares about those? I have 7 pages...
> > > 
> > > Is this with CONFIG_DEBUG_WX=y?
> > > 
> > > My guess is that it's for mapping the PM assembly to SRAM. This
> > > is already fixed for am335x that is using drivers/misc/sram*.c.
> > > I think omap2 - omap4 still need fixing if this is the culprit.
> > 
> > Adding Dave to Cc, here's a quick fix for this one.
> 
> I did a quick testing and warning is gone. Thanks!

Turns out init time configuration is not enough here.
It causes an oops on omap3 after off mode coming back
from idle. So we need to reconfigure things when
omap_sram_push() is called after idle too.

Below is an updated patch.

Regards,

Tony

8< ----------------------
>From tony Mon Sep 17 00:00:00 2001
From: Tony Lindgren <tony@atomide.com>
Date: Wed, 21 Mar 2018 08:16:29 -0700
Subject: [PATCH] ARM: OMAP: Fix SRAM W+X mapping

We are still using custom SRAM code for some SoCs and are not marking
the PM code mapped to SRAM as read-only and executable after we're
done. With CONFIG_DEBUG_WX=y, we will get "Found insecure W+X mapping
at address" warning.

Let's fix this issue the same way as commit 728bbe75c82f ("misc: sram:
Introduce support code for protect-exec sram type") is doing for
drivers/misc/sram-exec.c.

On omap3, we need to restore SRAM when returning from off mode after
idle, so init time configuration is not enough.

And as we no longer have users for omap_sram_push_address() we can
make it static while at it.

Note that eventually we should be using sram-exec.c for all SoCs.

Cc: stable@vger.kernel.org	# v4.12+
Cc: Dave Gerlach <d-gerlach@ti.com>
Reported-by: Pavel Machek <pavel@ucw.cz>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 arch/arm/plat-omap/include/plat/sram.h | 11 +----------
 arch/arm/plat-omap/sram.c              | 36 +++++++++++++++++++++++++++++++++-
 2 files changed, 36 insertions(+), 11 deletions(-)

diff --git a/arch/arm/plat-omap/include/plat/sram.h b/arch/arm/plat-omap/include/plat/sram.h
--- a/arch/arm/plat-omap/include/plat/sram.h
+++ b/arch/arm/plat-omap/include/plat/sram.h
@@ -5,13 +5,4 @@ void omap_map_sram(unsigned long start, unsigned long size,
 			unsigned long skip, int cached);
 void omap_sram_reset(void);
 
-extern void *omap_sram_push_address(unsigned long size);
-
-/* Macro to push a function to the internal SRAM, using the fncpy API */
-#define omap_sram_push(funcp, size) ({				\
-	typeof(&(funcp)) _res = NULL;				\
-	void *_sram_address = omap_sram_push_address(size);	\
-	if (_sram_address)					\
-		_res = fncpy(_sram_address, &(funcp), size);	\
-	_res;							\
-})
+extern void *omap_sram_push(void *funcp, unsigned long size);
diff --git a/arch/arm/plat-omap/sram.c b/arch/arm/plat-omap/sram.c
--- a/arch/arm/plat-omap/sram.c
+++ b/arch/arm/plat-omap/sram.c
@@ -23,6 +23,7 @@
 #include <asm/fncpy.h>
 #include <asm/tlb.h>
 #include <asm/cacheflush.h>
+#include <asm/set_memory.h>
 
 #include <asm/mach/map.h>
 
@@ -42,7 +43,7 @@ static void __iomem *omap_sram_ceil;
  * Note that fncpy requires the returned address to be aligned
  * to an 8-byte boundary.
  */
-void *omap_sram_push_address(unsigned long size)
+static void *omap_sram_push_address(unsigned long size)
 {
 	unsigned long available, new_ceil = (unsigned long)omap_sram_ceil;
 
@@ -60,6 +61,30 @@ void *omap_sram_push_address(unsigned long size)
 	return (void *)omap_sram_ceil;
 }
 
+void *omap_sram_push(void *funcp, unsigned long size)
+{
+	void *sram;
+	unsigned long base;
+	int pages;
+	void *dst = NULL;
+
+	sram = omap_sram_push_address(size);
+	if (!sram)
+		return NULL;
+
+	base = (unsigned long)sram & PAGE_MASK;
+	pages = PAGE_ALIGN(size) / PAGE_SIZE;
+
+	set_memory_rw(base, pages);
+
+	dst = fncpy(sram, funcp, size);
+
+	set_memory_ro(base, pages);
+	set_memory_x(base, pages);
+
+	return dst;
+}
+
 /*
  * The SRAM context is lost during off-idle and stack
  * needs to be reset.
@@ -75,6 +100,9 @@ void omap_sram_reset(void)
 void __init omap_map_sram(unsigned long start, unsigned long size,
 				 unsigned long skip, int cached)
 {
+	unsigned long base;
+	int pages;
+
 	if (size == 0)
 		return;
 
@@ -95,4 +123,10 @@ void __init omap_map_sram(unsigned long start, unsigned long size,
 	 */
 	memset_io(omap_sram_base + omap_sram_skip, 0,
 		  omap_sram_size - omap_sram_skip);
+
+	base = (unsigned long)omap_sram_base;
+	pages = PAGE_ALIGN(omap_sram_size) / PAGE_SIZE;
+
+	set_memory_ro(base, pages);
+	set_memory_x(base, pages);
 }
-- 
2.16.2