[kernel-hardening] [PATCH] ARM: mm: add testcases for RODATA

[kernel-hardening] [PATCH] ARM: mm: add testcases for RODATA

[Jinbum Park]

This patch adds testcases for the CONFIG_DEBUG_RODATA option.
It's similar to x86's testcases.
It tests read-only mapped data and page-size aligned rodata section.

Signed-off-by: Jinbum Park <jinb.park7@gmail.com>
---
 arch/arm/Kconfig.debug            |  5 +++
 arch/arm/include/asm/cacheflush.h | 10 +++++
 arch/arm/mm/Makefile              |  1 +
 arch/arm/mm/init.c                |  6 +++
 arch/arm/mm/test_rodata.c         | 80 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 102 insertions(+)
 create mode 100644 arch/arm/mm/test_rodata.c

diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
index d83f7c3..511e5e1 100644
--- a/arch/arm/Kconfig.debug
+++ b/arch/arm/Kconfig.debug
@@ -1749,6 +1749,11 @@ config DEBUG_SET_MODULE_RONX
 	  against certain classes of kernel exploits.
 	  If in doubt, say "N".
 
+config DEBUG_RODATA_TEST
+	bool "Testcase for the marking rodata read-only"
+	---help---
+	  This option enables a testcase for the setting rodata read-only.
+
 source "drivers/hwtracing/coresight/Kconfig"
 
 endmenu
diff --git a/arch/arm/include/asm/cacheflush.h b/arch/arm/include/asm/cacheflush.h
index bdd283b..741e2e8 100644
--- a/arch/arm/include/asm/cacheflush.h
+++ b/arch/arm/include/asm/cacheflush.h
@@ -498,6 +498,16 @@ static inline void set_kernel_text_rw(void) { }
 static inline void set_kernel_text_ro(void) { }
 #endif
 
+#ifdef CONFIG_DEBUG_RODATA_TEST
+extern const int rodata_test_data;
+int rodata_test(void);
+#else
+static inline int rodata_test(void)
+{
+	return 0;
+}
+#endif
+
 void flush_uprobe_xol_access(struct page *page, unsigned long uaddr,
 			     void *kaddr, unsigned long len);
 
diff --git a/arch/arm/mm/Makefile b/arch/arm/mm/Makefile
index b3dea80..dbb76ff 100644
--- a/arch/arm/mm/Makefile
+++ b/arch/arm/mm/Makefile
@@ -15,6 +15,7 @@ endif
 obj-$(CONFIG_ARM_PTDUMP)	+= dump.o
 obj-$(CONFIG_MODULES)		+= proc-syms.o
 obj-$(CONFIG_DEBUG_VIRTUAL)	+= physaddr.o
+obj-$(CONFIG_DEBUG_RODATA_TEST)	+= test_rodata.o
 
 obj-$(CONFIG_ALIGNMENT_TRAP)	+= alignment.o
 obj-$(CONFIG_HIGHMEM)		+= highmem.o
diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index 4127f57..3c15f3b 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -716,6 +716,7 @@ void fix_kernmem_perms(void)
 int __mark_rodata_ro(void *unused)
 {
 	update_sections_early(ro_perms, ARRAY_SIZE(ro_perms));
+	rodata_test();
 	return 0;
 }
 
@@ -740,6 +741,11 @@ void set_kernel_text_ro(void)
 static inline void fix_kernmem_perms(void) { }
 #endif /* CONFIG_DEBUG_RODATA */
 
