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

[PATCH 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.

Converts a putc() into a puts().

Signed-off-by: Russell Currey <ruscur@russell.cc>
---
 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..c50cb7faa334 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_puts(m, fmt)	\
+({					\
+	if (m)				\
+		seq_printf(m, fmt);	\
+})
+
 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_puts(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 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>
---
 arch/powerpc/Kconfig.debug         | 19 +++++++++++++++
 arch/powerpc/include/asm/pgtable.h |  5 ++++
 arch/powerpc/mm/pgtable_32.c       |  5 ++++
 arch/powerpc/mm/pgtable_64.c       |  5 ++++
 arch/powerpc/mm/ptdump/ptdump.c    | 38 ++++++++++++++++++++++++++++++
 5 files changed, 72 insertions(+)

diff --git a/arch/powerpc/Kconfig.debug b/arch/powerpc/Kconfig.debug
index 4e00cb0a5464..a4160ff02ed4 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 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..be785f221e56 100644
--- a/arch/powerpc/include/asm/pgtable.h
+++ b/arch/powerpc/include/asm/pgtable.h
@@ -104,6 +104,11 @@ void pgtable_cache_init(void);
 
 #if defined(CONFIG_STRICT_KERNEL_RWX) || defined(CONFIG_PPC32)
 void mark_initmem_nx(void);
+
+#ifdef CONFIG_DEBUG_WX
+extern void ptdump_check_wx(void);
+#endif /* CONFIG_DEBUG_WX */
+
 #else
 static inline void mark_initmem_nx(void) { }
 #endif
diff --git a/arch/powerpc/mm/pgtable_32.c b/arch/powerpc/mm/pgtable_32.c
index 6e56a6240bfa..054a6174ff7f 100644
--- a/arch/powerpc/mm/pgtable_32.c
+++ b/arch/powerpc/mm/pgtable_32.c
@@ -384,6 +384,11 @@ void mark_rodata_ro(void)
 		   PFN_DOWN((unsigned long)__start_rodata);
 
 	change_page_attr(page, numpages, PAGE_KERNEL_RO);
+
+#ifdef CONFIG_DEBUG_WX
+	// mark_initmem_nx() should have already run by now
+	ptdump_check_wx();
+#endif
 }
 #endif
 
diff --git a/arch/powerpc/mm/pgtable_64.c b/arch/powerpc/mm/pgtable_64.c
index fb1375c07e8c..48036b25a958 100644
--- a/arch/powerpc/mm/pgtable_64.c
+++ b/arch/powerpc/mm/pgtable_64.c
@@ -328,6 +328,11 @@ void mark_rodata_ro(void)
 		radix__mark_rodata_ro();
 	else
 		hash__mark_rodata_ro();
+
+#ifdef CONFIG_DEBUG_WX
+	// mark_initmem_nx() should have already run by now
+	ptdump_check_wx();
+#endif
 }
 
 void mark_initmem_nx(void)
diff --git a/arch/powerpc/mm/ptdump/ptdump.c b/arch/powerpc/mm/ptdump/ptdump.c
index c50cb7faa334..b4b09df839bb 100644
--- a/arch/powerpc/mm/ptdump/ptdump.c
+++ b/arch/powerpc/mm/ptdump/ptdump.c
@@ -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,19 @@ 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 & _PAGE_EXEC) && (st->current_flags & _PAGE_WRITE)))
+		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 +221,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 +394,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 1/2] powerpc/mm/ptdump: Wrap seq_printf() to handle NULL pointers

[Christophe Leroy]

