[PATCHv2 0/4] WX checking for arm64

[PATCHv2 0/4] WX checking for arm64

[Laura Abbott]

Hi,

This is v2 of the implementation to check for writable and executable pages on
arm64.

Major changes since v1:
- I realized my concerns about initialization and registration were unfounded
  so registration to register page tables with debugfs is simplified.
- New patch to remove max_addr since it was pointed out it was unused.
- Rebased to include changes for the EFI page tables as well.
- Checking is now only done on the init_mm page tables. It was mentioned that
  we should check the hyp page tables as well but that can be follow on work.
- Checking for UXN per suggestion from Mark Rutland.

Laura Abbott (4):
  arm64: dump: Make ptdump debugfs a separate option
  arm64: dump: Make the page table dumping seq_file optional
  arm64: dump: Remove max_addr
  arm64: dump: Add checking for writable and exectuable pages

 arch/arm64/Kconfig.debug           | 34 ++++++++++++++-
 arch/arm64/include/asm/ptdump.h    | 22 +++++++---
 arch/arm64/mm/Makefile             |  3 +-
 arch/arm64/mm/dump.c               | 89 ++++++++++++++++++++++++++------------
 arch/arm64/mm/mmu.c                |  2 +
 arch/arm64/mm/ptdump_debugfs.c     | 31 +++++++++++++
 drivers/firmware/efi/arm-runtime.c |  5 +--
 7 files changed, 147 insertions(+), 39 deletions(-)
 create mode 100644 arch/arm64/mm/ptdump_debugfs.c

-- 
2.7.4

[PATCHv2 1/4] arm64: dump: Make ptdump debugfs a separate option

[Laura Abbott]

ptdump_register currently initializes a set of page table information and
registers debugfs. There are uses for the ptdump option without wanting the
debugfs options. Split this out to make it a separate option.

Signed-off-by: Laura Abbott <labbott@redhat.com>
---
v2: Minor style fixups per Mark Rutland, intialization is now separate from
    register since it never needed to be combined in the first place, EFI
    page table registration.
---
 arch/arm64/Kconfig.debug           |  6 +++++-
 arch/arm64/include/asm/ptdump.h    | 13 ++++++++-----
 arch/arm64/mm/Makefile             |  3 ++-
 arch/arm64/mm/dump.c               | 26 +++++---------------------
 arch/arm64/mm/ptdump_debugfs.c     | 31 +++++++++++++++++++++++++++++++
 drivers/firmware/efi/arm-runtime.c |  5 ++---
 6 files changed, 53 insertions(+), 31 deletions(-)
 create mode 100644 arch/arm64/mm/ptdump_debugfs.c

diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
index b661fe7..21a5b74 100644
--- a/arch/arm64/Kconfig.debug
+++ b/arch/arm64/Kconfig.debug
@@ -2,9 +2,13 @@ menu "Kernel hacking"
 
 source "lib/Kconfig.debug"
 
-config ARM64_PTDUMP
+config ARM64_PTDUMP_CORE
+	def_bool n
+
+config ARM64_PTDUMP_DEBUGFS
 	bool "Export kernel pagetable layout to userspace via debugfs"
 	depends on DEBUG_KERNEL
+	select ARM64_PTDUMP_CORE
 	select DEBUG_FS
         help
 	  Say Y here if you want to show the kernel pagetable layout in a
diff --git a/arch/arm64/include/asm/ptdump.h b/arch/arm64/include/asm/ptdump.h
index 07b8ed0..7c35689 100644
--- a/arch/arm64/include/asm/ptdump.h
+++ b/arch/arm64/include/asm/ptdump.h
@@ -16,9 +16,10 @@
 #ifndef __ASM_PTDUMP_H
 #define __ASM_PTDUMP_H
 
-#ifdef CONFIG_ARM64_PTDUMP
+#ifdef CONFIG_ARM64_PTDUMP_CORE
 
 #include <linux/mm_types.h>
+#include <linux/seq_file.h>
 
 struct addr_marker {
 	unsigned long start_address;
@@ -32,13 +33,15 @@ struct ptdump_info {
 	unsigned long			max_addr;
 };
 
-int ptdump_register(struct ptdump_info *info, const char *name);
-
+void ptdump_walk_pgd(struct seq_file *s, struct ptdump_info *info);
+#ifdef CONFIG_ARM64_PTDUMP_DEBUGFS
+int ptdump_debugfs_register(struct ptdump_info *info, const char *name);
 #else
-static inline int ptdump_register(struct ptdump_info *info, const char *name)
+static inline int ptdump_debugfs_register(struct ptdump_info *info,
+					const char *name)
 {
 	return 0;
 }
-#endif /* CONFIG_ARM64_PTDUMP */
+#endif
 
 #endif /* __ASM_PTDUMP_H */
diff --git a/arch/arm64/mm/Makefile b/arch/arm64/mm/Makefile
index 54bb209..e703fb9 100644
--- a/arch/arm64/mm/Makefile
+++ b/arch/arm64/mm/Makefile
@@ -3,7 +3,8 @@ obj-y				:= dma-mapping.o extable.o fault.o init.o \
 				   ioremap.o mmap.o pgd.o mmu.o \
 				   context.o proc.o pageattr.o
 obj-$(CONFIG_HUGETLB_PAGE)	+= hugetlbpage.o
-obj-$(CONFIG_ARM64_PTDUMP)	+= dump.o
+obj-$(CONFIG_ARM64_PTDUMP_CORE)	+= dump.o
+obj-$(CONFIG_ARM64_PTDUMP_DEBUGFS)	+= ptdump_debugfs.o
 obj-$(CONFIG_NUMA)		+= numa.o
 
 obj-$(CONFIG_KASAN)		+= kasan_init.o
diff --git a/arch/arm64/mm/dump.c b/arch/arm64/mm/dump.c
index 9c3e75d..f0f0be7 100644
--- a/arch/arm64/mm/dump.c
+++ b/arch/arm64/mm/dump.c
@@ -304,9 +304,8 @@ static void walk_pgd(struct pg_state *st, struct mm_struct *mm,
 	}
 }
 
-static int ptdump_show(struct seq_file *m, void *v)
+void ptdump_walk_pgd(struct seq_file *m, struct ptdump_info *info)
 {
-	struct ptdump_info *info = m->private;
 	struct pg_state st = {
 		.seq = m,
 		.marker = info->markers,
@@ -315,33 +314,16 @@ static int ptdump_show(struct seq_file *m, void *v)
 	walk_pgd(&st, info->mm, info->base_addr);
 
 	note_page(&st, 0, 0, 0);
-	return 0;
 }
 
-static int ptdump_open(struct inode *inode, struct file *file)
+static void ptdump_initialize(void)
 {
-	return single_open(file, ptdump_show, inode->i_private);
-}
-
-static const struct file_operations ptdump_fops = {
-	.open		= ptdump_open,
-	.read		= seq_read,
-	.llseek		= seq_lseek,
-	.release	= single_release,
-};
-
-int ptdump_register(struct ptdump_info *info, const char *name)
-{
-	struct dentry *pe;
 	unsigned i, j;
 
 	for (i = 0; i < ARRAY_SIZE(pg_level); i++)
 		if (pg_level[i].bits)
 			for (j = 0; j < pg_level[i].num; j++)
 				pg_level[i].mask |= pg_level[i].bits[j].mask;
-
-	pe = debugfs_create_file(name, 0400, NULL, info, &ptdump_fops);
-	return pe ? 0 : -ENOMEM;
 }
 
 static struct ptdump_info kernel_ptdump_info = {
@@ -352,6 +334,8 @@ static struct ptdump_info kernel_ptdump_info = {
 
 static int ptdump_init(void)
 {
-	return ptdump_register(&kernel_ptdump_info, "kernel_page_tables");
+	ptdump_initialize();
+	return ptdump_debugfs_register(&kernel_ptdump_info,
+					"kernel_page_tables");
 }
 device_initcall(ptdump_init);
diff --git a/arch/arm64/mm/ptdump_debugfs.c b/arch/arm64/mm/ptdump_debugfs.c
new file mode 100644
index 0000000..eee4d86
--- /dev/null
+++ b/arch/arm64/mm/ptdump_debugfs.c
@@ -0,0 +1,31 @@
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
+
+#include <asm/ptdump.h>
+
+static int ptdump_show(struct seq_file *m, void *v)
+{
+	struct ptdump_info *info = m->private;
+	ptdump_walk_pgd(m, info);
+	return 0;
+}
+
+static int ptdump_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, ptdump_show, inode->i_private);
+}
+
+static const struct file_operations ptdump_fops = {
+	.open		= ptdump_open,
+	.read		= seq_read,
+	.llseek		= seq_lseek,
+	.release	= single_release,
+};
+
+int ptdump_debugfs_register(struct ptdump_info *info, const char *name)
+{
+	struct dentry *pe;
+	pe = debugfs_create_file(name, 0400, NULL, info, &ptdump_fops);
+	return pe ? 0 : -ENOMEM;
+
+}
diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c
index 7c75a8d..33d35e8 100644
--- a/drivers/firmware/efi/arm-runtime.c
+++ b/drivers/firmware/efi/arm-runtime.c
@@ -39,7 +39,7 @@ static struct mm_struct efi_mm = {
 	.mmlist			= LIST_HEAD_INIT(efi_mm.mmlist),
 };
 
-#ifdef CONFIG_ARM64_PTDUMP
+#ifdef CONFIG_ARM64_PTDUMP_DEBUGFS
 #include <asm/ptdump.h>
 
 static struct ptdump_info efi_ptdump_info = {
@@ -53,10 +53,9 @@ static struct ptdump_info efi_ptdump_info = {
 
 static int __init ptdump_init(void)
 {
-	return ptdump_register(&efi_ptdump_info, "efi_page_tables");
+	return ptdump_debugfs_register(&efi_ptdump_info, "efi_page_tables");
 }
 device_initcall(ptdump_init);
-
 #endif
 
 static bool __init efi_virtmap_init(void)
-- 
2.7.4

[PATCHv2 2/4] arm64: dump: Make the page table dumping seq_file optional

[Laura Abbott]

The page table dumping code always assumes it will be dumping to a
seq_file to userspace. Future code will be taking advantage of
the page table dumping code but will not need the seq_file. Make
the seq_file optional for these cases.

Acked-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Laura Abbott <labbott@redhat.com>
---
v2: Tweak commit text per Mark Rutland's suggestion
---
 arch/arm64/mm/dump.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/mm/dump.c b/arch/arm64/mm/dump.c
index f0f0be7..bb36649 100644
--- a/arch/arm64/mm/dump.c
+++ b/arch/arm64/mm/dump.c
@@ -50,6 +50,18 @@ static const struct addr_marker address_markers[] = {
 	{ -1,				NULL },
 };
 
+#define pt_dump_seq_printf(m, fmt, args...)	\
+({						\
+	if (m)					\
+		seq_printf(m, fmt, ##args);	\
+})
+
+#define pt_dump_seq_puts(m, fmt)	\
+({					\
+	if (m)				\
+		seq_printf(m, fmt);	\
+})
+
 /*
  * The page dumper groups page table entries of the same type into a single
  * description. It uses pg_state to track the range information while
@@ -186,7 +198,7 @@ static void dump_prot(struct pg_state *st, const struct prot_bits *bits,
 			s = bits->clear;
 
 		if (s)
-			seq_printf(st->seq, " %s", s);
+			pt_dump_seq_printf(st->seq, " %s", s);
 	}
 }
 
@@ -200,14 +212,14 @@ static void note_page(struct pg_state *st, unsigned long addr, unsigned level,
 		st->level = level;
 		st->current_prot = prot;
 		st->start_address = addr;
-		seq_printf(st->seq, "---[ %s ]---\n", st->marker->name);
+		pt_dump_seq_printf(st->seq, "---[ %s ]---\n", st->marker->name);
 	} else if (prot != st->current_prot || level != st->level ||
 		   addr >= st->marker[1].start_address) {
 		const char *unit = units;
 		unsigned long delta;
 
 		if (st->current_prot) {
-			seq_printf(st->seq, "0x%016lx-0x%016lx   ",
+			pt_dump_seq_printf(st->seq, "0x%016lx-0x%016lx   ",
 				   st->start_address, addr);
 
 			delta = (addr - st->start_address) >> 10;
@@ -215,17 +227,17 @@ static void note_page(struct pg_state *st, unsigned long addr, unsigned level,
 				delta >>= 10;
 				unit++;
 			}
-			seq_printf(st->seq, "%9lu%c %s", delta, *unit,
+			pt_dump_seq_printf(st->seq, "%9lu%c %s", delta, *unit,
 				   pg_level[st->level].name);
 			if (pg_level[st->level].bits)
 				dump_prot(st, pg_level[st->level].bits,
 					  pg_level[st->level].num);
-			seq_puts(st->seq, "\n");
+			pt_dump_seq_puts(st->seq, "\n");
 		}
 
 		if (addr >= st->marker[1].start_address) {
 			st->marker++;
-			seq_printf(st->seq, "---[ %s ]---\n", st->marker->name);
+			pt_dump_seq_printf(st->seq, "---[ %s ]---\n", st->marker->name);
 		}
 
 		st->start_address = addr;
@@ -235,7 +247,7 @@ static void note_page(struct pg_state *st, unsigned long addr, unsigned level,
 
 	if (addr >= st->marker[1].start_address) {
 		st->marker++;
-		seq_printf(st->seq, "---[ %s ]---\n", st->marker->name);
+		pt_dump_seq_printf(st->seq, "---[ %s ]---\n", st->marker->name);
 	}
 
 }
-- 
2.7.4

[PATCHv2 3/4] arm64: dump: Remove max_addr

[Laura Abbott]

max_addr was added as part of struct ptdump_info but has never actually
been used. Remove it.

Signed-off-by: Laura Abbott <labbott@redhat.com>
---
New for v2 of the series
---
 arch/arm64/include/asm/ptdump.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/arm64/include/asm/ptdump.h b/arch/arm64/include/asm/ptdump.h
index 7c35689..8fc0957 100644
--- a/arch/arm64/include/asm/ptdump.h
+++ b/arch/arm64/include/asm/ptdump.h
@@ -30,7 +30,6 @@ struct ptdump_info {
 	struct mm_struct		*mm;
 	const struct addr_marker	*markers;
 	unsigned long			base_addr;
-	unsigned long			max_addr;
 };
 
 void ptdump_walk_pgd(struct seq_file *s, struct ptdump_info *info);
-- 
2.7.4

[PATCHv2 4/4] arm64: dump: Add checking for writable and exectuable pages

[Laura Abbott]

Page mappings with full RWX permissions are a security risk. x86
has an option to walk the page tables and dump any bad pages.
(See e1a58320a38d ("x86/mm: Warn on W^X mappings")). Add a similar
implementation for arm64.

Signed-off-by: Laura Abbott <labbott@redhat.com>
---
v2: Check only init_mm, style cleanups, UXN checks, compiliation fixes for
    disabled case.
---
 arch/arm64/Kconfig.debug        | 30 ++++++++++++++++++++++++
 arch/arm64/include/asm/ptdump.h |  8 +++++++
 arch/arm64/mm/dump.c            | 52 +++++++++++++++++++++++++++++++++++++++++
 arch/arm64/mm/mmu.c             |  2 ++
 4 files changed, 92 insertions(+)

diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
index 21a5b74..3f627c94 100644
--- a/arch/arm64/Kconfig.debug
+++ b/arch/arm64/Kconfig.debug
@@ -42,6 +42,36 @@ config ARM64_RANDOMIZE_TEXT_OFFSET
 	  of TEXT_OFFSET and platforms must not require a specific
 	  value.
 
+config DEBUG_WX
+	bool "Warn on W+X mappings at boot"
+	select ARM64_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.
+	  This check also includes UXN, which should be set on all kernel
+	  mappings.
+
+	  Look for a message in dmesg output like this:
+
+	    arm64/mm: Checked W+X mappings: passed, no W+X pages found.
+
+	  or like this, if the check failed:
+
+	    arm64/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/arm64/include/asm/ptdump.h b/arch/arm64/include/asm/ptdump.h
index 8fc0957..6afd847 100644
--- a/arch/arm64/include/asm/ptdump.h
+++ b/arch/arm64/include/asm/ptdump.h
@@ -42,5 +42,13 @@ static inline int ptdump_debugfs_register(struct ptdump_info *info,
 	return 0;
 }
 #endif
+void ptdump_check_wx(void);
+#endif /* CONFIG_ARM64_PTDUMP_CORE */
+
+#ifdef CONFIG_DEBUG_WX
+#define debug_checkwx()	ptdump_check_wx()
+#else
+#define debug_checkwx()	do { } while (0)
+#endif
 
 #endif /* __ASM_PTDUMP_H */
diff --git a/arch/arm64/mm/dump.c b/arch/arm64/mm/dump.c
index bb36649..4913af5 100644
--- a/arch/arm64/mm/dump.c
+++ b/arch/arm64/mm/dump.c
@@ -74,6 +74,8 @@ struct pg_state {
 	unsigned long start_address;
 	unsigned level;
 	u64 current_prot;
+	bool check_wx;
+	unsigned long wx_pages;
 };
 
 struct prot_bits {
@@ -202,6 +204,35 @@ static void dump_prot(struct pg_state *st, const struct prot_bits *bits,
 	}
 }
 
+static void note_prot_uxn(struct pg_state *st, unsigned long addr)
+{
+	if (!st->check_wx)
+		return;
+
+	if ((st->current_prot & PTE_UXN) == PTE_UXN)
+		return;
+
+	WARN_ONCE(1, "arm64/mm: Found non-UXN 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_prot_wx(struct pg_state *st, unsigned long addr)
+{
+	if (!st->check_wx)
+		return;
+	if ((st->current_prot & PTE_RDONLY) == PTE_RDONLY)
+		return;
+	if ((st->current_prot & PTE_PXN) == PTE_PXN)
+		return;
+
+	WARN_ONCE(1, "arm64/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 level,
 				u64 val)
 {
@@ -219,6 +250,8 @@ static void note_page(struct pg_state *st, unsigned long addr, unsigned level,
 		unsigned long delta;
 
 		if (st->current_prot) {
+			note_prot_uxn(st, addr);
+			note_prot_wx(st, addr);
 			pt_dump_seq_printf(st->seq, "0x%016lx-0x%016lx   ",
 				   st->start_address, addr);
 
@@ -344,6 +377,25 @@ static struct ptdump_info kernel_ptdump_info = {
 	.base_addr	= VA_START,
 };
 
+void ptdump_check_wx(void)
+{
+	struct pg_state st = {
+		.seq = NULL,
+		.marker = (struct addr_marker[]) {
+			{ -1, NULL},
+		},
+		.check_wx = true,
+	};
+
+	walk_pgd(&st, &init_mm, 0);
+	note_page(&st, 0, 0, 0);
+	if (st.wx_pages)
+		pr_info("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)
 {
 	ptdump_initialize();
diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 05615a3..2cbe2fe 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -42,6 +42,7 @@
 #include <asm/tlb.h>
 #include <asm/memblock.h>
 #include <asm/mmu_context.h>
+#include <asm/ptdump.h>
 
 u64 idmap_t0sz = TCR_T0SZ(VA_BITS);
 
@@ -396,6 +397,7 @@ void mark_rodata_ro(void)
 	section_size = (unsigned long)__init_begin - (unsigned long)__start_rodata;
 	create_mapping_late(__pa(__start_rodata), (unsigned long)__start_rodata,
 			    section_size, PAGE_KERNEL_RO);
+	debug_checkwx();
 }
 
 static void __init map_kernel_segment(pgd_t *pgd, void *va_start, void *va_end,
-- 
2.7.4

[PATCHv2 1/4] arm64: dump: Make ptdump debugfs a separate option

[Kees Cook]

On Wed, Oct 12, 2016 at 3:31 PM, Laura Abbott <labbott@redhat.com> wrote:
>
> ptdump_register currently initializes a set of page table information and
> registers debugfs. There are uses for the ptdump option without wanting the
> debugfs options. Split this out to make it a separate option.
>
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> v2: Minor style fixups per Mark Rutland, intialization is now separate from
>     register since it never needed to be combined in the first place, EFI
>     page table registration.
> ---
>  arch/arm64/Kconfig.debug           |  6 +++++-
>  arch/arm64/include/asm/ptdump.h    | 13 ++++++++-----
>  arch/arm64/mm/Makefile             |  3 ++-
>  arch/arm64/mm/dump.c               | 26 +++++---------------------
>  arch/arm64/mm/ptdump_debugfs.c     | 31 +++++++++++++++++++++++++++++++
>  drivers/firmware/efi/arm-runtime.c |  5 ++---
>  6 files changed, 53 insertions(+), 31 deletions(-)
>  create mode 100644 arch/arm64/mm/ptdump_debugfs.c
>
> diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
> index b661fe7..21a5b74 100644
> --- a/arch/arm64/Kconfig.debug
> +++ b/arch/arm64/Kconfig.debug
> @@ -2,9 +2,13 @@ menu "Kernel hacking"
>
>  source "lib/Kconfig.debug"
>
> -config ARM64_PTDUMP
> +config ARM64_PTDUMP_CORE
> +       def_bool n
> +
> +config ARM64_PTDUMP_DEBUGFS
>         bool "Export kernel pagetable layout to userspace via debugfs"

I think this can be a tristate now, yes? (I did this on x86 so I could
load ptdump as a module for Chrome OS image testing but then leave the
module off the read-only partition for release images...)

https://git.kernel.org/linus/8609d1b5daa36350e020e737946c40887af1743a

-Kees

>         depends on DEBUG_KERNEL
> +       select ARM64_PTDUMP_CORE
>         select DEBUG_FS
>          help
>           Say Y here if you want to show the kernel pagetable layout in a



-- 
Kees Cook
Nexus Security

[PATCHv2 1/4] arm64: dump: Make ptdump debugfs a separate option

[Laura Abbott]

On 10/12/2016 03:45 PM, Kees Cook wrote:
> On Wed, Oct 12, 2016 at 3:31 PM, Laura Abbott <labbott@redhat.com> wrote:
>>
>> ptdump_register currently initializes a set of page table information and
>> registers debugfs. There are uses for the ptdump option without wanting the
>> debugfs options. Split this out to make it a separate option.
>>
>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>> ---
>> v2: Minor style fixups per Mark Rutland, intialization is now separate from
>>     register since it never needed to be combined in the first place, EFI
>>     page table registration.
>> ---
>>  arch/arm64/Kconfig.debug           |  6 +++++-
>>  arch/arm64/include/asm/ptdump.h    | 13 ++++++++-----
>>  arch/arm64/mm/Makefile             |  3 ++-
>>  arch/arm64/mm/dump.c               | 26 +++++---------------------
>>  arch/arm64/mm/ptdump_debugfs.c     | 31 +++++++++++++++++++++++++++++++
>>  drivers/firmware/efi/arm-runtime.c |  5 ++---
>>  6 files changed, 53 insertions(+), 31 deletions(-)
>>  create mode 100644 arch/arm64/mm/ptdump_debugfs.c
>>
>> diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
>> index b661fe7..21a5b74 100644
>> --- a/arch/arm64/Kconfig.debug
>> +++ b/arch/arm64/Kconfig.debug
>> @@ -2,9 +2,13 @@ menu "Kernel hacking"
>>
>>  source "lib/Kconfig.debug"
>>
>> -config ARM64_PTDUMP
>> +config ARM64_PTDUMP_CORE
>> +       def_bool n
>> +
>> +config ARM64_PTDUMP_DEBUGFS
>>         bool "Export kernel pagetable layout to userspace via debugfs"
>
> I think this can be a tristate now, yes? (I did this on x86 so I could
> load ptdump as a module for Chrome OS image testing but then leave the
> module off the read-only partition for release images...)
>
> https://git.kernel.org/linus/8609d1b5daa36350e020e737946c40887af1743a
>
> -Kees
>

I saw the module for x86. arm64 works slightly differently since there
are multiple debugfs dumping sources, the init_mm and EFI page tables.
These are built in and can call register but then have no way to
actually register the debugfs entry when the module is inserted since
the current config covers both/all of the exporters. I
decided against trying to add the infrastructure to make modularity
work because I was spending more time on that than the actual W^X checks.

Thanks,
Laura

>>         depends on DEBUG_KERNEL
>> +       select ARM64_PTDUMP_CORE
>>         select DEBUG_FS
>>          help
>>           Say Y here if you want to show the kernel pagetable layout in a
>
>
>

[PATCHv2 1/4] arm64: dump: Make ptdump debugfs a separate option

[Kees Cook]

On Wed, Oct 12, 2016 at 3:57 PM, Laura Abbott <labbott@redhat.com> wrote:
> On 10/12/2016 03:45 PM, Kees Cook wrote:
>>
>> On Wed, Oct 12, 2016 at 3:31 PM, Laura Abbott <labbott@redhat.com> wrote:
>>>
>>>
>>> ptdump_register currently initializes a set of page table information and
>>> registers debugfs. There are uses for the ptdump option without wanting
>>> the
>>> debugfs options. Split this out to make it a separate option.
>>>
>>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>>> ---
>>> v2: Minor style fixups per Mark Rutland, intialization is now separate
>>> from
>>>     register since it never needed to be combined in the first place, EFI
>>>     page table registration.
>>> ---
>>>  arch/arm64/Kconfig.debug           |  6 +++++-
>>>  arch/arm64/include/asm/ptdump.h    | 13 ++++++++-----
>>>  arch/arm64/mm/Makefile             |  3 ++-
>>>  arch/arm64/mm/dump.c               | 26 +++++---------------------
>>>  arch/arm64/mm/ptdump_debugfs.c     | 31 +++++++++++++++++++++++++++++++
>>>  drivers/firmware/efi/arm-runtime.c |  5 ++---
>>>  6 files changed, 53 insertions(+), 31 deletions(-)
>>>  create mode 100644 arch/arm64/mm/ptdump_debugfs.c
>>>
>>> diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
>>> index b661fe7..21a5b74 100644
>>> --- a/arch/arm64/Kconfig.debug
>>> +++ b/arch/arm64/Kconfig.debug
>>> @@ -2,9 +2,13 @@ menu "Kernel hacking"
>>>
>>>  source "lib/Kconfig.debug"
>>>
>>> -config ARM64_PTDUMP
>>> +config ARM64_PTDUMP_CORE
>>> +       def_bool n
>>> +
>>> +config ARM64_PTDUMP_DEBUGFS
>>>         bool "Export kernel pagetable layout to userspace via debugfs"
>>
>>
>> I think this can be a tristate now, yes? (I did this on x86 so I could
>> load ptdump as a module for Chrome OS image testing but then leave the
>> module off the read-only partition for release images...)
>>
>> https://git.kernel.org/linus/8609d1b5daa36350e020e737946c40887af1743a
>>
>> -Kees
>>
>
> I saw the module for x86. arm64 works slightly differently since there
> are multiple debugfs dumping sources, the init_mm and EFI page tables.
> These are built in and can call register but then have no way to
> actually register the debugfs entry when the module is inserted since
> the current config covers both/all of the exporters. I
> decided against trying to add the infrastructure to make modularity
> work because I was spending more time on that than the actual W^X checks.

Okay, sounds fine. I'll see if I can take a look at this in the future
if no one else beats me to it.

In the meantime, this whole series looks good to me. :)

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

-Kees

-- 
Kees Cook
Nexus Security

[PATCHv2 1/4] arm64: dump: Make ptdump debugfs a separate option

[Mark Rutland]

Hi Laura,

In looking at this, I realised I was confused about ptdump_initialize()
previously, and now see why we can't decouple the debugfs registration
of the kernel page tables from the rest of the ptdump init. Sorry for
the noise on that.

Aside from one issue below, this looks good to me.

On Wed, Oct 12, 2016 at 03:31:59PM -0700, Laura Abbott wrote:
> diff --git a/arch/arm64/include/asm/ptdump.h b/arch/arm64/include/asm/ptdump.h
> index 07b8ed0..7c35689 100644
> --- a/arch/arm64/include/asm/ptdump.h
> +++ b/arch/arm64/include/asm/ptdump.h
> @@ -16,9 +16,10 @@
>  #ifndef __ASM_PTDUMP_H
>  #define __ASM_PTDUMP_H
>  
> -#ifdef CONFIG_ARM64_PTDUMP
> +#ifdef CONFIG_ARM64_PTDUMP_CORE
>  
>  #include <linux/mm_types.h>
> +#include <linux/seq_file.h>
>  
>  struct addr_marker {
>  	unsigned long start_address;
> @@ -32,13 +33,15 @@ struct ptdump_info {
>  	unsigned long			max_addr;
>  };
>  
> -int ptdump_register(struct ptdump_info *info, const char *name);
> -
> +void ptdump_walk_pgd(struct seq_file *s, struct ptdump_info *info);
> +#ifdef CONFIG_ARM64_PTDUMP_DEBUGFS
> +int ptdump_debugfs_register(struct ptdump_info *info, const char *name);
>  #else
> -static inline int ptdump_register(struct ptdump_info *info, const char *name)
> +static inline int ptdump_debugfs_register(struct ptdump_info *info,
> +					const char *name)
>  {
>  	return 0;
>  }
> -#endif /* CONFIG_ARM64_PTDUMP */
> +#endif

I think you didn't mean to remove the existing endif here?

It's still needed to guard the CONFIG_ARM64_PTDUMP_CORE case, and the
new one is needed for the new #ifdef CONFIG_ARM64_PTDUMP_DEBUGFS.
Without it, I get a build error with this patch atop of v4.9-rc1 with
CONFIG_ARM64_PTDUMP_DEBUGFS selected:

[mark at leverpostej:~/src/linux]% uselinaro 15.08 make ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- -j10 -s
In file included from arch/arm64/mm/ptdump_debugfs.c:4:0:
./arch/arm64/include/asm/ptdump.h:16:0: error: unterminated #ifndef
 #ifndef __ASM_PTDUMP_H
 ^
make[1]: *** [arch/arm64/mm/ptdump_debugfs.o] Error 1
make[1]: *** Waiting for unfinished jobs....

With that #endif restored, everything works fine. So FWIW, with that:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Mark Rutland <mark.rutland@arm.com>

[...]

> diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c
> index 7c75a8d..33d35e8 100644
> --- a/drivers/firmware/efi/arm-runtime.c
> +++ b/drivers/firmware/efi/arm-runtime.c
> @@ -39,7 +39,7 @@ static struct mm_struct efi_mm = {
>  	.mmlist			= LIST_HEAD_INIT(efi_mm.mmlist),
>  };
>  
> -#ifdef CONFIG_ARM64_PTDUMP
> +#ifdef CONFIG_ARM64_PTDUMP_DEBUGFS
>  #include <asm/ptdump.h>
>  
>  static struct ptdump_info efi_ptdump_info = {
> @@ -53,10 +53,9 @@ static struct ptdump_info efi_ptdump_info = {
>  
>  static int __init ptdump_init(void)
>  {
> -	return ptdump_register(&efi_ptdump_info, "efi_page_tables");
> +	return ptdump_debugfs_register(&efi_ptdump_info, "efi_page_tables");
>  }
>  device_initcall(ptdump_init);
> -
>  #endif

For the EFI changes, we'll need an ack from Ard or Matt; this should
probably be Cc'd to the linux-efi list for that.

Thanks,
Mark.

[PATCHv2 2/4] arm64: dump: Make the page table dumping seq_file optional

[Mark Rutland]

On Wed, Oct 12, 2016 at 03:32:00PM -0700, Laura Abbott wrote:
> 
> The page table dumping code always assumes it will be dumping to a
> seq_file to userspace. Future code will be taking advantage of
> the page table dumping code but will not need the seq_file. Make
> the seq_file optional for these cases.
> 
> Acked-by: Mark Rutland <mark.rutland@arm.com>

Now that I'm back in the office with access to hardware, I've been able
to give this a spin. FWIW, feel free to upgrade the above to:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

[PATCHv2 3/4] arm64: dump: Remove max_addr

[Mark Rutland]

On Wed, Oct 12, 2016 at 03:32:01PM -0700, Laura Abbott wrote:
> 
> max_addr was added as part of struct ptdump_info but has never actually
> been used. Remove it.
> 
> Signed-off-by: Laura Abbott <labbott@redhat.com>

Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

> ---
> New for v2 of the series
> ---
>  arch/arm64/include/asm/ptdump.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/ptdump.h b/arch/arm64/include/asm/ptdump.h
> index 7c35689..8fc0957 100644
> --- a/arch/arm64/include/asm/ptdump.h
> +++ b/arch/arm64/include/asm/ptdump.h
> @@ -30,7 +30,6 @@ struct ptdump_info {
>  	struct mm_struct		*mm;
>  	const struct addr_marker	*markers;
>  	unsigned long			base_addr;
> -	unsigned long			max_addr;
>  };
>  
>  void ptdump_walk_pgd(struct seq_file *s, struct ptdump_info *info);
> -- 
> 2.7.4
> 

[PATCHv2 4/4] arm64: dump: Add checking for writable and exectuable pages

[Mark Rutland]

On Wed, Oct 12, 2016 at 03:32:02PM -0700, Laura Abbott wrote:
> +config DEBUG_WX
> +	bool "Warn on W+X mappings at boot"
> +	select ARM64_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.
> +	  This check also includes UXN, which should be set on all kernel
> +	  mappings.
> +
> +	  Look for a message in dmesg output like this:
> +
> +	    arm64/mm: Checked W+X mappings: passed, no W+X pages found.
> +
> +	  or like this, if the check failed:
> +
> +	    arm64/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".
> +
> +

Trivial nit: for consistency with the rest of the file, there should
only be one line space between options.

>  config DEBUG_SET_MODULE_RONX
>  	bool "Set loadable kernel module data as NX and text as RO"
>  	depends on MODULES
> diff --git a/arch/arm64/include/asm/ptdump.h b/arch/arm64/include/asm/ptdump.h
> index 8fc0957..6afd847 100644
> --- a/arch/arm64/include/asm/ptdump.h
> +++ b/arch/arm64/include/asm/ptdump.h
> @@ -42,5 +42,13 @@ static inline int ptdump_debugfs_register(struct ptdump_info *info,
>  	return 0;
>  }
>  #endif
> +void ptdump_check_wx(void);
> +#endif /* CONFIG_ARM64_PTDUMP_CORE */

... ah, here's the missing #endif from patch 1.

Assuming that gets sorted out, this looks good to me, and works on juno,
so FWIW:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Mark Rutland <mark.rutland@arm.com>

[...]

> +static void note_prot_uxn(struct pg_state *st, unsigned long addr)
> +{
> +	if (!st->check_wx)
> +		return;
> +
> +	if ((st->current_prot & PTE_UXN) == PTE_UXN)
> +		return;
> +
> +	WARN_ONCE(1, "arm64/mm: Found non-UXN mapping at address %p/%pS\n",
> +		  (void *)st->start_address, (void *)st->start_address);
> +
> +	st->wx_pages += (addr - st->start_address) / PAGE_SIZE;
> +}

As a future thought (I've scope-creeped this enough with the UXN check),
there are some other checks that we could add, like verifying the AP
bits don't allow user data access. That might be worth considering, with
DEBUG_WX becoming a more general kernel page table sanity check.

Thanks,
Mark.

[PATCHv2 1/4] arm64: dump: Make ptdump debugfs a separate option

[Laura Abbott]

On 10/17/2016 03:52 AM, Mark Rutland wrote:
> Hi Laura,
>
> In looking at this, I realised I was confused about ptdump_initialize()
> previously, and now see why we can't decouple the debugfs registration
> of the kernel page tables from the rest of the ptdump init. Sorry for
> the noise on that.
>
> Aside from one issue below, this looks good to me.
>
> On Wed, Oct 12, 2016 at 03:31:59PM -0700, Laura Abbott wrote:
>> diff --git a/arch/arm64/include/asm/ptdump.h b/arch/arm64/include/asm/ptdump.h
>> index 07b8ed0..7c35689 100644
>> --- a/arch/arm64/include/asm/ptdump.h
>> +++ b/arch/arm64/include/asm/ptdump.h
>> @@ -16,9 +16,10 @@
>>  #ifndef __ASM_PTDUMP_H
>>  #define __ASM_PTDUMP_H
>>
>> -#ifdef CONFIG_ARM64_PTDUMP
>> +#ifdef CONFIG_ARM64_PTDUMP_CORE
>>
>>  #include <linux/mm_types.h>
>> +#include <linux/seq_file.h>
>>
>>  struct addr_marker {
>>  	unsigned long start_address;
>> @@ -32,13 +33,15 @@ struct ptdump_info {
>>  	unsigned long			max_addr;
>>  };
>>
>> -int ptdump_register(struct ptdump_info *info, const char *name);
>> -
>> +void ptdump_walk_pgd(struct seq_file *s, struct ptdump_info *info);
>> +#ifdef CONFIG_ARM64_PTDUMP_DEBUGFS
>> +int ptdump_debugfs_register(struct ptdump_info *info, const char *name);
>>  #else
>> -static inline int ptdump_register(struct ptdump_info *info, const char *name)
>> +static inline int ptdump_debugfs_register(struct ptdump_info *info,
>> +					const char *name)
>>  {
>>  	return 0;
>>  }
>> -#endif /* CONFIG_ARM64_PTDUMP */
>> +#endif
>
> I think you didn't mean to remove the existing endif here?
>
> It's still needed to guard the CONFIG_ARM64_PTDUMP_CORE case, and the
> new one is needed for the new #ifdef CONFIG_ARM64_PTDUMP_DEBUGFS.
> Without it, I get a build error with this patch atop of v4.9-rc1 with
> CONFIG_ARM64_PTDUMP_DEBUGFS selected:
>
> [mark at leverpostej:~/src/linux]% uselinaro 15.08 make ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- -j10 -s
> In file included from arch/arm64/mm/ptdump_debugfs.c:4:0:
> ./arch/arm64/include/asm/ptdump.h:16:0: error: unterminated #ifndef
>  #ifndef __ASM_PTDUMP_H
>  ^
> make[1]: *** [arch/arm64/mm/ptdump_debugfs.o] Error 1
> make[1]: *** Waiting for unfinished jobs....
>
> With that #endif restored, everything works fine. So FWIW, with that:
>

Ugh rebase/refactor failure on my part.

> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> Tested-by: Mark Rutland <mark.rutland@arm.com>
>
> [...]
>
>> diff --git a/drivers/firmware/efi/arm-runtime.c b/drivers/firmware/efi/arm-runtime.c
>> index 7c75a8d..33d35e8 100644
>> --- a/drivers/firmware/efi/arm-runtime.c
>> +++ b/drivers/firmware/efi/arm-runtime.c
>> @@ -39,7 +39,7 @@ static struct mm_struct efi_mm = {
>>  	.mmlist			= LIST_HEAD_INIT(efi_mm.mmlist),
>>  };
>>
>> -#ifdef CONFIG_ARM64_PTDUMP
>> +#ifdef CONFIG_ARM64_PTDUMP_DEBUGFS
>>  #include <asm/ptdump.h>
>>
>>  static struct ptdump_info efi_ptdump_info = {
>> @@ -53,10 +53,9 @@ static struct ptdump_info efi_ptdump_info = {
>>
>>  static int __init ptdump_init(void)
>>  {
>> -	return ptdump_register(&efi_ptdump_info, "efi_page_tables");
>> +	return ptdump_debugfs_register(&efi_ptdump_info, "efi_page_tables");
>>  }
>>  device_initcall(ptdump_init);
>> -
>>  #endif
>
> For the EFI changes, we'll need an ack from Ard or Matt; this should
> probably be Cc'd to the linux-efi list for that.

Good point, I'll do that for v3.

>
> Thanks,
> Mark.
>

Thanks,
Laura