[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 at 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