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

[Shawn Guo <shawnguo@kernel.org>]

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

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