+#ifdef CONFIG_DEBUG_RODATA_TEST
+const int rodata_test_data = 0xC3;
+EXPORT_SYMBOL_GPL(rodata_test_data);
+#endif
+
 void free_tcmmem(void)
 {
 #ifdef CONFIG_HAVE_TCM
diff --git a/arch/arm/mm/test_rodata.c b/arch/arm/mm/test_rodata.c
new file mode 100644
index 0000000..133d092
--- /dev/null
+++ b/arch/arm/mm/test_rodata.c
@@ -0,0 +1,79 @@
+/*
+ * test_rodata.c: functional test for mark_rodata_ro function
+ *
+ * (C) Copyright 2017 Jinbum Park <jinb.park7@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; version 2
+ * of the License.
+ */
+#include <asm/cacheflush.h>
+#include <asm/sections.h>
+
+int rodata_test(void)
+{
+	unsigned long result;
+	unsigned long start, end;
+
+	/* test 1: read the value */
+	/* If this test fails, some previous testrun has clobbered the state */
+
+	if (!rodata_test_data) {
+		pr_err("rodata_test: test 1 fails (start data)\n");
+		return -ENODEV;
+	}
+
+	/* test 2: write to the variable; this should fault */
+	/*
+	 * If this test fails, we managed to overwrite the data
+	 *
+	 * This is written in assembly to be able to catch the
+	 * exception that is supposed to happen in the correct
+	 * case
+	*/
+
+	result = 1;
+	asm volatile(
+		"0:	str %[zero], [%[rodata_test]]\n"
+		"	mov %[rslt], %[zero]\n"
+		"1:\n"
+		".pushsection .text.fixup,\"ax\"\n"
+		".align 2\n"
+		"2:\n"
+		"b 1b\n"
+		".popsection\n"
+		".pushsection __ex_table,\"a\"\n"
+		".align 3\n"
+		".long 0b, 2b\n"
+		".popsection\n"
+		: [rslt] "=r" (result)
+		: [zero] "r" (0UL), [rodata_test] "r" (&rodata_test_data)
+	);
+
+	if (!result) {
+		pr_err("rodata_test: test data was not read only\n");
+		return -ENODEV;
+	}
+
+	/* test 3: check the value hasn't changed */
+	/* If this test fails, we managed to overwrite the data */
+	if (!rodata_test_data) {
+		pr_err("rodata_test: Test 3 fails (end data)\n");
+		return -ENODEV;
+	}
+
+	/* test 4: check if the rodata section is 4Kb aligned */
+	start = (unsigned long)__start_rodata;
+	end = (unsigned long)__end_rodata;
+	if (start & (PAGE_SIZE - 1)) {
+		pr_err("rodata_test: .rodata is not 4k aligned\n");
+		return -ENODEV;
+	}
+	if (end & (PAGE_SIZE - 1)) {
+		pr_err("rodata_test: .rodata end is not 4k aligned\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
-- 
1.9.1

[PATCH] ARM: mm: add testcases for RODATA

[Jinbum Park]

This patch adds testcases for the CONFIG_DEBUG_RODATA option.
It's similar to x86's testcases.
It tests read-only mapped data and page-size aligned rodata section.

Signed-off-by: Jinbum Park <jinb.park7@gmail.com>
---
 arch/arm/Kconfig.debug            |  5 +++
 arch/arm/include/asm/cacheflush.h | 10 +++++
 arch/arm/mm/Makefile              |  1 +
 arch/arm/mm/init.c                |  6 +++
 arch/arm/mm/test_rodata.c         | 80 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 102 insertions(+)
 create mode 100644 arch/arm/mm/test_rodata.c

diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
index d83f7c3..511e5e1 100644
--- a/arch/arm/Kconfig.debug
+++ b/arch/arm/Kconfig.debug
@@ -1749,6 +1749,11 @@ config DEBUG_SET_MODULE_RONX
 	  against certain classes of kernel exploits.
 	  If in doubt, say "N".
 
+config DEBUG_RODATA_TEST
+	bool "Testcase for the marking rodata read-only"
+	---help---
+	  This option enables a testcase for the setting rodata read-only.
+
 source "drivers/hwtracing/coresight/Kconfig"
 
 endmenu
diff --git a/arch/arm/include/asm/cacheflush.h b/arch/arm/include/asm/cacheflush.h
index bdd283b..741e2e8 100644
--- a/arch/arm/include/asm/cacheflush.h
+++ b/arch/arm/include/asm/cacheflush.h
@@ -498,6 +498,16 @@ static inline void set_kernel_text_rw(void) { }
 static inline void set_kernel_text_ro(void) { }
 #endif
 
+#ifdef CONFIG_DEBUG_RODATA_TEST
+extern const int rodata_test_data;
+int rodata_test(void);
+#else
+static inline int rodata_test(void)
+{
+	return 0;
+}
+#endif
+
 void flush_uprobe_xol_access(struct page *page, unsigned long uaddr,
 			     void *kaddr, unsigned long len);
 
diff --git a/arch/arm/mm/Makefile b/arch/arm/mm/Makefile
index b3dea80..dbb76ff 100644
--- a/arch/arm/mm/Makefile
+++ b/arch/arm/mm/Makefile
@@ -15,6 +15,7 @@ endif
 obj-$(CONFIG_ARM_PTDUMP)	+= dump.o
 obj-$(CONFIG_MODULES)		+= proc-syms.o
 obj-$(CONFIG_DEBUG_VIRTUAL)	+= physaddr.o
+obj-$(CONFIG_DEBUG_RODATA_TEST)	+= test_rodata.o
 
 obj-$(CONFIG_ALIGNMENT_TRAP)	+= alignment.o
 obj-$(CONFIG_HIGHMEM)		+= highmem.o
diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index 4127f57..3c15f3b 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -716,6 +716,7 @@ void fix_kernmem_perms(void)
 int __mark_rodata_ro(void *unused)
 {
 	update_sections_early(ro_perms, ARRAY_SIZE(ro_perms));
+	rodata_test();
 	return 0;
 }
 
@@ -740,6 +741,11 @@ void set_kernel_text_ro(void)
 static inline void fix_kernmem_perms(void) { }
 #endif /* CONFIG_DEBUG_RODATA */
 
+#ifdef CONFIG_DEBUG_RODATA_TEST
+const int rodata_test_data = 0xC3;
+EXPORT_SYMBOL_GPL(rodata_test_data);
+#endif
+
 void free_tcmmem(void)
 {
 #ifdef CONFIG_HAVE_TCM
diff --git a/arch/arm/mm/test_rodata.c b/arch/arm/mm/test_rodata.c
new file mode 100644
index 0000000..133d092
--- /dev/null
+++ b/arch/arm/mm/test_rodata.c
@@ -0,0 +1,79 @@
+/*
+ * test_rodata.c: functional test for mark_rodata_ro function
+ *
+ * (C) Copyright 2017 Jinbum Park <jinb.park7@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; version 2
+ * of the License.
+ */
+#include <asm/cacheflush.h>
+#include <asm/sections.h>
+
+int rodata_test(void)
+{
+	unsigned long result;
+	unsigned long start, end;
+
+	/* test 1: read the value */
+	/* If this test fails, some previous testrun has clobbered the state */
+
+	if (!rodata_test_data) {
+		pr_err("rodata_test: test 1 fails (start data)\n");
+		return -ENODEV;
+	}
+
+	/* test 2: write to the variable; this should fault */
+	/*
+	 * If this test fails, we managed to overwrite the data
+	 *
+	 * This is written in assembly to be able to catch the
+	 * exception that is supposed to happen in the correct
+	 * case
+	*/
+
+	result = 1;
+	asm volatile(
+		"0:	str %[zero], [%[rodata_test]]\n"
+		"	mov %[rslt], %[zero]\n"
+		"1:\n"
+		".pushsection .text.fixup,\"ax\"\n"
+		".align 2\n"
+		"2:\n"
+		"b 1b\n"
+		".popsection\n"
+		".pushsection __ex_table,\"a\"\n"
+		".align 3\n"
+		".long 0b, 2b\n"
+		".popsection\n"
+		: [rslt] "=r" (result)
+		: [zero] "r" (0UL), [rodata_test] "r" (&rodata_test_data)
+	);
+
+	if (!result) {
+		pr_err("rodata_test: test data was not read only\n");
+		return -ENODEV;
+	}
+
+	/* test 3: check the value hasn't changed */
+	/* If this test fails, we managed to overwrite the data */
+	if (!rodata_test_data) {
+		pr_err("rodata_test: Test 3 fails (end data)\n");
+		return -ENODEV;
+	}
+
+	/* test 4: check if the rodata section is 4Kb aligned */
+	start = (unsigned long)__start_rodata;
+	end = (unsigned long)__end_rodata;
+	if (start & (PAGE_SIZE - 1)) {
+		pr_err("rodata_test: .rodata is not 4k aligned\n");
+		return -ENODEV;
+	}
+	if (end & (PAGE_SIZE - 1)) {
+		pr_err("rodata_test: .rodata end is not 4k aligned\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
-- 
1.9.1

[PATCH] ARM: mm: add testcases for RODATA

[Jinbum Park]

This patch adds testcases for the CONFIG_DEBUG_RODATA option.
It's similar to x86's testcases.
It tests read-only mapped data and page-size aligned rodata section.

Signed-off-by: Jinbum Park <jinb.park7@gmail.com>
---
 arch/arm/Kconfig.debug            |  5 +++
 arch/arm/include/asm/cacheflush.h | 10 +++++
 arch/arm/mm/Makefile              |  1 +
 arch/arm/mm/init.c                |  6 +++
 arch/arm/mm/test_rodata.c         | 80 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 102 insertions(+)
 create mode 100644 arch/arm/mm/test_rodata.c

diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
index d83f7c3..511e5e1 100644
--- a/arch/arm/Kconfig.debug
+++ b/arch/arm/Kconfig.debug
@@ -1749,6 +1749,11 @@ config DEBUG_SET_MODULE_RONX
 	  against certain classes of kernel exploits.
 	  If in doubt, say "N".
 
+config DEBUG_RODATA_TEST
+	bool "Testcase for the marking rodata read-only"
+	---help---
+	  This option enables a testcase for the setting rodata read-only.
+
 source "drivers/hwtracing/coresight/Kconfig"
 
 endmenu
diff --git a/arch/arm/include/asm/cacheflush.h b/arch/arm/include/asm/cacheflush.h
index bdd283b..741e2e8 100644
--- a/arch/arm/include/asm/cacheflush.h
+++ b/arch/arm/include/asm/cacheflush.h
@@ -498,6 +498,16 @@ static inline void set_kernel_text_rw(void) { }
 static inline void set_kernel_text_ro(void) { }
 #endif
 
+#ifdef CONFIG_DEBUG_RODATA_TEST
+extern const int rodata_test_data;
+int rodata_test(void);
+#else
+static inline int rodata_test(void)
+{
+	return 0;
+}
+#endif
+
 void flush_uprobe_xol_access(struct page *page, unsigned long uaddr,
 			     void *kaddr, unsigned long len);
 
diff --git a/arch/arm/mm/Makefile b/arch/arm/mm/Makefile
index b3dea80..dbb76ff 100644
--- a/arch/arm/mm/Makefile
+++ b/arch/arm/mm/Makefile
@@ -15,6 +15,7 @@ endif
 obj-$(CONFIG_ARM_PTDUMP)	+= dump.o
 obj-$(CONFIG_MODULES)		+= proc-syms.o
 obj-$(CONFIG_DEBUG_VIRTUAL)	+= physaddr.o
+obj-$(CONFIG_DEBUG_RODATA_TEST)	+= test_rodata.o
 
 obj-$(CONFIG_ALIGNMENT_TRAP)	+= alignment.o
 obj-$(CONFIG_HIGHMEM)		+= highmem.o
diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index 4127f57..3c15f3b 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -716,6 +716,7 @@ void fix_kernmem_perms(void)
 int __mark_rodata_ro(void *unused)
 {
 	update_sections_early(ro_perms, ARRAY_SIZE(ro_perms));
+	rodata_test();
 	return 0;
 }
 
@@ -740,6 +741,11 @@ void set_kernel_text_ro(void)
 static inline void fix_kernmem_perms(void) { }
 #endif /* CONFIG_DEBUG_RODATA */
 
+#ifdef CONFIG_DEBUG_RODATA_TEST
+const int rodata_test_data = 0xC3;
+EXPORT_SYMBOL_GPL(rodata_test_data);
+#endif
+
 void free_tcmmem(void)
 {
 #ifdef CONFIG_HAVE_TCM
diff --git a/arch/arm/mm/test_rodata.c b/arch/arm/mm/test_rodata.c
new file mode 100644
index 0000000..133d092
--- /dev/null
+++ b/arch/arm/mm/test_rodata.c
@@ -0,0 +1,79 @@
+/*
+ * test_rodata.c: functional test for mark_rodata_ro function
+ *
+ * (C) Copyright 2017 Jinbum Park <jinb.park7@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; version 2
+ * of the License.
+ */
+#include <asm/cacheflush.h>
+#include <asm/sections.h>
+
+int rodata_test(void)
+{
+	unsigned long result;
+	unsigned long start, end;
+
+	/* test 1: read the value */
+	/* If this test fails, some previous testrun has clobbered the state */
+
+	if (!rodata_test_data) {
+		pr_err("rodata_test: test 1 fails (start data)\n");
+		return -ENODEV;
+	}
+
+	/* test 2: write to the variable; this should fault */
+	/*
+	 * If this test fails, we managed to overwrite the data
+	 *
+	 * This is written in assembly to be able to catch the
+	 * exception that is supposed to happen in the correct
+	 * case
+	*/
+
+	result = 1;
+	asm volatile(
+		"0:	str %[zero], [%[rodata_test]]\n"
+		"	mov %[rslt], %[zero]\n"
+		"1:\n"
+		".pushsection .text.fixup,\"ax\"\n"
+		".align 2\n"
+		"2:\n"
+		"b 1b\n"
+		".popsection\n"
+		".pushsection __ex_table,\"a\"\n"
+		".align 3\n"
+		".long 0b, 2b\n"
+		".popsection\n"
+		: [rslt] "=r" (result)
+		: [zero] "r" (0UL), [rodata_test] "r" (&rodata_test_data)
+	);
+
+	if (!result) {
+		pr_err("rodata_test: test data was not read only\n");
+		return -ENODEV;
+	}
+
+	/* test 3: check the value hasn't changed */
+	/* If this test fails, we managed to overwrite the data */
+	if (!rodata_test_data) {
+		pr_err("rodata_test: Test 3 fails (end data)\n");
+		return -ENODEV;
+	}
+
+	/* test 4: check if the rodata section is 4Kb aligned */
+	start = (unsigned long)__start_rodata;
+	end = (unsigned long)__end_rodata;
+	if (start & (PAGE_SIZE - 1)) {
+		pr_err("rodata_test: .rodata is not 4k aligned\n");
+		return -ENODEV;
+	}
+	if (end & (PAGE_SIZE - 1)) {
+		pr_err("rodata_test: .rodata end is not 4k aligned\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
-- 
1.9.1

[PATCH] ARM: mm: add testcases for RODATA

[Jinbum Park]

This patch adds testcases for the CONFIG_DEBUG_RODATA option.
It's similar to x86's testcases.
It tests read-only mapped data and page-size aligned rodata section.

Signed-off-by: Jinbum Park <jinb.park7@gmail.com>
---
 arch/arm/Kconfig.debug            |  5 +++
 arch/arm/include/asm/cacheflush.h | 10 +++++
 arch/arm/mm/Makefile              |  1 +
 arch/arm/mm/init.c                |  6 +++
 arch/arm/mm/test_rodata.c         | 80 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 102 insertions(+)
 create mode 100644 arch/arm/mm/test_rodata.c

diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
index d83f7c3..511e5e1 100644
--- a/arch/arm/Kconfig.debug
+++ b/arch/arm/Kconfig.debug
@@ -1749,6 +1749,11 @@ config DEBUG_SET_MODULE_RONX
 	  against certain classes of kernel exploits.
 	  If in doubt, say "N".
 
+config DEBUG_RODATA_TEST
+	bool "Testcase for the marking rodata read-only"
+	---help---
+	  This option enables a testcase for the setting rodata read-only.
+
 source "drivers/hwtracing/coresight/Kconfig"
 
 endmenu
diff --git a/arch/arm/include/asm/cacheflush.h b/arch/arm/include/asm/cacheflush.h
index bdd283b..741e2e8 100644
--- a/arch/arm/include/asm/cacheflush.h
+++ b/arch/arm/include/asm/cacheflush.h
@@ -498,6 +498,16 @@ static inline void set_kernel_text_rw(void) { }
 static inline void set_kernel_text_ro(void) { }
 #endif
 
+#ifdef CONFIG_DEBUG_RODATA_TEST
+extern const int rodata_test_data;
+int rodata_test(void);
+#else
+static inline int rodata_test(void)
+{
+	return 0;
+}
+#endif
+
 void flush_uprobe_xol_access(struct page *page, unsigned long uaddr,
 			     void *kaddr, unsigned long len);
 
diff --git a/arch/arm/mm/Makefile b/arch/arm/mm/Makefile
index b3dea80..dbb76ff 100644
--- a/arch/arm/mm/Makefile
+++ b/arch/arm/mm/Makefile
@@ -15,6 +15,7 @@ endif
 obj-$(CONFIG_ARM_PTDUMP)	+= dump.o
 obj-$(CONFIG_MODULES)		+= proc-syms.o
 obj-$(CONFIG_DEBUG_VIRTUAL)	+= physaddr.o
+obj-$(CONFIG_DEBUG_RODATA_TEST)	+= test_rodata.o
 
 obj-$(CONFIG_ALIGNMENT_TRAP)	+= alignment.o
 obj-$(CONFIG_HIGHMEM)		+= highmem.o
diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
index 4127f57..3c15f3b 100644
--- a/arch/arm/mm/init.c
+++ b/arch/arm/mm/init.c
@@ -716,6 +716,7 @@ void fix_kernmem_perms(void)
 int __mark_rodata_ro(void *unused)
 {
 	update_sections_early(ro_perms, ARRAY_SIZE(ro_perms));
+	rodata_test();
 	return 0;
 }
 
@@ -740,6 +741,11 @@ void set_kernel_text_ro(void)
 static inline void fix_kernmem_perms(void) { }
 #endif /* CONFIG_DEBUG_RODATA */
 
+#ifdef CONFIG_DEBUG_RODATA_TEST
+const int rodata_test_data = 0xC3;
+EXPORT_SYMBOL_GPL(rodata_test_data);
+#endif
+
 void free_tcmmem(void)
 {
 #ifdef CONFIG_HAVE_TCM
diff --git a/arch/arm/mm/test_rodata.c b/arch/arm/mm/test_rodata.c
new file mode 100644
index 0000000..133d092
--- /dev/null
+++ b/arch/arm/mm/test_rodata.c
@@ -0,0 +1,79 @@
+/*
+ * test_rodata.c: functional test for mark_rodata_ro function
+ *
+ * (C) Copyright 2017 Jinbum Park <jinb.park7@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; version 2
+ * of the License.
+ */
+#include <asm/cacheflush.h>
+#include <asm/sections.h>
+
+int rodata_test(void)
+{
+	unsigned long result;
+	unsigned long start, end;
+
+	/* test 1: read the value */
+	/* If this test fails, some previous testrun has clobbered the state */
+
+	if (!rodata_test_data) {
+		pr_err("rodata_test: test 1 fails (start data)\n");
+		return -ENODEV;
+	}
+
+	/* test 2: write to the variable; this should fault */
+	/*
+	 * If this test fails, we managed to overwrite the data
+	 *
+	 * This is written in assembly to be able to catch the
+	 * exception that is supposed to happen in the correct
+	 * case
+	*/
+
+	result = 1;
+	asm volatile(
+		"0:	str %[zero], [%[rodata_test]]\n"
+		"	mov %[rslt], %[zero]\n"
+		"1:\n"
+		".pushsection .text.fixup,\"ax\"\n"
+		".align 2\n"
+		"2:\n"
+		"b 1b\n"
+		".popsection\n"
+		".pushsection __ex_table,\"a\"\n"
+		".align 3\n"
+		".long 0b, 2b\n"
+		".popsection\n"
+		: [rslt] "=r" (result)
+		: [zero] "r" (0UL), [rodata_test] "r" (&rodata_test_data)
+	);
+
+	if (!result) {
+		pr_err("rodata_test: test data was not read only\n");
+		return -ENODEV;
+	}
+
+	/* test 3: check the value hasn't changed */
+	/* If this test fails, we managed to overwrite the data */
+	if (!rodata_test_data) {
+		pr_err("rodata_test: Test 3 fails (end data)\n");
+		return -ENODEV;
+	}
+
+	/* test 4: check if the rodata section is 4Kb aligned */
+	start = (unsigned long)__start_rodata;
+	end = (unsigned long)__end_rodata;
+	if (start & (PAGE_SIZE - 1)) {
+		pr_err("rodata_test: .rodata is not 4k aligned\n");
+		return -ENODEV;
+	}
+	if (end & (PAGE_SIZE - 1)) {
+		pr_err("rodata_test: .rodata end is not 4k aligned\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
-- 
1.9.1

[kernel-hardening] Re: [PATCH] ARM: mm: add testcases for RODATA

[Mark Rutland]

On Wed, Jan 18, 2017 at 10:53:10PM +0900, Jinbum Park wrote:
> This patch adds testcases for the CONFIG_DEBUG_RODATA option.
> It's similar to x86's testcases.
> It tests read-only mapped data and page-size aligned rodata section.

I note that LKDTM already has a similar test (though it just has a raw
write, and will crash the kernel).

> +	asm volatile(
> +		"0:	str %[zero], [%[rodata_test]]\n"
> +		"	mov %[rslt], %[zero]\n"
> +		"1:\n"
> +		".pushsection .text.fixup,\"ax\"\n"
> +		".align 2\n"
> +		"2:\n"
> +		"b 1b\n"
> +		".popsection\n"
> +		".pushsection __ex_table,\"a\"\n"
> +		".align 3\n"
> +		".long 0b, 2b\n"
> +		".popsection\n"
> +		: [rslt] "=r" (result)
> +		: [zero] "r" (0UL), [rodata_test] "r" (&rodata_test_data)
> +	);

This is the only architecture-specific part of the file.

Rather than duplicating the logic from x86, can't we use generic
infrastructure for this part, and move the existing test into a shared
location?

e.g. could we change to KERNEL_DS and use put_user here?

> +	if (!result) {
> +		pr_err("rodata_test: test data was not read only\n");
> +		return -ENODEV;
> +	}
> +
> +	/* test 3: check the value hasn't changed */
> +	/* If this test fails, we managed to overwrite the data */
> +	if (!rodata_test_data) {
> +		pr_err("rodata_test: Test 3 fails (end data)\n");
> +		return -ENODEV;
> +	}
> +
> +	/* test 4: check if the rodata section is 4Kb aligned */
> +	start = (unsigned long)__start_rodata;
> +	end = (unsigned long)__end_rodata;
> +	if (start & (PAGE_SIZE - 1)) {
> +		pr_err("rodata_test: .rodata is not 4k aligned\n");
> +		return -ENODEV;
> +	}
> +	if (end & (PAGE_SIZE - 1)) {
> +		pr_err("rodata_test: .rodata end is not 4k aligned\n");
> +		return -ENODEV;
> +	}

s/4k/page/ in the prints, if this becomes generic.

Thanks,
Mark.

Re: [PATCH] ARM: mm: add testcases for RODATA

[Mark Rutland]

On Wed, Jan 18, 2017 at 10:53:10PM +0900, Jinbum Park wrote:
> This patch adds testcases for the CONFIG_DEBUG_RODATA option.
> It's similar to x86's testcases.
> It tests read-only mapped data and page-size aligned rodata section.

I note that LKDTM already has a similar test (though it just has a raw
write, and will crash the kernel).

> +	asm volatile(
> +		"0:	str %[zero], [%[rodata_test]]\n"
> +		"	mov %[rslt], %[zero]\n"
> +		"1:\n"
> +		".pushsection .text.fixup,\"ax\"\n"
> +		".align 2\n"
> +		"2:\n"
> +		"b 1b\n"
> +		".popsection\n"
> +		".pushsection __ex_table,\"a\"\n"
> +		".align 3\n"
> +		".long 0b, 2b\n"
> +		".popsection\n"
> +		: [rslt] "=r" (result)
> +		: [zero] "r" (0UL), [rodata_test] "r" (&rodata_test_data)
> +	);

This is the only architecture-specific part of the file.

Rather than duplicating the logic from x86, can't we use generic
infrastructure for this part, and move the existing test into a shared
location?

e.g. could we change to KERNEL_DS and use put_user here?

> +	if (!result) {
> +		pr_err("rodata_test: test data was not read only\n");
> +		return -ENODEV;
> +	}
> +
> +	/* test 3: check the value hasn't changed */
> +	/* If this test fails, we managed to overwrite the data */
> +	if (!rodata_test_data) {
> +		pr_err("rodata_test: Test 3 fails (end data)\n");
> +		return -ENODEV;
> +	}
> +
> +	/* test 4: check if the rodata section is 4Kb aligned */
> +	start = (unsigned long)__start_rodata;
> +	end = (unsigned long)__end_rodata;
> +	if (start & (PAGE_SIZE - 1)) {
> +		pr_err("rodata_test: .rodata is not 4k aligned\n");
> +		return -ENODEV;
> +	}
> +	if (end & (PAGE_SIZE - 1)) {
> +		pr_err("rodata_test: .rodata end is not 4k aligned\n");
> +		return -ENODEV;
> +	}

s/4k/page/ in the prints, if this becomes generic.

Thanks,
Mark.

[PATCH] ARM: mm: add testcases for RODATA

[Mark Rutland]

On Wed, Jan 18, 2017 at 10:53:10PM +0900, Jinbum Park wrote:
> This patch adds testcases for the CONFIG_DEBUG_RODATA option.
> It's similar to x86's testcases.
> It tests read-only mapped data and page-size aligned rodata section.

I note that LKDTM already has a similar test (though it just has a raw
write, and will crash the kernel).

> +	asm volatile(
> +		"0:	str %[zero], [%[rodata_test]]\n"
> +		"	mov %[rslt], %[zero]\n"
> +		"1:\n"
> +		".pushsection .text.fixup,\"ax\"\n"
> +		".align 2\n"
> +		"2:\n"
> +		"b 1b\n"
> +		".popsection\n"
> +		".pushsection __ex_table,\"a\"\n"
> +		".align 3\n"
> +		".long 0b, 2b\n"
> +		".popsection\n"
> +		: [rslt] "=r" (result)
> +		: [zero] "r" (0UL), [rodata_test] "r" (&rodata_test_data)
> +	);

This is the only architecture-specific part of the file.

Rather than duplicating the logic from x86, can't we use generic
infrastructure for this part, and move the existing test into a shared
location?

e.g. could we change to KERNEL_DS and use put_user here?

> +	if (!result) {
> +		pr_err("rodata_test: test data was not read only\n");
> +		return -ENODEV;
> +	}
> +
> +	/* test 3: check the value hasn't changed */
> +	/* If this test fails, we managed to overwrite the data */
> +	if (!rodata_test_data) {
> +		pr_err("rodata_test: Test 3 fails (end data)\n");
> +		return -ENODEV;
> +	}
> +
> +	/* test 4: check if the rodata section is 4Kb aligned */
> +	start = (unsigned long)__start_rodata;
> +	end = (unsigned long)__end_rodata;
> +	if (start & (PAGE_SIZE - 1)) {
> +		pr_err("rodata_test: .rodata is not 4k aligned\n");
> +		return -ENODEV;
> +	}
> +	if (end & (PAGE_SIZE - 1)) {
> +		pr_err("rodata_test: .rodata end is not 4k aligned\n");
> +		return -ENODEV;
> +	}

s/4k/page/ in the prints, if this becomes generic.

Thanks,
Mark.

Re: [kernel-hardening] [PATCH] ARM: mm: add testcases for RODATA

[Solar Designer]

Re: [kernel-hardening] [PATCH] ARM: mm: add testcases for RODATA

[Solar Designer]

Oops, sorry about the empty posting.  I am too used to approving
messages sometimes held for moderation here, so that is what I was
trying to do - but the message in question was already on the list, not
needing approval.

I've just removed myself from the moderation bypass to prevent such
occurrences going forward.  In case anyone wonders, the moderation on
this list so far is primarily an anti-spam measure.  There are almost
no attempted off-topic postings by actual people, luckily.  (The worst
were some threads CC'ed to/from other lists wandering off topic a bit,
but not fatally so.  We kept those threads in here anyway, not to break
the threading and not to discourage people's good work that just happens
to fit onto multiple lists.)  However, without moderation at all, some
automated spam would have been getting through.

Alexander

[kernel-hardening] Re: [PATCH] ARM: mm: add testcases for RODATA

[Laura Abbott]

On 01/18/2017 05:53 AM, Jinbum Park wrote:
> This patch adds testcases for the CONFIG_DEBUG_RODATA option.
> It's similar to x86's testcases.
> It tests read-only mapped data and page-size aligned rodata section.
> 
> Signed-off-by: Jinbum Park <jinb.park7@gmail.com>
> ---
>  arch/arm/Kconfig.debug            |  5 +++
>  arch/arm/include/asm/cacheflush.h | 10 +++++
>  arch/arm/mm/Makefile              |  1 +
>  arch/arm/mm/init.c                |  6 +++
>  arch/arm/mm/test_rodata.c         | 80 +++++++++++++++++++++++++++++++++++++++
>  5 files changed, 102 insertions(+)
>  create mode 100644 arch/arm/mm/test_rodata.c
> 
> diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
> index d83f7c3..511e5e1 100644
> --- a/arch/arm/Kconfig.debug
> +++ b/arch/arm/Kconfig.debug
> @@ -1749,6 +1749,11 @@ config DEBUG_SET_MODULE_RONX
>  	  against certain classes of kernel exploits.
>  	  If in doubt, say "N".
>  
> +config DEBUG_RODATA_TEST
> +	bool "Testcase for the marking rodata read-only"
> +	---help---
> +	  This option enables a testcase for the setting rodata read-only.
> +
>  source "drivers/hwtracing/coresight/Kconfig"
>  
>  endmenu
> diff --git a/arch/arm/include/asm/cacheflush.h b/arch/arm/include/asm/cacheflush.h
> index bdd283b..741e2e8 100644
> --- a/arch/arm/include/asm/cacheflush.h
> +++ b/arch/arm/include/asm/cacheflush.h
> @@ -498,6 +498,16 @@ static inline void set_kernel_text_rw(void) { }
>  static inline void set_kernel_text_ro(void) { }
>  #endif
>  
> +#ifdef CONFIG_DEBUG_RODATA_TEST
> +extern const int rodata_test_data;
> +int rodata_test(void);
> +#else
> +static inline int rodata_test(void)
> +{
> +	return 0;
> +}
> +#endif
> +
>  void flush_uprobe_xol_access(struct page *page, unsigned long uaddr,
>  			     void *kaddr, unsigned long len);
>  
> diff --git a/arch/arm/mm/Makefile b/arch/arm/mm/Makefile
> index b3dea80..dbb76ff 100644
> --- a/arch/arm/mm/Makefile
> +++ b/arch/arm/mm/Makefile
> @@ -15,6 +15,7 @@ endif
>  obj-$(CONFIG_ARM_PTDUMP)	+= dump.o
>  obj-$(CONFIG_MODULES)		+= proc-syms.o
>  obj-$(CONFIG_DEBUG_VIRTUAL)	+= physaddr.o
> +obj-$(CONFIG_DEBUG_RODATA_TEST)	+= test_rodata.o
>  
>  obj-$(CONFIG_ALIGNMENT_TRAP)	+= alignment.o
>  obj-$(CONFIG_HIGHMEM)		+= highmem.o
> diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
> index 4127f57..3c15f3b 100644
> --- a/arch/arm/mm/init.c
> +++ b/arch/arm/mm/init.c
> @@ -716,6 +716,7 @@ void fix_kernmem_perms(void)
>  int __mark_rodata_ro(void *unused)
>  {
>  	update_sections_early(ro_perms, ARRAY_SIZE(ro_perms));
> +	rodata_test();

We don't do anything with this return value, should we at least
spit out a warning?

>  	return 0;
>  }
>  
> @@ -740,6 +741,11 @@ void set_kernel_text_ro(void)
>  static inline void fix_kernmem_perms(void) { }
>  #endif /* CONFIG_DEBUG_RODATA */
>  
> +#ifdef CONFIG_DEBUG_RODATA_TEST
> +const int rodata_test_data = 0xC3;

This isn't accessed outside of test_rodata.c, it can just
be moved there.

> +EXPORT_SYMBOL_GPL(rodata_test_data);
> +#endif
> +
>  void free_tcmmem(void)
>  {
>  #ifdef CONFIG_HAVE_TCM
> diff --git a/arch/arm/mm/test_rodata.c b/arch/arm/mm/test_rodata.c
> new file mode 100644
> index 0000000..133d092
> --- /dev/null
> +++ b/arch/arm/mm/test_rodata.c
> @@ -0,0 +1,79 @@
> +/*
> + * test_rodata.c: functional test for mark_rodata_ro function
> + *
> + * (C) Copyright 2017 Jinbum Park <jinb.park7@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; version 2
> + * of the License.
> + */
> +#include <asm/cacheflush.h>
> +#include <asm/sections.h>
> +
> +int rodata_test(void)
> +{
> +	unsigned long result;
> +	unsigned long start, end;
> +
> +	/* test 1: read the value */
> +	/* If this test fails, some previous testrun has clobbered the state */
> +
> +	if (!rodata_test_data) {
> +		pr_err("rodata_test: test 1 fails (start data)\n");
> +		return -ENODEV;
> +	}
> +
> +	/* test 2: write to the variable; this should fault */
> +	/*
> +	 * If this test fails, we managed to overwrite the data
> +	 *
> +	 * This is written in assembly to be able to catch the
> +	 * exception that is supposed to happen in the correct
> +	 * case
> +	*/
> +
> +	result = 1;
> +	asm volatile(
> +		"0:	str %[zero], [%[rodata_test]]\n"
> +		"	mov %[rslt], %[zero]\n"
> +		"1:\n"
> +		".pushsection .text.fixup,\"ax\"\n"
> +		".align 2\n"
> +		"2:\n"
> +		"b 1b\n"
> +		".popsection\n"
> +		".pushsection __ex_table,\"a\"\n"
> +		".align 3\n"
> +		".long 0b, 2b\n"
> +		".popsection\n"
> +		: [rslt] "=r" (result)
> +		: [zero] "r" (0UL), [rodata_test] "r" (&rodata_test_data)
> +	);
> +
> +	if (!result) {
> +		pr_err("rodata_test: test data was not read only\n");
> +		return -ENODEV;
> +	}
> +
> +	/* test 3: check the value hasn't changed */
> +	/* If this test fails, we managed to overwrite the data */
> +	if (!rodata_test_data) {
> +		pr_err("rodata_test: Test 3 fails (end data)\n");
> +		return -ENODEV;
> +	}

I'm confused why we are checking this again when we have the result
check above. Is there a case where we would still have result = 1
but rodata_test_data overwritten?

> +
> +	/* test 4: check if the rodata section is 4Kb aligned */
> +	start = (unsigned long)__start_rodata;
> +	end = (unsigned long)__end_rodata;
> +	if (start & (PAGE_SIZE - 1)) {
> +		pr_err("rodata_test: .rodata is not 4k aligned\n");
> +		return -ENODEV;
> +	}
> +	if (end & (PAGE_SIZE - 1)) {
> +		pr_err("rodata_test: .rodata end is not 4k aligned\n");
> +		return -ENODEV;
> +	}

Why does this need to be page aligned (yes, I know why but this
test case does not make it clear)

Better yet, this should be a build time assertion not a runtime
one.

> +
> +	return 0;
> +}
> 

As Mark mentioned, this is possibly redundant with LKDTM. It
would be good to explain what benefit this is bringing in addition
to LKDTM.

Thanks,
Laura

Re: [PATCH] ARM: mm: add testcases for RODATA

[Laura Abbott]

On 01/18/2017 05:53 AM, Jinbum Park wrote:
> This patch adds testcases for the CONFIG_DEBUG_RODATA option.
> It's similar to x86's testcases.
> It tests read-only mapped data and page-size aligned rodata section.
> 
> Signed-off-by: Jinbum Park <jinb.park7@gmail.com>
> ---
>  arch/arm/Kconfig.debug            |  5 +++
>  arch/arm/include/asm/cacheflush.h | 10 +++++
>  arch/arm/mm/Makefile              |  1 +
>  arch/arm/mm/init.c                |  6 +++
>  arch/arm/mm/test_rodata.c         | 80 +++++++++++++++++++++++++++++++++++++++
>  5 files changed, 102 insertions(+)
>  create mode 100644 arch/arm/mm/test_rodata.c
> 
> diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
> index d83f7c3..511e5e1 100644
> --- a/arch/arm/Kconfig.debug
> +++ b/arch/arm/Kconfig.debug
> @@ -1749,6 +1749,11 @@ config DEBUG_SET_MODULE_RONX
>  	  against certain classes of kernel exploits.
>  	  If in doubt, say "N".
>  
> +config DEBUG_RODATA_TEST
> +	bool "Testcase for the marking rodata read-only"
> +	---help---
> +	  This option enables a testcase for the setting rodata read-only.
> +
>  source "drivers/hwtracing/coresight/Kconfig"
>  
>  endmenu
> diff --git a/arch/arm/include/asm/cacheflush.h b/arch/arm/include/asm/cacheflush.h
> index bdd283b..741e2e8 100644
> --- a/arch/arm/include/asm/cacheflush.h
> +++ b/arch/arm/include/asm/cacheflush.h
> @@ -498,6 +498,16 @@ static inline void set_kernel_text_rw(void) { }
>  static inline void set_kernel_text_ro(void) { }
>  #endif
>  
> +#ifdef CONFIG_DEBUG_RODATA_TEST
> +extern const int rodata_test_data;
> +int rodata_test(void);
> +#else
> +static inline int rodata_test(void)
> +{
> +	return 0;
> +}
> +#endif
> +
>  void flush_uprobe_xol_access(struct page *page, unsigned long uaddr,
>  			     void *kaddr, unsigned long len);
>  
> diff --git a/arch/arm/mm/Makefile b/arch/arm/mm/Makefile
> index b3dea80..dbb76ff 100644
> --- a/arch/arm/mm/Makefile
> +++ b/arch/arm/mm/Makefile
> @@ -15,6 +15,7 @@ endif
>  obj-$(CONFIG_ARM_PTDUMP)	+= dump.o
>  obj-$(CONFIG_MODULES)		+= proc-syms.o
>  obj-$(CONFIG_DEBUG_VIRTUAL)	+= physaddr.o
> +obj-$(CONFIG_DEBUG_RODATA_TEST)	+= test_rodata.o
>  
>  obj-$(CONFIG_ALIGNMENT_TRAP)	+= alignment.o
>  obj-$(CONFIG_HIGHMEM)		+= highmem.o
> diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
> index 4127f57..3c15f3b 100644
> --- a/arch/arm/mm/init.c
> +++ b/arch/arm/mm/init.c
> @@ -716,6 +716,7 @@ void fix_kernmem_perms(void)
>  int __mark_rodata_ro(void *unused)
>  {
>  	update_sections_early(ro_perms, ARRAY_SIZE(ro_perms));
> +	rodata_test();

We don't do anything with this return value, should we at least
spit out a warning?

>  	return 0;
>  }
>  
> @@ -740,6 +741,11 @@ void set_kernel_text_ro(void)
>  static inline void fix_kernmem_perms(void) { }
>  #endif /* CONFIG_DEBUG_RODATA */
>  
> +#ifdef CONFIG_DEBUG_RODATA_TEST
> +const int rodata_test_data = 0xC3;

This isn't accessed outside of test_rodata.c, it can just
be moved there.

> +EXPORT_SYMBOL_GPL(rodata_test_data);
> +#endif
> +
>  void free_tcmmem(void)
>  {
>  #ifdef CONFIG_HAVE_TCM
> diff --git a/arch/arm/mm/test_rodata.c b/arch/arm/mm/test_rodata.c
> new file mode 100644
> index 0000000..133d092
> --- /dev/null
> +++ b/arch/arm/mm/test_rodata.c
> @@ -0,0 +1,79 @@
> +/*
> + * test_rodata.c: functional test for mark_rodata_ro function
> + *
> + * (C) Copyright 2017 Jinbum Park <jinb.park7@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; version 2
> + * of the License.
> + */
> +#include <asm/cacheflush.h>
> +#include <asm/sections.h>
> +
> +int rodata_test(void)
> +{
> +	unsigned long result;
> +	unsigned long start, end;
> +
> +	/* test 1: read the value */
> +	/* If this test fails, some previous testrun has clobbered the state */
> +
> +	if (!rodata_test_data) {
> +		pr_err("rodata_test: test 1 fails (start data)\n");
> +		return -ENODEV;
> +	}
> +
> +	/* test 2: write to the variable; this should fault */
> +	/*
> +	 * If this test fails, we managed to overwrite the data
> +	 *
> +	 * This is written in assembly to be able to catch the
> +	 * exception that is supposed to happen in the correct
> +	 * case
> +	*/
> +
> +	result = 1;
> +	asm volatile(
> +		"0:	str %[zero], [%[rodata_test]]\n"
> +		"	mov %[rslt], %[zero]\n"
> +		"1:\n"
> +		".pushsection .text.fixup,\"ax\"\n"
> +		".align 2\n"
> +		"2:\n"
> +		"b 1b\n"
> +		".popsection\n"
> +		".pushsection __ex_table,\"a\"\n"
> +		".align 3\n"
> +		".long 0b, 2b\n"
> +		".popsection\n"
> +		: [rslt] "=r" (result)
> +		: [zero] "r" (0UL), [rodata_test] "r" (&rodata_test_data)
> +	);
> +
> +	if (!result) {
> +		pr_err("rodata_test: test data was not read only\n");
> +		return -ENODEV;
> +	}
> +
> +	/* test 3: check the value hasn't changed */
> +	/* If this test fails, we managed to overwrite the data */
> +	if (!rodata_test_data) {
> +		pr_err("rodata_test: Test 3 fails (end data)\n");
> +		return -ENODEV;
> +	}

I'm confused why we are checking this again when we have the result
check above. Is there a case where we would still have result = 1
but rodata_test_data overwritten?

> +
> +	/* test 4: check if the rodata section is 4Kb aligned */
> +	start = (unsigned long)__start_rodata;
> +	end = (unsigned long)__end_rodata;
> +	if (start & (PAGE_SIZE - 1)) {
> +		pr_err("rodata_test: .rodata is not 4k aligned\n");
> +		return -ENODEV;
> +	}
> +	if (end & (PAGE_SIZE - 1)) {
> +		pr_err("rodata_test: .rodata end is not 4k aligned\n");
> +		return -ENODEV;
> +	}

Why does this need to be page aligned (yes, I know why but this
test case does not make it clear)

Better yet, this should be a build time assertion not a runtime
one.

> +
> +	return 0;
> +}
> 

As Mark mentioned, this is possibly redundant with LKDTM. It
would be good to explain what benefit this is bringing in addition
to LKDTM.

Thanks,
Laura

[PATCH] ARM: mm: add testcases for RODATA

[Laura Abbott]

On 01/18/2017 05:53 AM, Jinbum Park wrote:
> This patch adds testcases for the CONFIG_DEBUG_RODATA option.
> It's similar to x86's testcases.
> It tests read-only mapped data and page-size aligned rodata section.
> 
> Signed-off-by: Jinbum Park <jinb.park7@gmail.com>
> ---
>  arch/arm/Kconfig.debug            |  5 +++
>  arch/arm/include/asm/cacheflush.h | 10 +++++
>  arch/arm/mm/Makefile              |  1 +
>  arch/arm/mm/init.c                |  6 +++
>  arch/arm/mm/test_rodata.c         | 80 +++++++++++++++++++++++++++++++++++++++
>  5 files changed, 102 insertions(+)
>  create mode 100644 arch/arm/mm/test_rodata.c
> 
> diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
> index d83f7c3..511e5e1 100644
> --- a/arch/arm/Kconfig.debug
> +++ b/arch/arm/Kconfig.debug
> @@ -1749,6 +1749,11 @@ config DEBUG_SET_MODULE_RONX
>  	  against certain classes of kernel exploits.
>  	  If in doubt, say "N".
>  
> +config DEBUG_RODATA_TEST
> +	bool "Testcase for the marking rodata read-only"
> +	---help---
> +	  This option enables a testcase for the setting rodata read-only.
> +
>  source "drivers/hwtracing/coresight/Kconfig"
>  
>  endmenu
> diff --git a/arch/arm/include/asm/cacheflush.h b/arch/arm/include/asm/cacheflush.h
> index bdd283b..741e2e8 100644
> --- a/arch/arm/include/asm/cacheflush.h
> +++ b/arch/arm/include/asm/cacheflush.h
> @@ -498,6 +498,16 @@ static inline void set_kernel_text_rw(void) { }
>  static inline void set_kernel_text_ro(void) { }
>  #endif
>  
> +#ifdef CONFIG_DEBUG_RODATA_TEST
> +extern const int rodata_test_data;
> +int rodata_test(void);
> +#else
> +static inline int rodata_test(void)
> +{
> +	return 0;
> +}
> +#endif
> +
>  void flush_uprobe_xol_access(struct page *page, unsigned long uaddr,
>  			     void *kaddr, unsigned long len);
>  
> diff --git a/arch/arm/mm/Makefile b/arch/arm/mm/Makefile
> index b3dea80..dbb76ff 100644
> --- a/arch/arm/mm/Makefile
> +++ b/arch/arm/mm/Makefile
> @@ -15,6 +15,7 @@ endif
>  obj-$(CONFIG_ARM_PTDUMP)	+= dump.o
>  obj-$(CONFIG_MODULES)		+= proc-syms.o
>  obj-$(CONFIG_DEBUG_VIRTUAL)	+= physaddr.o
> +obj-$(CONFIG_DEBUG_RODATA_TEST)	+= test_rodata.o
>  
>  obj-$(CONFIG_ALIGNMENT_TRAP)	+= alignment.o
>  obj-$(CONFIG_HIGHMEM)		+= highmem.o
> diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
> index 4127f57..3c15f3b 100644
> --- a/arch/arm/mm/init.c
> +++ b/arch/arm/mm/init.c
> @@ -716,6 +716,7 @@ void fix_kernmem_perms(void)
>  int __mark_rodata_ro(void *unused)
>  {
>  	update_sections_early(ro_perms, ARRAY_SIZE(ro_perms));
> +	rodata_test();

We don't do anything with this return value, should we at least
spit out a warning?

>  	return 0;
>  }
>  
> @@ -740,6 +741,11 @@ void set_kernel_text_ro(void)
>  static inline void fix_kernmem_perms(void) { }
>  #endif /* CONFIG_DEBUG_RODATA */
>  
> +#ifdef CONFIG_DEBUG_RODATA_TEST
> +const int rodata_test_data = 0xC3;

This isn't accessed outside of test_rodata.c, it can just
be moved there.

> +EXPORT_SYMBOL_GPL(rodata_test_data);
> +#endif
> +
>  void free_tcmmem(void)
>  {
>  #ifdef CONFIG_HAVE_TCM
> diff --git a/arch/arm/mm/test_rodata.c b/arch/arm/mm/test_rodata.c
> new file mode 100644
> index 0000000..133d092
> --- /dev/null
> +++ b/arch/arm/mm/test_rodata.c
> @@ -0,0 +1,79 @@
> +/*
> + * test_rodata.c: functional test for mark_rodata_ro function
> + *
> + * (C) Copyright 2017 Jinbum Park <jinb.park7@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; version 2
> + * of the License.
> + */
> +#include <asm/cacheflush.h>
> +#include <asm/sections.h>
> +
> +int rodata_test(void)
> +{
> +	unsigned long result;
> +	unsigned long start, end;
> +
> +	/* test 1: read the value */
> +	/* If this test fails, some previous testrun has clobbered the state */
> +
> +	if (!rodata_test_data) {
> +		pr_err("rodata_test: test 1 fails (start data)\n");
> +		return -ENODEV;
> +	}
> +
> +	/* test 2: write to the variable; this should fault */
> +	/*
> +	 * If this test fails, we managed to overwrite the data
> +	 *
> +	 * This is written in assembly to be able to catch the
> +	 * exception that is supposed to happen in the correct
> +	 * case
> +	*/
> +
> +	result = 1;
> +	asm volatile(
> +		"0:	str %[zero], [%[rodata_test]]\n"
> +		"	mov %[rslt], %[zero]\n"
> +		"1:\n"
> +		".pushsection .text.fixup,\"ax\"\n"
> +		".align 2\n"
> +		"2:\n"
> +		"b 1b\n"
> +		".popsection\n"
> +		".pushsection __ex_table,\"a\"\n"
> +		".align 3\n"
> +		".long 0b, 2b\n"
> +		".popsection\n"
> +		: [rslt] "=r" (result)
> +		: [zero] "r" (0UL), [rodata_test] "r" (&rodata_test_data)
> +	);
> +
> +	if (!result) {
> +		pr_err("rodata_test: test data was not read only\n");
> +		return -ENODEV;
> +	}
> +
> +	/* test 3: check the value hasn't changed */
> +	/* If this test fails, we managed to overwrite the data */
> +	if (!rodata_test_data) {
> +		pr_err("rodata_test: Test 3 fails (end data)\n");
> +		return -ENODEV;
> +	}

I'm confused why we are checking this again when we have the result
check above. Is there a case where we would still have result = 1
but rodata_test_data overwritten?

> +
> +	/* test 4: check if the rodata section is 4Kb aligned */
> +	start = (unsigned long)__start_rodata;
> +	end = (unsigned long)__end_rodata;
> +	if (start & (PAGE_SIZE - 1)) {
> +		pr_err("rodata_test: .rodata is not 4k aligned\n");
> +		return -ENODEV;
> +	}
> +	if (end & (PAGE_SIZE - 1)) {
> +		pr_err("rodata_test: .rodata end is not 4k aligned\n");
> +		return -ENODEV;
> +	}

Why does this need to be page aligned (yes, I know why but this
test case does not make it clear)

Better yet, this should be a build time assertion not a runtime
one.

> +
> +	return 0;
> +}
> 

As Mark mentioned, this is possibly redundant with LKDTM. It
would be good to explain what benefit this is bringing in addition
to LKDTM.

Thanks,
Laura

Re: [PATCH] ARM: mm: add testcases for RODATA

[Laura Abbott]

On 01/18/2017 05:53 AM, Jinbum Park wrote:
> This patch adds testcases for the CONFIG_DEBUG_RODATA option.
> It's similar to x86's testcases.
> It tests read-only mapped data and page-size aligned rodata section.
> 
> Signed-off-by: Jinbum Park <jinb.park7@gmail.com>
> ---
>  arch/arm/Kconfig.debug            |  5 +++
>  arch/arm/include/asm/cacheflush.h | 10 +++++
>  arch/arm/mm/Makefile              |  1 +
>  arch/arm/mm/init.c                |  6 +++
>  arch/arm/mm/test_rodata.c         | 80 +++++++++++++++++++++++++++++++++++++++
>  5 files changed, 102 insertions(+)
>  create mode 100644 arch/arm/mm/test_rodata.c
> 
> diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
> index d83f7c3..511e5e1 100644
> --- a/arch/arm/Kconfig.debug
> +++ b/arch/arm/Kconfig.debug
> @@ -1749,6 +1749,11 @@ config DEBUG_SET_MODULE_RONX
>  	  against certain classes of kernel exploits.
>  	  If in doubt, say "N".
>  
> +config DEBUG_RODATA_TEST
> +	bool "Testcase for the marking rodata read-only"
> +	---help---
> +	  This option enables a testcase for the setting rodata read-only.
> +
>  source "drivers/hwtracing/coresight/Kconfig"
>  
>  endmenu
> diff --git a/arch/arm/include/asm/cacheflush.h b/arch/arm/include/asm/cacheflush.h
> index bdd283b..741e2e8 100644
> --- a/arch/arm/include/asm/cacheflush.h
> +++ b/arch/arm/include/asm/cacheflush.h
> @@ -498,6 +498,16 @@ static inline void set_kernel_text_rw(void) { }
>  static inline void set_kernel_text_ro(void) { }
>  #endif
>  
> +#ifdef CONFIG_DEBUG_RODATA_TEST
> +extern const int rodata_test_data;
> +int rodata_test(void);
> +#else
> +static inline int rodata_test(void)
> +{
> +	return 0;
> +}
> +#endif
> +
>  void flush_uprobe_xol_access(struct page *page, unsigned long uaddr,
>  			     void *kaddr, unsigned long len);
>  
> diff --git a/arch/arm/mm/Makefile b/arch/arm/mm/Makefile
> index b3dea80..dbb76ff 100644
> --- a/arch/arm/mm/Makefile
> +++ b/arch/arm/mm/Makefile
> @@ -15,6 +15,7 @@ endif
>  obj-$(CONFIG_ARM_PTDUMP)	+= dump.o
>  obj-$(CONFIG_MODULES)		+= proc-syms.o
>  obj-$(CONFIG_DEBUG_VIRTUAL)	+= physaddr.o
> +obj-$(CONFIG_DEBUG_RODATA_TEST)	+= test_rodata.o
>  
>  obj-$(CONFIG_ALIGNMENT_TRAP)	+= alignment.o
>  obj-$(CONFIG_HIGHMEM)		+= highmem.o
> diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
> index 4127f57..3c15f3b 100644
> --- a/arch/arm/mm/init.c
> +++ b/arch/arm/mm/init.c
> @@ -716,6 +716,7 @@ void fix_kernmem_perms(void)
>  int __mark_rodata_ro(void *unused)
>  {
>  	update_sections_early(ro_perms, ARRAY_SIZE(ro_perms));
> +	rodata_test();

We don't do anything with this return value, should we at least
spit out a warning?

>  	return 0;
>  }
>  
> @@ -740,6 +741,11 @@ void set_kernel_text_ro(void)
>  static inline void fix_kernmem_perms(void) { }
>  #endif /* CONFIG_DEBUG_RODATA */
>  
> +#ifdef CONFIG_DEBUG_RODATA_TEST
> +const int rodata_test_data = 0xC3;

This isn't accessed outside of test_rodata.c, it can just
be moved there.

> +EXPORT_SYMBOL_GPL(rodata_test_data);
> +#endif
> +
>  void free_tcmmem(void)
>  {
>  #ifdef CONFIG_HAVE_TCM
> diff --git a/arch/arm/mm/test_rodata.c b/arch/arm/mm/test_rodata.c
> new file mode 100644
> index 0000000..133d092
> --- /dev/null
> +++ b/arch/arm/mm/test_rodata.c
> @@ -0,0 +1,79 @@
> +/*
> + * test_rodata.c: functional test for mark_rodata_ro function
> + *
> + * (C) Copyright 2017 Jinbum Park <jinb.park7@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; version 2
> + * of the License.
> + */
> +#include <asm/cacheflush.h>
> +#include <asm/sections.h>
> +
> +int rodata_test(void)
> +{
> +	unsigned long result;
> +	unsigned long start, end;
> +
> +	/* test 1: read the value */
> +	/* If this test fails, some previous testrun has clobbered the state */
> +
> +	if (!rodata_test_data) {
> +		pr_err("rodata_test: test 1 fails (start data)\n");
> +		return -ENODEV;
> +	}
> +
> +	/* test 2: write to the variable; this should fault */
> +	/*
> +	 * If this test fails, we managed to overwrite the data
> +	 *
> +	 * This is written in assembly to be able to catch the
> +	 * exception that is supposed to happen in the correct
> +	 * case
> +	*/
> +
> +	result = 1;
> +	asm volatile(
> +		"0:	str %[zero], [%[rodata_test]]\n"
> +		"	mov %[rslt], %[zero]\n"
> +		"1:\n"
> +		".pushsection .text.fixup,\"ax\"\n"
> +		".align 2\n"
> +		"2:\n"
> +		"b 1b\n"
> +		".popsection\n"
> +		".pushsection __ex_table,\"a\"\n"
> +		".align 3\n"
> +		".long 0b, 2b\n"
> +		".popsection\n"
> +		: [rslt] "=r" (result)
> +		: [zero] "r" (0UL), [rodata_test] "r" (&rodata_test_data)
> +	);
> +
> +	if (!result) {
> +		pr_err("rodata_test: test data was not read only\n");
> +		return -ENODEV;
> +	}
> +
> +	/* test 3: check the value hasn't changed */
> +	/* If this test fails, we managed to overwrite the data */
> +	if (!rodata_test_data) {
> +		pr_err("rodata_test: Test 3 fails (end data)\n");
> +		return -ENODEV;
> +	}

I'm confused why we are checking this again when we have the result
check above. Is there a case where we would still have result = 1
but rodata_test_data overwritten?

> +
> +	/* test 4: check if the rodata section is 4Kb aligned */
> +	start = (unsigned long)__start_rodata;
> +	end = (unsigned long)__end_rodata;
> +	if (start & (PAGE_SIZE - 1)) {
> +		pr_err("rodata_test: .rodata is not 4k aligned\n");
> +		return -ENODEV;
> +	}
> +	if (end & (PAGE_SIZE - 1)) {
> +		pr_err("rodata_test: .rodata end is not 4k aligned\n");
> +		return -ENODEV;
> +	}

Why does this need to be page aligned (yes, I know why but this
test case does not make it clear)

Better yet, this should be a build time assertion not a runtime
one.

> +
> +	return 0;
> +}
> 

As Mark mentioned, this is possibly redundant with LKDTM. It
would be good to explain what benefit this is bringing in addition
to LKDTM.

Thanks,
Laura

[kernel-hardening] Re: [PATCH] ARM: mm: add testcases for RODATA

[Kees Cook]

On Wed, Jan 18, 2017 at 11:20 AM, Laura Abbott <labbott@redhat.com> wrote:
> On 01/18/2017 05:53 AM, Jinbum Park wrote:
>> This patch adds testcases for the CONFIG_DEBUG_RODATA option.
>> It's similar to x86's testcases.
>> It tests read-only mapped data and page-size aligned rodata section.
>>
>> Signed-off-by: Jinbum Park <jinb.park7@gmail.com>
>> ---
>>  arch/arm/Kconfig.debug            |  5 +++
>>  arch/arm/include/asm/cacheflush.h | 10 +++++
>>  arch/arm/mm/Makefile              |  1 +
>>  arch/arm/mm/init.c                |  6 +++
>>  arch/arm/mm/test_rodata.c         | 80 +++++++++++++++++++++++++++++++++++++++
>>  5 files changed, 102 insertions(+)
>>  create mode 100644 arch/arm/mm/test_rodata.c
>>
>> diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
>> index d83f7c3..511e5e1 100644
>> --- a/arch/arm/Kconfig.debug
>> +++ b/arch/arm/Kconfig.debug
>> @@ -1749,6 +1749,11 @@ config DEBUG_SET_MODULE_RONX
>>         against certain classes of kernel exploits.
>>         If in doubt, say "N".
>>
>> +config DEBUG_RODATA_TEST
>> +     bool "Testcase for the marking rodata read-only"
>> +     ---help---
>> +       This option enables a testcase for the setting rodata read-only.
>> +
>>  source "drivers/hwtracing/coresight/Kconfig"
>>
>>  endmenu
>> diff --git a/arch/arm/include/asm/cacheflush.h b/arch/arm/include/asm/cacheflush.h
>> index bdd283b..741e2e8 100644
>> --- a/arch/arm/include/asm/cacheflush.h
>> +++ b/arch/arm/include/asm/cacheflush.h
>> @@ -498,6 +498,16 @@ static inline void set_kernel_text_rw(void) { }
>>  static inline void set_kernel_text_ro(void) { }
>>  #endif
>>
>> +#ifdef CONFIG_DEBUG_RODATA_TEST
>> +extern const int rodata_test_data;
>> +int rodata_test(void);
>> +#else
>> +static inline int rodata_test(void)
>> +{
>> +     return 0;
>> +}
>> +#endif
>> +
>>  void flush_uprobe_xol_access(struct page *page, unsigned long uaddr,
>>                            void *kaddr, unsigned long len);
>>
>> diff --git a/arch/arm/mm/Makefile b/arch/arm/mm/Makefile
>> index b3dea80..dbb76ff 100644
>> --- a/arch/arm/mm/Makefile
>> +++ b/arch/arm/mm/Makefile
>> @@ -15,6 +15,7 @@ endif
>>  obj-$(CONFIG_ARM_PTDUMP)     += dump.o
>>  obj-$(CONFIG_MODULES)                += proc-syms.o
>>  obj-$(CONFIG_DEBUG_VIRTUAL)  += physaddr.o
>> +obj-$(CONFIG_DEBUG_RODATA_TEST)      += test_rodata.o
>>
>>  obj-$(CONFIG_ALIGNMENT_TRAP) += alignment.o
>>  obj-$(CONFIG_HIGHMEM)                += highmem.o
>> diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
>> index 4127f57..3c15f3b 100644
>> --- a/arch/arm/mm/init.c
>> +++ b/arch/arm/mm/init.c
>> @@ -716,6 +716,7 @@ void fix_kernmem_perms(void)
>>  int __mark_rodata_ro(void *unused)
>>  {
>>       update_sections_early(ro_perms, ARRAY_SIZE(ro_perms));
>> +     rodata_test();
>
> We don't do anything with this return value, should we at least
> spit out a warning?
>
>>       return 0;
>>  }
>>
>> @@ -740,6 +741,11 @@ void set_kernel_text_ro(void)
>>  static inline void fix_kernmem_perms(void) { }
>>  #endif /* CONFIG_DEBUG_RODATA */
>>
>> +#ifdef CONFIG_DEBUG_RODATA_TEST
>> +const int rodata_test_data = 0xC3;
>
> This isn't accessed outside of test_rodata.c, it can just
> be moved there.
>
>> +EXPORT_SYMBOL_GPL(rodata_test_data);
>> +#endif
>> +
>>  void free_tcmmem(void)
>>  {
>>  #ifdef CONFIG_HAVE_TCM
>> diff --git a/arch/arm/mm/test_rodata.c b/arch/arm/mm/test_rodata.c
>> new file mode 100644
>> index 0000000..133d092
>> --- /dev/null
>> +++ b/arch/arm/mm/test_rodata.c
>> @@ -0,0 +1,79 @@
>> +/*
>> + * test_rodata.c: functional test for mark_rodata_ro function
>> + *
>> + * (C) Copyright 2017 Jinbum Park <jinb.park7@gmail.com>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; version 2
>> + * of the License.
>> + */
>> +#include <asm/cacheflush.h>
>> +#include <asm/sections.h>
>> +
>> +int rodata_test(void)
>> +{
>> +     unsigned long result;
>> +     unsigned long start, end;
>> +
>> +     /* test 1: read the value */
>> +     /* If this test fails, some previous testrun has clobbered the state */
>> +
>> +     if (!rodata_test_data) {
>> +             pr_err("rodata_test: test 1 fails (start data)\n");
>> +             return -ENODEV;
>> +     }
>> +
>> +     /* test 2: write to the variable; this should fault */
>> +     /*
>> +      * If this test fails, we managed to overwrite the data
>> +      *
>> +      * This is written in assembly to be able to catch the
>> +      * exception that is supposed to happen in the correct
>> +      * case
>> +     */
>> +
>> +     result = 1;
>> +     asm volatile(
>> +             "0:     str %[zero], [%[rodata_test]]\n"
>> +             "       mov %[rslt], %[zero]\n"
>> +             "1:\n"
>> +             ".pushsection .text.fixup,\"ax\"\n"
>> +             ".align 2\n"
>> +             "2:\n"
>> +             "b 1b\n"
>> +             ".popsection\n"
>> +             ".pushsection __ex_table,\"a\"\n"
>> +             ".align 3\n"
>> +             ".long 0b, 2b\n"
>> +             ".popsection\n"
>> +             : [rslt] "=r" (result)
>> +             : [zero] "r" (0UL), [rodata_test] "r" (&rodata_test_data)
>> +     );
>> +
>> +     if (!result) {
>> +             pr_err("rodata_test: test data was not read only\n");
>> +             return -ENODEV;
>> +     }
>> +
>> +     /* test 3: check the value hasn't changed */
>> +     /* If this test fails, we managed to overwrite the data */
>> +     if (!rodata_test_data) {
>> +             pr_err("rodata_test: Test 3 fails (end data)\n");
>> +             return -ENODEV;
>> +     }
>
> I'm confused why we are checking this again when we have the result
> check above. Is there a case where we would still have result = 1
> but rodata_test_data overwritten?
>
>> +
>> +     /* test 4: check if the rodata section is 4Kb aligned */
>> +     start = (unsigned long)__start_rodata;
>> +     end = (unsigned long)__end_rodata;
>> +     if (start & (PAGE_SIZE - 1)) {
>> +             pr_err("rodata_test: .rodata is not 4k aligned\n");
>> +             return -ENODEV;
>> +     }
>> +     if (end & (PAGE_SIZE - 1)) {
>> +             pr_err("rodata_test: .rodata end is not 4k aligned\n");
>> +             return -ENODEV;
>> +     }
>
> Why does this need to be page aligned (yes, I know why but this
> test case does not make it clear)
>
> Better yet, this should be a build time assertion not a runtime
> one.
>
>> +
>> +     return 0;
>> +}
>>
>
> As Mark mentioned, this is possibly redundant with LKDTM. It
> would be good to explain what benefit this is bringing in addition
> to LKDTM.

The only thing I could think of would be to make failures block the
boot. (Though that would mean changing x86 too.) That's kind of like
the W^X test, which spits out a BUG IIRC.

-Kees

-- 
Kees Cook
Nexus Security

Re: [PATCH] ARM: mm: add testcases for RODATA

[Kees Cook]

On Wed, Jan 18, 2017 at 11:20 AM, Laura Abbott <labbott@redhat.com> wrote:
> On 01/18/2017 05:53 AM, Jinbum Park wrote:
>> This patch adds testcases for the CONFIG_DEBUG_RODATA option.
>> It's similar to x86's testcases.
>> It tests read-only mapped data and page-size aligned rodata section.
>>
>> Signed-off-by: Jinbum Park <jinb.park7@gmail.com>
>> ---
>>  arch/arm/Kconfig.debug            |  5 +++
>>  arch/arm/include/asm/cacheflush.h | 10 +++++
>>  arch/arm/mm/Makefile              |  1 +
>>  arch/arm/mm/init.c                |  6 +++
>>  arch/arm/mm/test_rodata.c         | 80 +++++++++++++++++++++++++++++++++++++++
>>  5 files changed, 102 insertions(+)
>>  create mode 100644 arch/arm/mm/test_rodata.c
>>
>> diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
>> index d83f7c3..511e5e1 100644
>> --- a/arch/arm/Kconfig.debug
>> +++ b/arch/arm/Kconfig.debug
>> @@ -1749,6 +1749,11 @@ config DEBUG_SET_MODULE_RONX
>>         against certain classes of kernel exploits.
>>         If in doubt, say "N".
>>
>> +config DEBUG_RODATA_TEST
>> +     bool "Testcase for the marking rodata read-only"
>> +     ---help---
>> +       This option enables a testcase for the setting rodata read-only.
>> +
>>  source "drivers/hwtracing/coresight/Kconfig"
>>
>>  endmenu
>> diff --git a/arch/arm/include/asm/cacheflush.h b/arch/arm/include/asm/cacheflush.h
>> index bdd283b..741e2e8 100644
>> --- a/arch/arm/include/asm/cacheflush.h
>> +++ b/arch/arm/include/asm/cacheflush.h
>> @@ -498,6 +498,16 @@ static inline void set_kernel_text_rw(void) { }
>>  static inline void set_kernel_text_ro(void) { }
>>  #endif
>>
>> +#ifdef CONFIG_DEBUG_RODATA_TEST
>> +extern const int rodata_test_data;
>> +int rodata_test(void);
>> +#else
>> +static inline int rodata_test(void)
>> +{
>> +     return 0;
>> +}
>> +#endif
>> +
>>  void flush_uprobe_xol_access(struct page *page, unsigned long uaddr,
>>                            void *kaddr, unsigned long len);
>>
>> diff --git a/arch/arm/mm/Makefile b/arch/arm/mm/Makefile
>> index b3dea80..dbb76ff 100644
>> --- a/arch/arm/mm/Makefile
>> +++ b/arch/arm/mm/Makefile
>> @@ -15,6 +15,7 @@ endif
>>  obj-$(CONFIG_ARM_PTDUMP)     += dump.o
>>  obj-$(CONFIG_MODULES)                += proc-syms.o
>>  obj-$(CONFIG_DEBUG_VIRTUAL)  += physaddr.o
>> +obj-$(CONFIG_DEBUG_RODATA_TEST)      += test_rodata.o
>>
>>  obj-$(CONFIG_ALIGNMENT_TRAP) += alignment.o
>>  obj-$(CONFIG_HIGHMEM)                += highmem.o
>> diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
>> index 4127f57..3c15f3b 100644
>> --- a/arch/arm/mm/init.c
>> +++ b/arch/arm/mm/init.c
>> @@ -716,6 +716,7 @@ void fix_kernmem_perms(void)
>>  int __mark_rodata_ro(void *unused)
>>  {
>>       update_sections_early(ro_perms, ARRAY_SIZE(ro_perms));
>> +     rodata_test();
>
> We don't do anything with this return value, should we at least
> spit out a warning?
>
>>       return 0;
>>  }
>>
>> @@ -740,6 +741,11 @@ void set_kernel_text_ro(void)
>>  static inline void fix_kernmem_perms(void) { }
>>  #endif /* CONFIG_DEBUG_RODATA */
>>
>> +#ifdef CONFIG_DEBUG_RODATA_TEST
>> +const int rodata_test_data = 0xC3;
>
> This isn't accessed outside of test_rodata.c, it can just
> be moved there.
>
>> +EXPORT_SYMBOL_GPL(rodata_test_data);
>> +#endif
>> +
>>  void free_tcmmem(void)
>>  {
>>  #ifdef CONFIG_HAVE_TCM
>> diff --git a/arch/arm/mm/test_rodata.c b/arch/arm/mm/test_rodata.c
>> new file mode 100644
>> index 0000000..133d092
>> --- /dev/null
>> +++ b/arch/arm/mm/test_rodata.c
>> @@ -0,0 +1,79 @@
>> +/*
>> + * test_rodata.c: functional test for mark_rodata_ro function
>> + *
>> + * (C) Copyright 2017 Jinbum Park <jinb.park7@gmail.com>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; version 2
>> + * of the License.
>> + */
>> +#include <asm/cacheflush.h>
>> +#include <asm/sections.h>
>> +
>> +int rodata_test(void)
>> +{
>> +     unsigned long result;
>> +     unsigned long start, end;
>> +
>> +     /* test 1: read the value */
>> +     /* If this test fails, some previous testrun has clobbered the state */
>> +
>> +     if (!rodata_test_data) {
>> +             pr_err("rodata_test: test 1 fails (start data)\n");
>> +             return -ENODEV;
>> +     }
>> +
>> +     /* test 2: write to the variable; this should fault */
>> +     /*
>> +      * If this test fails, we managed to overwrite the data
>> +      *
>> +      * This is written in assembly to be able to catch the
>> +      * exception that is supposed to happen in the correct
>> +      * case
>> +     */
>> +
>> +     result = 1;
>> +     asm volatile(
>> +             "0:     str %[zero], [%[rodata_test]]\n"
>> +             "       mov %[rslt], %[zero]\n"
>> +             "1:\n"
>> +             ".pushsection .text.fixup,\"ax\"\n"
>> +             ".align 2\n"
>> +             "2:\n"
>> +             "b 1b\n"
>> +             ".popsection\n"
>> +             ".pushsection __ex_table,\"a\"\n"
>> +             ".align 3\n"
>> +             ".long 0b, 2b\n"
>> +             ".popsection\n"
>> +             : [rslt] "=r" (result)
>> +             : [zero] "r" (0UL), [rodata_test] "r" (&rodata_test_data)
>> +     );
>> +
>> +     if (!result) {
>> +             pr_err("rodata_test: test data was not read only\n");
>> +             return -ENODEV;
>> +     }
>> +
>> +     /* test 3: check the value hasn't changed */
>> +     /* If this test fails, we managed to overwrite the data */
>> +     if (!rodata_test_data) {
>> +             pr_err("rodata_test: Test 3 fails (end data)\n");
>> +             return -ENODEV;
>> +     }
>
> I'm confused why we are checking this again when we have the result
> check above. Is there a case where we would still have result = 1
> but rodata_test_data overwritten?
>
>> +
>> +     /* test 4: check if the rodata section is 4Kb aligned */
>> +     start = (unsigned long)__start_rodata;
>> +     end = (unsigned long)__end_rodata;
>> +     if (start & (PAGE_SIZE - 1)) {
>> +             pr_err("rodata_test: .rodata is not 4k aligned\n");
>> +             return -ENODEV;
>> +     }
>> +     if (end & (PAGE_SIZE - 1)) {
>> +             pr_err("rodata_test: .rodata end is not 4k aligned\n");
>> +             return -ENODEV;
>> +     }
>
> Why does this need to be page aligned (yes, I know why but this
> test case does not make it clear)
>
> Better yet, this should be a build time assertion not a runtime
> one.
>
>> +
>> +     return 0;
>> +}
>>
>
> As Mark mentioned, this is possibly redundant with LKDTM. It
> would be good to explain what benefit this is bringing in addition
> to LKDTM.

The only thing I could think of would be to make failures block the
boot. (Though that would mean changing x86 too.) That's kind of like
the W^X test, which spits out a BUG IIRC.

-Kees

-- 
Kees Cook
Nexus Security

[PATCH] ARM: mm: add testcases for RODATA

[Kees Cook]

On Wed, Jan 18, 2017 at 11:20 AM, Laura Abbott <labbott@redhat.com> wrote:
> On 01/18/2017 05:53 AM, Jinbum Park wrote:
>> This patch adds testcases for the CONFIG_DEBUG_RODATA option.
>> It's similar to x86's testcases.
>> It tests read-only mapped data and page-size aligned rodata section.
>>
>> Signed-off-by: Jinbum Park <jinb.park7@gmail.com>
>> ---
>>  arch/arm/Kconfig.debug            |  5 +++
>>  arch/arm/include/asm/cacheflush.h | 10 +++++
>>  arch/arm/mm/Makefile              |  1 +
>>  arch/arm/mm/init.c                |  6 +++
>>  arch/arm/mm/test_rodata.c         | 80 +++++++++++++++++++++++++++++++++++++++
>>  5 files changed, 102 insertions(+)
>>  create mode 100644 arch/arm/mm/test_rodata.c
>>
>> diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
>> index d83f7c3..511e5e1 100644
>> --- a/arch/arm/Kconfig.debug
>> +++ b/arch/arm/Kconfig.debug
>> @@ -1749,6 +1749,11 @@ config DEBUG_SET_MODULE_RONX
>>         against certain classes of kernel exploits.
>>         If in doubt, say "N".
>>
>> +config DEBUG_RODATA_TEST
>> +     bool "Testcase for the marking rodata read-only"
>> +     ---help---
>> +       This option enables a testcase for the setting rodata read-only.
>> +
>>  source "drivers/hwtracing/coresight/Kconfig"
>>
>>  endmenu
>> diff --git a/arch/arm/include/asm/cacheflush.h b/arch/arm/include/asm/cacheflush.h
>> index bdd283b..741e2e8 100644
>> --- a/arch/arm/include/asm/cacheflush.h
>> +++ b/arch/arm/include/asm/cacheflush.h
>> @@ -498,6 +498,16 @@ static inline void set_kernel_text_rw(void) { }
>>  static inline void set_kernel_text_ro(void) { }
>>  #endif
>>
>> +#ifdef CONFIG_DEBUG_RODATA_TEST
>> +extern const int rodata_test_data;
>> +int rodata_test(void);
>> +#else
>> +static inline int rodata_test(void)
>> +{
>> +     return 0;
>> +}
>> +#endif
>> +
>>  void flush_uprobe_xol_access(struct page *page, unsigned long uaddr,
>>                            void *kaddr, unsigned long len);
>>
>> diff --git a/arch/arm/mm/Makefile b/arch/arm/mm/Makefile
>> index b3dea80..dbb76ff 100644
>> --- a/arch/arm/mm/Makefile
>> +++ b/arch/arm/mm/Makefile
>> @@ -15,6 +15,7 @@ endif
>>  obj-$(CONFIG_ARM_PTDUMP)     += dump.o
>>  obj-$(CONFIG_MODULES)                += proc-syms.o
>>  obj-$(CONFIG_DEBUG_VIRTUAL)  += physaddr.o
>> +obj-$(CONFIG_DEBUG_RODATA_TEST)      += test_rodata.o
>>
>>  obj-$(CONFIG_ALIGNMENT_TRAP) += alignment.o
>>  obj-$(CONFIG_HIGHMEM)                += highmem.o
>> diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
>> index 4127f57..3c15f3b 100644
>> --- a/arch/arm/mm/init.c
>> +++ b/arch/arm/mm/init.c
>> @@ -716,6 +716,7 @@ void fix_kernmem_perms(void)
>>  int __mark_rodata_ro(void *unused)
>>  {
>>       update_sections_early(ro_perms, ARRAY_SIZE(ro_perms));
>> +     rodata_test();
>
> We don't do anything with this return value, should we at least
> spit out a warning?
>
>>       return 0;
>>  }
>>
>> @@ -740,6 +741,11 @@ void set_kernel_text_ro(void)
>>  static inline void fix_kernmem_perms(void) { }
>>  #endif /* CONFIG_DEBUG_RODATA */
>>
>> +#ifdef CONFIG_DEBUG_RODATA_TEST
>> +const int rodata_test_data = 0xC3;
>
> This isn't accessed outside of test_rodata.c, it can just
> be moved there.
>
>> +EXPORT_SYMBOL_GPL(rodata_test_data);
>> +#endif
>> +
>>  void free_tcmmem(void)
>>  {
>>  #ifdef CONFIG_HAVE_TCM
>> diff --git a/arch/arm/mm/test_rodata.c b/arch/arm/mm/test_rodata.c
>> new file mode 100644
>> index 0000000..133d092
>> --- /dev/null
>> +++ b/arch/arm/mm/test_rodata.c
>> @@ -0,0 +1,79 @@
>> +/*
>> + * test_rodata.c: functional test for mark_rodata_ro function
>> + *
>> + * (C) Copyright 2017 Jinbum Park <jinb.park7@gmail.com>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; version 2
>> + * of the License.
>> + */
>> +#include <asm/cacheflush.h>
>> +#include <asm/sections.h>
>> +
>> +int rodata_test(void)
>> +{
>> +     unsigned long result;
>> +     unsigned long start, end;
>> +
>> +     /* test 1: read the value */
>> +     /* If this test fails, some previous testrun has clobbered the state */
>> +
>> +     if (!rodata_test_data) {
>> +             pr_err("rodata_test: test 1 fails (start data)\n");
>> +             return -ENODEV;
>> +     }
>> +
>> +     /* test 2: write to the variable; this should fault */
>> +     /*
>> +      * If this test fails, we managed to overwrite the data
>> +      *
>> +      * This is written in assembly to be able to catch the
>> +      * exception that is supposed to happen in the correct
>> +      * case
>> +     */
>> +
>> +     result = 1;
>> +     asm volatile(
>> +             "0:     str %[zero], [%[rodata_test]]\n"
>> +             "       mov %[rslt], %[zero]\n"
>> +             "1:\n"
>> +             ".pushsection .text.fixup,\"ax\"\n"
>> +             ".align 2\n"
>> +             "2:\n"
>> +             "b 1b\n"
>> +             ".popsection\n"
>> +             ".pushsection __ex_table,\"a\"\n"
>> +             ".align 3\n"
>> +             ".long 0b, 2b\n"
>> +             ".popsection\n"
>> +             : [rslt] "=r" (result)
>> +             : [zero] "r" (0UL), [rodata_test] "r" (&rodata_test_data)
>> +     );
>> +
>> +     if (!result) {
>> +             pr_err("rodata_test: test data was not read only\n");
>> +             return -ENODEV;
>> +     }
>> +
>> +     /* test 3: check the value hasn't changed */
>> +     /* If this test fails, we managed to overwrite the data */
>> +     if (!rodata_test_data) {
>> +             pr_err("rodata_test: Test 3 fails (end data)\n");
>> +             return -ENODEV;
>> +     }
>
> I'm confused why we are checking this again when we have the result
> check above. Is there a case where we would still have result = 1
> but rodata_test_data overwritten?
>
>> +
>> +     /* test 4: check if the rodata section is 4Kb aligned */
>> +     start = (unsigned long)__start_rodata;
>> +     end = (unsigned long)__end_rodata;
>> +     if (start & (PAGE_SIZE - 1)) {
>> +             pr_err("rodata_test: .rodata is not 4k aligned\n");
>> +             return -ENODEV;
>> +     }
>> +     if (end & (PAGE_SIZE - 1)) {
>> +             pr_err("rodata_test: .rodata end is not 4k aligned\n");
>> +             return -ENODEV;
>> +     }
>
> Why does this need to be page aligned (yes, I know why but this
> test case does not make it clear)
>
> Better yet, this should be a build time assertion not a runtime
> one.
>
>> +
>> +     return 0;
>> +}
>>
>
> As Mark mentioned, this is possibly redundant with LKDTM. It
> would be good to explain what benefit this is bringing in addition
> to LKDTM.

The only thing I could think of would be to make failures block the
boot. (Though that would mean changing x86 too.) That's kind of like
the W^X test, which spits out a BUG IIRC.

-Kees

-- 
Kees Cook
Nexus Security

Re: [PATCH] ARM: mm: add testcases for RODATA

[Kees Cook]

On Wed, Jan 18, 2017 at 11:20 AM, Laura Abbott <labbott@redhat.com> wrote:
> On 01/18/2017 05:53 AM, Jinbum Park wrote:
>> This patch adds testcases for the CONFIG_DEBUG_RODATA option.
>> It's similar to x86's testcases.
>> It tests read-only mapped data and page-size aligned rodata section.
>>
>> Signed-off-by: Jinbum Park <jinb.park7@gmail.com>
>> ---
>>  arch/arm/Kconfig.debug            |  5 +++
>>  arch/arm/include/asm/cacheflush.h | 10 +++++
>>  arch/arm/mm/Makefile              |  1 +
>>  arch/arm/mm/init.c                |  6 +++
>>  arch/arm/mm/test_rodata.c         | 80 +++++++++++++++++++++++++++++++++++++++
>>  5 files changed, 102 insertions(+)
>>  create mode 100644 arch/arm/mm/test_rodata.c
>>
>> diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
>> index d83f7c3..511e5e1 100644
>> --- a/arch/arm/Kconfig.debug
>> +++ b/arch/arm/Kconfig.debug
>> @@ -1749,6 +1749,11 @@ config DEBUG_SET_MODULE_RONX
>>         against certain classes of kernel exploits.
>>         If in doubt, say "N".
>>
>> +config DEBUG_RODATA_TEST
>> +     bool "Testcase for the marking rodata read-only"
>> +     ---help---
>> +       This option enables a testcase for the setting rodata read-only.
>> +
>>  source "drivers/hwtracing/coresight/Kconfig"
>>
>>  endmenu
>> diff --git a/arch/arm/include/asm/cacheflush.h b/arch/arm/include/asm/cacheflush.h
>> index bdd283b..741e2e8 100644
>> --- a/arch/arm/include/asm/cacheflush.h
>> +++ b/arch/arm/include/asm/cacheflush.h
>> @@ -498,6 +498,16 @@ static inline void set_kernel_text_rw(void) { }
>>  static inline void set_kernel_text_ro(void) { }
>>  #endif
>>
>> +#ifdef CONFIG_DEBUG_RODATA_TEST
>> +extern const int rodata_test_data;
>> +int rodata_test(void);
>> +#else
>> +static inline int rodata_test(void)
>> +{
>> +     return 0;
>> +}
>> +#endif
>> +
>>  void flush_uprobe_xol_access(struct page *page, unsigned long uaddr,
>>                            void *kaddr, unsigned long len);
>>
>> diff --git a/arch/arm/mm/Makefile b/arch/arm/mm/Makefile
>> index b3dea80..dbb76ff 100644
>> --- a/arch/arm/mm/Makefile
>> +++ b/arch/arm/mm/Makefile
>> @@ -15,6 +15,7 @@ endif
>>  obj-$(CONFIG_ARM_PTDUMP)     += dump.o
>>  obj-$(CONFIG_MODULES)                += proc-syms.o
>>  obj-$(CONFIG_DEBUG_VIRTUAL)  += physaddr.o
>> +obj-$(CONFIG_DEBUG_RODATA_TEST)      += test_rodata.o
>>
>>  obj-$(CONFIG_ALIGNMENT_TRAP) += alignment.o
>>  obj-$(CONFIG_HIGHMEM)                += highmem.o
>> diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
>> index 4127f57..3c15f3b 100644
>> --- a/arch/arm/mm/init.c
>> +++ b/arch/arm/mm/init.c
>> @@ -716,6 +716,7 @@ void fix_kernmem_perms(void)
>>  int __mark_rodata_ro(void *unused)
>>  {
>>       update_sections_early(ro_perms, ARRAY_SIZE(ro_perms));
>> +     rodata_test();
>
> We don't do anything with this return value, should we at least
> spit out a warning?
>
>>       return 0;
>>  }
>>
>> @@ -740,6 +741,11 @@ void set_kernel_text_ro(void)
>>  static inline void fix_kernmem_perms(void) { }
>>  #endif /* CONFIG_DEBUG_RODATA */
>>
>> +#ifdef CONFIG_DEBUG_RODATA_TEST
>> +const int rodata_test_data = 0xC3;
>
> This isn't accessed outside of test_rodata.c, it can just
> be moved there.
>
>> +EXPORT_SYMBOL_GPL(rodata_test_data);
>> +#endif
>> +
>>  void free_tcmmem(void)
>>  {
>>  #ifdef CONFIG_HAVE_TCM
>> diff --git a/arch/arm/mm/test_rodata.c b/arch/arm/mm/test_rodata.c
>> new file mode 100644
>> index 0000000..133d092
>> --- /dev/null
>> +++ b/arch/arm/mm/test_rodata.c
>> @@ -0,0 +1,79 @@
>> +/*
>> + * test_rodata.c: functional test for mark_rodata_ro function
>> + *
>> + * (C) Copyright 2017 Jinbum Park <jinb.park7@gmail.com>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; version 2
>> + * of the License.
>> + */
>> +#include <asm/cacheflush.h>
>> +#include <asm/sections.h>
>> +
>> +int rodata_test(void)
>> +{
>> +     unsigned long result;
>> +     unsigned long start, end;
>> +
>> +     /* test 1: read the value */
>> +     /* If this test fails, some previous testrun has clobbered the state */
>> +
>> +     if (!rodata_test_data) {
>> +             pr_err("rodata_test: test 1 fails (start data)\n");
>> +             return -ENODEV;
>> +     }
>> +
>> +     /* test 2: write to the variable; this should fault */
>> +     /*
>> +      * If this test fails, we managed to overwrite the data
>> +      *
>> +      * This is written in assembly to be able to catch the
>> +      * exception that is supposed to happen in the correct
>> +      * case
>> +     */
>> +
>> +     result = 1;
>> +     asm volatile(
>> +             "0:     str %[zero], [%[rodata_test]]\n"
>> +             "       mov %[rslt], %[zero]\n"
>> +             "1:\n"
>> +             ".pushsection .text.fixup,\"ax\"\n"
>> +             ".align 2\n"
>> +             "2:\n"
>> +             "b 1b\n"
>> +             ".popsection\n"
>> +             ".pushsection __ex_table,\"a\"\n"
>> +             ".align 3\n"
>> +             ".long 0b, 2b\n"
>> +             ".popsection\n"
>> +             : [rslt] "=r" (result)
>> +             : [zero] "r" (0UL), [rodata_test] "r" (&rodata_test_data)
>> +     );
>> +
>> +     if (!result) {
>> +             pr_err("rodata_test: test data was not read only\n");
>> +             return -ENODEV;
>> +     }
>> +
>> +     /* test 3: check the value hasn't changed */
>> +     /* If this test fails, we managed to overwrite the data */
>> +     if (!rodata_test_data) {
>> +             pr_err("rodata_test: Test 3 fails (end data)\n");
>> +             return -ENODEV;
>> +     }
>
> I'm confused why we are checking this again when we have the result
> check above. Is there a case where we would still have result = 1
> but rodata_test_data overwritten?
>
>> +
>> +     /* test 4: check if the rodata section is 4Kb aligned */
>> +     start = (unsigned long)__start_rodata;
>> +     end = (unsigned long)__end_rodata;
>> +     if (start & (PAGE_SIZE - 1)) {
>> +             pr_err("rodata_test: .rodata is not 4k aligned\n");
>> +             return -ENODEV;
>> +     }
>> +     if (end & (PAGE_SIZE - 1)) {
>> +             pr_err("rodata_test: .rodata end is not 4k aligned\n");
>> +             return -ENODEV;
>> +     }
>
> Why does this need to be page aligned (yes, I know why but this
> test case does not make it clear)
>
> Better yet, this should be a build time assertion not a runtime
> one.
>
>> +
>> +     return 0;
>> +}
>>
>
> As Mark mentioned, this is possibly redundant with LKDTM. It
> would be good to explain what benefit this is bringing in addition
> to LKDTM.

The only thing I could think of would be to make failures block the
boot. (Though that would mean changing x86 too.) That's kind of like
the W^X test, which spits out a BUG IIRC.

-Kees

-- 
Kees Cook
Nexus Security

[kernel-hardening] Re: [PATCH] ARM: mm: add testcases for RODATA

[Russell King - ARM Linux]

On Wed, Jan 18, 2017 at 11:20:54AM -0800, Laura Abbott wrote:
> On 01/18/2017 05:53 AM, Jinbum Park wrote:
> > diff --git a/arch/arm/include/asm/cacheflush.h b/arch/arm/include/asm/cacheflush.h
> > index bdd283b..741e2e8 100644
> > --- a/arch/arm/include/asm/cacheflush.h
> > +++ b/arch/arm/include/asm/cacheflush.h
> > @@ -498,6 +498,16 @@ static inline void set_kernel_text_rw(void) { }
> >  static inline void set_kernel_text_ro(void) { }
> >  #endif
> >  
> > +#ifdef CONFIG_DEBUG_RODATA_TEST
> > +extern const int rodata_test_data;
> > +int rodata_test(void);
> > +#else
> > +static inline int rodata_test(void)
> > +{
> > +	return 0;
> > +}
> > +#endif
> > +

I don't see why this needs to be in cacheflush.h - it doesn't seem to
have anything to do with cache flushing, and placing it in here means
that if you change the state of CONFIG_DEBUG_RODATA_TEST, most likely
the entire kernel gets rebuilt.  Please put it in a separate header
file.

> > diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
> > index 4127f57..3c15f3b 100644
> > --- a/arch/arm/mm/init.c
> > +++ b/arch/arm/mm/init.c
> > @@ -716,6 +716,7 @@ void fix_kernmem_perms(void)
> >  int __mark_rodata_ro(void *unused)
> >  {
> >  	update_sections_early(ro_perms, ARRAY_SIZE(ro_perms));
> > +	rodata_test();
> 
> We don't do anything with this return value, should we at least
> spit out a warning?
> 
> >  	return 0;
> >  }
> >  
> > @@ -740,6 +741,11 @@ void set_kernel_text_ro(void)
> >  static inline void fix_kernmem_perms(void) { }
> >  #endif /* CONFIG_DEBUG_RODATA */
> >  
> > +#ifdef CONFIG_DEBUG_RODATA_TEST
> > +const int rodata_test_data = 0xC3;
> 
> This isn't accessed outside of test_rodata.c, it can just
> be moved there.

I think the intention was to place it in some .c file which gets built
into the kernel, rather than a module, so testing whether read-only
data in the kernel image is read-only.

If it's placed in a module, then you're only checking that read-only
data in the module is read-only (which is another test which should
be done!)

In any case, placing it in arch/arm/mm/init.c seems unnecessary, I'd
rather it went in its own separate file.

> > diff --git a/arch/arm/mm/test_rodata.c b/arch/arm/mm/test_rodata.c
> > new file mode 100644
> > index 0000000..133d092
> > --- /dev/null
> > +++ b/arch/arm/mm/test_rodata.c
> > @@ -0,0 +1,79 @@
> > +/*
> > + * test_rodata.c: functional test for mark_rodata_ro function
> > + *
> > + * (C) Copyright 2017 Jinbum Park <jinb.park7@gmail.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; version 2
> > + * of the License.
> > + */
> > +#include <asm/cacheflush.h>
> > +#include <asm/sections.h>
> > +
> > +int rodata_test(void)
> > +{
> > +	unsigned long result;
> > +	unsigned long start, end;
> > +
> > +	/* test 1: read the value */
> > +	/* If this test fails, some previous testrun has clobbered the state */
> > +
> > +	if (!rodata_test_data) {
> > +		pr_err("rodata_test: test 1 fails (start data)\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	/* test 2: write to the variable; this should fault */
> > +	/*
> > +	 * If this test fails, we managed to overwrite the data
> > +	 *
> > +	 * This is written in assembly to be able to catch the
> > +	 * exception that is supposed to happen in the correct
> > +	 * case
> > +	*/
> > +
> > +	result = 1;
> > +	asm volatile(
> > +		"0:	str %[zero], [%[rodata_test]]\n"
> > +		"	mov %[rslt], %[zero]\n"
> > +		"1:\n"
> > +		".pushsection .text.fixup,\"ax\"\n"
> > +		".align 2\n"
> > +		"2:\n"
> > +		"b 1b\n"
> > +		".popsection\n"
> > +		".pushsection __ex_table,\"a\"\n"
> > +		".align 3\n"
> > +		".long 0b, 2b\n"
> > +		".popsection\n"
> > +		: [rslt] "=r" (result)
> > +		: [zero] "r" (0UL), [rodata_test] "r" (&rodata_test_data)
> > +	);
> > +
> > +	if (!result) {
> > +		pr_err("rodata_test: test data was not read only\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	/* test 3: check the value hasn't changed */
> > +	/* If this test fails, we managed to overwrite the data */
> > +	if (!rodata_test_data) {
> > +		pr_err("rodata_test: Test 3 fails (end data)\n");
> > +		return -ENODEV;
> > +	}
> 
> I'm confused why we are checking this again when we have the result
> check above. Is there a case where we would still have result = 1
> but rodata_test_data overwritten?

Seems sensible when you consider that "result" tests that _a_ fault
happened.  Verifying that the data wasn't changed seems like a belt
and braces approach, which ensures that the location really has not
been modified.

IOW, the test is "make sure that read-only data is not modified" not
"make sure that writing to read-only data causes a fault".  They're
two subtly different tests.

> As Mark mentioned, this is possibly redundant with LKDTM. It
> would be good to explain what benefit this is bringing in addition
> to LKDTM.

Finding https://lwn.net/Articles/198690/ and github links, it doesn't
seem obvious that LKDTM actually does this.  It seems more focused on
creating crashdumps than testing that (in this instance) write
protection works - and it seems to be a destructive test.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] ARM: mm: add testcases for RODATA

[Russell King - ARM Linux]

On Wed, Jan 18, 2017 at 11:20:54AM -0800, Laura Abbott wrote:
> On 01/18/2017 05:53 AM, Jinbum Park wrote:
> > diff --git a/arch/arm/include/asm/cacheflush.h b/arch/arm/include/asm/cacheflush.h
> > index bdd283b..741e2e8 100644
> > --- a/arch/arm/include/asm/cacheflush.h
> > +++ b/arch/arm/include/asm/cacheflush.h
> > @@ -498,6 +498,16 @@ static inline void set_kernel_text_rw(void) { }
> >  static inline void set_kernel_text_ro(void) { }
> >  #endif
> >  
> > +#ifdef CONFIG_DEBUG_RODATA_TEST
> > +extern const int rodata_test_data;
> > +int rodata_test(void);
> > +#else
> > +static inline int rodata_test(void)
> > +{
> > +	return 0;
> > +}
> > +#endif
> > +

I don't see why this needs to be in cacheflush.h - it doesn't seem to
have anything to do with cache flushing, and placing it in here means
that if you change the state of CONFIG_DEBUG_RODATA_TEST, most likely
the entire kernel gets rebuilt.  Please put it in a separate header
file.

> > diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
> > index 4127f57..3c15f3b 100644
> > --- a/arch/arm/mm/init.c
> > +++ b/arch/arm/mm/init.c
> > @@ -716,6 +716,7 @@ void fix_kernmem_perms(void)
> >  int __mark_rodata_ro(void *unused)
> >  {
> >  	update_sections_early(ro_perms, ARRAY_SIZE(ro_perms));
> > +	rodata_test();
> 
> We don't do anything with this return value, should we at least
> spit out a warning?
> 
> >  	return 0;
> >  }
> >  
> > @@ -740,6 +741,11 @@ void set_kernel_text_ro(void)
> >  static inline void fix_kernmem_perms(void) { }
> >  #endif /* CONFIG_DEBUG_RODATA */
> >  
> > +#ifdef CONFIG_DEBUG_RODATA_TEST
> > +const int rodata_test_data = 0xC3;
> 
> This isn't accessed outside of test_rodata.c, it can just
> be moved there.

I think the intention was to place it in some .c file which gets built
into the kernel, rather than a module, so testing whether read-only
data in the kernel image is read-only.

If it's placed in a module, then you're only checking that read-only
data in the module is read-only (which is another test which should
be done!)

In any case, placing it in arch/arm/mm/init.c seems unnecessary, I'd
rather it went in its own separate file.

> > diff --git a/arch/arm/mm/test_rodata.c b/arch/arm/mm/test_rodata.c
> > new file mode 100644
> > index 0000000..133d092
> > --- /dev/null
> > +++ b/arch/arm/mm/test_rodata.c
> > @@ -0,0 +1,79 @@
> > +/*
> > + * test_rodata.c: functional test for mark_rodata_ro function
> > + *
> > + * (C) Copyright 2017 Jinbum Park <jinb.park7@gmail.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; version 2
> > + * of the License.
> > + */
> > +#include <asm/cacheflush.h>
> > +#include <asm/sections.h>
> > +
> > +int rodata_test(void)
> > +{
> > +	unsigned long result;
> > +	unsigned long start, end;
> > +
> > +	/* test 1: read the value */
> > +	/* If this test fails, some previous testrun has clobbered the state */
> > +
> > +	if (!rodata_test_data) {
> > +		pr_err("rodata_test: test 1 fails (start data)\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	/* test 2: write to the variable; this should fault */
> > +	/*
> > +	 * If this test fails, we managed to overwrite the data
> > +	 *
> > +	 * This is written in assembly to be able to catch the
> > +	 * exception that is supposed to happen in the correct
> > +	 * case
> > +	*/
> > +
> > +	result = 1;
> > +	asm volatile(
> > +		"0:	str %[zero], [%[rodata_test]]\n"
> > +		"	mov %[rslt], %[zero]\n"
> > +		"1:\n"
> > +		".pushsection .text.fixup,\"ax\"\n"
> > +		".align 2\n"
> > +		"2:\n"
> > +		"b 1b\n"
> > +		".popsection\n"
> > +		".pushsection __ex_table,\"a\"\n"
> > +		".align 3\n"
> > +		".long 0b, 2b\n"
> > +		".popsection\n"
> > +		: [rslt] "=r" (result)
> > +		: [zero] "r" (0UL), [rodata_test] "r" (&rodata_test_data)
> > +	);
> > +
> > +	if (!result) {
> > +		pr_err("rodata_test: test data was not read only\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	/* test 3: check the value hasn't changed */
> > +	/* If this test fails, we managed to overwrite the data */
> > +	if (!rodata_test_data) {
> > +		pr_err("rodata_test: Test 3 fails (end data)\n");
> > +		return -ENODEV;
> > +	}
> 
> I'm confused why we are checking this again when we have the result
> check above. Is there a case where we would still have result = 1
> but rodata_test_data overwritten?

Seems sensible when you consider that "result" tests that _a_ fault
happened.  Verifying that the data wasn't changed seems like a belt
and braces approach, which ensures that the location really has not
been modified.

IOW, the test is "make sure that read-only data is not modified" not
"make sure that writing to read-only data causes a fault".  They're
two subtly different tests.

> As Mark mentioned, this is possibly redundant with LKDTM. It
> would be good to explain what benefit this is bringing in addition
> to LKDTM.

Finding https://lwn.net/Articles/198690/ and github links, it doesn't
seem obvious that LKDTM actually does this.  It seems more focused on
creating crashdumps than testing that (in this instance) write
protection works - and it seems to be a destructive test.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] ARM: mm: add testcases for RODATA

[Russell King - ARM Linux]

On Wed, Jan 18, 2017 at 11:20:54AM -0800, Laura Abbott wrote:
> On 01/18/2017 05:53 AM, Jinbum Park wrote:
> > diff --git a/arch/arm/include/asm/cacheflush.h b/arch/arm/include/asm/cacheflush.h
> > index bdd283b..741e2e8 100644
> > --- a/arch/arm/include/asm/cacheflush.h
> > +++ b/arch/arm/include/asm/cacheflush.h
> > @@ -498,6 +498,16 @@ static inline void set_kernel_text_rw(void) { }
> >  static inline void set_kernel_text_ro(void) { }
> >  #endif
> >  
> > +#ifdef CONFIG_DEBUG_RODATA_TEST
> > +extern const int rodata_test_data;
> > +int rodata_test(void);
> > +#else
> > +static inline int rodata_test(void)
> > +{
> > +	return 0;
> > +}
> > +#endif
> > +

I don't see why this needs to be in cacheflush.h - it doesn't seem to
have anything to do with cache flushing, and placing it in here means
that if you change the state of CONFIG_DEBUG_RODATA_TEST, most likely
the entire kernel gets rebuilt.  Please put it in a separate header
file.

> > diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
> > index 4127f57..3c15f3b 100644
> > --- a/arch/arm/mm/init.c
> > +++ b/arch/arm/mm/init.c
> > @@ -716,6 +716,7 @@ void fix_kernmem_perms(void)
> >  int __mark_rodata_ro(void *unused)
> >  {
> >  	update_sections_early(ro_perms, ARRAY_SIZE(ro_perms));
> > +	rodata_test();
> 
> We don't do anything with this return value, should we at least
> spit out a warning?
> 
> >  	return 0;
> >  }
> >  
> > @@ -740,6 +741,11 @@ void set_kernel_text_ro(void)
> >  static inline void fix_kernmem_perms(void) { }
> >  #endif /* CONFIG_DEBUG_RODATA */
> >  
> > +#ifdef CONFIG_DEBUG_RODATA_TEST
> > +const int rodata_test_data = 0xC3;
> 
> This isn't accessed outside of test_rodata.c, it can just
> be moved there.

I think the intention was to place it in some .c file which gets built
into the kernel, rather than a module, so testing whether read-only
data in the kernel image is read-only.

If it's placed in a module, then you're only checking that read-only
data in the module is read-only (which is another test which should
be done!)

In any case, placing it in arch/arm/mm/init.c seems unnecessary, I'd
rather it went in its own separate file.

> > diff --git a/arch/arm/mm/test_rodata.c b/arch/arm/mm/test_rodata.c
> > new file mode 100644
> > index 0000000..133d092
> > --- /dev/null
> > +++ b/arch/arm/mm/test_rodata.c
> > @@ -0,0 +1,79 @@
> > +/*
> > + * test_rodata.c: functional test for mark_rodata_ro function
> > + *
> > + * (C) Copyright 2017 Jinbum Park <jinb.park7@gmail.com>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; version 2
> > + * of the License.
> > + */
> > +#include <asm/cacheflush.h>
> > +#include <asm/sections.h>
> > +
> > +int rodata_test(void)
> > +{
> > +	unsigned long result;
> > +	unsigned long start, end;
> > +
> > +	/* test 1: read the value */
> > +	/* If this test fails, some previous testrun has clobbered the state */
> > +
> > +	if (!rodata_test_data) {
> > +		pr_err("rodata_test: test 1 fails (start data)\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	/* test 2: write to the variable; this should fault */
> > +	/*
> > +	 * If this test fails, we managed to overwrite the data
> > +	 *
> > +	 * This is written in assembly to be able to catch the
> > +	 * exception that is supposed to happen in the correct
> > +	 * case
> > +	*/
> > +
> > +	result = 1;
> > +	asm volatile(
> > +		"0:	str %[zero], [%[rodata_test]]\n"
> > +		"	mov %[rslt], %[zero]\n"
> > +		"1:\n"
> > +		".pushsection .text.fixup,\"ax\"\n"
> > +		".align 2\n"
> > +		"2:\n"
> > +		"b 1b\n"
> > +		".popsection\n"
> > +		".pushsection __ex_table,\"a\"\n"
> > +		".align 3\n"
> > +		".long 0b, 2b\n"
> > +		".popsection\n"
> > +		: [rslt] "=r" (result)
> > +		: [zero] "r" (0UL), [rodata_test] "r" (&rodata_test_data)
> > +	);
> > +
> > +	if (!result) {
> > +		pr_err("rodata_test: test data was not read only\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	/* test 3: check the value hasn't changed */
> > +	/* If this test fails, we managed to overwrite the data */
> > +	if (!rodata_test_data) {
> > +		pr_err("rodata_test: Test 3 fails (end data)\n");
> > +		return -ENODEV;
> > +	}
> 
> I'm confused why we are checking this again when we have the result
> check above. Is there a case where we would still have result = 1
> but rodata_test_data overwritten?

Seems sensible when you consider that "result" tests that _a_ fault
happened.  Verifying that the data wasn't changed seems like a belt
and braces approach, which ensures that the location really has not
been modified.

IOW, the test is "make sure that read-only data is not modified" not
"make sure that writing to read-only data causes a fault".  They're
two subtly different tests.

> As Mark mentioned, this is possibly redundant with LKDTM. It
> would be good to explain what benefit this is bringing in addition
> to LKDTM.

Finding https://lwn.net/Articles/198690/ and github links, it doesn't
seem obvious that LKDTM actually does this.  It seems more focused on
creating crashdumps than testing that (in this instance) write
protection works - and it seems to be a destructive test.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[kernel-hardening] Re: [PATCH] ARM: mm: add testcases for RODATA

[Kees Cook]

On Wed, Jan 18, 2017 at 2:36 PM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Wed, Jan 18, 2017 at 11:20:54AM -0800, Laura Abbott wrote:
>> On 01/18/2017 05:53 AM, Jinbum Park wrote:
>> > diff --git a/arch/arm/include/asm/cacheflush.h b/arch/arm/include/asm/cacheflush.h
>> > index bdd283b..741e2e8 100644
>> > --- a/arch/arm/include/asm/cacheflush.h
>> > +++ b/arch/arm/include/asm/cacheflush.h
>> > @@ -498,6 +498,16 @@ static inline void set_kernel_text_rw(void) { }
>> >  static inline void set_kernel_text_ro(void) { }
>> >  #endif
>> >
>> > +#ifdef CONFIG_DEBUG_RODATA_TEST
>> > +extern const int rodata_test_data;
>> > +int rodata_test(void);
>> > +#else
>> > +static inline int rodata_test(void)
>> > +{
>> > +   return 0;
>> > +}
>> > +#endif
>> > +
>
> I don't see why this needs to be in cacheflush.h - it doesn't seem to
> have anything to do with cache flushing, and placing it in here means
> that if you change the state of CONFIG_DEBUG_RODATA_TEST, most likely
> the entire kernel gets rebuilt.  Please put it in a separate header
> file.
>
>> > diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
>> > index 4127f57..3c15f3b 100644
>> > --- a/arch/arm/mm/init.c
>> > +++ b/arch/arm/mm/init.c
>> > @@ -716,6 +716,7 @@ void fix_kernmem_perms(void)
>> >  int __mark_rodata_ro(void *unused)
>> >  {
>> >     update_sections_early(ro_perms, ARRAY_SIZE(ro_perms));
>> > +   rodata_test();
>>
>> We don't do anything with this return value, should we at least
>> spit out a warning?
>>
>> >     return 0;
>> >  }
>> >
>> > @@ -740,6 +741,11 @@ void set_kernel_text_ro(void)
>> >  static inline void fix_kernmem_perms(void) { }
>> >  #endif /* CONFIG_DEBUG_RODATA */
>> >
>> > +#ifdef CONFIG_DEBUG_RODATA_TEST
>> > +const int rodata_test_data = 0xC3;

Oh, I missed this the first time: this may be misleading: 0xc3 is x86
"ret", and is used on x86 for it's test_nx.c file. (Which, IIRC,
hasn't worked correctly for years now since the exception tables were
made relative on x86. Getting this fixed too would be nice!)

>> This isn't accessed outside of test_rodata.c, it can just
>> be moved there.

Just for background, on x86 it's referenced by both test_rodata.c and test_nx.c.

> I think the intention was to place it in some .c file which gets built
> into the kernel, rather than a module, so testing whether read-only
> data in the kernel image is read-only.
>
> If it's placed in a module, then you're only checking that read-only
> data in the module is read-only (which is another test which should
> be done!)
>
> In any case, placing it in arch/arm/mm/init.c seems unnecessary, I'd
> rather it went in its own separate file.
>
>> > diff --git a/arch/arm/mm/test_rodata.c b/arch/arm/mm/test_rodata.c
>> > new file mode 100644
>> > index 0000000..133d092
>> > --- /dev/null
>> > +++ b/arch/arm/mm/test_rodata.c
>> > @@ -0,0 +1,79 @@
>> > +/*
>> > + * test_rodata.c: functional test for mark_rodata_ro function
>> > + *
>> > + * (C) Copyright 2017 Jinbum Park <jinb.park7@gmail.com>
>> > + *
>> > + * This program is free software; you can redistribute it and/or
>> > + * modify it under the terms of the GNU General Public License
>> > + * as published by the Free Software Foundation; version 2
>> > + * of the License.
>> > + */
>> > +#include <asm/cacheflush.h>
>> > +#include <asm/sections.h>
>> > +
>> > +int rodata_test(void)
>> > +{
>> > +   unsigned long result;
>> > +   unsigned long start, end;
>> > +
>> > +   /* test 1: read the value */
>> > +   /* If this test fails, some previous testrun has clobbered the state */
>> > +
>> > +   if (!rodata_test_data) {
>> > +           pr_err("rodata_test: test 1 fails (start data)\n");
>> > +           return -ENODEV;
>> > +   }
>> > +
>> > +   /* test 2: write to the variable; this should fault */
>> > +   /*
>> > +    * If this test fails, we managed to overwrite the data
>> > +    *
>> > +    * This is written in assembly to be able to catch the
>> > +    * exception that is supposed to happen in the correct
>> > +    * case
>> > +   */
>> > +
>> > +   result = 1;
>> > +   asm volatile(
>> > +           "0:     str %[zero], [%[rodata_test]]\n"
>> > +           "       mov %[rslt], %[zero]\n"
>> > +           "1:\n"
>> > +           ".pushsection .text.fixup,\"ax\"\n"
>> > +           ".align 2\n"
>> > +           "2:\n"
>> > +           "b 1b\n"
>> > +           ".popsection\n"
>> > +           ".pushsection __ex_table,\"a\"\n"
>> > +           ".align 3\n"
>> > +           ".long 0b, 2b\n"
>> > +           ".popsection\n"
>> > +           : [rslt] "=r" (result)
>> > +           : [zero] "r" (0UL), [rodata_test] "r" (&rodata_test_data)
>> > +   );
>> > +
>> > +   if (!result) {
>> > +           pr_err("rodata_test: test data was not read only\n");
>> > +           return -ENODEV;
>> > +   }
>> > +
>> > +   /* test 3: check the value hasn't changed */
>> > +   /* If this test fails, we managed to overwrite the data */
>> > +   if (!rodata_test_data) {
>> > +           pr_err("rodata_test: Test 3 fails (end data)\n");
>> > +           return -ENODEV;
>> > +   }
>>
>> I'm confused why we are checking this again when we have the result
>> check above. Is there a case where we would still have result = 1
>> but rodata_test_data overwritten?
>
> Seems sensible when you consider that "result" tests that _a_ fault
> happened.  Verifying that the data wasn't changed seems like a belt
> and braces approach, which ensures that the location really has not
> been modified.
>
> IOW, the test is "make sure that read-only data is not modified" not
> "make sure that writing to read-only data causes a fault".  They're
> two subtly different tests.
>
>> As Mark mentioned, this is possibly redundant with LKDTM. It
>> would be good to explain what benefit this is bringing in addition
>> to LKDTM.
>
> Finding https://lwn.net/Articles/198690/ and github links, it doesn't
> seem obvious that LKDTM actually does this.  It seems more focused on
> creating crashdumps than testing that (in this instance) write
> protection works - and it seems to be a destructive test.

The WRITE_RO and EXEC_RODATA tests perform similar tests, but yes, it
is intentionally destructive: it is exercising the entire protection
and kernel oops, etc.

-Kees

-- 
Kees Cook
Nexus Security

Re: [PATCH] ARM: mm: add testcases for RODATA

[Kees Cook]

On Wed, Jan 18, 2017 at 2:36 PM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Wed, Jan 18, 2017 at 11:20:54AM -0800, Laura Abbott wrote:
>> On 01/18/2017 05:53 AM, Jinbum Park wrote:
>> > diff --git a/arch/arm/include/asm/cacheflush.h b/arch/arm/include/asm/cacheflush.h
>> > index bdd283b..741e2e8 100644
>> > --- a/arch/arm/include/asm/cacheflush.h
>> > +++ b/arch/arm/include/asm/cacheflush.h
>> > @@ -498,6 +498,16 @@ static inline void set_kernel_text_rw(void) { }
>> >  static inline void set_kernel_text_ro(void) { }
>> >  #endif
>> >
>> > +#ifdef CONFIG_DEBUG_RODATA_TEST
>> > +extern const int rodata_test_data;
>> > +int rodata_test(void);
>> > +#else
>> > +static inline int rodata_test(void)
>> > +{
>> > +   return 0;
>> > +}
>> > +#endif
>> > +
>
> I don't see why this needs to be in cacheflush.h - it doesn't seem to
> have anything to do with cache flushing, and placing it in here means
> that if you change the state of CONFIG_DEBUG_RODATA_TEST, most likely
> the entire kernel gets rebuilt.  Please put it in a separate header
> file.
>
>> > diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
>> > index 4127f57..3c15f3b 100644
>> > --- a/arch/arm/mm/init.c
>> > +++ b/arch/arm/mm/init.c
>> > @@ -716,6 +716,7 @@ void fix_kernmem_perms(void)
>> >  int __mark_rodata_ro(void *unused)
>> >  {
>> >     update_sections_early(ro_perms, ARRAY_SIZE(ro_perms));
>> > +   rodata_test();
>>
>> We don't do anything with this return value, should we at least
>> spit out a warning?
>>
>> >     return 0;
>> >  }
>> >
>> > @@ -740,6 +741,11 @@ void set_kernel_text_ro(void)
>> >  static inline void fix_kernmem_perms(void) { }
>> >  #endif /* CONFIG_DEBUG_RODATA */
>> >
>> > +#ifdef CONFIG_DEBUG_RODATA_TEST
>> > +const int rodata_test_data = 0xC3;

Oh, I missed this the first time: this may be misleading: 0xc3 is x86
"ret", and is used on x86 for it's test_nx.c file. (Which, IIRC,
hasn't worked correctly for years now since the exception tables were
made relative on x86. Getting this fixed too would be nice!)

>> This isn't accessed outside of test_rodata.c, it can just
>> be moved there.

Just for background, on x86 it's referenced by both test_rodata.c and test_nx.c.

> I think the intention was to place it in some .c file which gets built
> into the kernel, rather than a module, so testing whether read-only
> data in the kernel image is read-only.
>
> If it's placed in a module, then you're only checking that read-only
> data in the module is read-only (which is another test which should
> be done!)
>
> In any case, placing it in arch/arm/mm/init.c seems unnecessary, I'd
> rather it went in its own separate file.
>
>> > diff --git a/arch/arm/mm/test_rodata.c b/arch/arm/mm/test_rodata.c
>> > new file mode 100644
>> > index 0000000..133d092
>> > --- /dev/null
>> > +++ b/arch/arm/mm/test_rodata.c
>> > @@ -0,0 +1,79 @@
>> > +/*
>> > + * test_rodata.c: functional test for mark_rodata_ro function
>> > + *
>> > + * (C) Copyright 2017 Jinbum Park <jinb.park7@gmail.com>
>> > + *
>> > + * This program is free software; you can redistribute it and/or
>> > + * modify it under the terms of the GNU General Public License
>> > + * as published by the Free Software Foundation; version 2
>> > + * of the License.
>> > + */
>> > +#include <asm/cacheflush.h>
>> > +#include <asm/sections.h>
>> > +
>> > +int rodata_test(void)
>> > +{
>> > +   unsigned long result;
>> > +   unsigned long start, end;
>> > +
>> > +   /* test 1: read the value */
>> > +   /* If this test fails, some previous testrun has clobbered the state */
>> > +
>> > +   if (!rodata_test_data) {
>> > +           pr_err("rodata_test: test 1 fails (start data)\n");
>> > +           return -ENODEV;
>> > +   }
>> > +
>> > +   /* test 2: write to the variable; this should fault */
>> > +   /*
>> > +    * If this test fails, we managed to overwrite the data
>> > +    *
>> > +    * This is written in assembly to be able to catch the
>> > +    * exception that is supposed to happen in the correct
>> > +    * case
>> > +   */
>> > +
>> > +   result = 1;
>> > +   asm volatile(
>> > +           "0:     str %[zero], [%[rodata_test]]\n"
>> > +           "       mov %[rslt], %[zero]\n"
>> > +           "1:\n"
>> > +           ".pushsection .text.fixup,\"ax\"\n"
>> > +           ".align 2\n"
>> > +           "2:\n"
>> > +           "b 1b\n"
>> > +           ".popsection\n"
>> > +           ".pushsection __ex_table,\"a\"\n"
>> > +           ".align 3\n"
>> > +           ".long 0b, 2b\n"
>> > +           ".popsection\n"
>> > +           : [rslt] "=r" (result)
>> > +           : [zero] "r" (0UL), [rodata_test] "r" (&rodata_test_data)
>> > +   );
>> > +
>> > +   if (!result) {
>> > +           pr_err("rodata_test: test data was not read only\n");
>> > +           return -ENODEV;
>> > +   }
>> > +
>> > +   /* test 3: check the value hasn't changed */
>> > +   /* If this test fails, we managed to overwrite the data */
>> > +   if (!rodata_test_data) {
>> > +           pr_err("rodata_test: Test 3 fails (end data)\n");
>> > +           return -ENODEV;
>> > +   }
>>
>> I'm confused why we are checking this again when we have the result
>> check above. Is there a case where we would still have result = 1
>> but rodata_test_data overwritten?
>
> Seems sensible when you consider that "result" tests that _a_ fault
> happened.  Verifying that the data wasn't changed seems like a belt
> and braces approach, which ensures that the location really has not
> been modified.
>
> IOW, the test is "make sure that read-only data is not modified" not
> "make sure that writing to read-only data causes a fault".  They're
> two subtly different tests.
>
>> As Mark mentioned, this is possibly redundant with LKDTM. It
>> would be good to explain what benefit this is bringing in addition
>> to LKDTM.
>
> Finding https://lwn.net/Articles/198690/ and github links, it doesn't
> seem obvious that LKDTM actually does this.  It seems more focused on
> creating crashdumps than testing that (in this instance) write
> protection works - and it seems to be a destructive test.

The WRITE_RO and EXEC_RODATA tests perform similar tests, but yes, it
is intentionally destructive: it is exercising the entire protection
and kernel oops, etc.

-Kees

-- 
Kees Cook
Nexus Security

[PATCH] ARM: mm: add testcases for RODATA

[Kees Cook]

On Wed, Jan 18, 2017 at 2:36 PM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Wed, Jan 18, 2017 at 11:20:54AM -0800, Laura Abbott wrote:
>> On 01/18/2017 05:53 AM, Jinbum Park wrote:
>> > diff --git a/arch/arm/include/asm/cacheflush.h b/arch/arm/include/asm/cacheflush.h
>> > index bdd283b..741e2e8 100644
>> > --- a/arch/arm/include/asm/cacheflush.h
>> > +++ b/arch/arm/include/asm/cacheflush.h
>> > @@ -498,6 +498,16 @@ static inline void set_kernel_text_rw(void) { }
>> >  static inline void set_kernel_text_ro(void) { }
>> >  #endif
>> >
>> > +#ifdef CONFIG_DEBUG_RODATA_TEST
>> > +extern const int rodata_test_data;
>> > +int rodata_test(void);
>> > +#else
>> > +static inline int rodata_test(void)
>> > +{
>> > +   return 0;
>> > +}
>> > +#endif
>> > +
>
> I don't see why this needs to be in cacheflush.h - it doesn't seem to
> have anything to do with cache flushing, and placing it in here means
> that if you change the state of CONFIG_DEBUG_RODATA_TEST, most likely
> the entire kernel gets rebuilt.  Please put it in a separate header
> file.
>
>> > diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
>> > index 4127f57..3c15f3b 100644
>> > --- a/arch/arm/mm/init.c
>> > +++ b/arch/arm/mm/init.c
>> > @@ -716,6 +716,7 @@ void fix_kernmem_perms(void)
>> >  int __mark_rodata_ro(void *unused)
>> >  {
>> >     update_sections_early(ro_perms, ARRAY_SIZE(ro_perms));
>> > +   rodata_test();
>>
>> We don't do anything with this return value, should we at least
>> spit out a warning?
>>
>> >     return 0;
>> >  }
>> >
>> > @@ -740,6 +741,11 @@ void set_kernel_text_ro(void)
>> >  static inline void fix_kernmem_perms(void) { }
>> >  #endif /* CONFIG_DEBUG_RODATA */
>> >
>> > +#ifdef CONFIG_DEBUG_RODATA_TEST
>> > +const int rodata_test_data = 0xC3;

Oh, I missed this the first time: this may be misleading: 0xc3 is x86
"ret", and is used on x86 for it's test_nx.c file. (Which, IIRC,
hasn't worked correctly for years now since the exception tables were
made relative on x86. Getting this fixed too would be nice!)

>> This isn't accessed outside of test_rodata.c, it can just
>> be moved there.

Just for background, on x86 it's referenced by both test_rodata.c and test_nx.c.

> I think the intention was to place it in some .c file which gets built
> into the kernel, rather than a module, so testing whether read-only
> data in the kernel image is read-only.
>
> If it's placed in a module, then you're only checking that read-only
> data in the module is read-only (which is another test which should
> be done!)
>
> In any case, placing it in arch/arm/mm/init.c seems unnecessary, I'd
> rather it went in its own separate file.
>
>> > diff --git a/arch/arm/mm/test_rodata.c b/arch/arm/mm/test_rodata.c
>> > new file mode 100644
>> > index 0000000..133d092
>> > --- /dev/null
>> > +++ b/arch/arm/mm/test_rodata.c
>> > @@ -0,0 +1,79 @@
>> > +/*
>> > + * test_rodata.c: functional test for mark_rodata_ro function
>> > + *
>> > + * (C) Copyright 2017 Jinbum Park <jinb.park7@gmail.com>
>> > + *
>> > + * This program is free software; you can redistribute it and/or
>> > + * modify it under the terms of the GNU General Public License
>> > + * as published by the Free Software Foundation; version 2
>> > + * of the License.
>> > + */
>> > +#include <asm/cacheflush.h>
>> > +#include <asm/sections.h>
>> > +
>> > +int rodata_test(void)
>> > +{
>> > +   unsigned long result;
>> > +   unsigned long start, end;
>> > +
>> > +   /* test 1: read the value */
>> > +   /* If this test fails, some previous testrun has clobbered the state */
>> > +
>> > +   if (!rodata_test_data) {
>> > +           pr_err("rodata_test: test 1 fails (start data)\n");
>> > +           return -ENODEV;
>> > +   }
>> > +
>> > +   /* test 2: write to the variable; this should fault */
>> > +   /*
>> > +    * If this test fails, we managed to overwrite the data
>> > +    *
>> > +    * This is written in assembly to be able to catch the
>> > +    * exception that is supposed to happen in the correct
>> > +    * case
>> > +   */
>> > +
>> > +   result = 1;
>> > +   asm volatile(
>> > +           "0:     str %[zero], [%[rodata_test]]\n"
>> > +           "       mov %[rslt], %[zero]\n"
>> > +           "1:\n"
>> > +           ".pushsection .text.fixup,\"ax\"\n"
>> > +           ".align 2\n"
>> > +           "2:\n"
>> > +           "b 1b\n"
>> > +           ".popsection\n"
>> > +           ".pushsection __ex_table,\"a\"\n"
>> > +           ".align 3\n"
>> > +           ".long 0b, 2b\n"
>> > +           ".popsection\n"
>> > +           : [rslt] "=r" (result)
>> > +           : [zero] "r" (0UL), [rodata_test] "r" (&rodata_test_data)
>> > +   );
>> > +
>> > +   if (!result) {
>> > +           pr_err("rodata_test: test data was not read only\n");
>> > +           return -ENODEV;
>> > +   }
>> > +
>> > +   /* test 3: check the value hasn't changed */
>> > +   /* If this test fails, we managed to overwrite the data */
>> > +   if (!rodata_test_data) {
>> > +           pr_err("rodata_test: Test 3 fails (end data)\n");
>> > +           return -ENODEV;
>> > +   }
>>
>> I'm confused why we are checking this again when we have the result
>> check above. Is there a case where we would still have result = 1
>> but rodata_test_data overwritten?
>
> Seems sensible when you consider that "result" tests that _a_ fault
> happened.  Verifying that the data wasn't changed seems like a belt
> and braces approach, which ensures that the location really has not
> been modified.
>
> IOW, the test is "make sure that read-only data is not modified" not
> "make sure that writing to read-only data causes a fault".  They're
> two subtly different tests.
>
>> As Mark mentioned, this is possibly redundant with LKDTM. It
>> would be good to explain what benefit this is bringing in addition
>> to LKDTM.
>
> Finding https://lwn.net/Articles/198690/ and github links, it doesn't
> seem obvious that LKDTM actually does this.  It seems more focused on
> creating crashdumps than testing that (in this instance) write
> protection works - and it seems to be a destructive test.

The WRITE_RO and EXEC_RODATA tests perform similar tests, but yes, it
is intentionally destructive: it is exercising the entire protection
and kernel oops, etc.

-Kees

-- 
Kees Cook
Nexus Security

Re: [PATCH] ARM: mm: add testcases for RODATA

[Kees Cook]

On Wed, Jan 18, 2017 at 2:36 PM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Wed, Jan 18, 2017 at 11:20:54AM -0800, Laura Abbott wrote:
>> On 01/18/2017 05:53 AM, Jinbum Park wrote:
>> > diff --git a/arch/arm/include/asm/cacheflush.h b/arch/arm/include/asm/cacheflush.h
>> > index bdd283b..741e2e8 100644
>> > --- a/arch/arm/include/asm/cacheflush.h
>> > +++ b/arch/arm/include/asm/cacheflush.h
>> > @@ -498,6 +498,16 @@ static inline void set_kernel_text_rw(void) { }
>> >  static inline void set_kernel_text_ro(void) { }
>> >  #endif
>> >
>> > +#ifdef CONFIG_DEBUG_RODATA_TEST
>> > +extern const int rodata_test_data;
>> > +int rodata_test(void);
>> > +#else
>> > +static inline int rodata_test(void)
>> > +{
>> > +   return 0;
>> > +}
>> > +#endif
>> > +
>
> I don't see why this needs to be in cacheflush.h - it doesn't seem to
> have anything to do with cache flushing, and placing it in here means
> that if you change the state of CONFIG_DEBUG_RODATA_TEST, most likely
> the entire kernel gets rebuilt.  Please put it in a separate header
> file.
>
>> > diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c
>> > index 4127f57..3c15f3b 100644
>> > --- a/arch/arm/mm/init.c
>> > +++ b/arch/arm/mm/init.c
>> > @@ -716,6 +716,7 @@ void fix_kernmem_perms(void)
>> >  int __mark_rodata_ro(void *unused)
>> >  {
>> >     update_sections_early(ro_perms, ARRAY_SIZE(ro_perms));
>> > +   rodata_test();
>>
>> We don't do anything with this return value, should we at least
>> spit out a warning?
>>
>> >     return 0;
>> >  }
>> >
>> > @@ -740,6 +741,11 @@ void set_kernel_text_ro(void)
>> >  static inline void fix_kernmem_perms(void) { }
>> >  #endif /* CONFIG_DEBUG_RODATA */
>> >
>> > +#ifdef CONFIG_DEBUG_RODATA_TEST
>> > +const int rodata_test_data = 0xC3;

Oh, I missed this the first time: this may be misleading: 0xc3 is x86
"ret", and is used on x86 for it's test_nx.c file. (Which, IIRC,
hasn't worked correctly for years now since the exception tables were
made relative on x86. Getting this fixed too would be nice!)

>> This isn't accessed outside of test_rodata.c, it can just
>> be moved there.

Just for background, on x86 it's referenced by both test_rodata.c and test_nx.c.

> I think the intention was to place it in some .c file which gets built
> into the kernel, rather than a module, so testing whether read-only
> data in the kernel image is read-only.
>
> If it's placed in a module, then you're only checking that read-only
> data in the module is read-only (which is another test which should
> be done!)
>
> In any case, placing it in arch/arm/mm/init.c seems unnecessary, I'd
> rather it went in its own separate file.
>
>> > diff --git a/arch/arm/mm/test_rodata.c b/arch/arm/mm/test_rodata.c
>> > new file mode 100644
>> > index 0000000..133d092
>> > --- /dev/null
>> > +++ b/arch/arm/mm/test_rodata.c
>> > @@ -0,0 +1,79 @@
>> > +/*
>> > + * test_rodata.c: functional test for mark_rodata_ro function
>> > + *
>> > + * (C) Copyright 2017 Jinbum Park <jinb.park7@gmail.com>
>> > + *
>> > + * This program is free software; you can redistribute it and/or
>> > + * modify it under the terms of the GNU General Public License
>> > + * as published by the Free Software Foundation; version 2
>> > + * of the License.
>> > + */
>> > +#include <asm/cacheflush.h>
>> > +#include <asm/sections.h>
>> > +
>> > +int rodata_test(void)
>> > +{
>> > +   unsigned long result;
>> > +   unsigned long start, end;
>> > +
>> > +   /* test 1: read the value */
>> > +   /* If this test fails, some previous testrun has clobbered the state */
>> > +
>> > +   if (!rodata_test_data) {
>> > +           pr_err("rodata_test: test 1 fails (start data)\n");
>> > +           return -ENODEV;
>> > +   }
>> > +
>> > +   /* test 2: write to the variable; this should fault */
>> > +   /*
>> > +    * If this test fails, we managed to overwrite the data
>> > +    *
>> > +    * This is written in assembly to be able to catch the
>> > +    * exception that is supposed to happen in the correct
>> > +    * case
>> > +   */
>> > +
>> > +   result = 1;
>> > +   asm volatile(
>> > +           "0:     str %[zero], [%[rodata_test]]\n"
>> > +           "       mov %[rslt], %[zero]\n"
>> > +           "1:\n"
>> > +           ".pushsection .text.fixup,\"ax\"\n"
>> > +           ".align 2\n"
>> > +           "2:\n"
>> > +           "b 1b\n"
>> > +           ".popsection\n"
>> > +           ".pushsection __ex_table,\"a\"\n"
>> > +           ".align 3\n"
>> > +           ".long 0b, 2b\n"
>> > +           ".popsection\n"
>> > +           : [rslt] "=r" (result)
>> > +           : [zero] "r" (0UL), [rodata_test] "r" (&rodata_test_data)
>> > +   );
>> > +
>> > +   if (!result) {
>> > +           pr_err("rodata_test: test data was not read only\n");
>> > +           return -ENODEV;
>> > +   }
>> > +
>> > +   /* test 3: check the value hasn't changed */
>> > +   /* If this test fails, we managed to overwrite the data */
>> > +   if (!rodata_test_data) {
>> > +           pr_err("rodata_test: Test 3 fails (end data)\n");
>> > +           return -ENODEV;
>> > +   }
>>
>> I'm confused why we are checking this again when we have the result
>> check above. Is there a case where we would still have result = 1
>> but rodata_test_data overwritten?
>
> Seems sensible when you consider that "result" tests that _a_ fault
> happened.  Verifying that the data wasn't changed seems like a belt
> and braces approach, which ensures that the location really has not
> been modified.
>
> IOW, the test is "make sure that read-only data is not modified" not
> "make sure that writing to read-only data causes a fault".  They're
> two subtly different tests.
>
>> As Mark mentioned, this is possibly redundant with LKDTM. It
>> would be good to explain what benefit this is bringing in addition
>> to LKDTM.
>
> Finding https://lwn.net/Articles/198690/ and github links, it doesn't
> seem obvious that LKDTM actually does this.  It seems more focused on
> creating crashdumps than testing that (in this instance) write
> protection works - and it seems to be a destructive test.

The WRITE_RO and EXEC_RODATA tests perform similar tests, but yes, it
is intentionally destructive: it is exercising the entire protection
and kernel oops, etc.

-Kees

-- 
Kees Cook
Nexus Security

Re: [kernel-hardening] Re: [PATCH] ARM: mm: add testcases for RODATA

[Laura Abbott]

On 01/18/2017 02:36 PM, Russell King - ARM Linux wrote:
> On Wed, Jan 18, 2017 at 11:20:54AM -0800, Laura Abbott wrote:
>> On 01/18/2017 05:53 AM, Jinbum Park wrote:
>>> diff --git a/arch/arm/include/asm/cacheflush.h b/arch/arm/include/asm/cacheflush.h
>>> index bdd283b..741e2e8 100644
>>> --- a/arch/arm/include/asm/cacheflush.h
>>> +++ b/arch/arm/include/asm/cacheflush.h
>>> @@ -498,6 +498,16 @@ static inline void set_kernel_text_rw(void) { }
>>>  static inline void set_kernel_text_ro(void) { }
>>>  #endif
>>>  
>>> +#ifdef CONFIG_DEBUG_RODATA_TEST
>>> +extern const int rodata_test_data;
>>> +int rodata_test(void);
>>> +#else
>>> +static inline int rodata_test(void)
>>> +{
>>> +	return 0;
>>> +}
>>> +#endif
>>> +
> 
> I don't see why this needs to be in cacheflush.h - it doesn't seem to
> have anything to do with cache flushing, and placing it in here means
> that if you change the state of CONFIG_DEBUG_RODATA_TEST, most likely
> the entire kernel gets rebuilt.  Please put it in a separate header
> file.
> 

cacheflush.h seems to be where all the set_memory_* functions have
ended up. I was just looking at cleaning that up unless someone
beats me to it.

Thanks,
Laura

Re: [kernel-hardening] Re: [PATCH] ARM: mm: add testcases for RODATA

[Laura Abbott]

On 01/18/2017 02:36 PM, Russell King - ARM Linux wrote:
> On Wed, Jan 18, 2017 at 11:20:54AM -0800, Laura Abbott wrote:
>> On 01/18/2017 05:53 AM, Jinbum Park wrote:
>>> diff --git a/arch/arm/include/asm/cacheflush.h b/arch/arm/include/asm/cacheflush.h
>>> index bdd283b..741e2e8 100644
>>> --- a/arch/arm/include/asm/cacheflush.h
>>> +++ b/arch/arm/include/asm/cacheflush.h
>>> @@ -498,6 +498,16 @@ static inline void set_kernel_text_rw(void) { }
>>>  static inline void set_kernel_text_ro(void) { }
>>>  #endif
>>>  
>>> +#ifdef CONFIG_DEBUG_RODATA_TEST
>>> +extern const int rodata_test_data;
>>> +int rodata_test(void);
>>> +#else
>>> +static inline int rodata_test(void)
>>> +{
>>> +	return 0;
>>> +}
>>> +#endif
>>> +
> 
> I don't see why this needs to be in cacheflush.h - it doesn't seem to
> have anything to do with cache flushing, and placing it in here means
> that if you change the state of CONFIG_DEBUG_RODATA_TEST, most likely
> the entire kernel gets rebuilt.  Please put it in a separate header
> file.
> 

cacheflush.h seems to be where all the set_memory_* functions have
ended up. I was just looking at cleaning that up unless someone
beats me to it.

Thanks,
Laura

[kernel-hardening] Re: [PATCH] ARM: mm: add testcases for RODATA

[Laura Abbott]

On 01/18/2017 02:36 PM, Russell King - ARM Linux wrote:
> On Wed, Jan 18, 2017 at 11:20:54AM -0800, Laura Abbott wrote:
>> On 01/18/2017 05:53 AM, Jinbum Park wrote:
>>> diff --git a/arch/arm/include/asm/cacheflush.h b/arch/arm/include/asm/cacheflush.h
>>> index bdd283b..741e2e8 100644
>>> --- a/arch/arm/include/asm/cacheflush.h
>>> +++ b/arch/arm/include/asm/cacheflush.h
>>> @@ -498,6 +498,16 @@ static inline void set_kernel_text_rw(void) { }
>>>  static inline void set_kernel_text_ro(void) { }
>>>  #endif
>>>  
>>> +#ifdef CONFIG_DEBUG_RODATA_TEST
>>> +extern const int rodata_test_data;
>>> +int rodata_test(void);
>>> +#else
>>> +static inline int rodata_test(void)
>>> +{
>>> +	return 0;
>>> +}
>>> +#endif
>>> +
> 
> I don't see why this needs to be in cacheflush.h - it doesn't seem to
> have anything to do with cache flushing, and placing it in here means
> that if you change the state of CONFIG_DEBUG_RODATA_TEST, most likely
> the entire kernel gets rebuilt.  Please put it in a separate header
> file.
> 

cacheflush.h seems to be where all the set_memory_* functions have
ended up. I was just looking at cleaning that up unless someone
beats me to it.

Thanks,
Laura

Re: [kernel-hardening] Re: [PATCH] ARM: mm: add testcases for RODATA

[Russell King - ARM Linux]

On Wed, Jan 18, 2017 at 03:38:52PM -0800, Laura Abbott wrote:
> On 01/18/2017 02:36 PM, Russell King - ARM Linux wrote:
> > I don't see why this needs to be in cacheflush.h - it doesn't seem to
> > have anything to do with cache flushing, and placing it in here means
> > that if you change the state of CONFIG_DEBUG_RODATA_TEST, most likely
> > the entire kernel gets rebuilt.  Please put it in a separate header
> > file.
> 
> cacheflush.h seems to be where all the set_memory_* functions have
> ended up. I was just looking at cleaning that up unless someone
> beats me to it.

+1

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[kernel-hardening] Re: [PATCH] ARM: mm: add testcases for RODATA

[Russell King - ARM Linux]

On Wed, Jan 18, 2017 at 03:38:52PM -0800, Laura Abbott wrote:
> On 01/18/2017 02:36 PM, Russell King - ARM Linux wrote:
> > I don't see why this needs to be in cacheflush.h - it doesn't seem to
> > have anything to do with cache flushing, and placing it in here means
> > that if you change the state of CONFIG_DEBUG_RODATA_TEST, most likely
> > the entire kernel gets rebuilt.  Please put it in a separate header
> > file.
> 
> cacheflush.h seems to be where all the set_memory_* functions have
> ended up. I was just looking at cleaning that up unless someone
> beats me to it.

+1

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.