Le 24/04/2019 à 08:39, Russell Currey a écrit :
> 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.
> 
> Converts a putc() into a puts().
> 
> Signed-off-by: Russell Currey <ruscur@russell.cc>
> ---
>   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..c50cb7faa334 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_puts(m, fmt)	\
> +({					\
> +	if (m)				\
> +		seq_printf(m, fmt);	\

Why not use seq_puts() here ?

Christophe

> +})
> +
>   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_puts(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;
> 

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

[Russell Currey]

whoops, will fix 

-- 
  Russell Currey
  ruscur@russell.cc

On Wed, Apr 24, 2019, at 4:56 PM, Christophe Leroy wrote:
> 
> 
> Le 24/04/2019 à 08:39, Russell Currey a écrit :
> > 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.
> > 
> > Converts a putc() into a puts().
> > 
> > Signed-off-by: Russell Currey <ruscur@russell.cc>
> > ---
> >   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..c50cb7faa334 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_puts(m, fmt)	\
> > +({					\
> > +	if (m)				\
> > +		seq_printf(m, fmt);	\
> 
> Why not use seq_puts() here ?
> 
> Christophe
> 
> > +})
> > +
> >   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_puts(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;
> > 
>

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

[Christophe Leroy]

Le 24/04/2019 à 08: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>
> ---
>   arch/powerpc/Kconfig.debug         | 19 +++++++++++++++
>   arch/powerpc/include/asm/pgtable.h |  5 ++++
>   arch/powerpc/mm/pgtable_32.c       |  5 ++++
>   arch/powerpc/mm/pgtable_64.c       |  5 ++++
>   arch/powerpc/mm/ptdump/ptdump.c    | 38 ++++++++++++++++++++++++++++++
>   5 files changed, 72 insertions(+)
> 
> diff --git a/arch/powerpc/Kconfig.debug b/arch/powerpc/Kconfig.debug
> index 4e00cb0a5464..a4160ff02ed4 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 DEBUG_WX

I would call it PPC_DEBUG_WX to avoid confusion.

> +	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..be785f221e56 100644
> --- a/arch/powerpc/include/asm/pgtable.h
> +++ b/arch/powerpc/include/asm/pgtable.h
> @@ -104,6 +104,11 @@ void pgtable_cache_init(void);
>   
>   #if defined(CONFIG_STRICT_KERNEL_RWX) || defined(CONFIG_PPC32)
>   void mark_initmem_nx(void);
> +
> +#ifdef CONFIG_DEBUG_WX

I don't think this #ifdef is necessary at all when there is no matching 
#else. You could leave the declaration of the function all the time.

> +extern void ptdump_check_wx(void);

'extern' keyword is superflous and checkpatch --strict will likely complain.

> +#endif /* CONFIG_DEBUG_WX */
> +
>   #else
>   static inline void mark_initmem_nx(void) { }
>   #endif
> diff --git a/arch/powerpc/mm/pgtable_32.c b/arch/powerpc/mm/pgtable_32.c
> index 6e56a6240bfa..054a6174ff7f 100644
> --- a/arch/powerpc/mm/pgtable_32.c
> +++ b/arch/powerpc/mm/pgtable_32.c
> @@ -384,6 +384,11 @@ void mark_rodata_ro(void)
>   		   PFN_DOWN((unsigned long)__start_rodata);
>   
>   	change_page_attr(page, numpages, PAGE_KERNEL_RO);
> +
> +#ifdef CONFIG_DEBUG_WX
> +	// mark_initmem_nx() should have already run by now
> +	ptdump_check_wx();

Please avoid #ifdefs in .c files as much as possible.

It would be better to define ptdump_check_wx() as static inline {} in 
pgtable.h when CONFIG_DEBUG_WX is not selected.

> +#endif
>   }
>   #endif
>   
> diff --git a/arch/powerpc/mm/pgtable_64.c b/arch/powerpc/mm/pgtable_64.c
> index fb1375c07e8c..48036b25a958 100644
> --- a/arch/powerpc/mm/pgtable_64.c
> +++ b/arch/powerpc/mm/pgtable_64.c
> @@ -328,6 +328,11 @@ void mark_rodata_ro(void)
>   		radix__mark_rodata_ro();
>   	else
>   		hash__mark_rodata_ro();
> +
> +#ifdef CONFIG_DEBUG_WX
> +	// mark_initmem_nx() should have already run by now
> +	ptdump_check_wx();
> +#endif

Idem

>   }
>   
>   void mark_initmem_nx(void)
> diff --git a/arch/powerpc/mm/ptdump/ptdump.c b/arch/powerpc/mm/ptdump/ptdump.c
> index c50cb7faa334..b4b09df839bb 100644
> --- a/arch/powerpc/mm/ptdump/ptdump.c
> +++ b/arch/powerpc/mm/ptdump/ptdump.c
> @@ -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,19 @@ 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 & _PAGE_EXEC) && (st->current_flags & _PAGE_WRITE)))

The above won't work in all cases. _PAGE_WRITE is only defined in 
book3s64. Other arches have _PAGE_RW or _PAGE_RO.

Please use the helpers defined in pgtable.h to check and not flags directly.


> +		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 +221,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 +394,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;

KERN_VIRT_START doesn't exist on PPC32.

Christophe

> +
> +	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;
> 

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

[Russell Currey]

On Wed, 2019-04-24 at 09:14 +0200, Christophe Leroy wrote:
> 
> Le 24/04/2019 à 08: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>
> > ---
> >   arch/powerpc/Kconfig.debug         | 19 +++++++++++++++
> >   arch/powerpc/include/asm/pgtable.h |  5 ++++
> >   arch/powerpc/mm/pgtable_32.c       |  5 ++++
> >   arch/powerpc/mm/pgtable_64.c       |  5 ++++
> >   arch/powerpc/mm/ptdump/ptdump.c    | 38
> > ++++++++++++++++++++++++++++++
> >   5 files changed, 72 insertions(+)
> > 
> > diff --git a/arch/powerpc/Kconfig.debug
> > b/arch/powerpc/Kconfig.debug
> > index 4e00cb0a5464..a4160ff02ed4 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 DEBUG_WX
> 
> I would call it PPC_DEBUG_WX to avoid confusion.

