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

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

[Stephen Smalley]

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@tycho.nsa.gov>
---
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

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

[Kees Cook]

On Mon, Oct 5, 2015 at 9:55 AM, Stephen Smalley <sds@tycho.nsa.gov> 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@tycho.nsa.gov>

Acked-by: Kees Cook <keescook@chromium.org>

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

[tip:x86/mm] x86/mm: Warn on W^X mappings

[tip-bot for Stephen Smalley]

Commit-ID:  e1a58320a38dfa72be48a0f1a3a92273663ba6db
Gitweb:     http://git.kernel.org/tip/e1a58320a38dfa72be48a0f1a3a92273663ba6db
Author:     Stephen Smalley <sds@tycho.nsa.gov>
AuthorDate: Mon, 5 Oct 2015 12:55:20 -0400
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 6 Oct 2015 11:11:48 +0200

x86/mm: Warn on W^X mappings

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 it prints this to the kernel log:

  x86/mm: Checked W+X mappings: passed, no W+X pages found.

On failure it prints a warning and a count of the failed pages:

  ------------[ 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
  [...]
  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@tycho.nsa.gov>
Acked-by: Kees Cook <keescook@chromium.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Arjan van de Ven <arjan@linux.intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-kernel@vger.kernel.org
Link: http://lkml.kernel.org/r/1444064120-11450-1-git-send-email-sds@tycho.nsa.gov
[ Improved the Kconfig help text and made the new option default-y
  if CONFIG_DEBUG_RODATA=y, because it already found buggy mappings,
  so we really want people to have this on by default. ]
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/Kconfig.debug         | 36 +++++++++++++++++++++++++++++++++++-
 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, 88 insertions(+), 3 deletions(-)

diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
index d8c0d32..3e0baf7 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,35 @@ 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
+	default y
+	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, if the check failed:
+
+	    x86/mm: Checked W+X mappings: FAILED, <N> W+X pages found.
+
+	  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 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 59fc341..c0b41f11 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 71ab2d7..1bf417e 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
@@ -346,7 +358,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;
@@ -362,6 +375,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)) {
@@ -381,8 +398,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);
@@ -400,10 +435,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 */
@@ -415,10 +453,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 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

Re: [tip:x86/mm] x86/mm: Warn on W^X mappings

[Arjan van de Ven]

On 10/6/2015 2:54 AM, tip-bot for Stephen Smalley wrote:
> Commit-ID:  e1a58320a38dfa72be48a0f1a3a92273663ba6db
> Gitweb:     http://git.kernel.org/tip/e1a58320a38dfa72be48a0f1a3a92273663ba6db
> Author:     Stephen Smalley <sds@tycho.nsa.gov>
> AuthorDate: Mon, 5 Oct 2015 12:55:20 -0400
> Committer:  Ingo Molnar <mingo@kernel.org>
> CommitDate: Tue, 6 Oct 2015 11:11:48 +0200
>
> x86/mm: Warn on W^X mappings
>
> 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.

I like it, so Acked-by: Arjan van de Ven <arjan@Linux.intel.com>

I also have/had an old userland script to do similar checks but using the debugfs interface...
... would that be useful to have somewhere more central?

http://git.fenrus.org/tmp/i386-check-pagetables.pl

Re: [tip:x86/mm] x86/mm: Warn on W^X mappings

[Ingo Molnar]

* Arjan van de Ven <arjan@linux.intel.com> wrote:

> On 10/6/2015 2:54 AM, tip-bot for Stephen Smalley wrote:
> >Commit-ID:  e1a58320a38dfa72be48a0f1a3a92273663ba6db
> >Gitweb:     http://git.kernel.org/tip/e1a58320a38dfa72be48a0f1a3a92273663ba6db
> >Author:     Stephen Smalley <sds@tycho.nsa.gov>
> >AuthorDate: Mon, 5 Oct 2015 12:55:20 -0400
> >Committer:  Ingo Molnar <mingo@kernel.org>
> >CommitDate: Tue, 6 Oct 2015 11:11:48 +0200
> >
> >x86/mm: Warn on W^X mappings
> >
> >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.
> 
> I like it, so Acked-by: Arjan van de Ven <arjan@Linux.intel.com>
> 
> I also have/had an old userland script to do similar checks but using the 
> debugfs interface... ... would that be useful to have somewhere more central?
> 
> http://git.fenrus.org/tmp/i386-check-pagetables.pl

