[PATCH 0/3] WX Checking for arm64

[PATCH 0/3] WX Checking for arm64

[Laura Abbott]

Hi,

This is an implementation to check for writable and executable pages on arm64.
This is heavily based on the x86 version which uses the existing page table
dumping code to do the checking. Some notes:

- The W^X checking is important so this option should become defaut eventually.
  To make this feasible, the debugfs functionality has been split out as a
  separate option. I didn't see a good way to make it modular like x86 but
  an option should be good enough.
- This checks all page tables registered with ptdump_register. I don't see this
  being called elsewhere right now though.
- Once this is merged, I'd like to see about moving DEBUG_WX to the top level
  instead of having each arch call it in mark_rodata.

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

 arch/arm64/Kconfig.debug        | 34 ++++++++++++++-
 arch/arm64/include/asm/ptdump.h | 25 ++++++++++-
 arch/arm64/mm/Makefile          |  3 +-
 arch/arm64/mm/dump.c            | 92 ++++++++++++++++++++++++++++-------------
 arch/arm64/mm/mmu.c             |  2 +
 arch/arm64/mm/ptdump_debugfs.c  | 33 +++++++++++++++
 6 files changed, 157 insertions(+), 32 deletions(-)
 create mode 100644 arch/arm64/mm/ptdump_debugfs.c

-- 
2.10.0

[PATCH 1/3] 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>
---
 arch/arm64/Kconfig.debug        |  6 +++++-
 arch/arm64/include/asm/ptdump.h | 15 +++++++++++++--
 arch/arm64/mm/Makefile          |  3 ++-
 arch/arm64/mm/dump.c            | 30 +++++++++---------------------
 arch/arm64/mm/ptdump_debugfs.c  | 33 +++++++++++++++++++++++++++++++++
 5 files changed, 62 insertions(+), 25 deletions(-)
 create mode 100644 arch/arm64/mm/ptdump_debugfs.c

diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
index 0cc758c..9015f02 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..b18a62c 100644
--- a/arch/arm64/include/asm/ptdump.h
+++ b/arch/arm64/include/asm/ptdump.h
@@ -16,8 +16,9 @@
 #ifndef __ASM_PTDUMP_H
 #define __ASM_PTDUMP_H
 
-#ifdef CONFIG_ARM64_PTDUMP
+#ifdef CONFIG_ARM64_PTDUMP_CORE
 
+#include <linux/seq_file.h>
 #include <linux/mm_types.h>
 
 struct addr_marker {
@@ -33,12 +34,22 @@ struct ptdump_info {
 };
 
 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_create(struct ptdump_info *info, const char *name);
+#else
+static inline int ptdump_debugfs_create(struct ptdump_info *info,
+					const char *name)
+{
+	return 0;
+}
+#endif
 
 #else
 static inline int ptdump_register(struct ptdump_info *info, const char *name)
 {
 	return 0;
 }
-#endif /* CONFIG_ARM64_PTDUMP */
+#endif /* CONFIG_ARM64_PTDUMP_CORE */
 
 #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..29e0838 100644
--- a/arch/arm64/mm/dump.c
+++ b/arch/arm64/mm/dump.c
@@ -286,7 +286,7 @@ static void walk_pud(struct pg_state *st, pgd_t *pgd, unsigned long start)
 	}
 }
 
-static void walk_pgd(struct pg_state *st, struct mm_struct *mm,
+static void __walk_pgd(struct pg_state *st, struct mm_struct *mm,
 		     unsigned long start)
 {
 	pgd_t *pgd = pgd_offset(mm, 0UL);
@@ -304,44 +304,32 @@ 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,
 	};
 
-	walk_pgd(&st, info->mm, info->base_addr);
+	__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(struct ptdump_info *info)
 {
-	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;
+int ptdump_register(struct ptdump_info *info, const char *name)
+{
+	ptdump_initialize(info);
+	return ptdump_debugfs_create(info, name);
 }
 
 static struct ptdump_info kernel_ptdump_info = {
diff --git a/arch/arm64/mm/ptdump_debugfs.c b/arch/arm64/mm/ptdump_debugfs.c
new file mode 100644
index 0000000..03e161f
--- /dev/null
+++ b/arch/arm64/mm/ptdump_debugfs.c
@@ -0,0 +1,33 @@
+#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_create(struct ptdump_info *info, const char *name)
+{
+	struct dentry *pe;
+	pe = debugfs_create_file(name, 0400, NULL, info, &ptdump_fops);
+	return pe ? 0 : -ENOMEM;
+
+}
+
+
-- 
2.10.0

[PATCH 1/3] arm64: dump: Make ptdump debugfs a separate option

[Mark Rutland]

Hi,

On Thu, Sep 29, 2016 at 02:32:55PM -0700, Laura Abbott 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>
> ---
>  arch/arm64/Kconfig.debug        |  6 +++++-
>  arch/arm64/include/asm/ptdump.h | 15 +++++++++++++--
>  arch/arm64/mm/Makefile          |  3 ++-
>  arch/arm64/mm/dump.c            | 30 +++++++++---------------------
>  arch/arm64/mm/ptdump_debugfs.c  | 33 +++++++++++++++++++++++++++++++++
>  5 files changed, 62 insertions(+), 25 deletions(-)
>  create mode 100644 arch/arm64/mm/ptdump_debugfs.c

As a heads-up, Ard has new ARM64_PTUMP user under drivers/firmware/efi queued
up in the EFI tree, which will also need fixing up. See commit d80448ac92b72051
("efi/arm64: Add debugfs node to dump UEFI runtime page tables") [1].

[...]

> +#include <linux/seq_file.h>
>  #include <linux/mm_types.h>

Nit: please keep headers in alphabetical order.

> -static void walk_pgd(struct pg_state *st, struct mm_struct *mm,
> +static void __walk_pgd(struct pg_state *st, struct mm_struct *mm,

Can we leave this name as-is? We didn't change walk_{pud,pmd,pte}, so this is
inconsistent, and we haven't reused the name.

[...]

> +int ptdump_register(struct ptdump_info *info, const char *name)
> +{
> +	ptdump_initialize(info);
> +	return ptdump_debugfs_create(info, name);
>  }

It feels like a layering violation to have the core ptdump code call the
debugfs ptdump code. Is there some reason this has to live here?

Other than the above points, this looks good to me.

Thanks,
Mark.

[1] https://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/commit/?h=next&id=9d80448ac92b720512c415265597d349d8b5c3e8

[PATCH 1/3] arm64: dump: Make ptdump debugfs a separate option

[Laura Abbott]

On 09/29/2016 05:13 PM, Mark Rutland wrote:
> Hi,
>
> On Thu, Sep 29, 2016 at 02:32:55PM -0700, Laura Abbott 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>
>> ---
>>  arch/arm64/Kconfig.debug        |  6 +++++-
>>  arch/arm64/include/asm/ptdump.h | 15 +++++++++++++--
>>  arch/arm64/mm/Makefile          |  3 ++-
>>  arch/arm64/mm/dump.c            | 30 +++++++++---------------------
>>  arch/arm64/mm/ptdump_debugfs.c  | 33 +++++++++++++++++++++++++++++++++
>>  5 files changed, 62 insertions(+), 25 deletions(-)
>>  create mode 100644 arch/arm64/mm/ptdump_debugfs.c
>
> As a heads-up, Ard has new ARM64_PTUMP user under drivers/firmware/efi queued
> up in the EFI tree, which will also need fixing up. See commit d80448ac92b72051
> ("efi/arm64: Add debugfs node to dump UEFI runtime page tables") [1].
>
> [...]
>

I'll take a look at that, thanks for the pointer!

>> +#include <linux/seq_file.h>
>>  #include <linux/mm_types.h>
>
> Nit: please keep headers in alphabetical order.
>
>> -static void walk_pgd(struct pg_state *st, struct mm_struct *mm,
>> +static void __walk_pgd(struct pg_state *st, struct mm_struct *mm,
>
> Can we leave this name as-is? We didn't change walk_{pud,pmd,pte}, so this is
> inconsistent, and we haven't reused the name.
>

Yes, I think this is a relic of earlier refactoring attempts.

> [...]
>
>> +int ptdump_register(struct ptdump_info *info, const char *name)
>> +{
>> +	ptdump_initialize(info);
>> +	return ptdump_debugfs_create(info, name);
>>  }
>
> It feels like a layering violation to have the core ptdump code call the
> debugfs ptdump code. Is there some reason this has to live here?
>

Which 'this' are you referring to here? Are you suggesting moving
the ptdump_register elsewhere or moving the debugfs create elsewhere?

> Other than the above points, this looks good to me.
>
> Thanks,
> Mark.
>
> [1] https://git.kernel.org/cgit/linux/kernel/git/mfleming/efi.git/commit/?h=next&id=9d80448ac92b720512c415265597d349d8b5c3e8
>

[PATCH 1/3] arm64: dump: Make ptdump debugfs a separate option

[Mark Rutland]

On Thu, Sep 29, 2016 at 05:31:09PM -0700, Laura Abbott wrote:
> On 09/29/2016 05:13 PM, Mark Rutland wrote:
> >On Thu, Sep 29, 2016 at 02:32:55PM -0700, Laura Abbott wrote:
> >>+int ptdump_register(struct ptdump_info *info, const char *name)
> >>+{
> >>+	ptdump_initialize(info);
> >>+	return ptdump_debugfs_create(info, name);
> >> }
> >
> >It feels like a layering violation to have the core ptdump code call the
> >debugfs ptdump code. Is there some reason this has to live here?
> 
> Which 'this' are you referring to here? Are you suggesting moving
> the ptdump_register elsewhere or moving the debugfs create elsewhere?

Sorry, I should have worded that better.

I meant moving ptdump_register into ptdump_debugfs.c, perhaps renamed to make it
clear it's debugfs-specific.

We could instead update existing users to call ptdump_debugfs_create()
directly, and have that call ptdump_initialize(), which could itself become a
staic inline in a header.

Thanks,
Mark.

[PATCH 1/3] arm64: dump: Make ptdump debugfs a separate option

[Laura Abbott]

On 09/29/2016 05:48 PM, Mark Rutland wrote:
> On Thu, Sep 29, 2016 at 05:31:09PM -0700, Laura Abbott wrote:
>> On 09/29/2016 05:13 PM, Mark Rutland wrote:
>>> On Thu, Sep 29, 2016 at 02:32:55PM -0700, Laura Abbott wrote:
>>>> +int ptdump_register(struct ptdump_info *info, const char *name)
>>>> +{
>>>> +	ptdump_initialize(info);
>>>> +	return ptdump_debugfs_create(info, name);
>>>> }
>>>
>>> It feels like a layering violation to have the core ptdump code call the
>>> debugfs ptdump code. Is there some reason this has to live here?
>>
>> Which 'this' are you referring to here? Are you suggesting moving
>> the ptdump_register elsewhere or moving the debugfs create elsewhere?
>
> Sorry, I should have worded that better.
>
> I meant moving ptdump_register into ptdump_debugfs.c, perhaps renamed to make it
> clear it's debugfs-specific.
>
> We could instead update existing users to call ptdump_debugfs_create()
> directly, and have that call ptdump_initialize(), which could itself become a
> staic inline in a header.

Ah okay, I see what you are suggesting. ptdump_initialize should still
happen regardless of debugfs status though so I guess 
ptdump_debugfs_create would just get turned into just ptdump_initialize
which seems a little unclear. I'll come up with some other shed 
colors^W^Wfunction names.

Thanks,
Laura

[PATCH 1/3] arm64: dump: Make ptdump debugfs a separate option

[Mark Rutland]

On Thu, Sep 29, 2016 at 06:11:44PM -0700, Laura Abbott wrote:
> On 09/29/2016 05:48 PM, Mark Rutland wrote:
> >On Thu, Sep 29, 2016 at 05:31:09PM -0700, Laura Abbott wrote:
> >>On 09/29/2016 05:13 PM, Mark Rutland wrote:
> >>>On Thu, Sep 29, 2016 at 02:32:55PM -0700, Laura Abbott wrote:
> >>>>+int ptdump_register(struct ptdump_info *info, const char *name)
> >>>>+{
> >>>>+	ptdump_initialize(info);
> >>>>+	return ptdump_debugfs_create(info, name);
> >>>>}

> >I meant moving ptdump_register into ptdump_debugfs.c, perhaps renamed to make it
> >clear it's debugfs-specific.
> >
> >We could instead update existing users to call ptdump_debugfs_create()
> >directly, and have that call ptdump_initialize(), which could itself become a
> >staic inline in a header.
> 
> Ah okay, I see what you are suggesting. ptdump_initialize should still
> happen regardless of debugfs status though so I guess ptdump_debugfs_create
> would just get turned into just ptdump_initialize
> which seems a little unclear. I'll come up with some other shed
> colors^W^Wfunction names.

Cheers!

FWIW, given ptsump_initialize() is only going to be called with the ptdump core
and debugfs code, I'm not all that concerned by what it's called. A few leading
underscores is about the only thing that comes to mind, but even as-is I think
it should be fine.

Thanks,
Mark.

[PATCH 2/3] 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. The dumping code is useful in other situations.
Let the seq_file be optional.

Signed-off-by: Laura Abbott <labbott@redhat.com>
---
 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 29e0838..e318f3d 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.10.0

[PATCH 2/3] arm64: dump: Make the page table dumping seq_file optional

[Mark Rutland]

On Thu, Sep 29, 2016 at 02:32:56PM -0700, Laura Abbott wrote:
> The page table dumping code always assumes it will be dumping to a
> seq_file to userspace. The dumping code is useful in other situations.
> Let the seq_file be optional.
>
> Signed-off-by: Laura Abbott <labbott@redhat.com>

It might be worth elaborating on those other situations, e.g. that for those
we'll have some additional logic that will only run in the absence of a
seq_file.

The NOPing out of logic in the !seq_file case does feel a bit like spaghetti
code, but it's not obvious to me that adding finer-grained callbacks is much
better, and I guess we can reconsider that if and when we need to add more
logic.

Regardless of the above:

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

Thanks,
Mark.

> ---
>  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 29e0838..e318f3d 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.10.0
> 

[PATCH 3/3] 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>
---
 arch/arm64/Kconfig.debug        | 28 ++++++++++++++++++++++++++++
 arch/arm64/include/asm/ptdump.h | 10 ++++++++++
 arch/arm64/mm/dump.c            | 36 ++++++++++++++++++++++++++++++++++++
 arch/arm64/mm/mmu.c             |  2 ++
 4 files changed, 76 insertions(+)

diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
index 9015f02..037dba4 100644
--- a/arch/arm64/Kconfig.debug
+++ b/arch/arm64/Kconfig.debug
@@ -42,6 +42,34 @@ 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.
+
+	  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 b18a62c..e3c6bc0 100644
--- a/arch/arm64/include/asm/ptdump.h
+++ b/arch/arm64/include/asm/ptdump.h
@@ -20,6 +20,7 @@
 
 #include <linux/seq_file.h>
 #include <linux/mm_types.h>
+#include <linux/list.h>
 
 struct addr_marker {
 	unsigned long start_address;
@@ -31,6 +32,8 @@ struct ptdump_info {
 	const struct addr_marker	*markers;
 	unsigned long			base_addr;
 	unsigned long			max_addr;
+	/* Internal, do not touch */
+	struct list_head		node;
 };
 
 int ptdump_register(struct ptdump_info *info, const char *name);
@@ -44,6 +47,13 @@ static inline int ptdump_debugfs_create(struct ptdump_info *info,
 	return 0;
 }
 #endif
+void ptdump_check_wx(void);
+
+#ifdef CONFIG_DEBUG_WX
+#define debug_checkwx()	ptdump_check_wx()
+#else
+#define debug_checkwx()	do { } while (0)
+#endif
 
 #else
 static inline int ptdump_register(struct ptdump_info *info, const char *name)
diff --git a/arch/arm64/mm/dump.c b/arch/arm64/mm/dump.c
index e318f3d..b0b1dd6 100644
--- a/arch/arm64/mm/dump.c
+++ b/arch/arm64/mm/dump.c
@@ -29,6 +29,8 @@
 #include <asm/pgtable-hwdef.h>
 #include <asm/ptdump.h>
 
+static LIST_HEAD(dump_info);
+
 static const struct addr_marker address_markers[] = {
 #ifdef CONFIG_KASAN
 	{ KASAN_SHADOW_START,		"Kasan shadow start" },
@@ -74,6 +76,8 @@ struct pg_state {
 	unsigned long start_address;
 	unsigned level;
 	u64 current_prot;
+	bool check_wx;
+	unsigned long wx_pages;
 };
 
 struct prot_bits {
@@ -219,6 +223,15 @@ static void note_page(struct pg_state *st, unsigned long addr, unsigned level,
 		unsigned long delta;
 
 		if (st->current_prot) {
+			if (st->check_wx &&
+			((st->current_prot & PTE_RDONLY) != PTE_RDONLY) &&
+			((st->current_prot & PTE_PXN) != PTE_PXN)) {
+				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;
+			}
+
 			pt_dump_seq_printf(st->seq, "0x%016lx-0x%016lx   ",
 				   st->start_address, addr);
 
@@ -341,6 +354,7 @@ static void ptdump_initialize(struct ptdump_info *info)
 int ptdump_register(struct ptdump_info *info, const char *name)
 {
 	ptdump_initialize(info);
+	list_add(&info->node, &dump_info);
 	return ptdump_debugfs_create(info, name);
 }
 
@@ -350,6 +364,28 @@ static struct ptdump_info kernel_ptdump_info = {
 	.base_addr	= VA_START,
 };
 
+void ptdump_check_wx(void)
+{
+	struct ptdump_info *info;
+
+	list_for_each_entry(info, &dump_info, node) {
+		struct pg_state st = {
+			.seq = NULL,
+			.marker = info->markers,
+			.check_wx = true,
+		};
+
+		__walk_pgd(&st, info->mm, info->base_addr);
+		note_page(&st, 0, 0, 0);
+		if (st.wx_pages)
+			pr_info("Checked W+X mappings (%p): FAILED, %lu W+X pages found\n",
+				info->mm,
+				st.wx_pages);
+		else
+			pr_info("Checked W+X mappings (%p): passed, no W+X pages found\n", info->mm);
+	}
+}
+
 static int ptdump_init(void)
 {
 	return ptdump_register(&kernel_ptdump_info, "kernel_page_tables");
diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 4989948..1f036d2 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -41,6 +41,7 @@
 #include <asm/tlb.h>
 #include <asm/memblock.h>
 #include <asm/mmu_context.h>
+#include <asm/ptdump.h>
 
 #include "mm.h"
 
@@ -397,6 +398,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();
 }
 
 void fixup_init(void)
-- 
2.10.0

[PATCH 3/3] arm64: dump: Add checking for writable and exectuable pages

[Mark Rutland]

Hi,

On Thu, Sep 29, 2016 at 02:32:57PM -0700, Laura Abbott wrote:
> 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>

> @@ -31,6 +32,8 @@ struct ptdump_info {
>  	const struct addr_marker	*markers;
>  	unsigned long			base_addr;
>  	unsigned long			max_addr;

(unrelated aside: it looks like max_addr is never used or even assigned to;
care to delete it in a prep patch?)

> +	/* Internal, do not touch */
> +	struct list_head		node;
>  };

> +static LIST_HEAD(dump_info);

With the EFI runtime map tables it's unfortunately valid (and very likely with
64K pages) that there will be RWX mappings, at least with contemporary versions
of the UEFI spec. Luckily, those are only installed rarely and transiently.

Given that (and other potential ptdump users), I don't think we should have a
dynamic list of ptdump_infos for W^X checks, and should instead have
ptdump_check_wx() explicitly check the tables we care about. More comments
below on that.

I think we only care about the swapper and hyp tables, as nothing else is
permanent. Does that sound sane?

>  struct prot_bits {
> @@ -219,6 +223,15 @@ static void note_page(struct pg_state *st, unsigned long addr, unsigned level,
>  		unsigned long delta;
>  
>  		if (st->current_prot) {
> +			if (st->check_wx &&
> +			((st->current_prot & PTE_RDONLY) != PTE_RDONLY) &&
> +			((st->current_prot & PTE_PXN) != PTE_PXN)) {
> +				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;
> +			}
> +

Currently note_page() is painful to read due to the indentation and logic.
Rather than adding to that, could we factor this into a helper? e.g.

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;
}

> +void ptdump_check_wx(void)
> +{
> +	struct ptdump_info *info;
> +
> +	list_for_each_entry(info, &dump_info, node) {
> +		struct pg_state st = {
> +			.seq = NULL,
> +			.marker = info->markers,
> +			.check_wx = true,
> +		};
> +
> +		__walk_pgd(&st, info->mm, info->base_addr);
> +		note_page(&st, 0, 0, 0);
> +		if (st.wx_pages)
> +			pr_info("Checked W+X mappings (%p): FAILED, %lu W+X pages found\n",
> +				info->mm,
> +				st.wx_pages);
> +		else
> +			pr_info("Checked W+X mappings (%p): passed, no W+X pages found\n", info->mm);
> +	}
> +}

As above, I don't think we should use a list of arbitrary ptdump_infos.

Given we won't log addresses in the walking code, I think that we can make up a
trivial marker array, and then just use init_mm direct, e.g. (never even
compile-tested):

void ptdump_check_wx(void)
{
	struct pg_state st = {
		.seq = NULL,
		.marker = (struct addr_markers[]) {
			{ -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 (%p): FAILED, %lu W+X pages found\n",
			info->mm,
			st.wx_pages);
	else
		pr_info("Checked W+X mappings (%p): passed, no W+X pages found\n", info->mm);
}

Otherwise, this looks good to me. Thanks for putting this together!

Mark.

[PATCH 3/3] arm64: dump: Add checking for writable and exectuable pages

[Mark Rutland]

On Thu, Sep 29, 2016 at 02:32:57PM -0700, Laura Abbott wrote:
> @@ -219,6 +223,15 @@ static void note_page(struct pg_state *st, unsigned long addr, unsigned level,
>  		unsigned long delta;
>  
>  		if (st->current_prot) {
> +			if (st->check_wx &&
> +			((st->current_prot & PTE_RDONLY) != PTE_RDONLY) &&
> +			((st->current_prot & PTE_PXN) != PTE_PXN)) {
> +				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;
> +			}
> +

Would it be worth verifying that all kernel mappings are UXN, too?

ARMv8 allows execute-only mappings, and a !UXN mapping could result in an info
leak (e.g. pointers in MOVZ+MOVK sequences), or potential asynchronous issues
(e.g. user instruction fetches accessing read-destructive device registers).
All kernel mappings *should* be UXN.

Thanks,
Mark.

[PATCH 3/3] arm64: dump: Add checking for writable and exectuable pages

[Kees Cook]

On Fri, Sep 30, 2016 at 8:58 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Thu, Sep 29, 2016 at 02:32:57PM -0700, Laura Abbott wrote:
>> @@ -219,6 +223,15 @@ static void note_page(struct pg_state *st, unsigned long addr, unsigned level,
>>               unsigned long delta;
>>
>>               if (st->current_prot) {
>> +                     if (st->check_wx &&
>> +                     ((st->current_prot & PTE_RDONLY) != PTE_RDONLY) &&
>> +                     ((st->current_prot & PTE_PXN) != PTE_PXN)) {
>> +                             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;
>> +                     }
>> +
>
> Would it be worth verifying that all kernel mappings are UXN, too?
>
> ARMv8 allows execute-only mappings, and a !UXN mapping could result in an info
> leak (e.g. pointers in MOVZ+MOVK sequences), or potential asynchronous issues
> (e.g. user instruction fetches accessing read-destructive device registers).
> All kernel mappings *should* be UXN.

I love this idea, but based on what came up with hardened usercopy,
there are a lot of readers of kernel memory still. I think the
expectations around UXN need to be clarified so we can reason about
things like perf that want to read the kernel text.

-Kees

-- 
Kees Cook
Nexus Security

[PATCH 3/3] arm64: dump: Add checking for writable and exectuable pages

[Mark Rutland]

On Fri, Sep 30, 2016 at 09:25:45AM -0700, Kees Cook wrote:
> On Fri, Sep 30, 2016 at 8:58 AM, Mark Rutland <mark.rutland@arm.com> wrote:

> > Would it be worth verifying that all kernel mappings are UXN, too?
> >
> > ARMv8 allows execute-only mappings, and a !UXN mapping could result in an info
> > leak (e.g. pointers in MOVZ+MOVK sequences), or potential asynchronous issues
> > (e.g. user instruction fetches accessing read-destructive device registers).
> > All kernel mappings *should* be UXN.
> 
> I love this idea, but based on what came up with hardened usercopy,
> there are a lot of readers of kernel memory still. I think the
> expectations around UXN need to be clarified so we can reason about
> things like perf that want to read the kernel text.

The UXN (User eXecute Never) bit only controls whether userspace can execute a
page, not whether the kernel can read it. The RW permissions come from the AP
bits regardless.

We already try to ensure that all kernel memory is UXN by construction, so this
would just be a sanity check, as with the rest of the W^X checks.

The MOVZ+MOVK case above is where a sequence of 16-bit immediate MOVs are used
to encode a pointer. If a kernel mapping lacked UXN, userspace could execute it
(unprivileged), and extract the pointer generated into a GPR.

Having kernel exec-only memory is a different story entirely, though I agree
it's something to look into.

Thanks,
Mark

[PATCH 3/3] arm64: dump: Add checking for writable and exectuable pages

[Kees Cook]

On Fri, Sep 30, 2016 at 9:41 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Fri, Sep 30, 2016 at 09:25:45AM -0700, Kees Cook wrote:
>> On Fri, Sep 30, 2016 at 8:58 AM, Mark Rutland <mark.rutland@arm.com> wrote:
>
>> > Would it be worth verifying that all kernel mappings are UXN, too?
>> >
>> > ARMv8 allows execute-only mappings, and a !UXN mapping could result in an info
>> > leak (e.g. pointers in MOVZ+MOVK sequences), or potential asynchronous issues
>> > (e.g. user instruction fetches accessing read-destructive device registers).
>> > All kernel mappings *should* be UXN.
>>
>> I love this idea, but based on what came up with hardened usercopy,
>> there are a lot of readers of kernel memory still. I think the
>> expectations around UXN need to be clarified so we can reason about
>> things like perf that want to read the kernel text.
>
> The UXN (User eXecute Never) bit only controls whether userspace can execute a
> page, not whether the kernel can read it. The RW permissions come from the AP
> bits regardless.

Ah! Sorry, I misunderstood. Yeah, UXN checking makes sense there then. :)

> We already try to ensure that all kernel memory is UXN by construction, so this
> would just be a sanity check, as with the rest of the W^X checks.
>
> The MOVZ+MOVK case above is where a sequence of 16-bit immediate MOVs are used
> to encode a pointer. If a kernel mapping lacked UXN, userspace could execute it
> (unprivileged), and extract the pointer generated into a GPR.
>
> Having kernel exec-only memory is a different story entirely, though I agree
> it's something to look into.

Yeah, this'll need to get sorted out for x86 too.

-Kees

-- 
Kees Cook
Nexus Security

[kernel-hardening] [PATCH 0/3] WX Checking for arm64

[Kees Cook]

On Thu, Sep 29, 2016 at 2:32 PM, Laura Abbott <labbott@redhat.com> wrote:
>
> Hi,
>
> This is an implementation to check for writable and executable pages on arm64.
> This is heavily based on the x86 version which uses the existing page table
> dumping code to do the checking. Some notes:
>
> - The W^X checking is important so this option should become defaut eventually.
>   To make this feasible, the debugfs functionality has been split out as a
>   separate option. I didn't see a good way to make it modular like x86 but
>   an option should be good enough.
> - This checks all page tables registered with ptdump_register. I don't see this
>   being called elsewhere right now though.
> - Once this is merged, I'd like to see about moving DEBUG_WX to the top level
>   instead of having each arch call it in mark_rodata.

Awesome!

Yeah, I think we should take a look at refactoring x86, arm, and arm64
to use a common infrastructure with callbacks. That way other
architectures can gain all these features with just a few callbacks
implemented.

-Kees

>
> Laura Abbott (3):
>   arm64: dump: Make ptdump debugfs a separate option
>   arm64: dump: Make the page table dumping seq_file optional
>   arm64: dump: Add checking for writable and exectuable pages
>
>  arch/arm64/Kconfig.debug        | 34 ++++++++++++++-
>  arch/arm64/include/asm/ptdump.h | 25 ++++++++++-
>  arch/arm64/mm/Makefile          |  3 +-
>  arch/arm64/mm/dump.c            | 92 ++++++++++++++++++++++++++++-------------
>  arch/arm64/mm/mmu.c             |  2 +
>  arch/arm64/mm/ptdump_debugfs.c  | 33 +++++++++++++++
>  6 files changed, 157 insertions(+), 32 deletions(-)
>  create mode 100644 arch/arm64/mm/ptdump_debugfs.c
>
> --
> 2.10.0
>



-- 
Kees Cook
Nexus Security