It's the same functionality as on other architectures and is an arch-
local thing, I personally think it should be left as-is but given we
already put the PPC prefix on PTDUMP, I'll add it so it's consistent

<snip>

> > +	if (radix_enabled())
> > +		st.start_address = PAGE_OFFSET;
> > +	else
> +		st.start_address = KERN_VIRT_START;
>
> KERN_VIRT_START doesn't exist on PPC32.
> 
> Christophe
> 
Thanks a lot for the review!  Applied all your suggestions.  What
should I use on PPC32 instead?

- Russell

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

[Christophe Leroy]

Le 01/05/2019 à 09:04, Russell Currey a écrit :
> On Wed, 2019-04-24 at 09:14 +0200, Christophe Leroy wrote:
>>
>> Le 24/04/2019 à 08: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>
>>> ---
>>>    arch/powerpc/Kconfig.debug         | 19 +++++++++++++++
>>>    arch/powerpc/include/asm/pgtable.h |  5 ++++
>>>    arch/powerpc/mm/pgtable_32.c       |  5 ++++
>>>    arch/powerpc/mm/pgtable_64.c       |  5 ++++
>>>    arch/powerpc/mm/ptdump/ptdump.c    | 38
>>> ++++++++++++++++++++++++++++++
>>>    5 files changed, 72 insertions(+)
>>>
>>> diff --git a/arch/powerpc/Kconfig.debug
>>> b/arch/powerpc/Kconfig.debug
>>> index 4e00cb0a5464..a4160ff02ed4 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 DEBUG_WX
>>
>> I would call it PPC_DEBUG_WX to avoid confusion.
> 
> It's the same functionality as on other architectures and is an arch-
> local thing, I personally think it should be left as-is but given we
> already put the PPC prefix on PTDUMP, I'll add it so it's consistent
> 
> <snip>
> 
>>> +	if (radix_enabled())
>>> +		st.start_address = PAGE_OFFSET;
>>> +	else
>> +		st.start_address = KERN_VIRT_START;
>>
>> KERN_VIRT_START doesn't exist on PPC32.
>>
>> Christophe
>>
> Thanks a lot for the review!  Applied all your suggestions.  What
> should I use on PPC32 instead?

Indeed it looks like KERN_VIRT_START is defined as 0 for PPC32 at the 
top of ptdump.c, which look strange to me.

I guess PAGE_OFFSET should be the good value for KERN_VIRT_START on PPC32.

Christophe

> 
> - Russell
> 

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

[Russell Currey]

> > > > +	if (radix_enabled())
> > > > +		st.start_address = PAGE_OFFSET;
> > > > +	else
> > > +		st.start_address = KERN_VIRT_START;
> > > 
> > > KERN_VIRT_START doesn't exist on PPC32.
> > > 
> > > Christophe
> > > 
> > Thanks a lot for the review!  Applied all your suggestions.  What
> > should I use on PPC32 instead?
> 
> Indeed it looks like KERN_VIRT_START is defined as 0 for PPC32 at
> the 
> top of ptdump.c, which look strange to me.
> 
> I guess PAGE_OFFSET should be the good value for KERN_VIRT_START on
> PPC32.
> 
> Christophe

git blame says you put it there :) I'll set it to PAGE_OFFSET instead
of zero.  Cheers

- Russell

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

[Christophe Leroy]

Le 02/05/2019 à 07:51, Russell Currey a écrit :
>>>>> +	if (radix_enabled())
>>>>> +		st.start_address = PAGE_OFFSET;
>>>>> +	else
>>>> +		st.start_address = KERN_VIRT_START;
>>>>
>>>> KERN_VIRT_START doesn't exist on PPC32.
>>>>
>>>> Christophe
>>>>
>>> Thanks a lot for the review!  Applied all your suggestions.  What
>>> should I use on PPC32 instead?
>>
>> Indeed it looks like KERN_VIRT_START is defined as 0 for PPC32 at
>> the
>> top of ptdump.c, which look strange to me.
>>
>> I guess PAGE_OFFSET should be the good value for KERN_VIRT_START on
>> PPC32.
>>
>> Christophe
> 
> git blame says you put it there :) I'll set it to PAGE_OFFSET instead
> of zero.  Cheers
> 

Finally it seems that I was right at first place. KERN_VIRT_START should 
be 0 because in walk_pagetables(), it starts with:

pgd_t *pgd = pgd_offset_k(0UL);

Now that KERN_VIRT_START has changed to 0xc0000000, I get a shift of 
0xc0000000 in the display, ie the kernel pages are displayed starting at 
0x80000000 instead of 0xc0000000 (0x80000000 = 0xc0000000 + 0xc0000000)

Since we only want to display kernel pages, I guess we should use

pgd_t *pgd = pgd_offset_k(KERN_VIRT_START); but then we can't use the 
for () loop as it is.

Does it work properly on PPC64 ? If so, that's surprising.

Christophe