From: Christophe Leroy <christophe.leroy@c-s.fr>
To: Russell Currey <ruscur@russell.cc>, linuxppc-dev@lists.ozlabs.org
Cc: Julia.Lawall@lip6.fr, rashmica.g@gmail.com
Subject: Re: [PATCH 2/2] powerpc/mm: Warn if W+X pages found on boot
Date: Wed, 24 Apr 2019 09:14:50 +0200 [thread overview]
Message-ID: <8e659a8f-af3f-e889-3f7a-560178c1f7b1@c-s.fr> (raw)
In-Reply-To: <20190424063958.24559-2-ruscur@russell.cc>
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;
>
next prev parent reply other threads:[~2019-04-24 7:17 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-24 6:39 [PATCH 1/2] powerpc/mm/ptdump: Wrap seq_printf() to handle NULL pointers Russell Currey
2019-04-24 6:39 ` [PATCH 2/2] powerpc/mm: Warn if W+X pages found on boot Russell Currey
2019-04-24 7:14 ` Christophe Leroy [this message]
2019-05-01 7:04 ` Russell Currey
2019-05-02 5:23 ` Christophe Leroy
2019-05-02 5:51 ` Russell Currey
2019-08-09 13:11 ` Christophe Leroy
2019-04-24 6:56 ` [PATCH 1/2] powerpc/mm/ptdump: Wrap seq_printf() to handle NULL pointers Christophe Leroy
2019-04-24 7:00 ` Russell Currey
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8e659a8f-af3f-e889-3f7a-560178c1f7b1@c-s.fr \
--to=christophe.leroy@c-s.fr \
--cc=Julia.Lawall@lip6.fr \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=rashmica.g@gmail.com \
--cc=ruscur@russell.cc \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).