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

[Christophe Leroy <christophe.leroy@c-s.fr>]

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