[PATCH 2/2] powerpc/mm: Warn if W+X pages found on boot
Christophe Leroy
christophe.leroy at c-s.fr
Wed Apr 24 17:14:50 AEST 2019
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 at 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;
>
More information about the Linuxppc-dev
mailing list