Re: [PATCH v2] x86/mm: warn on W+x mappings

From: Kees Cook
Date: Fri Oct 02 2015 - 16:44:53 EST

Next message: kbuild test robot: "Re: [PATCH 5/5 v2] drivers/nvme: default to the IOMMU page size"
Previous message: Jason Baron: "[PATCH v2 3/3] af_unix: optimize the unix_dgram_recvmsg()"
In reply to: Stephen Smalley: "[PATCH v2] x86/mm: warn on W+x mappings"
Next in thread: Ingo Molnar: "Re: [PATCH v2] x86/mm: warn on W+x mappings"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Fri, Oct 2, 2015 at 12:29 PM, Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:
> Warn on any residual W+x mappings after setting NX
> if DEBUG_WX is enabled. Introduce a separate
> X86_PTDUMP_CORE config that enables the code for
> dumping the page tables without enabling the debugfs
> interface, so that DEBUG_WX can be enabled without
> exposing the debugfs interface. Switch EFI_PGT_DUMP
> to using X86_PTDUMP_CORE so that it also does not require
> enabling the debugfs interface.
>
> On success:
> x86/mm: Checked W+X mappings: passed, no W+X pages found.
>
> On failure:
> ------------[ cut here ]------------
> WARNING: CPU: 1 PID: 1 at arch/x86/mm/dump_pagetables.c:226 note_page+0x610/0x7b0()
> x86/mm: Found insecure W+X mapping at address ffffffff81755000/__stop___ex_table+0xfa8/0xabfa8
> Modules linked in:
> CPU: 1 PID: 1 Comm: swapper/0 Tainted: G W 4.3.0-rc3+ #19
> 0000000000000000 00000000e96b193f ffff88042c5dbd48 ffffffff81380a5f
> ffff88042c5dbd90 ffff88042c5dbd80 ffffffff8109d3f2 80000000000001e1
> 0000000000000003 ffff88042c5dbe90 ffff88042c5dbe90 0000000000000000
> Call Trace:
> [<ffffffff81380a5f>] dump_stack+0x44/0x55
> [<ffffffff8109d3f2>] warn_slowpath_common+0x82/0xc0
> [<ffffffff8109d48c>] warn_slowpath_fmt+0x5c/0x80
> [<ffffffff8106cfc9>] ? note_page+0x5c9/0x7b0
> [<ffffffff8106d010>] note_page+0x610/0x7b0
> [<ffffffff8106d409>] ptdump_walk_pgd_level_core+0x259/0x3c0
> [<ffffffff8106d5a7>] ptdump_walk_pgd_level_checkwx+0x17/0x20
> [<ffffffff81063905>] mark_rodata_ro+0xf5/0x100
> [<ffffffff817415a0>] ? rest_init+0x80/0x80
> [<ffffffff817415bd>] kernel_init+0x1d/0xe0
> [<ffffffff8174cd1f>] ret_from_fork+0x3f/0x70
> [<ffffffff817415a0>] ? rest_init+0x80/0x80
> ---[ end trace a1f23a1e42a2ac76 ]---
> x86/mm: Checked W+X mappings: FAILED, 171 W+X pages found.
>
> Signed-off-by: Stephen Smalley <sds@xxxxxxxxxxxxx>

This looks good to me. I'd like to (separately) merge the x86, arm,
and arm64 ptdump logic so that this can be common infrastructure. Then
the other arch would gain this as well.

Acked-by: Kees Cook <keescook@xxxxxxxxxxxx>

-Kees

