2020-02-17 08:33:07

by Zong Li

[permalink] [raw]
Subject: [PATCH 7/8] riscv: add DEBUG_WX support

Support DEBUG_WX to check whether there are mapping with write and
execute permission at the same time.

Signed-off-by: Zong Li <[email protected]>
---
arch/riscv/Kconfig.debug | 30 ++++++++++++++++++++++++++++++
arch/riscv/include/asm/ptdump.h | 6 ++++++
arch/riscv/mm/init.c | 2 ++
3 files changed, 38 insertions(+)

diff --git a/arch/riscv/Kconfig.debug b/arch/riscv/Kconfig.debug
index e69de29bb2d1..2bcd88e75626 100644
--- a/arch/riscv/Kconfig.debug
+++ b/arch/riscv/Kconfig.debug
@@ -0,0 +1,30 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+config DEBUG_WX
+ bool "Warn on W+X mappings at boot"
+ select PTDUMP_CORE
+ help
+ Generate a warning if any W+X mappings are found at boot.
+
+ This is useful for discovering cases where the kernel is leaving
+ W+X mappings after applying NX, as such mappings are a security risk.
+ This check also includes UXN, which should be set on all kernel
+ mappings.
+
+ Look for a message in dmesg output like this:
+
+ riscv/mm: Checked W+X mappings: passed, no W+X pages found.
+
+ or like this, if the check failed:
+
+ riscv/mm: Checked W+X mappings: FAILED, <N> W+X pages found.
+
+ Note that even if the check fails, your kernel is possibly
+ still fine, as W+X mappings are not a security hole in
+ themselves, what they do is that they make the exploitation
+ of other unfixed kernel bugs easier.
+
+ There is no runtime or memory usage effect of this option
+ once the kernel has booted up - it's a one time check.
+
+ If in doubt, say "Y".
diff --git a/arch/riscv/include/asm/ptdump.h b/arch/riscv/include/asm/ptdump.h
index e29af7191909..eb2a1cc5f22c 100644
--- a/arch/riscv/include/asm/ptdump.h
+++ b/arch/riscv/include/asm/ptdump.h
@@ -8,4 +8,10 @@

void ptdump_check_wx(void);

+#ifdef CONFIG_DEBUG_WX
+#define debug_checkwx() ptdump_check_wx()
+#else
+#define debug_checkwx() do { } while (0)
+#endif
+
#endif /* _ASM_RISCV_PTDUMP_H */
diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
index 09fa643e079c..a05d76e5fefe 100644
--- a/arch/riscv/mm/init.c
+++ b/arch/riscv/mm/init.c
@@ -509,6 +509,8 @@ void mark_rodata_ro(void)
set_memory_ro(rodata_start, (data_start - rodata_start) >> PAGE_SHIFT);
set_memory_nx(rodata_start, (data_start - rodata_start) >> PAGE_SHIFT);
set_memory_nx(data_start, (max_low - data_start) >> PAGE_SHIFT);
+
+ debug_checkwx();
}
#endif

--
2.25.0


2020-03-05 01:44:50

by Palmer Dabbelt

[permalink] [raw]
Subject: Re: [PATCH 7/8] riscv: add DEBUG_WX support

On Mon, 17 Feb 2020 00:32:22 PST (-0800), [email protected] wrote:
> Support DEBUG_WX to check whether there are mapping with write and
> execute permission at the same time.
>
> Signed-off-by: Zong Li <[email protected]>
> ---
> arch/riscv/Kconfig.debug | 30 ++++++++++++++++++++++++++++++
> arch/riscv/include/asm/ptdump.h | 6 ++++++
> arch/riscv/mm/init.c | 2 ++
> 3 files changed, 38 insertions(+)
>
> diff --git a/arch/riscv/Kconfig.debug b/arch/riscv/Kconfig.debug
> index e69de29bb2d1..2bcd88e75626 100644
> --- a/arch/riscv/Kconfig.debug
> +++ b/arch/riscv/Kconfig.debug
> @@ -0,0 +1,30 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +config DEBUG_WX
> + bool "Warn on W+X mappings at boot"
> + select PTDUMP_CORE
> + help
> + Generate a warning if any W+X mappings are found at boot.
> +
> + This is useful for discovering cases where the kernel is leaving
> + W+X mappings after applying NX, as such mappings are a security risk.
> + This check also includes UXN, which should be set on all kernel
> + mappings.
> +
> + Look for a message in dmesg output like this:
> +
> + riscv/mm: Checked W+X mappings: passed, no W+X pages found.
> +
> + or like this, if the check failed:
> +
> + riscv/mm: Checked W+X mappings: FAILED, <N> W+X pages found.
> +
> + Note that even if the check fails, your kernel is possibly
> + still fine, as W+X mappings are not a security hole in
> + themselves, what they do is that they make the exploitation
> + of other unfixed kernel bugs easier.
> +
> + There is no runtime or memory usage effect of this option
> + once the kernel has booted up - it's a one time check.
> +
> + If in doubt, say "Y".

It looks like this comes verbatim from the arm64 port, at least. I think we
should just refactor this to some sort of ARCH_HAS_DEBUG_WX so we can share the
code. I usually do this by adding the shared support, using it for RISC-V, and
then converting the other ports over.

