[PATCH] ARM: imx: W+X ocram page in imx6q. Might someone with working imx6 suspend please test ?

* [PATCH] ARM: imx: W+X ocram page in imx6q. Might someone with working imx6 suspend please test ?
@ 2020-09-15 20:30 Xavi Drudis Ferran
  2020-09-22  7:15 ` Shawn Guo
  0 siblings, 1 reply; 3+ messages in thread
From: Xavi Drudis Ferran @ 2020-09-15 20:30 UTC (permalink / raw)
  To: linux-arm-kernel; +Cc: Sascha Hauer, Shawn Guo, NXP Linux Team

Hello and thanks for your work.

A custom .config of linux-libre-5.8, linux-libre-5.8.3,
linux-libre-5.8.8 and linux-libre-5.8.9 with CONFIG_DEBUG_WX on
gave me a W+X memory protection warning for the page mapping the
On-Chip RAM used to hold the resume assembler code.  Or at least
the warning goes away with a patch that marks the page read-only
once the code has been copied there.

I've only tested this patch on linux-libre-5.8.9, the other versions
were tested with a worse patch that at least seemed to confirm the ocram
suspend page had the W+X. Thanks to Laura Abbott for pointing me to
set_memory_ro.

I don't have any exploit, not sure how possible/difficult it would be,
and I'm not sure what impact this could have. I haven't looked at what
would be the first version affected (since suspend for imx6 maybe?).

I'm not sure whether the patch breaks anything. In my tests I have
the same problems with suspend to ram and hibernation on a
Wandboard Quad SBC, both with or without my patch. So maybe I
should wait until I have working suspend and hibernation or maybe
someone with a working suspend on imx6q could test it ?

It did not seem to me, reading the assembler in that page
(arch/arm/mach-imx/suspend-imx6.S), that it could need write access to
its page (only for loading the code there), but my assembler skill is
not very good.

Completely new here, so thanks for any tips. Do you need more info,
logs, etc.? 

Also, with this patch if all went well but the page could not be made
readonly imx6q_suspend_init will return error (because of a potential
vulnerability that shouldn't be there). But it could also be argued
that since suspend should work just fine with W+X it should only print
the warning and return 0?

Thank you very much.

The patch would be:


imx6q: Mark the SRAM page for resume code read-only,executable (once written)

The code used for resume after suspend in imx6 is copied to a SRAM
on chip, so that it stays in the CPU chip while the system is
partially powered down.  The page mapping that SRAM was marked as
RW+X, which gave a warning when CONFIG_DEBUG_WX=y. Mark it
read-only and executable once the writting from kernel text is done
to get rid of the warning and prevent possible exploitation.

Signed-off-by: Xavier Drudis Ferran <xdrudis@tinet.cat>

diff --git a/arch/arm/mach-imx/pm-imx6.c b/arch/arm/mach-imx/pm-imx6.c
index 40c74b4c4..61f296599 100644
--- a/arch/arm/mach-imx/pm-imx6.c
+++ b/arch/arm/mach-imx/pm-imx6.c
@@ -16,6 +16,7 @@
 #include <linux/of_platform.h>
 #include <linux/regmap.h>
 #include <linux/suspend.h>
+#include <linux/set_memory.h>
 #include <asm/cacheflush.h>
 #include <asm/fncpy.h>
 #include <asm/proc-fns.h>
@@ -570,6 +571,13 @@ static int __init imx6q_suspend_init(const struct imx6_pm_socdata *socdata)
 		&imx6_suspend,
 		MX6Q_SUSPEND_OCRAM_SIZE - sizeof(*pm_info));
 
+	ret = set_memory_ro(suspend_ocram_base,
+			    __KERNEL_DIV_ROUND_UP(MX6Q_SUSPEND_OCRAM_SIZE, PAGE_SIZE));
+	if (ret) {
+		pr_warn("%s: failed to mark executable on chip SRAM as read only after writing to it: %d!\n",
+			__func__, ret);
+	}
+
 	goto put_device;
 
 pl310_cache_map_failed:

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

^ permalink raw reply related	[flat|nested] 3+ messages in thread