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

From: Kees Cook
Date: Mon Oct 05 2015 - 15:36:30 EST

Next message: Andreas Gruenbacher: "[PATCH v9 00/43] Richacls"
Previous message: Felipe Balbi: "Re: [PATCH] usb: remove unnecessary CONFIG_PM dependency from USB_OTG"
In reply to: Stephen Smalley: "[PATCH v3] x86/mm: warn on W+x mappings"
Next in thread: tip-bot for Stephen Smalley: "[tip:x86/mm] x86/mm: Warn on W^X mappings"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, Oct 5, 2015 at 9:55 AM, 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>

Acked-by: Kees Cook <keescook@xxxxxxxxxxxx>

Thanks!

-Kees

> ---
> v3 enables the checks on 32-bit if NX is supported, and also
> makes DEBUG_WX depend on DEBUG_RODATA since both the NX marking
> and the checking occurs from mark_rodata_ro().
>
> arch/x86/Kconfig.debug | 20 +++++++++++++++++++-
> arch/x86/include/asm/pgtable.h | 7 +++++++
> arch/x86/mm/Makefile | 2 +-
> arch/x86/mm/dump_pagetables.c | 42 +++++++++++++++++++++++++++++++++++++++++-
> arch/x86/mm/init_32.c | 2 ++
> arch/x86/mm/init_64.c | 2 ++
> 6 files changed, 72 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
> index d8c0d32..d09fde7 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,7 +83,8 @@ 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
> @@ -105,6 +110,19 @@ config DEBUG_RODATA_TEST
> feature as well as for the change_page_attr() infrastructure.
> If in doubt, say "N"
>
> +config DEBUG_WX
> + bool "Warn on W+X mappings at boot"
> + depends on DEBUG_RODATA
> + 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_SET_MODULE_RONX
> bool "Set loadable kernel module data as NX and text as RO"
> depends on MODULES
> 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_32.c b/arch/x86/mm/init_32.c
> index 7562f42..cb4ef3d 100644
> --- a/arch/x86/mm/init_32.c
> +++ b/arch/x86/mm/init_32.c
> @@ -957,6 +957,8 @@ void mark_rodata_ro(void)
> set_pages_ro(virt_to_page(start), size >> PAGE_SHIFT);
> #endif
> mark_nxdata_nx();
> + if (__supported_pte_mask & _PAGE_NX)
> + debug_checkwx();
> }
> #endif
>
> diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
> index df48430..5ed62ef 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: Andreas Gruenbacher: "[PATCH v9 00/43] Richacls"
Previous message: Felipe Balbi: "Re: [PATCH] usb: remove unnecessary CONFIG_PM dependency from USB_OTG"
In reply to: Stephen Smalley: "[PATCH v3] x86/mm: warn on W+x mappings"
Next in thread: tip-bot for Stephen Smalley: "[tip:x86/mm] x86/mm: Warn on W^X mappings"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]