Sure, I think it could be put into tools/testing/selftests/x86/.

Thanks,

	Ingo

Re: [tip:x86/mm] x86/mm: Warn on W^X mappings

[Borislav Petkov]

On Tue, Oct 06, 2015 at 02:54:53AM -0700, tip-bot for Stephen Smalley wrote:
> Commit-ID:  e1a58320a38dfa72be48a0f1a3a92273663ba6db
> Gitweb:     http://git.kernel.org/tip/e1a58320a38dfa72be48a0f1a3a92273663ba6db
> Author:     Stephen Smalley <sds@tycho.nsa.gov>
> AuthorDate: Mon, 5 Oct 2015 12:55:20 -0400
> Committer:  Ingo Molnar <mingo@kernel.org>
> CommitDate: Tue, 6 Oct 2015 11:11:48 +0200
> 
> x86/mm: Warn on W^X mappings
> 
> 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 it prints this to the kernel log:
> 
>   x86/mm: Checked W+X mappings: passed, no W+X pages found.
> 
> On failure it prints a warning and a count of the failed pages:
> 
>   ------------[ 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
>   [...]
>   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@tycho.nsa.gov>
> Acked-by: Kees Cook <keescook@chromium.org>
> Cc: Andy Lutomirski <luto@amacapital.net>
> Cc: Arjan van de Ven <arjan@linux.intel.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Brian Gerst <brgerst@gmail.com>
> Cc: Denys Vlasenko <dvlasenk@redhat.com>
> Cc: H. Peter Anvin <hpa@zytor.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Mike Galbraith <efault@gmx.de>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: linux-kernel@vger.kernel.org
> Link: http://lkml.kernel.org/r/1444064120-11450-1-git-send-email-sds@tycho.nsa.gov
> [ Improved the Kconfig help text and made the new option default-y
>   if CONFIG_DEBUG_RODATA=y, because it already found buggy mappings,
>   so we really want people to have this on by default. ]
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> ---
>  arch/x86/Kconfig.debug         | 36 +++++++++++++++++++++++++++++++++++-
>  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, 88 insertions(+), 3 deletions(-)

...

> @@ -381,8 +398,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");

Do we really want to issue anything here in the success case? IMO, we
should be quiet if the check passes and only scream when something's
wrong...

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [tip:x86/mm] x86/mm: Warn on W^X mappings

[Arjan van de Ven]

On 10/8/2015 7:57 AM, Borislav Petkov wrote:
>> +		pr_info("x86/mm: Checked W+X mappings: passed, no W+X pages found.\n");
> Do we really want to issue anything here in the success case? IMO, we
> should be quiet if the check passes and only scream when something's
> wrong...

I would like the success message to be there.
 From an automated testing perspective (for the distro I work on for example),

"the test runs and it fails",
"the test runs and it passes" and
"the test has not run (because of a bug in the code or config file)"

are different outcomes, where the first and third are test failures,
but without the pr_info at info level, the 2nd and 3rd are indistinguishable.

Re: [tip:x86/mm] x86/mm: Warn on W^X mappings

[Borislav Petkov]

On Thu, Oct 08, 2015 at 08:00:24AM -0700, Arjan van de Ven wrote:
> I would like the success message to be there.
> From an automated testing perspective (for the distro I work on for example),

Is that particular success message important? I mean, if we started
issuing success messages for *everything*, we'll flood dmesg with bunch
of useless jibber-jabber. And we don't want that either.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [tip:x86/mm] x86/mm: Warn on W^X mappings

[Ingo Molnar]

* Borislav Petkov <bp@alien8.de> wrote:

> On Thu, Oct 08, 2015 at 08:00:24AM -0700, Arjan van de Ven wrote:
> > I would like the success message to be there.
> > From an automated testing perspective (for the distro I work on for example),
> 
> Is that particular success message important? I mean, if we started
> issuing success messages for *everything*, we'll flood dmesg with bunch
> of useless jibber-jabber. And we don't want that either.

For security related checks it's important, yes - we had cases of security tests 
being broken by init code reordering: the security test was not ran at all, and 
nobody noticed!

Thanks,

	Ingo