> diff --git a/arch/riscv/include/asm/ptdump.h b/arch/riscv/include/asm/ptdump.h
> index e29af7191909..eb2a1cc5f22c 100644
> --- a/arch/riscv/include/asm/ptdump.h
> +++ b/arch/riscv/include/asm/ptdump.h
> @@ -8,4 +8,10 @@
>
> void ptdump_check_wx(void);
>
> +#ifdef CONFIG_DEBUG_WX
> +#define debug_checkwx() ptdump_check_wx()
> +#else
> +#define debug_checkwx() do { } while (0)
> +#endif
> +
> #endif /* _ASM_RISCV_PTDUMP_H */
> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> index 09fa643e079c..a05d76e5fefe 100644
> --- a/arch/riscv/mm/init.c
> +++ b/arch/riscv/mm/init.c
> @@ -509,6 +509,8 @@ void mark_rodata_ro(void)
> set_memory_ro(rodata_start, (data_start - rodata_start) >> PAGE_SHIFT);
> set_memory_nx(rodata_start, (data_start - rodata_start) >> PAGE_SHIFT);
> set_memory_nx(data_start, (max_low - data_start) >> PAGE_SHIFT);
> +
> + debug_checkwx();
> }
> #endif

2020-03-05 05:54:36

by Zong Li

[permalink] [raw]
Subject: Re: [PATCH 7/8] riscv: add DEBUG_WX support

Palmer Dabbelt <[email protected]> 於 2020年3月5日 週四 上午9:44寫道:
>
> On Mon, 17 Feb 2020 00:32:22 PST (-0800), [email protected] wrote:
> > Support DEBUG_WX to check whether there are mapping with write and
> > execute permission at the same time.
> >
> > Signed-off-by: Zong Li <[email protected]>
> > ---
> > arch/riscv/Kconfig.debug | 30 ++++++++++++++++++++++++++++++
> > arch/riscv/include/asm/ptdump.h | 6 ++++++
> > arch/riscv/mm/init.c | 2 ++
> > 3 files changed, 38 insertions(+)
> >
> > diff --git a/arch/riscv/Kconfig.debug b/arch/riscv/Kconfig.debug
> > index e69de29bb2d1..2bcd88e75626 100644
> > --- a/arch/riscv/Kconfig.debug
> > +++ b/arch/riscv/Kconfig.debug
> > @@ -0,0 +1,30 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +
> > +config DEBUG_WX
> > + bool "Warn on W+X mappings at boot"
> > + select PTDUMP_CORE
> > + help
> > + Generate a warning if any W+X mappings are found at boot.
> > +
> > + This is useful for discovering cases where the kernel is leaving
> > + W+X mappings after applying NX, as such mappings are a security risk.
> > + This check also includes UXN, which should be set on all kernel
> > + mappings.
> > +
> > + Look for a message in dmesg output like this:
> > +
> > + riscv/mm: Checked W+X mappings: passed, no W+X pages found.
> > +
> > + or like this, if the check failed:
> > +
> > + riscv/mm: Checked W+X mappings: FAILED, <N> W+X pages found.
> > +
> > + Note that even if the check fails, your kernel is possibly
> > + still fine, as W+X mappings are not a security hole in
> > + themselves, what they do is that they make the exploitation
> > + of other unfixed kernel bugs easier.
> > +
> > + There is no runtime or memory usage effect of this option
> > + once the kernel has booted up - it's a one time check.
> > +
> > + If in doubt, say "Y".
>
> It looks like this comes verbatim from the arm64 port, at least. I think we
> should just refactor this to some sort of ARCH_HAS_DEBUG_WX so we can share the
> code. I usually do this by adding the shared support, using it for RISC-V, and
> then converting the other ports over.
>

OK. It seems to be different work, maybe I could separate this patch
in next version. Thanks.

> > diff --git a/arch/riscv/include/asm/ptdump.h b/arch/riscv/include/asm/ptdump.h
> > index e29af7191909..eb2a1cc5f22c 100644
> > --- a/arch/riscv/include/asm/ptdump.h
> > +++ b/arch/riscv/include/asm/ptdump.h
> > @@ -8,4 +8,10 @@
> >
> > void ptdump_check_wx(void);
> >
> > +#ifdef CONFIG_DEBUG_WX
> > +#define debug_checkwx() ptdump_check_wx()
> > +#else
> > +#define debug_checkwx() do { } while (0)
> > +#endif
> > +
> > #endif /* _ASM_RISCV_PTDUMP_H */
> > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> > index 09fa643e079c..a05d76e5fefe 100644
> > --- a/arch/riscv/mm/init.c
> > +++ b/arch/riscv/mm/init.c
> > @@ -509,6 +509,8 @@ void mark_rodata_ro(void)
> > set_memory_ro(rodata_start, (data_start - rodata_start) >> PAGE_SHIFT);
> > set_memory_nx(rodata_start, (data_start - rodata_start) >> PAGE_SHIFT);
> > set_memory_nx(data_start, (max_low - data_start) >> PAGE_SHIFT);
> > +
> > + debug_checkwx();
> > }
> > #endif
>