> ---
> v2 addresses Kees' concern about being able to enable this check
> without enabling the debugfs interface, and reworks the output to
> present failure and success in the manner suggested by Ingo.
>
> arch/x86/Kconfig.debug | 19 ++++++++++++++++++-
> arch/x86/include/asm/pgtable.h | 7 +++++++
> arch/x86/mm/Makefile | 2 +-
> arch/x86/mm/dump_pagetables.c | 42 +++++++++++++++++++++++++++++++++++++++++-
> arch/x86/mm/init_64.c | 2 ++
> 5 files changed, 69 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
> index d8c0d32..c6fe16b 100644
> --- a/arch/x86/Kconfig.debug
> +++ b/arch/x86/Kconfig.debug
> @@ -65,10 +65,14 @@ config EARLY_PRINTK_EFI
> This is useful for kernel debugging when your machine crashes very
> early before the console code is initialized.
>
> +config X86_PTDUMP_CORE
> + def_bool n
> +
> config X86_PTDUMP
> bool "Export kernel pagetable layout to userspace via debugfs"
> depends on DEBUG_KERNEL
> select DEBUG_FS
> + select X86_PTDUMP_CORE
> ---help---
> Say Y here if you want to show the kernel pagetable layout in a
> debugfs file. This information is only useful for kernel developers
> @@ -79,13 +83,26 @@ config X86_PTDUMP
>
> config EFI_PGT_DUMP
> bool "Dump the EFI pagetable"
> - depends on EFI && X86_PTDUMP
> + depends on EFI
> + select X86_PTDUMP_CORE
> ---help---
> Enable this if you want to dump the EFI page table before
> enabling virtual mode. This can be used to debug miscellaneous
> issues with the mapping of the EFI runtime regions into that
> table.
>
> +config DEBUG_WX
> + bool "Warn on W+X mappings at boot"
> + select X86_PTDUMP_CORE
> + ---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.
> + Look for a message in dmesg output like this:
> + x86/mm: Checked W+X mappings: passed, no W+X pages found.
> + or like this:
> + x86/mm: Checked W+X mappings: FAILED, <N> W+X pages found.
> +
> config DEBUG_RODATA
> bool "Write protect kernel read-only data structures"
> default y
> diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
> index 867da5b..f2b6bed 100644
> --- a/arch/x86/include/asm/pgtable.h
> +++ b/arch/x86/include/asm/pgtable.h
> @@ -19,6 +19,13 @@
> #include <asm/x86_init.h>
>
> void ptdump_walk_pgd_level(struct seq_file *m, pgd_t *pgd);
> +void ptdump_walk_pgd_level_checkwx(void);
> +
> +#ifdef CONFIG_DEBUG_WX
> +#define debug_checkwx() ptdump_walk_pgd_level_checkwx()
> +#else
> +#define debug_checkwx() do { } while (0)
> +#endif
>
> /*
> * ZERO_PAGE is a global shared page that is always zero: used
> diff --git a/arch/x86/mm/Makefile b/arch/x86/mm/Makefile
> index a482d10..65c47fd 100644
> --- a/arch/x86/mm/Makefile
> +++ b/arch/x86/mm/Makefile
> @@ -14,7 +14,7 @@ obj-$(CONFIG_SMP) += tlb.o
> obj-$(CONFIG_X86_32) += pgtable_32.o iomap_32.o
>
> obj-$(CONFIG_HUGETLB_PAGE) += hugetlbpage.o
> -obj-$(CONFIG_X86_PTDUMP) += dump_pagetables.o
> +obj-$(CONFIG_X86_PTDUMP_CORE) += dump_pagetables.o
>
> obj-$(CONFIG_HIGHMEM) += highmem_32.o
>
> diff --git a/arch/x86/mm/dump_pagetables.c b/arch/x86/mm/dump_pagetables.c
> index f0cedf3..19c64af 100644
> --- a/arch/x86/mm/dump_pagetables.c
> +++ b/arch/x86/mm/dump_pagetables.c
> @@ -32,6 +32,8 @@ struct pg_state {
> const struct addr_marker *marker;
> unsigned long lines;
> bool to_dmesg;
> + bool check_wx;
> + unsigned long wx_pages;
> };
>
> struct addr_marker {
> @@ -214,6 +216,16 @@ static void note_page(struct seq_file *m, struct pg_state *st,
> const char *unit = units;
> unsigned long delta;
> int width = sizeof(unsigned long) * 2;
> + pgprotval_t pr = pgprot_val(st->current_prot);
> +
> + if (st->check_wx && (pr & _PAGE_RW) && !(pr & _PAGE_NX)) {
> + WARN_ONCE(1,
> + "x86/mm: Found insecure W+X mapping at address %p/%pS\n",
> + (void *)st->start_address,
> + (void *)st->start_address);
> + st->wx_pages += (st->current_address -
> + st->start_address) / PAGE_SIZE;
> + }
>
> /*
> * Now print the actual finished series
> @@ -344,7 +356,8 @@ static void walk_pud_level(struct seq_file *m, struct pg_state *st, pgd_t addr,
> #define pgd_none(a) pud_none(__pud(pgd_val(a)))
> #endif
>
> -void ptdump_walk_pgd_level(struct seq_file *m, pgd_t *pgd)
> +static void ptdump_walk_pgd_level_core(struct seq_file *m, pgd_t *pgd,
> + bool checkwx)
> {
> #ifdef CONFIG_X86_64
> pgd_t *start = (pgd_t *) &init_level4_pgt;
> @@ -359,6 +372,10 @@ void ptdump_walk_pgd_level(struct seq_file *m, pgd_t *pgd)
> st.to_dmesg = true;
> }
>
> + st.check_wx = checkwx;
> + if (checkwx)
> + st.wx_pages = 0;
> +
> for (i = 0; i < PTRS_PER_PGD; i++) {
> st.current_address = normalize_addr(i * PGD_LEVEL_MULT);
> if (!pgd_none(*start)) {
> @@ -378,8 +395,26 @@ void ptdump_walk_pgd_level(struct seq_file *m, pgd_t *pgd)
> /* Flush out the last page */
> st.current_address = normalize_addr(PTRS_PER_PGD*PGD_LEVEL_MULT);
> note_page(m, &st, __pgprot(0), 0);
> + if (!checkwx)
> + return;
> + if (st.wx_pages)
> + pr_info("x86/mm: Checked W+X mappings: FAILED, %lu W+X pages found.\n",
> + st.wx_pages);
> + else
> + pr_info("x86/mm: Checked W+X mappings: passed, no W+X pages found.\n");
> +}
> +
> +void ptdump_walk_pgd_level(struct seq_file *m, pgd_t *pgd)
> +{
> + ptdump_walk_pgd_level_core(m, pgd, false);
> }
>
> +void ptdump_walk_pgd_level_checkwx(void)
> +{
> + ptdump_walk_pgd_level_core(NULL, NULL, true);
> +}
> +
> +#ifdef CONFIG_X86_PTDUMP
> static int ptdump_show(struct seq_file *m, void *v)
> {
> ptdump_walk_pgd_level(m, NULL);
> @@ -397,10 +432,13 @@ static const struct file_operations ptdump_fops = {
> .llseek = seq_lseek,
> .release = single_release,
> };
> +#endif
>
> static int pt_dump_init(void)
> {
> +#ifdef CONFIG_X86_PTDUMP
> struct dentry *pe;
> +#endif
>
> #ifdef CONFIG_X86_32
> /* Not a compile-time constant on x86-32 */
> @@ -412,10 +450,12 @@ static int pt_dump_init(void)
> address_markers[FIXADDR_START_NR].start_address = FIXADDR_START;
> #endif
>
> +#ifdef CONFIG_X86_PTDUMP
> pe = debugfs_create_file("kernel_page_tables", 0600, NULL, NULL,
> &ptdump_fops);
> if (!pe)
> return -ENOMEM;
> +#endif
>
> return 0;
> }
> diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
> index 30564e2..f8b1573 100644
> --- a/arch/x86/mm/init_64.c
> +++ b/arch/x86/mm/init_64.c
> @@ -1150,6 +1150,8 @@ void mark_rodata_ro(void)
> free_init_pages("unused kernel",
> (unsigned long) __va(__pa_symbol(rodata_end)),
> (unsigned long) __va(__pa_symbol(_sdata)));
> +
> + debug_checkwx();
> }
>
> #endif
> --
> 2.1.0
>

--
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: kbuild test robot: "Re: [PATCH 5/5 v2] drivers/nvme: default to the IOMMU page size"
Previous message: Jason Baron: "[PATCH v2 3/3] af_unix: optimize the unix_dgram_recvmsg()"
In reply to: Stephen Smalley: "[PATCH v2] x86/mm: warn on W+x mappings"
Next in thread: Ingo Molnar: "Re: [PATCH v2] x86/mm: warn on W+x mappings"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]