diff mbox series

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

Message ID 20200915203037.GA13650@martinet.casa.ct
State New
Headers show
Series ARM: imx: W+X ocram page in imx6q. Might someone with working imx6 suspend please test ? | expand

Commit Message

Xavi Drudis Ferran Sept. 15, 2020, 8:30 p.m. UTC
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>

Comments

Shawn Guo Sept. 22, 2020, 7:15 a.m. UTC | #1
On Tue, Sep 15, 2020 at 10:30:37PM +0200, Xavi Drudis Ferran wrote:
> 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 ?

I tested it on my imx6q-sabresd board with v5.9-rc6 kernel, and
suspend/resume still works with your patch applied.

There is a build warning with your patch though.

  CC      arch/arm/mach-imx/pm-imx6.o
arch/arm/mach-imx/pm-imx6.c: In function ‘imx6q_suspend_init’:
arch/arm/mach-imx/pm-imx6.c:574:22: warning: passing argument 1 of ‘set_memory_ro’ makes integer from pointer without a cast [-Wint-conversion]
  ret = set_memory_ro(suspend_ocram_base,
                      ^~~~~~~~~~~~~~~~~~
In file included from ./include/linux/set_memory.h:9:0,
                 from arch/arm/mach-imx/pm-imx6.c:19:
./arch/arm/include/asm/set_memory.h:10:5: note: expected ‘long unsigned int’ but argument is of type ‘void *’
 int set_memory_ro(unsigned long addr, int numpages);
     ^~~~~~~~~~~~~

Shawn

> 
> 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
Xavi Drudis Ferran Sept. 22, 2020, 12:02 p.m. UTC | #2
El Tue, Sep 22, 2020 at 03:15:38PM +0800, Shawn Guo deia:
> I tested it on my imx6q-sabresd board with v5.9-rc6 kernel, and
> suspend/resume still works with your patch applied.
>

Thank you very much.

> There is a build warning with your patch though.
>

Silly me. I'm sorry about that. This new version removes the warning.

I've tested on linux-libre-5.8.10 on a wandboard quad (imx6q) and it
works the same with or without the patch.  Hibernate gives a runtine
warning less in 5.8.10 than 5.8.9 (both with or without patch), but
suspend still fails to resume the eth phy (both with or without the patch).

 
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..8baa1307f 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((unsigned long)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:
diff mbox series

Patch

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: