All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCHv3 0/4] WX checking for arm64
@ 2016-10-18 22:01 ` Laura Abbott
  0 siblings, 0 replies; 29+ messages in thread
From: Laura Abbott @ 2016-10-18 22:01 UTC (permalink / raw)
  To: AKASHI Takahiro, Mark Rutland, Ard Biesheuvel, David Brown,
	Will Deacon, Catalin Marinas
  Cc: Laura Abbott, linux-arm-kernel, linux-kernel, Kees Cook,
	kernel-hardening, Matt Fleming, linux-efi

Hi,

This is v3 of the implementation to check for writable and executable pages on
arm64. This is a basically a rebase + acks.

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           |  35 ++++++++++++-
 arch/arm64/include/asm/ptdump.h    |  22 +++++---
 arch/arm64/mm/Makefile             |   3 +-
 arch/arm64/mm/dump.c               | 104 +++++++++++++++++++++++++++----------
 arch/arm64/mm/mmu.c                |   2 +
 arch/arm64/mm/ptdump_debugfs.c     |  31 +++++++++++
 drivers/firmware/efi/arm-runtime.c |   5 +-
 7 files changed, 163 insertions(+), 39 deletions(-)
 create mode 100644 arch/arm64/mm/ptdump_debugfs.c

-- 
2.7.4

^ permalink raw reply	[flat|nested] 29+ messages in thread
* [PATCHv3 0/4] WX checking for arm64
@ 2016-10-18 22:01 ` Laura Abbott
  0 siblings, 0 replies; 29+ messages in thread
From: Laura Abbott @ 2016-10-18 22:01 UTC (permalink / raw)
  To: AKASHI Takahiro, Mark Rutland, Ard Biesheuvel, David Brown,
	Will Deacon, Catalin Marinas
  Cc: Laura Abbott, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Kees Cook,
	kernel-hardening-ZwoEplunGu1jrUoiu81ncdBPR1lH4CV8, Matt Fleming,
	linux-efi-u79uwXL29TY76Z2rM5mHXA

Hi,

This is v3 of the implementation to check for writable and executable pages on
arm64. This is a basically a rebase + acks.

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           |  35 ++++++++++++-
 arch/arm64/include/asm/ptdump.h    |  22 +++++---
 arch/arm64/mm/Makefile             |   3 +-
 arch/arm64/mm/dump.c               | 104 +++++++++++++++++++++++++++----------
 arch/arm64/mm/mmu.c                |   2 +
 arch/arm64/mm/ptdump_debugfs.c     |  31 +++++++++++
 drivers/firmware/efi/arm-runtime.c |   5 +-
 7 files changed, 163 insertions(+), 39 deletions(-)
 create mode 100644 arch/arm64/mm/ptdump_debugfs.c

-- 
2.7.4

^ permalink raw reply	[flat|nested] 29+ messages in thread
* [PATCHv3 0/4] WX checking for arm64
@ 2016-10-18 22:01 ` Laura Abbott
  0 siblings, 0 replies; 29+ messages in thread
From: Laura Abbott @ 2016-10-18 22:01 UTC (permalink / raw)
  To: linux-arm-kernel

Hi,

This is v3 of the implementation to check for writable and executable pages on
arm64. This is a basically a rebase + acks.

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           |  35 ++++++++++++-
 arch/arm64/include/asm/ptdump.h    |  22 +++++---
 arch/arm64/mm/Makefile             |   3 +-
 arch/arm64/mm/dump.c               | 104 +++++++++++++++++++++++++++----------
 arch/arm64/mm/mmu.c                |   2 +
 arch/arm64/mm/ptdump_debugfs.c     |  31 +++++++++++
 drivers/firmware/efi/arm-runtime.c |   5 +-
 7 files changed, 163 insertions(+), 39 deletions(-)
 create mode 100644 arch/arm64/mm/ptdump_debugfs.c

-- 
2.7.4

^ permalink raw reply	[flat|nested] 29+ messages in thread
* [kernel-hardening] [PATCHv3 0/4] WX checking for arm64
@ 2016-10-18 22:01 ` Laura Abbott
  0 siblings, 0 replies; 29+ messages in thread
From: Laura Abbott @ 2016-10-18 22:01 UTC (permalink / raw)
  To: AKASHI Takahiro, Mark Rutland, Ard Biesheuvel, David Brown,
	Will Deacon, Catalin Marinas
  Cc: Laura Abbott, linux-arm-kernel, linux-kernel, Kees Cook,
	kernel-hardening, Matt Fleming, linux-efi

Hi,

This is v3 of the implementation to check for writable and executable pages on
arm64. This is a basically a rebase + acks.

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           |  35 ++++++++++++-
 arch/arm64/include/asm/ptdump.h    |  22 +++++---
 arch/arm64/mm/Makefile             |   3 +-
 arch/arm64/mm/dump.c               | 104 +++++++++++++++++++++++++++----------
 arch/arm64/mm/mmu.c                |   2 +
 arch/arm64/mm/ptdump_debugfs.c     |  31 +++++++++++
 drivers/firmware/efi/arm-runtime.c |   5 +-
 7 files changed, 163 insertions(+), 39 deletions(-)
 create mode 100644 arch/arm64/mm/ptdump_debugfs.c

-- 
2.7.4

^ permalink raw reply	[flat|nested] 29+ messages in thread
* [PATCHv3 1/4] arm64: dump: Make ptdump debugfs a separate option
  2016-10-18 22:01 ` Laura Abbott
  (?)
@ 2016-10-18 22:01   ` Laura Abbott
  -1 siblings, 0 replies; 29+ messages in thread
From: Laura Abbott @ 2016-10-18 22:01 UTC (permalink / raw)
  To: AKASHI Takahiro, Mark Rutland, Ard Biesheuvel, David Brown,
	Will Deacon, Catalin Marinas, Matt Fleming
  Cc: Laura Abbott, linux-arm-kernel, linux-kernel, Kees Cook,
	kernel-hardening, linux-efi


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.

Reviewed-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Laura Abbott <labbott@redhat.com>
---
v3: Minor header guard fixup.

Mark Rutland pointed out that this needs to be reviewed by the EFI maintainers
so I'm explicitly adding appropriate people/lists.
---
 arch/arm64/Kconfig.debug           |  6 +++++-
 arch/arm64/include/asm/ptdump.h    | 15 +++++++++------
 arch/arm64/mm/Makefile             |  3 ++-
 arch/arm64/mm/dump.c               | 26 +++++---------------------
 arch/arm64/mm/ptdump_debugfs.c     | 31 +++++++++++++++++++++++++++++++
 drivers/firmware/efi/arm-runtime.c |  4 ++--
 6 files changed, 54 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..16335da 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 /* 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..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..349dc3e 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,7 +53,7 @@ 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);
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 29+ messages in thread
* [PATCHv3 1/4] arm64: dump: Make ptdump debugfs a separate option
@ 2016-10-18 22:01   ` Laura Abbott
  0 siblings, 0 replies; 29+ messages in thread
From: Laura Abbott @ 2016-10-18 22:01 UTC (permalink / raw)
  To: linux-arm-kernel


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.

Reviewed-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Laura Abbott <labbott@redhat.com>
---
v3: Minor header guard fixup.

Mark Rutland pointed out that this needs to be reviewed by the EFI maintainers
so I'm explicitly adding appropriate people/lists.
---
 arch/arm64/Kconfig.debug           |  6 +++++-
 arch/arm64/include/asm/ptdump.h    | 15 +++++++++------
 arch/arm64/mm/Makefile             |  3 ++-
 arch/arm64/mm/dump.c               | 26 +++++---------------------
 arch/arm64/mm/ptdump_debugfs.c     | 31 +++++++++++++++++++++++++++++++
 drivers/firmware/efi/arm-runtime.c |  4 ++--
 6 files changed, 54 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..16335da 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 /* 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..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..349dc3e 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,7 +53,7 @@ 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);
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 29+ messages in thread
* [kernel-hardening] [PATCHv3 1/4] arm64: dump: Make ptdump debugfs a separate option
@ 2016-10-18 22:01   ` Laura Abbott
  0 siblings, 0 replies; 29+ messages in thread
From: Laura Abbott @ 2016-10-18 22:01 UTC (permalink / raw)
  To: AKASHI Takahiro, Mark Rutland, Ard Biesheuvel, David Brown,
	Will Deacon, Catalin Marinas, Matt Fleming
  Cc: Laura Abbott, linux-arm-kernel, linux-kernel, Kees Cook,
	kernel-hardening, linux-efi


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.

Reviewed-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Laura Abbott <labbott@redhat.com>
---
v3: Minor header guard fixup.

Mark Rutland pointed out that this needs to be reviewed by the EFI maintainers
so I'm explicitly adding appropriate people/lists.
---
 arch/arm64/Kconfig.debug           |  6 +++++-
 arch/arm64/include/asm/ptdump.h    | 15 +++++++++------
 arch/arm64/mm/Makefile             |  3 ++-
 arch/arm64/mm/dump.c               | 26 +++++---------------------
 arch/arm64/mm/ptdump_debugfs.c     | 31 +++++++++++++++++++++++++++++++
 drivers/firmware/efi/arm-runtime.c |  4 ++--
 6 files changed, 54 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..16335da 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 /* 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..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..349dc3e 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,7 +53,7 @@ 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);
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 29+ messages in thread
* [PATCHv3 2/4] arm64: dump: Make the page table dumping seq_file optional
  2016-10-18 22:01 ` Laura Abbott
  (?)
@ 2016-10-18 22:01   ` Laura Abbott
  -1 siblings, 0 replies; 29+ messages in thread
From: Laura Abbott @ 2016-10-18 22:01 UTC (permalink / raw)
  To: AKASHI Takahiro, Mark Rutland, Ard Biesheuvel, David Brown,
	Will Deacon, Catalin Marinas
  Cc: Laura Abbott, linux-arm-kernel, linux-kernel, Kees Cook,
	kernel-hardening, Matt Fleming, linux-efi


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.

Reviewed-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Mark Rutland <mark.rutland@arm.com>
Acked-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Laura Abbott <labbott@redhat.com>
---
v3: No changes
---
 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

^ permalink raw reply related	[flat|nested] 29+ messages in thread
* [PATCHv3 2/4] arm64: dump: Make the page table dumping seq_file optional
@ 2016-10-18 22:01   ` Laura Abbott
  0 siblings, 0 replies; 29+ messages in thread
From: Laura Abbott @ 2016-10-18 22:01 UTC (permalink / raw)
  To: linux-arm-kernel


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.

Reviewed-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Mark Rutland <mark.rutland@arm.com>
Acked-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Laura Abbott <labbott@redhat.com>
---
v3: No changes
---
 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

^ permalink raw reply related	[flat|nested] 29+ messages in thread
* [kernel-hardening] [PATCHv3 2/4] arm64: dump: Make the page table dumping seq_file optional
@ 2016-10-18 22:01   ` Laura Abbott
  0 siblings, 0 replies; 29+ messages in thread
From: Laura Abbott @ 2016-10-18 22:01 UTC (permalink / raw)
  To: AKASHI Takahiro, Mark Rutland, Ard Biesheuvel, David Brown,
	Will Deacon, Catalin Marinas
  Cc: Laura Abbott, linux-arm-kernel, linux-kernel, Kees Cook,
	kernel-hardening, Matt Fleming, linux-efi


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.

Reviewed-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Mark Rutland <mark.rutland@arm.com>
Acked-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Laura Abbott <labbott@redhat.com>
---
v3: No changes
---
 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

^ permalink raw reply related	[flat|nested] 29+ messages in thread
* [PATCHv3 3/4] arm64: dump: Remove max_addr
  2016-10-18 22:01 ` Laura Abbott
  (?)
@ 2016-10-18 22:01   ` Laura Abbott
  -1 siblings, 0 replies; 29+ messages in thread
From: Laura Abbott @ 2016-10-18 22:01 UTC (permalink / raw)
  To: AKASHI Takahiro, Mark Rutland, Ard Biesheuvel, David Brown,
	Will Deacon, Catalin Marinas
  Cc: Laura Abbott, linux-arm-kernel, linux-kernel, Kees Cook,
	kernel-hardening, Matt Fleming, linux-efi


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

Reviewed-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Laura Abbott <labbott@redhat.com>
---
v3: No changes
---
 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 16335da..f72ee69 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

^ permalink raw reply related	[flat|nested] 29+ messages in thread
* [PATCHv3 3/4] arm64: dump: Remove max_addr
@ 2016-10-18 22:01   ` Laura Abbott
  0 siblings, 0 replies; 29+ messages in thread
From: Laura Abbott @ 2016-10-18 22:01 UTC (permalink / raw)
  To: linux-arm-kernel


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

Reviewed-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Laura Abbott <labbott@redhat.com>
---
v3: No changes
---
 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 16335da..f72ee69 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

^ permalink raw reply related	[flat|nested] 29+ messages in thread
* [kernel-hardening] [PATCHv3 3/4] arm64: dump: Remove max_addr
@ 2016-10-18 22:01   ` Laura Abbott
  0 siblings, 0 replies; 29+ messages in thread
From: Laura Abbott @ 2016-10-18 22:01 UTC (permalink / raw)
  To: AKASHI Takahiro, Mark Rutland, Ard Biesheuvel, David Brown,
	Will Deacon, Catalin Marinas
  Cc: Laura Abbott, linux-arm-kernel, linux-kernel, Kees Cook,
	kernel-hardening, Matt Fleming, linux-efi


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

Reviewed-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Laura Abbott <labbott@redhat.com>
---
v3: No changes
---
 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 16335da..f72ee69 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

^ permalink raw reply related	[flat|nested] 29+ messages in thread
* [PATCHv3 4/4] arm64: dump: Add checking for writable and exectuable pages
@ 2016-10-18 22:01   ` Laura Abbott
  0 siblings, 0 replies; 29+ messages in thread
From: Laura Abbott @ 2016-10-18 22:01 UTC (permalink / raw)
  To: AKASHI Takahiro, Mark Rutland, Ard Biesheuvel, David Brown,
	Will Deacon, Catalin Marinas
  Cc: Laura Abbott, linux-arm-kernel, linux-kernel, Kees Cook,
	kernel-hardening, Matt Fleming, linux-efi


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.

Reviewed-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Laura Abbott <labbott@redhat.com>
---
v3: Rebased for header guard fixup, whitespace fixes
---
 arch/arm64/Kconfig.debug        | 29 +++++++++++++++++++++++
 arch/arm64/include/asm/ptdump.h |  8 +++++++
 arch/arm64/mm/dump.c            | 52 +++++++++++++++++++++++++++++++++++++++++
 arch/arm64/mm/mmu.c             |  2 ++
 4 files changed, 91 insertions(+)

diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
index 21a5b74..d1ebd46 100644
--- a/arch/arm64/Kconfig.debug
+++ b/arch/arm64/Kconfig.debug
@@ -42,6 +42,35 @@ 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 f72ee69..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

^ permalink raw reply related	[flat|nested] 29+ messages in thread
* [PATCHv3 4/4] arm64: dump: Add checking for writable and exectuable pages
@ 2016-10-18 22:01   ` Laura Abbott
  0 siblings, 0 replies; 29+ messages in thread
From: Laura Abbott @ 2016-10-18 22:01 UTC (permalink / raw)
  To: AKASHI Takahiro, Mark Rutland, Ard Biesheuvel, David Brown,
	Will Deacon, Catalin Marinas
  Cc: Laura Abbott, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Kees Cook,
	kernel-hardening-ZwoEplunGu1jrUoiu81ncdBPR1lH4CV8, Matt Fleming,
	linux-efi-u79uwXL29TY76Z2rM5mHXA


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.

Reviewed-by: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Reviewed-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
Tested-by: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
Signed-off-by: Laura Abbott <labbott-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
v3: Rebased for header guard fixup, whitespace fixes
---
 arch/arm64/Kconfig.debug        | 29 +++++++++++++++++++++++
 arch/arm64/include/asm/ptdump.h |  8 +++++++
 arch/arm64/mm/dump.c            | 52 +++++++++++++++++++++++++++++++++++++++++
 arch/arm64/mm/mmu.c             |  2 ++
 4 files changed, 91 insertions(+)

diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
index 21a5b74..d1ebd46 100644
--- a/arch/arm64/Kconfig.debug
+++ b/arch/arm64/Kconfig.debug
@@ -42,6 +42,35 @@ 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 f72ee69..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

^ permalink raw reply related	[flat|nested] 29+ messages in thread
* [PATCHv3 4/4] arm64: dump: Add checking for writable and exectuable pages
@ 2016-10-18 22:01   ` Laura Abbott
  0 siblings, 0 replies; 29+ messages in thread
From: Laura Abbott @ 2016-10-18 22:01 UTC (permalink / raw)
  To: linux-arm-kernel


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.

Reviewed-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Laura Abbott <labbott@redhat.com>
---
v3: Rebased for header guard fixup, whitespace fixes
---
 arch/arm64/Kconfig.debug        | 29 +++++++++++++++++++++++
 arch/arm64/include/asm/ptdump.h |  8 +++++++
 arch/arm64/mm/dump.c            | 52 +++++++++++++++++++++++++++++++++++++++++
 arch/arm64/mm/mmu.c             |  2 ++
 4 files changed, 91 insertions(+)

diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
index 21a5b74..d1ebd46 100644
--- a/arch/arm64/Kconfig.debug
+++ b/arch/arm64/Kconfig.debug
@@ -42,6 +42,35 @@ 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 f72ee69..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

^ permalink raw reply related	[flat|nested] 29+ messages in thread
* [kernel-hardening] [PATCHv3 4/4] arm64: dump: Add checking for writable and exectuable pages
@ 2016-10-18 22:01   ` Laura Abbott
  0 siblings, 0 replies; 29+ messages in thread
From: Laura Abbott @ 2016-10-18 22:01 UTC (permalink / raw)
  To: AKASHI Takahiro, Mark Rutland, Ard Biesheuvel, David Brown,
	Will Deacon, Catalin Marinas
  Cc: Laura Abbott, linux-arm-kernel, linux-kernel, Kees Cook,
	kernel-hardening, Matt Fleming, linux-efi


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.

Reviewed-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Tested-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Laura Abbott <labbott@redhat.com>
---
v3: Rebased for header guard fixup, whitespace fixes
---
 arch/arm64/Kconfig.debug        | 29 +++++++++++++++++++++++
 arch/arm64/include/asm/ptdump.h |  8 +++++++
 arch/arm64/mm/dump.c            | 52 +++++++++++++++++++++++++++++++++++++++++
 arch/arm64/mm/mmu.c             |  2 ++
 4 files changed, 91 insertions(+)

diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
index 21a5b74..d1ebd46 100644
--- a/arch/arm64/Kconfig.debug
+++ b/arch/arm64/Kconfig.debug
@@ -42,6 +42,35 @@ 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 f72ee69..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

^ permalink raw reply related	[flat|nested] 29+ messages in thread
* Re: [PATCHv3 1/4] arm64: dump: Make ptdump debugfs a separate option
  2016-10-18 22:01   ` Laura Abbott
  (?)
  (?)
@ 2016-10-20 10:17     ` Ard Biesheuvel
  -1 siblings, 0 replies; 29+ messages in thread
From: Ard Biesheuvel @ 2016-10-20 10:17 UTC (permalink / raw)
  To: Laura Abbott
  Cc: AKASHI Takahiro, Mark Rutland, David Brown, Will Deacon,
	Catalin Marinas, Matt Fleming, linux-arm-kernel, linux-kernel,
	Kees Cook, kernel-hardening, linux-efi

On 18 October 2016 at 23:01, 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.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> Tested-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> v3: Minor header guard fixup.
>
> Mark Rutland pointed out that this needs to be reviewed by the EFI maintainers
> so I'm explicitly adding appropriate people/lists.

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
>  arch/arm64/Kconfig.debug           |  6 +++++-
>  arch/arm64/include/asm/ptdump.h    | 15 +++++++++------
>  arch/arm64/mm/Makefile             |  3 ++-
>  arch/arm64/mm/dump.c               | 26 +++++---------------------
>  arch/arm64/mm/ptdump_debugfs.c     | 31 +++++++++++++++++++++++++++++++
>  drivers/firmware/efi/arm-runtime.c |  4 ++--
>  6 files changed, 54 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..16335da 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 /* 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..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..349dc3e 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,7 +53,7 @@ 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);
>
> --
> 2.7.4
>

^ permalink raw reply	[flat|nested] 29+ messages in thread
* Re: [PATCHv3 1/4] arm64: dump: Make ptdump debugfs a separate option
@ 2016-10-20 10:17     ` Ard Biesheuvel
  0 siblings, 0 replies; 29+ messages in thread
From: Ard Biesheuvel @ 2016-10-20 10:17 UTC (permalink / raw)
  To: Laura Abbott
  Cc: AKASHI Takahiro, Mark Rutland, David Brown, Will Deacon,
	Catalin Marinas, Matt Fleming, linux-arm-kernel, linux-kernel,
	Kees Cook, kernel-hardening, linux-efi

On 18 October 2016 at 23:01, 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.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> Tested-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> v3: Minor header guard fixup.
>
> Mark Rutland pointed out that this needs to be reviewed by the EFI maintainers
> so I'm explicitly adding appropriate people/lists.

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
>  arch/arm64/Kconfig.debug           |  6 +++++-
>  arch/arm64/include/asm/ptdump.h    | 15 +++++++++------
>  arch/arm64/mm/Makefile             |  3 ++-
>  arch/arm64/mm/dump.c               | 26 +++++---------------------
>  arch/arm64/mm/ptdump_debugfs.c     | 31 +++++++++++++++++++++++++++++++
>  drivers/firmware/efi/arm-runtime.c |  4 ++--
>  6 files changed, 54 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..16335da 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 /* 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..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..349dc3e 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,7 +53,7 @@ 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);
>
> --
> 2.7.4
>

^ permalink raw reply	[flat|nested] 29+ messages in thread
* [PATCHv3 1/4] arm64: dump: Make ptdump debugfs a separate option
@ 2016-10-20 10:17     ` Ard Biesheuvel
  0 siblings, 0 replies; 29+ messages in thread
From: Ard Biesheuvel @ 2016-10-20 10:17 UTC (permalink / raw)
  To: linux-arm-kernel

On 18 October 2016 at 23:01, 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.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> Tested-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> v3: Minor header guard fixup.
>
> Mark Rutland pointed out that this needs to be reviewed by the EFI maintainers
> so I'm explicitly adding appropriate people/lists.

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
>  arch/arm64/Kconfig.debug           |  6 +++++-
>  arch/arm64/include/asm/ptdump.h    | 15 +++++++++------
>  arch/arm64/mm/Makefile             |  3 ++-
>  arch/arm64/mm/dump.c               | 26 +++++---------------------
>  arch/arm64/mm/ptdump_debugfs.c     | 31 +++++++++++++++++++++++++++++++
>  drivers/firmware/efi/arm-runtime.c |  4 ++--
>  6 files changed, 54 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..16335da 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 /* 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..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..349dc3e 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,7 +53,7 @@ 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);
>
> --
> 2.7.4
>

^ permalink raw reply	[flat|nested] 29+ messages in thread
* [kernel-hardening] Re: [PATCHv3 1/4] arm64: dump: Make ptdump debugfs a separate option
@ 2016-10-20 10:17     ` Ard Biesheuvel
  0 siblings, 0 replies; 29+ messages in thread
From: Ard Biesheuvel @ 2016-10-20 10:17 UTC (permalink / raw)
  To: Laura Abbott
  Cc: AKASHI Takahiro, Mark Rutland, David Brown, Will Deacon,
	Catalin Marinas, Matt Fleming, linux-arm-kernel, linux-kernel,
	Kees Cook, kernel-hardening, linux-efi

On 18 October 2016 at 23:01, 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.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> Tested-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> v3: Minor header guard fixup.
>
> Mark Rutland pointed out that this needs to be reviewed by the EFI maintainers
> so I'm explicitly adding appropriate people/lists.

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
>  arch/arm64/Kconfig.debug           |  6 +++++-
>  arch/arm64/include/asm/ptdump.h    | 15 +++++++++------
>  arch/arm64/mm/Makefile             |  3 ++-
>  arch/arm64/mm/dump.c               | 26 +++++---------------------
>  arch/arm64/mm/ptdump_debugfs.c     | 31 +++++++++++++++++++++++++++++++
>  drivers/firmware/efi/arm-runtime.c |  4 ++--
>  6 files changed, 54 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..16335da 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 /* 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..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..349dc3e 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,7 +53,7 @@ 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);
>
> --
> 2.7.4
>

^ permalink raw reply	[flat|nested] 29+ messages in thread
* Re: [PATCHv3 4/4] arm64: dump: Add checking for writable and exectuable pages
  2016-10-18 22:01   ` Laura Abbott
  (?)
  (?)
@ 2016-10-20 10:32     ` Ard Biesheuvel
  -1 siblings, 0 replies; 29+ messages in thread
From: Ard Biesheuvel @ 2016-10-20 10:32 UTC (permalink / raw)
  To: Laura Abbott
  Cc: AKASHI Takahiro, Mark Rutland, David Brown, Will Deacon,
	Catalin Marinas, linux-arm-kernel, linux-kernel, Kees Cook,
	kernel-hardening, Matt Fleming, linux-efi

On 18 October 2016 at 23:01, Laura Abbott <labbott@redhat.com> 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.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> Tested-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> v3: Rebased for header guard fixup, whitespace fixes
> ---
>  arch/arm64/Kconfig.debug        | 29 +++++++++++++++++++++++
>  arch/arm64/include/asm/ptdump.h |  8 +++++++
>  arch/arm64/mm/dump.c            | 52 +++++++++++++++++++++++++++++++++++++++++
>  arch/arm64/mm/mmu.c             |  2 ++
>  4 files changed, 91 insertions(+)
>
> diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
> index 21a5b74..d1ebd46 100644
> --- a/arch/arm64/Kconfig.debug
> +++ b/arch/arm64/Kconfig.debug
> @@ -42,6 +42,35 @@ 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 f72ee69..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;
> +}
> +

Why are these separate functions, and why is wx_pages increased twice,
potentially?

Given how rare non-UXN kernel mappings should be, could we not just add

       if ((st->current_prot & PTE_UXN) == 0)
               WARN(xxx)

(without the _ONCE) to note_prot_wx(), and drop note_prot_uxn() entirely?


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

Could we upgrade this to pr_warn?

> +       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
>

^ permalink raw reply	[flat|nested] 29+ messages in thread
* Re: [PATCHv3 4/4] arm64: dump: Add checking for writable and exectuable pages
@ 2016-10-20 10:32     ` Ard Biesheuvel
  0 siblings, 0 replies; 29+ messages in thread
From: Ard Biesheuvel @ 2016-10-20 10:32 UTC (permalink / raw)
  To: Laura Abbott
  Cc: AKASHI Takahiro, Mark Rutland, David Brown, Will Deacon,
	Catalin Marinas, linux-arm-kernel, linux-kernel, Kees Cook,
	kernel-hardening, Matt Fleming, linux-efi

On 18 October 2016 at 23:01, Laura Abbott <labbott@redhat.com> 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.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> Tested-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> v3: Rebased for header guard fixup, whitespace fixes
> ---
>  arch/arm64/Kconfig.debug        | 29 +++++++++++++++++++++++
>  arch/arm64/include/asm/ptdump.h |  8 +++++++
>  arch/arm64/mm/dump.c            | 52 +++++++++++++++++++++++++++++++++++++++++
>  arch/arm64/mm/mmu.c             |  2 ++
>  4 files changed, 91 insertions(+)
>
> diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
> index 21a5b74..d1ebd46 100644
> --- a/arch/arm64/Kconfig.debug
> +++ b/arch/arm64/Kconfig.debug
> @@ -42,6 +42,35 @@ 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 f72ee69..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;
> +}
> +

Why are these separate functions, and why is wx_pages increased twice,
potentially?

Given how rare non-UXN kernel mappings should be, could we not just add

       if ((st->current_prot & PTE_UXN) == 0)
               WARN(xxx)

(without the _ONCE) to note_prot_wx(), and drop note_prot_uxn() entirely?


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

Could we upgrade this to pr_warn?

> +       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
>

^ permalink raw reply	[flat|nested] 29+ messages in thread
* [PATCHv3 4/4] arm64: dump: Add checking for writable and exectuable pages
@ 2016-10-20 10:32     ` Ard Biesheuvel
  0 siblings, 0 replies; 29+ messages in thread
From: Ard Biesheuvel @ 2016-10-20 10:32 UTC (permalink / raw)
  To: linux-arm-kernel

On 18 October 2016 at 23:01, Laura Abbott <labbott@redhat.com> 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.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> Tested-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> v3: Rebased for header guard fixup, whitespace fixes
> ---
>  arch/arm64/Kconfig.debug        | 29 +++++++++++++++++++++++
>  arch/arm64/include/asm/ptdump.h |  8 +++++++
>  arch/arm64/mm/dump.c            | 52 +++++++++++++++++++++++++++++++++++++++++
>  arch/arm64/mm/mmu.c             |  2 ++
>  4 files changed, 91 insertions(+)
>
> diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
> index 21a5b74..d1ebd46 100644
> --- a/arch/arm64/Kconfig.debug
> +++ b/arch/arm64/Kconfig.debug
> @@ -42,6 +42,35 @@ 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 f72ee69..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;
> +}
> +

Why are these separate functions, and why is wx_pages increased twice,
potentially?

Given how rare non-UXN kernel mappings should be, could we not just add

       if ((st->current_prot & PTE_UXN) == 0)
               WARN(xxx)

(without the _ONCE) to note_prot_wx(), and drop note_prot_uxn() entirely?


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

Could we upgrade this to pr_warn?

> +       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
>

^ permalink raw reply	[flat|nested] 29+ messages in thread
* [kernel-hardening] Re: [PATCHv3 4/4] arm64: dump: Add checking for writable and exectuable pages
@ 2016-10-20 10:32     ` Ard Biesheuvel
  0 siblings, 0 replies; 29+ messages in thread
From: Ard Biesheuvel @ 2016-10-20 10:32 UTC (permalink / raw)
  To: Laura Abbott
  Cc: AKASHI Takahiro, Mark Rutland, David Brown, Will Deacon,
	Catalin Marinas, linux-arm-kernel, linux-kernel, Kees Cook,
	kernel-hardening, Matt Fleming, linux-efi

On 18 October 2016 at 23:01, Laura Abbott <labbott@redhat.com> 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.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> Tested-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> v3: Rebased for header guard fixup, whitespace fixes
> ---
>  arch/arm64/Kconfig.debug        | 29 +++++++++++++++++++++++
>  arch/arm64/include/asm/ptdump.h |  8 +++++++
>  arch/arm64/mm/dump.c            | 52 +++++++++++++++++++++++++++++++++++++++++
>  arch/arm64/mm/mmu.c             |  2 ++
>  4 files changed, 91 insertions(+)
>
> diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
> index 21a5b74..d1ebd46 100644
> --- a/arch/arm64/Kconfig.debug
> +++ b/arch/arm64/Kconfig.debug
> @@ -42,6 +42,35 @@ 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 f72ee69..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;
> +}
> +

Why are these separate functions, and why is wx_pages increased twice,
potentially?

Given how rare non-UXN kernel mappings should be, could we not just add

       if ((st->current_prot & PTE_UXN) == 0)
               WARN(xxx)

(without the _ONCE) to note_prot_wx(), and drop note_prot_uxn() entirely?


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

Could we upgrade this to pr_warn?

> +       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
>

^ permalink raw reply	[flat|nested] 29+ messages in thread
* Re: [PATCHv3 4/4] arm64: dump: Add checking for writable and exectuable pages
  2016-10-20 10:32     ` Ard Biesheuvel
  (?)
  (?)
@ 2016-10-20 13:01       ` Laura Abbott
  -1 siblings, 0 replies; 29+ messages in thread
From: Laura Abbott @ 2016-10-20 13:01 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: AKASHI Takahiro, Mark Rutland, David Brown, Will Deacon,
	Catalin Marinas, linux-arm-kernel, linux-kernel, Kees Cook,
	kernel-hardening, Matt Fleming, linux-efi

On 10/20/2016 03:32 AM, Ard Biesheuvel wrote:
> On 18 October 2016 at 23:01, Laura Abbott <labbott@redhat.com> 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.
>>
>> Reviewed-by: Kees Cook <keescook@chromium.org>
>> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
>> Tested-by: Mark Rutland <mark.rutland@arm.com>
>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>> ---
>> v3: Rebased for header guard fixup, whitespace fixes
>> ---
>>  arch/arm64/Kconfig.debug        | 29 +++++++++++++++++++++++
>>  arch/arm64/include/asm/ptdump.h |  8 +++++++
>>  arch/arm64/mm/dump.c            | 52 +++++++++++++++++++++++++++++++++++++++++
>>  arch/arm64/mm/mmu.c             |  2 ++
>>  4 files changed, 91 insertions(+)
>>
>> diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
>> index 21a5b74..d1ebd46 100644
>> --- a/arch/arm64/Kconfig.debug
>> +++ b/arch/arm64/Kconfig.debug
>> @@ -42,6 +42,35 @@ 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 f72ee69..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;
>> +}
>> +
>
> Why are these separate functions, and why is wx_pages increased twice,
> potentially?
>
> Given how rare non-UXN kernel mappings should be, could we not just add
>
>        if ((st->current_prot & PTE_UXN) == 0)
>                WARN(xxx)
>
> (without the _ONCE) to note_prot_wx(), and drop note_prot_uxn() entirely?
>
>

UXN is a separate bit from PTE_PXN/PTE_RDONLY and both pairs need to
be checked. The current return == 0 logic means that one set or the
other may not get checked. Rather than complicate the logic, it seemed
better to have separate functions. I see your point about the wx_pages
double counting so I can fix that.

>>  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);
>
> Could we upgrade this to pr_warn?
>

Sure

>> +       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
>>

^ permalink raw reply	[flat|nested] 29+ messages in thread
* Re: [PATCHv3 4/4] arm64: dump: Add checking for writable and exectuable pages
@ 2016-10-20 13:01       ` Laura Abbott
  0 siblings, 0 replies; 29+ messages in thread
From: Laura Abbott @ 2016-10-20 13:01 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: AKASHI Takahiro, Mark Rutland, David Brown, Will Deacon,
	Catalin Marinas, linux-arm-kernel, linux-kernel, Kees Cook,
	kernel-hardening, Matt Fleming, linux-efi

On 10/20/2016 03:32 AM, Ard Biesheuvel wrote:
> On 18 October 2016 at 23:01, Laura Abbott <labbott@redhat.com> 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.
>>
>> Reviewed-by: Kees Cook <keescook@chromium.org>
>> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
>> Tested-by: Mark Rutland <mark.rutland@arm.com>
>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>> ---
>> v3: Rebased for header guard fixup, whitespace fixes
>> ---
>>  arch/arm64/Kconfig.debug        | 29 +++++++++++++++++++++++
>>  arch/arm64/include/asm/ptdump.h |  8 +++++++
>>  arch/arm64/mm/dump.c            | 52 +++++++++++++++++++++++++++++++++++++++++
>>  arch/arm64/mm/mmu.c             |  2 ++
>>  4 files changed, 91 insertions(+)
>>
>> diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
>> index 21a5b74..d1ebd46 100644
>> --- a/arch/arm64/Kconfig.debug
>> +++ b/arch/arm64/Kconfig.debug
>> @@ -42,6 +42,35 @@ 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 f72ee69..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;
>> +}
>> +
>
> Why are these separate functions, and why is wx_pages increased twice,
> potentially?
>
> Given how rare non-UXN kernel mappings should be, could we not just add
>
>        if ((st->current_prot & PTE_UXN) == 0)
>                WARN(xxx)
>
> (without the _ONCE) to note_prot_wx(), and drop note_prot_uxn() entirely?
>
>

UXN is a separate bit from PTE_PXN/PTE_RDONLY and both pairs need to
be checked. The current return == 0 logic means that one set or the
other may not get checked. Rather than complicate the logic, it seemed
better to have separate functions. I see your point about the wx_pages
double counting so I can fix that.

>>  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);
>
> Could we upgrade this to pr_warn?
>

Sure

>> +       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
>>

^ permalink raw reply	[flat|nested] 29+ messages in thread
* [PATCHv3 4/4] arm64: dump: Add checking for writable and exectuable pages
@ 2016-10-20 13:01       ` Laura Abbott
  0 siblings, 0 replies; 29+ messages in thread
From: Laura Abbott @ 2016-10-20 13:01 UTC (permalink / raw)
  To: linux-arm-kernel

On 10/20/2016 03:32 AM, Ard Biesheuvel wrote:
> On 18 October 2016 at 23:01, Laura Abbott <labbott@redhat.com> 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.
>>
>> Reviewed-by: Kees Cook <keescook@chromium.org>
>> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
>> Tested-by: Mark Rutland <mark.rutland@arm.com>
>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>> ---
>> v3: Rebased for header guard fixup, whitespace fixes
>> ---
>>  arch/arm64/Kconfig.debug        | 29 +++++++++++++++++++++++
>>  arch/arm64/include/asm/ptdump.h |  8 +++++++
>>  arch/arm64/mm/dump.c            | 52 +++++++++++++++++++++++++++++++++++++++++
>>  arch/arm64/mm/mmu.c             |  2 ++
>>  4 files changed, 91 insertions(+)
>>
>> diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
>> index 21a5b74..d1ebd46 100644
>> --- a/arch/arm64/Kconfig.debug
>> +++ b/arch/arm64/Kconfig.debug
>> @@ -42,6 +42,35 @@ 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 f72ee69..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;
>> +}
>> +
>
> Why are these separate functions, and why is wx_pages increased twice,
> potentially?
>
> Given how rare non-UXN kernel mappings should be, could we not just add
>
>        if ((st->current_prot & PTE_UXN) == 0)
>                WARN(xxx)
>
> (without the _ONCE) to note_prot_wx(), and drop note_prot_uxn() entirely?
>
>

UXN is a separate bit from PTE_PXN/PTE_RDONLY and both pairs need to
be checked. The current return == 0 logic means that one set or the
other may not get checked. Rather than complicate the logic, it seemed
better to have separate functions. I see your point about the wx_pages
double counting so I can fix that.

>>  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);
>
> Could we upgrade this to pr_warn?
>

Sure

>> +       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
>>

^ permalink raw reply	[flat|nested] 29+ messages in thread
* [kernel-hardening] Re: [PATCHv3 4/4] arm64: dump: Add checking for writable and exectuable pages
@ 2016-10-20 13:01       ` Laura Abbott
  0 siblings, 0 replies; 29+ messages in thread
From: Laura Abbott @ 2016-10-20 13:01 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: AKASHI Takahiro, Mark Rutland, David Brown, Will Deacon,
	Catalin Marinas, linux-arm-kernel, linux-kernel, Kees Cook,
	kernel-hardening, Matt Fleming, linux-efi

On 10/20/2016 03:32 AM, Ard Biesheuvel wrote:
> On 18 October 2016 at 23:01, Laura Abbott <labbott@redhat.com> 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.
>>
>> Reviewed-by: Kees Cook <keescook@chromium.org>
>> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
>> Tested-by: Mark Rutland <mark.rutland@arm.com>
>> Signed-off-by: Laura Abbott <labbott@redhat.com>
>> ---
>> v3: Rebased for header guard fixup, whitespace fixes
>> ---
>>  arch/arm64/Kconfig.debug        | 29 +++++++++++++++++++++++
>>  arch/arm64/include/asm/ptdump.h |  8 +++++++
>>  arch/arm64/mm/dump.c            | 52 +++++++++++++++++++++++++++++++++++++++++
>>  arch/arm64/mm/mmu.c             |  2 ++
>>  4 files changed, 91 insertions(+)
>>
>> diff --git a/arch/arm64/Kconfig.debug b/arch/arm64/Kconfig.debug
>> index 21a5b74..d1ebd46 100644
>> --- a/arch/arm64/Kconfig.debug
>> +++ b/arch/arm64/Kconfig.debug
>> @@ -42,6 +42,35 @@ 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 f72ee69..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;
>> +}
>> +
>
> Why are these separate functions, and why is wx_pages increased twice,
> potentially?
>
> Given how rare non-UXN kernel mappings should be, could we not just add
>
>        if ((st->current_prot & PTE_UXN) == 0)
>                WARN(xxx)
>
> (without the _ONCE) to note_prot_wx(), and drop note_prot_uxn() entirely?
>
>

UXN is a separate bit from PTE_PXN/PTE_RDONLY and both pairs need to
be checked. The current return == 0 logic means that one set or the
other may not get checked. Rather than complicate the logic, it seemed
better to have separate functions. I see your point about the wx_pages
double counting so I can fix that.

>>  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);
>
> Could we upgrade this to pr_warn?
>

Sure

>> +       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
>>

^ permalink raw reply	[flat|nested] 29+ messages in thread
end of thread, other threads:[~2016-10-20 13:01 UTC | newest]

Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-18 22:01 [PATCHv3 0/4] WX checking for arm64 Laura Abbott
2016-10-18 22:01 ` [kernel-hardening] " Laura Abbott
2016-10-18 22:01 ` Laura Abbott
2016-10-18 22:01 ` Laura Abbott
2016-10-18 22:01 ` [PATCHv3 1/4] arm64: dump: Make ptdump debugfs a separate option Laura Abbott
2016-10-18 22:01   ` [kernel-hardening] " Laura Abbott
2016-10-18 22:01   ` Laura Abbott
2016-10-20 10:17   ` Ard Biesheuvel
2016-10-20 10:17     ` [kernel-hardening] " Ard Biesheuvel
2016-10-20 10:17     ` Ard Biesheuvel
2016-10-20 10:17     ` Ard Biesheuvel
2016-10-18 22:01 ` [PATCHv3 2/4] arm64: dump: Make the page table dumping seq_file optional Laura Abbott
2016-10-18 22:01   ` [kernel-hardening] " Laura Abbott
2016-10-18 22:01   ` Laura Abbott
2016-10-18 22:01 ` [PATCHv3 3/4] arm64: dump: Remove max_addr Laura Abbott
2016-10-18 22:01   ` [kernel-hardening] " Laura Abbott
2016-10-18 22:01   ` Laura Abbott
2016-10-18 22:01 ` [PATCHv3 4/4] arm64: dump: Add checking for writable and exectuable pages Laura Abbott
2016-10-18 22:01   ` [kernel-hardening] " Laura Abbott
2016-10-18 22:01   ` Laura Abbott
2016-10-18 22:01   ` Laura Abbott
2016-10-20 10:32   ` Ard Biesheuvel
2016-10-20 10:32     ` [kernel-hardening] " Ard Biesheuvel
2016-10-20 10:32     ` Ard Biesheuvel
2016-10-20 10:32     ` Ard Biesheuvel
2016-10-20 13:01     ` Laura Abbott
2016-10-20 13:01       ` [kernel-hardening] " Laura Abbott
2016-10-20 13:01       ` Laura Abbott
2016-10-20 13:01       ` Laura Abbott

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.