Re: arm32 insecure W+X mapping

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Mon, Sep 20, 2021 at 03:53:24PM -0700, Tim Harvey wrote:
> On Mon, Sep 20, 2021 at 2:13 PM Russell King (Oracle)
> <linux@armlinux.org.uk> wrote:
> > I'm not sure we are disagreeing. I don't have CONFIG_DEBUG_WX enabled,
> > but in 5.13, I see in /sys/kernel/debug/kernel_page_tables:
> >
> > 0xf087d000-0xf087e000           4K KERNEL      RW x  SHD MEM/BUFFERABLE/WC
> >
> > and /proc/vmallocinfo has:
> >
> > 0xf087d000-0xf087f000    8192 imx6_pm_common_init+0x13c/0x390 phys=0x00900000 ioremap
> >
> > So this will give a W+X failure.
> >
> > Under 5.14, there is no mapping for this RAM in kernel_page_tables nor
> > vmallocinfo - which is not surprising because imx6_pm_common_init()
> > said it failed to find the ocram, and it only gets one shot at it.
> > So there won't be a W+X failure.
> >
> > In other words, we are in complete agreement.
> >
> 
> Ok - makes sense.
> 
> I bisected this to cc8870bf4c3a ("ARM: imx6q: drop
> of_platform_default_populate() from init_machine").
> 
> After that patch we get:
> [    0.133082] imx6q_suspend_init: failed to find ocram device!
> 
> and no longer see the W+X failure.
> 
> Fabio, I suspect this is the regression that you are hitting regarding
> suspend and that this needs to be reverted.

I agree - the assumption in the commit message for that commit is
incomplete:

        imx6q_enet_phy_init();
-
-       of_platform_default_populate(NULL, NULL, NULL);
-
        imx_anatop_init();
        cpu_is_imx6q() ?  imx6q_pm_init() : imx6dl_pm_init();

Both imx6q_pm_init() and imx6dl_pm_init(), which then eventually call
into imx6q_suspend_init(), which wants the OCRAM device to exist. That
only happens by that point due to the of_platform_default_populate()
call just above.

Shawn - please can we get cc8870bf4c3a ("ARM: imx6q: drop of_platform_default_populate() from init_machine")
reverted? Thanks.

> That will still leave the W+X issue needing to be fixed at some point.

Yes, also agreed.

I think what I'd like to see is a wrapper around set_memory_ro() that
gets passed the __iomem pointer and size. change_memory_common()
should accept it as the ioremap() will be located in the vmalloc
area which change_memory_common() accepts.

Probably something like:

void __arm_iomem_set_ro(void __iomem *ptr, size_t size)
{
	set_memory_ro((unsigned long)ptr, PAGE_ALIGN(size) / PAGE_SIZE);
}

in arch/arm/mm/ioremap.c would be nice, just after __arm_ioremap_exec().
I've probably just written the patch in wordy words. :) Something like
this (untested):

diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
index f74944c6fe8d..c576fa7d9bf8 100644
--- a/arch/arm/include/asm/io.h
+++ b/arch/arm/include/asm/io.h
@@ -138,6 +138,7 @@ extern void __iomem *__arm_ioremap_caller(phys_addr_t, size_t, unsigned int,
 	void *);
 extern void __iomem *__arm_ioremap_pfn(unsigned long, unsigned long, size_t, unsigned int);
 extern void __iomem *__arm_ioremap_exec(phys_addr_t, size_t, bool cached);
+void __arm_iomem_set_ro(void __iomem *ptr, size_t size);
 extern void __iounmap(volatile void __iomem *addr);
 
 extern void __iomem * (*arch_ioremap_caller)(phys_addr_t, size_t,
diff --git a/arch/arm/mach-imx/pm-imx6.c b/arch/arm/mach-imx/pm-imx6.c
index 9244437cb1b9..5c16257872a5 100644
--- a/arch/arm/mach-imx/pm-imx6.c
+++ b/arch/arm/mach-imx/pm-imx6.c
@@ -571,6 +571,8 @@ static int __init imx6q_suspend_init(const struct imx6_pm_socdata *socdata)
 		&imx6_suspend,
 		MX6Q_SUSPEND_OCRAM_SIZE - sizeof(*pm_info));
 
+	__arm_iomem_set_ro(suspend_ocram_base, MX6Q_SUSPEND_OCRAM_SIZE);
+
 	goto put_device;
 
 pl310_cache_map_failed:
diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
index 80fb5a4a5c05..6e830b9418c9 100644
--- a/arch/arm/mm/ioremap.c
+++ b/arch/arm/mm/ioremap.c
@@ -36,6 +36,7 @@
 #include <asm/mmu_context.h>
 #include <asm/pgalloc.h>
 #include <asm/tlbflush.h>
+#include <asm/set_memory.h>
 #include <asm/system_info.h>
 
 #include <asm/mach/map.h>
@@ -401,6 +402,11 @@ __arm_ioremap_exec(phys_addr_t phys_addr, size_t size, bool cached)
 			__builtin_return_address(0));
 }
 
+void __arm_iomem_set_ro(void __iomem *ptr, size_t size)
+{
+	set_memory_ro((unsigned long)ptr, PAGE_ALIGN(size) / PAGE_SIZE);
+}
+
 void *arch_memremap_wb(phys_addr_t phys_addr, size_t size)
 {
 	return (__force void *)arch_ioremap_caller(phys_addr, size,

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel