[PATCH v2 1/2] powerpc/mm/ptdump: Wrap seq_printf() to handle NULL pointers

[PATCH v2 1/2] powerpc/mm/ptdump: Wrap seq_printf() to handle NULL pointers

[Russell Currey]

Lovingly borrowed from the arch/arm64 ptdump code.

This doesn't seem to be an issue in practice, but is necessary for my
upcoming commit.

Signed-off-by: Russell Currey <ruscur@russell.cc>
---
v2: Fix putc to actually putc thanks to Christophe Leroy

 arch/powerpc/mm/ptdump/ptdump.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/mm/ptdump/ptdump.c b/arch/powerpc/mm/ptdump/ptdump.c
index 37138428ab55..a4a132f92810 100644
--- a/arch/powerpc/mm/ptdump/ptdump.c
+++ b/arch/powerpc/mm/ptdump/ptdump.c
@@ -104,6 +104,18 @@ static struct addr_marker address_markers[] = {
 	{ -1,	NULL },
 };
 
+#define pt_dump_seq_printf(m, fmt, args...)	\
+({						\
+	if (m)					\
+		seq_printf(m, fmt, ##args);	\
+})
+
+#define pt_dump_seq_putc(m, c)		\
+({					\
+	if (m)				\
+		seq_putc(m, c);		\
+})
+
 static void dump_flag_info(struct pg_state *st, const struct flag_info
 		*flag, u64 pte, int num)
 {
@@ -121,19 +133,19 @@ static void dump_flag_info(struct pg_state *st, const struct flag_info
 			val = pte & flag->val;
 			if (flag->shift)
 				val = val >> flag->shift;
-			seq_printf(st->seq, "  %s:%llx", flag->set, val);
+			pt_dump_seq_printf(st->seq, "  %s:%llx", flag->set, val);
 		} else {
 			if ((pte & flag->mask) == flag->val)
 				s = flag->set;
 			else
 				s = flag->clear;
 			if (s)
-				seq_printf(st->seq, "  %s", s);
+				pt_dump_seq_printf(st->seq, "  %s", s);
 		}
 		st->current_flags &= ~flag->mask;
 	}
 	if (st->current_flags != 0)
-		seq_printf(st->seq, "  unknown flags:%llx", st->current_flags);
+		pt_dump_seq_printf(st->seq, "  unknown flags:%llx", st->current_flags);
 }
 
 static void dump_addr(struct pg_state *st, unsigned long addr)
@@ -148,12 +160,12 @@ static void dump_addr(struct pg_state *st, unsigned long addr)
 #define REG		"0x%08lx"
 #endif
 
-	seq_printf(st->seq, REG "-" REG " ", st->start_address, addr - 1);
+	pt_dump_seq_printf(st->seq, REG "-" REG " ", st->start_address, addr - 1);
 	if (st->start_pa == st->last_pa && st->start_address + PAGE_SIZE != addr) {
-		seq_printf(st->seq, "[" REG "]", st->start_pa);
+		pt_dump_seq_printf(st->seq, "[" REG "]", st->start_pa);
 		delta = PAGE_SIZE >> 10;
 	} else {
-		seq_printf(st->seq, " " REG " ", st->start_pa);
+		pt_dump_seq_printf(st->seq, " " REG " ", st->start_pa);
 		delta = (addr - st->start_address) >> 10;
 	}
 	/* Work out what appropriate unit to use */
@@ -161,7 +173,7 @@ static void dump_addr(struct pg_state *st, unsigned long addr)
 		delta >>= 10;
 		unit++;
 	}
-	seq_printf(st->seq, "%9lu%c", delta, *unit);
+	pt_dump_seq_printf(st->seq, "%9lu%c", delta, *unit);
 
 }
 
@@ -178,7 +190,7 @@ static void note_page(struct pg_state *st, unsigned long addr,
 		st->start_address = addr;
 		st->start_pa = pa;
 		st->last_pa = pa;
-		seq_printf(st->seq, "---[ %s ]---\n", st->marker->name);
+		pt_dump_seq_printf(st->seq, "---[ %s ]---\n", st->marker->name);
 	/*
 	 * Dump the section of virtual memory when:
 	 *   - the PTE flags from one entry to the next differs.
@@ -202,7 +214,7 @@ static void note_page(struct pg_state *st, unsigned long addr,
 					  st->current_flags,
 					  pg_level[st->level].num);
 
-			seq_putc(st->seq, '\n');
+			pt_dump_seq_putc(st->seq, '\n');
 		}
 
 		/*
@@ -211,7 +223,7 @@ static void note_page(struct pg_state *st, unsigned long addr,
 		 */
 		while (addr >= st->marker[1].start_address) {
 			st->marker++;
-			seq_printf(st->seq, "---[ %s ]---\n", st->marker->name);
+			pt_dump_seq_printf(st->seq, "---[ %s ]---\n", st->marker->name);
 		}
 		st->start_address = addr;
 		st->start_pa = pa;
-- 
2.21.0

[PATCH v2 2/2] powerpc/mm: Warn if W+X pages found on boot

[Russell Currey]

Implement code to walk all pages and warn if any are found to be both
writable and executable.  Depends on STRICT_KERNEL_RWX enabled, and is
behind the DEBUG_WX config option.

This only runs on boot and has no runtime performance implications.

Very heavily influenced (and in some cases copied verbatim) from the
ARM64 code written by Laura Abbott (thanks!), since our ptdump
infrastructure is similar.

Signed-off-by: Russell Currey <ruscur@russell.cc>
---
v2: A myriad of fixes and cleanups thanks to Christophe Leroy

 arch/powerpc/Kconfig.debug         | 19 ++++++++++++++
 arch/powerpc/include/asm/pgtable.h |  6 +++++
 arch/powerpc/mm/pgtable_32.c       |  3 +++
 arch/powerpc/mm/pgtable_64.c       |  3 +++
 arch/powerpc/mm/ptdump/ptdump.c    | 41 +++++++++++++++++++++++++++++-
 5 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/Kconfig.debug b/arch/powerpc/Kconfig.debug
index 4e00cb0a5464..9e8bcddd8b8f 100644
--- a/arch/powerpc/Kconfig.debug
+++ b/arch/powerpc/Kconfig.debug
@@ -361,6 +361,25 @@ config PPC_PTDUMP
 
 	  If you are unsure, say N.
 
+config PPC_DEBUG_WX
+	bool "Warn on W+X mappings at boot"
+	select PPC_PTDUMP
+	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.
+
+	  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".
+
 config PPC_FAST_ENDIAN_SWITCH
 	bool "Deprecated fast endian-switch syscall"
         depends on DEBUG_KERNEL && PPC_BOOK3S_64
diff --git a/arch/powerpc/include/asm/pgtable.h b/arch/powerpc/include/asm/pgtable.h
index 505550fb2935..50c0d06fac2f 100644
--- a/arch/powerpc/include/asm/pgtable.h
+++ b/arch/powerpc/include/asm/pgtable.h
@@ -108,6 +108,12 @@ void mark_initmem_nx(void);
 static inline void mark_initmem_nx(void) { }
 #endif
 
+#ifdef CONFIG_PPC_DEBUG_WX
+void ptdump_check_wx(void);
+#else
+static inline void ptdump_check_wx(void) { }
+#endif
+
 /*
  * When used, PTE_FRAG_NR is defined in subarch pgtable.h
  * so we are sure it is included when arriving here.
diff --git a/arch/powerpc/mm/pgtable_32.c b/arch/powerpc/mm/pgtable_32.c
index 6e56a6240bfa..6f919779ee06 100644
--- a/arch/powerpc/mm/pgtable_32.c
+++ b/arch/powerpc/mm/pgtable_32.c
@@ -384,6 +384,9 @@ void mark_rodata_ro(void)
 		   PFN_DOWN((unsigned long)__start_rodata);
 
 	change_page_attr(page, numpages, PAGE_KERNEL_RO);
+
+	// mark_initmem_nx() should have already run by now
+	ptdump_check_wx();
 }
 #endif
 
diff --git a/arch/powerpc/mm/pgtable_64.c b/arch/powerpc/mm/pgtable_64.c
index fb1375c07e8c..bfa18453625e 100644
--- a/arch/powerpc/mm/pgtable_64.c
+++ b/arch/powerpc/mm/pgtable_64.c
@@ -328,6 +328,9 @@ void mark_rodata_ro(void)
 		radix__mark_rodata_ro();
 	else
 		hash__mark_rodata_ro();
+
+	// mark_initmem_nx() should have already run by now
+	ptdump_check_wx();
 }
 
 void mark_initmem_nx(void)
diff --git a/arch/powerpc/mm/ptdump/ptdump.c b/arch/powerpc/mm/ptdump/ptdump.c
index a4a132f92810..e69b53a8a841 100644
--- a/arch/powerpc/mm/ptdump/ptdump.c
+++ b/arch/powerpc/mm/ptdump/ptdump.c
@@ -31,7 +31,7 @@
 #include "ptdump.h"
 
 #ifdef CONFIG_PPC32
-#define KERN_VIRT_START	0
+#define KERN_VIRT_START	PAGE_OFFSET
 #endif
 
 /*
@@ -68,6 +68,8 @@ struct pg_state {
 	unsigned long last_pa;
 	unsigned int level;
 	u64 current_flags;
+	bool check_wx;
+	unsigned long wx_pages;
 };
 
 struct addr_marker {
@@ -177,6 +179,20 @@ static void dump_addr(struct pg_state *st, unsigned long addr)
 
 }
 
+static void note_prot_wx(struct pg_state *st, unsigned long addr)
+{
+	if (!st->check_wx)
+		return;
+
+	if (!((st->current_flags & pgprot_val(PAGE_KERNEL_X)) == pgprot_val(PAGE_KERNEL_X)))
+		return;
+
+	WARN_ONCE(1, "powerpc/mm: Found insecure W+X mapping at address %p/%pS\n",
+		  (void *)st->start_address, (void *)st->start_address);
+
+	st->wx_pages += (addr - st->start_address) / PAGE_SIZE;
+}
+
 static void note_page(struct pg_state *st, unsigned long addr,
 	       unsigned int level, u64 val)
 {
@@ -206,6 +222,7 @@ static void note_page(struct pg_state *st, unsigned long addr,
 
 		/* Check the PTE flags */
 		if (st->current_flags) {
+			note_prot_wx(st, addr);
 			dump_addr(st, addr);
 
 			/* Dump all the flags */
@@ -378,6 +395,28 @@ static void build_pgtable_complete_mask(void)
 				pg_level[i].mask |= pg_level[i].flag[j].mask;
 }
 
+void ptdump_check_wx(void)
+{
+	struct pg_state st = {
+		.seq = NULL,
+		.marker = address_markers,
+		.check_wx = true,
+	};
+
+	if (radix_enabled())
+		st.start_address = PAGE_OFFSET;
+	else
+		st.start_address = KERN_VIRT_START;
+
+	walk_pagetables(&st);
+
+	if (st.wx_pages)
+		pr_warn("Checked W+X mappings: FAILED, %lu W+X pages found\n",
+			st.wx_pages);
+	else
+		pr_info("Checked W+X mappings: passed, no W+X pages found\n");
+}
+
 static int ptdump_init(void)
 {
 	struct dentry *debugfs_file;
-- 
2.21.0

Re: [PATCH v2 2/2] powerpc/mm: Warn if W+X pages found on boot

[Michael Ellerman]

Russell Currey <ruscur@russell.cc> writes:

> Implement code to walk all pages and warn if any are found to be both
> writable and executable.  Depends on STRICT_KERNEL_RWX enabled, and is
> behind the DEBUG_WX config option.
>
> This only runs on boot and has no runtime performance implications.
>
> Very heavily influenced (and in some cases copied verbatim) from the
> ARM64 code written by Laura Abbott (thanks!), since our ptdump
> infrastructure is similar.
>
> Signed-off-by: Russell Currey <ruscur@russell.cc>
> ---
> v2: A myriad of fixes and cleanups thanks to Christophe Leroy
>
>  arch/powerpc/Kconfig.debug         | 19 ++++++++++++++
>  arch/powerpc/include/asm/pgtable.h |  6 +++++
>  arch/powerpc/mm/pgtable_32.c       |  3 +++
>  arch/powerpc/mm/pgtable_64.c       |  3 +++
>  arch/powerpc/mm/ptdump/ptdump.c    | 41 +++++++++++++++++++++++++++++-
>  5 files changed, 71 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/Kconfig.debug b/arch/powerpc/Kconfig.debug
> index 4e00cb0a5464..9e8bcddd8b8f 100644
> --- a/arch/powerpc/Kconfig.debug
> +++ b/arch/powerpc/Kconfig.debug
> @@ -361,6 +361,25 @@ config PPC_PTDUMP
>  
>  	  If you are unsure, say N.
>  
> +config PPC_DEBUG_WX
> +	bool "Warn on W+X mappings at boot"
> +	select PPC_PTDUMP

That should be depends not select, I'll fix it up.

cheers

Re: [PATCH v2 2/2] powerpc/mm: Warn if W+X pages found on boot

[Joel Stanley]

On Thu, 2 May 2019 at 07:42, Russell Currey <ruscur@russell.cc> wrote:
>
> Implement code to walk all pages and warn if any are found to be both
> writable and executable.  Depends on STRICT_KERNEL_RWX enabled, and is
> behind the DEBUG_WX config option.
>
> This only runs on boot and has no runtime performance implications.
>
> Very heavily influenced (and in some cases copied verbatim) from the
> ARM64 code written by Laura Abbott (thanks!), since our ptdump
> infrastructure is similar.
>
> Signed-off-by: Russell Currey <ruscur@russell.cc>
> ---
> v2: A myriad of fixes and cleanups thanks to Christophe Leroy
>
>  arch/powerpc/Kconfig.debug         | 19 ++++++++++++++
>  arch/powerpc/include/asm/pgtable.h |  6 +++++
>  arch/powerpc/mm/pgtable_32.c       |  3 +++
>  arch/powerpc/mm/pgtable_64.c       |  3 +++
>  arch/powerpc/mm/ptdump/ptdump.c    | 41 +++++++++++++++++++++++++++++-
>  5 files changed, 71 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/Kconfig.debug b/arch/powerpc/Kconfig.debug
> index 4e00cb0a5464..9e8bcddd8b8f 100644
> --- a/arch/powerpc/Kconfig.debug
> +++ b/arch/powerpc/Kconfig.debug
> @@ -361,6 +361,25 @@ config PPC_PTDUMP
>
>           If you are unsure, say N.
>
> +config PPC_DEBUG_WX

The other architectures call this DEBUG_WX, in case you wanted to name
it the same.

> +       bool "Warn on W+X mappings at boot"
> +       select PPC_PTDUMP
> +       help
> +         Generate a warning if any W+X mappings are found at boot.

Re: [PATCH v2 2/2] powerpc/mm: Warn if W+X pages found on boot

[Russell Currey]

On Fri, 2019-05-03 at 00:37 +0000, Joel Stanley wrote:
> On Thu, 2 May 2019 at 07:42, Russell Currey <ruscur@russell.cc>
> wrote:
> > Implement code to walk all pages and warn if any are found to be
> > both
> > writable and executable.  Depends on STRICT_KERNEL_RWX enabled, and
> > is
> > behind the DEBUG_WX config option.
> > 
> > This only runs on boot and has no runtime performance implications.
> > 
> > Very heavily influenced (and in some cases copied verbatim) from
> > the
> > ARM64 code written by Laura Abbott (thanks!), since our ptdump
> > infrastructure is similar.
> > 
> > Signed-off-by: Russell Currey <ruscur@russell.cc>
> > ---
> > v2: A myriad of fixes and cleanups thanks to Christophe Leroy
> > 
> >  arch/powerpc/Kconfig.debug         | 19 ++++++++++++++
> >  arch/powerpc/include/asm/pgtable.h |  6 +++++
> >  arch/powerpc/mm/pgtable_32.c       |  3 +++
> >  arch/powerpc/mm/pgtable_64.c       |  3 +++
> >  arch/powerpc/mm/ptdump/ptdump.c    | 41
> > +++++++++++++++++++++++++++++-
> >  5 files changed, 71 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/powerpc/Kconfig.debug
> > b/arch/powerpc/Kconfig.debug
> > index 4e00cb0a5464..9e8bcddd8b8f 100644
> > --- a/arch/powerpc/Kconfig.debug
> > +++ b/arch/powerpc/Kconfig.debug
> > @@ -361,6 +361,25 @@ config PPC_PTDUMP
> > 
> >           If you are unsure, say N.
> > 
> > +config PPC_DEBUG_WX
> 
> The other architectures call this DEBUG_WX, in case you wanted to
> name
> it the same.

I did originally, I changed it since we have PPC_PTDUMP but I don't
really care either way.  mpe can change it if he wants

> 
> > +       bool "Warn on W+X mappings at boot"
> > +       select PPC_PTDUMP
> > +       help
> > +         Generate a warning if any W+X mappings are found at boot.

Re: [PATCH v2 2/2] powerpc/mm: Warn if W+X pages found on boot

[Christophe Leroy]

Le 02/05/2019 à 09:39, Russell Currey a écrit :
> Implement code to walk all pages and warn if any are found to be both
> writable and executable.  Depends on STRICT_KERNEL_RWX enabled, and is
> behind the DEBUG_WX config option.
> 
> This only runs on boot and has no runtime performance implications.
> 
> Very heavily influenced (and in some cases copied verbatim) from the
> ARM64 code written by Laura Abbott (thanks!), since our ptdump
> infrastructure is similar.
> 
> Signed-off-by: Russell Currey <ruscur@russell.cc>
> ---
> v2: A myriad of fixes and cleanups thanks to Christophe Leroy
> 
>   arch/powerpc/Kconfig.debug         | 19 ++++++++++++++
>   arch/powerpc/include/asm/pgtable.h |  6 +++++
>   arch/powerpc/mm/pgtable_32.c       |  3 +++
>   arch/powerpc/mm/pgtable_64.c       |  3 +++
>   arch/powerpc/mm/ptdump/ptdump.c    | 41 +++++++++++++++++++++++++++++-
>   5 files changed, 71 insertions(+), 1 deletion(-)
> 

[...]

> diff --git a/arch/powerpc/mm/ptdump/ptdump.c b/arch/powerpc/mm/ptdump/ptdump.c
> index a4a132f92810..e69b53a8a841 100644
> --- a/arch/powerpc/mm/ptdump/ptdump.c
> +++ b/arch/powerpc/mm/ptdump/ptdump.c
> @@ -31,7 +31,7 @@
>   #include "ptdump.h"
>   
>   #ifdef CONFIG_PPC32
> -#define KERN_VIRT_START	0
> +#define KERN_VIRT_START	PAGE_OFFSET
>   #endif
>   
>   /*
> @@ -68,6 +68,8 @@ struct pg_state {
>   	unsigned long last_pa;
>   	unsigned int level;
>   	u64 current_flags;
> +	bool check_wx;
> +	unsigned long wx_pages;
>   };
>   
>   struct addr_marker {
> @@ -177,6 +179,20 @@ static void dump_addr(struct pg_state *st, unsigned long addr)
>   
>   }
>   
> +static void note_prot_wx(struct pg_state *st, unsigned long addr)
> +{
> +	if (!st->check_wx)
> +		return;
> +
> +	if (!((st->current_flags & pgprot_val(PAGE_KERNEL_X)) == pgprot_val(PAGE_KERNEL_X)))
> +		return;
> +

I just realised that the above test is insuffisient, allthought it works 
by chance.

If I understand correctly, you want to make sure that no page is set 
with PAGE_KERNEL_X, ie that all X pages are PAGE_KERNEL_ROX

If you take the exemple of the 8xx, we have:

#define PAGE_KERNEL_X	__pgprot(_PAGE_BASE | _PAGE_KERNEL_RWX)
#define PAGE_KERNEL_ROX	__pgprot(_PAGE_BASE | _PAGE_KERNEL_ROX)

#define _PAGE_KERNEL_RWX	(_PAGE_SH | _PAGE_DIRTY | _PAGE_EXEC)
#define _PAGE_KERNEL_ROX	(_PAGE_SH | _PAGE_RO | _PAGE_EXEC)

Your test is checking which bits are set, but doesn't test which bits 
are not set. So your test only relies on the fact that _PAGE_DIRTY is 
set when the page is RW. It looks rather fragile as for some reason, a 
page might be RW without being DIRTY yet.

I think the test should be more robust, something like:

	pte_t pte = __pte(st->current_flags);

	if (!pte_exec(pte) || !pte_write(pte))
		return;

Christophe

Re: [PATCH v2 1/2] powerpc/mm/ptdump: Wrap seq_printf() to handle NULL pointers

[Michael Ellerman]

On Thu, 2019-05-02 at 07:39:46 UTC, Russell Currey wrote:
> Lovingly borrowed from the arch/arm64 ptdump code.
> 
> This doesn't seem to be an issue in practice, but is necessary for my
> upcoming commit.
> 
> Signed-off-by: Russell Currey <ruscur@russell.cc>

Series applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/5f18cbdbdd42b050c51eb9859f8ce43d

cheers