[PATCH v2 0/4] introduce post-init read-only memory

[PATCH v2 0/4] introduce post-init read-only memory

[Kees Cook]

One of the easiest ways to protect the kernel from attack is to reduce
the internal attack surface exposed when a "write" flaw is available. By
making as much of the kernel read-only as possible, we reduce the
attack surface.

Many things are written to only during __init, and never changed
again. These cannot be made "const" since the compiler will do the wrong
thing (we do actually need to write to them). Instead, move these items
into a memory region that will be made read-only during mark_rodata_ro()
which happens after all kernel __init code has finished.

This introduces __ro_after_init as a way to mark such memory, and uses
it on the x86 vDSO to kill an extant kernel exploitation method. Also
adds a new kernel parameter to help debug future use and adds an lkdtm
test to check the results.

-Kees

[kernel-hardening] [PATCH v2 0/4] introduce post-init read-only memory

[Kees Cook]

One of the easiest ways to protect the kernel from attack is to reduce
the internal attack surface exposed when a "write" flaw is available. By
making as much of the kernel read-only as possible, we reduce the
attack surface.

Many things are written to only during __init, and never changed
again. These cannot be made "const" since the compiler will do the wrong
thing (we do actually need to write to them). Instead, move these items
into a memory region that will be made read-only during mark_rodata_ro()
which happens after all kernel __init code has finished.

This introduces __ro_after_init as a way to mark such memory, and uses
it on the x86 vDSO to kill an extant kernel exploitation method. Also
adds a new kernel parameter to help debug future use and adds an lkdtm
test to check the results.

-Kees

[PATCH v2 1/4] init: create cmdline param to disable readonly

[Kees Cook]

It may be useful to debug writes to the readonly sections of memory,
so provide a cmdline "rodata=off" to allow for this.

Suggested-by: H. Peter Anvin <hpa@zytor.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 Documentation/kernel-parameters.txt |  4 ++++
 init/main.c                         | 31 +++++++++++++++++++++++++++----
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 742f69d18fc8..21cf76dbba90 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -3409,6 +3409,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 
 	ro		[KNL] Mount root device read-only on boot
 
+	rodata=		[KNL]
+		on	Mark read-only kernel memory as read-only (default).
+		off	Leave read-only kernel memory writable for debugging.
+
 	root=		[KNL] Root filesystem
 			See name_to_dev_t comment in init/do_mounts.c.
 
diff --git a/init/main.c b/init/main.c
index 9e64d7097f1a..06200d2fbf08 100644
--- a/init/main.c
+++ b/init/main.c
@@ -93,9 +93,6 @@ static int kernel_init(void *);
 extern void init_IRQ(void);
 extern void fork_init(void);
 extern void radix_tree_init(void);
-#ifndef CONFIG_DEBUG_RODATA
-static inline void mark_rodata_ro(void) { }
-#endif
 
 /*
  * Debug helper: via this flag we know that we are in 'early bootup code'
@@ -929,6 +926,32 @@ static int try_to_run_init_process(const char *init_filename)
 
 static noinline void __init kernel_init_freeable(void);
 
+#ifdef CONFIG_DEBUG_RODATA
+bool disable_mark_readonly;
+static int __init set_debug_rodata(char *str)
+{
+	if (!str)
+		return -EINVAL;
+	if (!strncmp(str, "on", 2))
+		disable_mark_readonly = false;
+	else if (!strncmp(str, "off", 3))
+		disable_mark_readonly = true;
+
+	return 0;
+}
+__setup("rodata=", set_debug_rodata);
+
+static void mark_readonly(void)
+{
+	if (disable_mark_readonly)
+		pr_info("Kernel read-only memory marking disabled.\n");
+	else
+		mark_rodata_ro();
+}
+#else
+static inline void mark_readonly(void) { };
+#endif
+
 static int __ref kernel_init(void *unused)
 {
 	int ret;
@@ -937,7 +960,7 @@ static int __ref kernel_init(void *unused)
 	/* need to finish all async __init code before freeing the memory */
 	async_synchronize_full();
 	free_initmem();
-	mark_rodata_ro();
+	mark_readonly();
 	system_state = SYSTEM_RUNNING;
 	numa_default_policy();
 
-- 
1.9.1

[kernel-hardening] [PATCH v2 1/4] init: create cmdline param to disable readonly

[Kees Cook]

It may be useful to debug writes to the readonly sections of memory,
so provide a cmdline "rodata=off" to allow for this.

Suggested-by: H. Peter Anvin <hpa@zytor.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 Documentation/kernel-parameters.txt |  4 ++++
 init/main.c                         | 31 +++++++++++++++++++++++++++----
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 742f69d18fc8..21cf76dbba90 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -3409,6 +3409,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 
 	ro		[KNL] Mount root device read-only on boot
 
+	rodata=		[KNL]
+		on	Mark read-only kernel memory as read-only (default).
+		off	Leave read-only kernel memory writable for debugging.
+
 	root=		[KNL] Root filesystem
 			See name_to_dev_t comment in init/do_mounts.c.
 
diff --git a/init/main.c b/init/main.c
index 9e64d7097f1a..06200d2fbf08 100644
--- a/init/main.c
+++ b/init/main.c
@@ -93,9 +93,6 @@ static int kernel_init(void *);
 extern void init_IRQ(void);
 extern void fork_init(void);
 extern void radix_tree_init(void);
-#ifndef CONFIG_DEBUG_RODATA
-static inline void mark_rodata_ro(void) { }
-#endif
 
 /*
  * Debug helper: via this flag we know that we are in 'early bootup code'
@@ -929,6 +926,32 @@ static int try_to_run_init_process(const char *init_filename)
 
 static noinline void __init kernel_init_freeable(void);
 
+#ifdef CONFIG_DEBUG_RODATA
+bool disable_mark_readonly;
+static int __init set_debug_rodata(char *str)
+{
+	if (!str)
+		return -EINVAL;
+	if (!strncmp(str, "on", 2))
+		disable_mark_readonly = false;
+	else if (!strncmp(str, "off", 3))
+		disable_mark_readonly = true;
+
+	return 0;
+}
+__setup("rodata=", set_debug_rodata);
+
+static void mark_readonly(void)
+{
+	if (disable_mark_readonly)
+		pr_info("Kernel read-only memory marking disabled.\n");
+	else
+		mark_rodata_ro();
+}
+#else
+static inline void mark_readonly(void) { };
+#endif
+
 static int __ref kernel_init(void *unused)
 {
 	int ret;
@@ -937,7 +960,7 @@ static int __ref kernel_init(void *unused)
 	/* need to finish all async __init code before freeing the memory */
 	async_synchronize_full();
 	free_initmem();
-	mark_rodata_ro();
+	mark_readonly();
 	system_state = SYSTEM_RUNNING;
 	numa_default_policy();
 
-- 
1.9.1

[PATCH v2 2/4] introduce post-init read-only memory

[Kees Cook]

One of the easiest ways to protect the kernel from attack is to reduce
the internal attack surface exposed when a "write" flaw is available. By
making as much of the kernel read-only as possible, we reduce the
attack surface.

Many things are written to only during __init, and never changed
again. These cannot be made "const" since the compiler will do the wrong
thing (we do actually need to write to them). Instead, move these items
into a memory region that will be made read-only during mark_rodata_ro()
which happens after all kernel __init code has finished.

This introduces __ro_after_init as a way to mark such memory, and adds
some documentation about the existing __read_mostly marking.

Based on work by PaX Team and Brad Spengler.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/asm-generic/vmlinux.lds.h |  1 +
 include/linux/cache.h             | 14 ++++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index c4bd0e2c173c..772c784ba763 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -256,6 +256,7 @@
 	.rodata           : AT(ADDR(.rodata) - LOAD_OFFSET) {		\
 		VMLINUX_SYMBOL(__start_rodata) = .;			\
 		*(.rodata) *(.rodata.*)					\
+		*(.data..ro_after_init)	/* Read only after init */	\
 		*(__vermagic)		/* Kernel version magic */	\
 		. = ALIGN(8);						\
 		VMLINUX_SYMBOL(__start___tracepoints_ptrs) = .;		\
diff --git a/include/linux/cache.h b/include/linux/cache.h
index 17e7e82d2aa7..ac777fe01d92 100644
--- a/include/linux/cache.h
+++ b/include/linux/cache.h
@@ -12,10 +12,24 @@
 #define SMP_CACHE_BYTES L1_CACHE_BYTES
 #endif
 
+/*
+ * __read_mostly is used to keep rarely changing variables out of frequently
+ * updated cachelines. If an architecture doesn't support it, ignore the
+ * hint.
+ */
 #ifndef __read_mostly
 #define __read_mostly
 #endif
 
+/*
+ * __read_only is used to mark things that are read-only after init (i.e.
+ * after mark_rodata_ro() has been called). These are effectively read-only,
+ * but may get written to during init, so can't live in .rodata (via "const").
+ */
+#ifndef __ro_after_init
+#define __ro_after_init __attribute__((__section__(".data..ro_after_init")))
+#endif
+
 #ifndef ____cacheline_aligned
 #define ____cacheline_aligned __attribute__((__aligned__(SMP_CACHE_BYTES)))
 #endif
-- 
1.9.1

[kernel-hardening] [PATCH v2 2/4] introduce post-init read-only memory

[Kees Cook]

One of the easiest ways to protect the kernel from attack is to reduce
the internal attack surface exposed when a "write" flaw is available. By
making as much of the kernel read-only as possible, we reduce the
attack surface.

Many things are written to only during __init, and never changed
again. These cannot be made "const" since the compiler will do the wrong
thing (we do actually need to write to them). Instead, move these items
into a memory region that will be made read-only during mark_rodata_ro()
which happens after all kernel __init code has finished.

This introduces __ro_after_init as a way to mark such memory, and adds
some documentation about the existing __read_mostly marking.

Based on work by PaX Team and Brad Spengler.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/asm-generic/vmlinux.lds.h |  1 +
 include/linux/cache.h             | 14 ++++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index c4bd0e2c173c..772c784ba763 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -256,6 +256,7 @@
 	.rodata           : AT(ADDR(.rodata) - LOAD_OFFSET) {		\
 		VMLINUX_SYMBOL(__start_rodata) = .;			\
 		*(.rodata) *(.rodata.*)					\
+		*(.data..ro_after_init)	/* Read only after init */	\
 		*(__vermagic)		/* Kernel version magic */	\
 		. = ALIGN(8);						\
 		VMLINUX_SYMBOL(__start___tracepoints_ptrs) = .;		\
diff --git a/include/linux/cache.h b/include/linux/cache.h
index 17e7e82d2aa7..ac777fe01d92 100644
--- a/include/linux/cache.h
+++ b/include/linux/cache.h
@@ -12,10 +12,24 @@
 #define SMP_CACHE_BYTES L1_CACHE_BYTES
 #endif
 
+/*
+ * __read_mostly is used to keep rarely changing variables out of frequently
+ * updated cachelines. If an architecture doesn't support it, ignore the
+ * hint.
+ */
 #ifndef __read_mostly
 #define __read_mostly
 #endif
 
+/*
+ * __read_only is used to mark things that are read-only after init (i.e.
+ * after mark_rodata_ro() has been called). These are effectively read-only,
+ * but may get written to during init, so can't live in .rodata (via "const").
+ */
+#ifndef __ro_after_init
+#define __ro_after_init __attribute__((__section__(".data..ro_after_init")))
+#endif
+
 #ifndef ____cacheline_aligned
 #define ____cacheline_aligned __attribute__((__aligned__(SMP_CACHE_BYTES)))
 #endif
-- 
1.9.1

[PATCH v2 3/4] lkdtm: verify that __ro_after_init works correctly

[Kees Cook]

The new __ro_after_init section should be writable before init, but
not after. Validate that it gets updated at init and can't be written
to afterwards.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/misc/lkdtm.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/misc/lkdtm.c b/drivers/misc/lkdtm.c
index 11fdadc68e53..2a6eaf1122b4 100644
--- a/drivers/misc/lkdtm.c
+++ b/drivers/misc/lkdtm.c
@@ -103,6 +103,7 @@ enum ctype {
 	CT_EXEC_USERSPACE,
 	CT_ACCESS_USERSPACE,
 	CT_WRITE_RO,
+	CT_WRITE_RO_AFTER_INIT,
 	CT_WRITE_KERN,
 };
 
@@ -140,6 +141,7 @@ static char* cp_type[] = {
 	"EXEC_USERSPACE",
 	"ACCESS_USERSPACE",
 	"WRITE_RO",
+	"WRITE_RO_AFTER_INIT",
 	"WRITE_KERN",
 };
 
@@ -162,6 +164,7 @@ static DEFINE_SPINLOCK(lock_me_up);
 static u8 data_area[EXEC_SIZE];
 
 static const unsigned long rodata = 0xAA55AA55;
+static unsigned long ro_after_init __ro_after_init = 0x55AA5500;
 
 module_param(recur_count, int, 0644);
 MODULE_PARM_DESC(recur_count, " Recursion level for the stack overflow test");
@@ -503,11 +506,28 @@ static void lkdtm_do_action(enum ctype which)
 		break;
 	}
 	case CT_WRITE_RO: {
-		unsigned long *ptr;
+		/* Explicitly cast away "const" for the test. */
+		unsigned long *ptr = (unsigned long *)&rodata;
 
-		ptr = (unsigned long *)&rodata;
+		pr_info("attempting bad rodata write at %p\n", ptr);
+		*ptr ^= 0xabcd1234;
 
-		pr_info("attempting bad write at %p\n", ptr);
+		break;
+	}
+	case CT_WRITE_RO_AFTER_INIT: {
+		unsigned long *ptr = &ro_after_init;
+
+		/*
+		 * Verify we were written to during init. Since an Oops
+		 * is considered a "success", a failure is to just skip the
+		 * real test.
+		 */
+		if ((*ptr & 0xAA) != 0xAA) {
+			pr_info("%p was NOT written during init!?\n", ptr);
+			break;
+		}
+
+		pr_info("attempting bad ro_after_init write at %p\n", ptr);
 		*ptr ^= 0xabcd1234;
 
 		break;
@@ -817,6 +837,9 @@ static int __init lkdtm_module_init(void)
 	int n_debugfs_entries = 1; /* Assume only the direct entry */
 	int i;
 
+	/* Make sure we can write to __ro_after_init values during __init */
+	ro_after_init |= 0xAA;
+
 	/* Register debugfs interface */
 	lkdtm_debugfs_root = debugfs_create_dir("provoke-crash", NULL);
 	if (!lkdtm_debugfs_root) {
-- 
1.9.1

[kernel-hardening] [PATCH v2 3/4] lkdtm: verify that __ro_after_init works correctly

[Kees Cook]

The new __ro_after_init section should be writable before init, but
not after. Validate that it gets updated at init and can't be written
to afterwards.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/misc/lkdtm.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/misc/lkdtm.c b/drivers/misc/lkdtm.c
index 11fdadc68e53..2a6eaf1122b4 100644
--- a/drivers/misc/lkdtm.c
+++ b/drivers/misc/lkdtm.c
@@ -103,6 +103,7 @@ enum ctype {
 	CT_EXEC_USERSPACE,
 	CT_ACCESS_USERSPACE,
 	CT_WRITE_RO,
+	CT_WRITE_RO_AFTER_INIT,
 	CT_WRITE_KERN,
 };
 
@@ -140,6 +141,7 @@ static char* cp_type[] = {
 	"EXEC_USERSPACE",
 	"ACCESS_USERSPACE",
 	"WRITE_RO",
+	"WRITE_RO_AFTER_INIT",
 	"WRITE_KERN",
 };
 
@@ -162,6 +164,7 @@ static DEFINE_SPINLOCK(lock_me_up);
 static u8 data_area[EXEC_SIZE];
 
 static const unsigned long rodata = 0xAA55AA55;
+static unsigned long ro_after_init __ro_after_init = 0x55AA5500;
 
 module_param(recur_count, int, 0644);
 MODULE_PARM_DESC(recur_count, " Recursion level for the stack overflow test");
@@ -503,11 +506,28 @@ static void lkdtm_do_action(enum ctype which)
 		break;
 	}
 	case CT_WRITE_RO: {
-		unsigned long *ptr;
+		/* Explicitly cast away "const" for the test. */
+		unsigned long *ptr = (unsigned long *)&rodata;
 
-		ptr = (unsigned long *)&rodata;
+		pr_info("attempting bad rodata write at %p\n", ptr);
+		*ptr ^= 0xabcd1234;
 
-		pr_info("attempting bad write at %p\n", ptr);
+		break;
+	}
+	case CT_WRITE_RO_AFTER_INIT: {
+		unsigned long *ptr = &ro_after_init;
+
+		/*
+		 * Verify we were written to during init. Since an Oops
+		 * is considered a "success", a failure is to just skip the
+		 * real test.
+		 */
+		if ((*ptr & 0xAA) != 0xAA) {
+			pr_info("%p was NOT written during init!?\n", ptr);
+			break;
+		}
+
+		pr_info("attempting bad ro_after_init write at %p\n", ptr);
 		*ptr ^= 0xabcd1234;
 
 		break;
@@ -817,6 +837,9 @@ static int __init lkdtm_module_init(void)
 	int n_debugfs_entries = 1; /* Assume only the direct entry */
 	int i;
 
+	/* Make sure we can write to __ro_after_init values during __init */
+	ro_after_init |= 0xAA;
+
 	/* Register debugfs interface */
 	lkdtm_debugfs_root = debugfs_create_dir("provoke-crash", NULL);
 	if (!lkdtm_debugfs_root) {
-- 
1.9.1

[PATCH v2 4/4] x86, vdso: mark vDSO read-only after init

[Kees Cook]

The vDSO does not need to be writable after __init, so mark it as
__ro_after_init. The result kills the exploit method of writing to the
vDSO from kernel space resulting in userspace executing the modified code,
as shown here to bypass SMEP restrictions: http://itszn.com/blog/?p=21

The memory map (with added vDSO address reporting) shows the vDSO moving
into read-only memory:

Before:
[    0.143067] vDSO @ ffffffff82004000
[    0.143551] vDSO @ ffffffff82006000
---[ High Kernel Mapping ]---
0xffffffff80000000-0xffffffff81000000      16M                         pmd
0xffffffff81000000-0xffffffff81800000       8M   ro     PSE     GLB x  pmd
0xffffffff81800000-0xffffffff819f3000    1996K   ro             GLB x  pte
0xffffffff819f3000-0xffffffff81a00000      52K   ro                 NX pte
0xffffffff81a00000-0xffffffff81e00000       4M   ro     PSE     GLB NX pmd
0xffffffff81e00000-0xffffffff81e05000      20K   ro             GLB NX pte
0xffffffff81e05000-0xffffffff82000000    2028K   ro                 NX pte
0xffffffff82000000-0xffffffff8214f000    1340K   RW             GLB NX pte
0xffffffff8214f000-0xffffffff82281000    1224K   RW                 NX pte
0xffffffff82281000-0xffffffff82400000    1532K   RW             GLB NX pte
0xffffffff82400000-0xffffffff83200000      14M   RW     PSE     GLB NX pmd
0xffffffff83200000-0xffffffffc0000000     974M                         pmd

After:
[    0.145062] vDSO @ ffffffff81da1000
[    0.146057] vDSO @ ffffffff81da4000
---[ High Kernel Mapping ]---
0xffffffff80000000-0xffffffff81000000      16M                         pmd
0xffffffff81000000-0xffffffff81800000       8M   ro     PSE     GLB x  pmd
0xffffffff81800000-0xffffffff819f3000    1996K   ro             GLB x  pte
0xffffffff819f3000-0xffffffff81a00000      52K   ro                 NX pte
0xffffffff81a00000-0xffffffff81e00000       4M   ro     PSE     GLB NX pmd
0xffffffff81e00000-0xffffffff81e0b000      44K   ro             GLB NX pte
0xffffffff81e0b000-0xffffffff82000000    2004K   ro                 NX pte
0xffffffff82000000-0xffffffff8214c000    1328K   RW             GLB NX pte
0xffffffff8214c000-0xffffffff8227e000    1224K   RW                 NX pte
0xffffffff8227e000-0xffffffff82400000    1544K   RW             GLB NX pte
0xffffffff82400000-0xffffffff83200000      14M   RW     PSE     GLB NX pmd
0xffffffff83200000-0xffffffffc0000000     974M                         pmd

Based on work by PaX Team and Brad Spengler.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/x86/entry/vdso/vdso2c.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/entry/vdso/vdso2c.h b/arch/x86/entry/vdso/vdso2c.h
index 0224987556ce..eb93a3137ed2 100644
--- a/arch/x86/entry/vdso/vdso2c.h
+++ b/arch/x86/entry/vdso/vdso2c.h
@@ -140,7 +140,7 @@ static void BITSFUNC(go)(void *raw_addr, size_t raw_len,
 	fprintf(outfile, "#include <asm/vdso.h>\n");
 	fprintf(outfile, "\n");
 	fprintf(outfile,
-		"static unsigned char raw_data[%lu] __page_aligned_data = {",
+		"static unsigned char raw_data[%lu] __ro_after_init __aligned(PAGE_SIZE) = {",
 		mapping_size);
 	for (j = 0; j < stripped_len; j++) {
 		if (j % 10 == 0)
@@ -150,7 +150,7 @@ static void BITSFUNC(go)(void *raw_addr, size_t raw_len,
 	}
 	fprintf(outfile, "\n};\n\n");
 
-	fprintf(outfile, "static struct page *pages[%lu];\n\n",
+	fprintf(outfile, "static struct page *pages[%lu] __ro_after_init;\n\n",
 		mapping_size / 4096);
 
 	fprintf(outfile, "const struct vdso_image %s = {\n", name);
-- 
1.9.1

[kernel-hardening] [PATCH v2 4/4] x86, vdso: mark vDSO read-only after init

[Kees Cook]

The vDSO does not need to be writable after __init, so mark it as
__ro_after_init. The result kills the exploit method of writing to the
vDSO from kernel space resulting in userspace executing the modified code,
as shown here to bypass SMEP restrictions: http://itszn.com/blog/?p=21

The memory map (with added vDSO address reporting) shows the vDSO moving
into read-only memory:

Before:
[    0.143067] vDSO @ ffffffff82004000
[    0.143551] vDSO @ ffffffff82006000
---[ High Kernel Mapping ]---
0xffffffff80000000-0xffffffff81000000      16M                         pmd
0xffffffff81000000-0xffffffff81800000       8M   ro     PSE     GLB x  pmd
0xffffffff81800000-0xffffffff819f3000    1996K   ro             GLB x  pte
0xffffffff819f3000-0xffffffff81a00000      52K   ro                 NX pte
0xffffffff81a00000-0xffffffff81e00000       4M   ro     PSE     GLB NX pmd
0xffffffff81e00000-0xffffffff81e05000      20K   ro             GLB NX pte
0xffffffff81e05000-0xffffffff82000000    2028K   ro                 NX pte
0xffffffff82000000-0xffffffff8214f000    1340K   RW             GLB NX pte
0xffffffff8214f000-0xffffffff82281000    1224K   RW                 NX pte
0xffffffff82281000-0xffffffff82400000    1532K   RW             GLB NX pte
0xffffffff82400000-0xffffffff83200000      14M   RW     PSE     GLB NX pmd
0xffffffff83200000-0xffffffffc0000000     974M                         pmd

After:
[    0.145062] vDSO @ ffffffff81da1000
[    0.146057] vDSO @ ffffffff81da4000
---[ High Kernel Mapping ]---
0xffffffff80000000-0xffffffff81000000      16M                         pmd
0xffffffff81000000-0xffffffff81800000       8M   ro     PSE     GLB x  pmd
0xffffffff81800000-0xffffffff819f3000    1996K   ro             GLB x  pte
0xffffffff819f3000-0xffffffff81a00000      52K   ro                 NX pte
0xffffffff81a00000-0xffffffff81e00000       4M   ro     PSE     GLB NX pmd
0xffffffff81e00000-0xffffffff81e0b000      44K   ro             GLB NX pte
0xffffffff81e0b000-0xffffffff82000000    2004K   ro                 NX pte
0xffffffff82000000-0xffffffff8214c000    1328K   RW             GLB NX pte
0xffffffff8214c000-0xffffffff8227e000    1224K   RW                 NX pte
0xffffffff8227e000-0xffffffff82400000    1544K   RW             GLB NX pte
0xffffffff82400000-0xffffffff83200000      14M   RW     PSE     GLB NX pmd
0xffffffff83200000-0xffffffffc0000000     974M                         pmd

Based on work by PaX Team and Brad Spengler.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/x86/entry/vdso/vdso2c.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/entry/vdso/vdso2c.h b/arch/x86/entry/vdso/vdso2c.h
index 0224987556ce..eb93a3137ed2 100644
--- a/arch/x86/entry/vdso/vdso2c.h
+++ b/arch/x86/entry/vdso/vdso2c.h
@@ -140,7 +140,7 @@ static void BITSFUNC(go)(void *raw_addr, size_t raw_len,
 	fprintf(outfile, "#include <asm/vdso.h>\n");
 	fprintf(outfile, "\n");
 	fprintf(outfile,
-		"static unsigned char raw_data[%lu] __page_aligned_data = {",
+		"static unsigned char raw_data[%lu] __ro_after_init __aligned(PAGE_SIZE) = {",
 		mapping_size);
 	for (j = 0; j < stripped_len; j++) {
 		if (j % 10 == 0)
@@ -150,7 +150,7 @@ static void BITSFUNC(go)(void *raw_addr, size_t raw_len,
 	}
 	fprintf(outfile, "\n};\n\n");
 
-	fprintf(outfile, "static struct page *pages[%lu];\n\n",
+	fprintf(outfile, "static struct page *pages[%lu] __ro_after_init;\n\n",
 		mapping_size / 4096);
 
 	fprintf(outfile, "const struct vdso_image %s = {\n", name);
-- 
1.9.1

Re: [PATCH v2 2/4] introduce post-init read-only memory

[PaX Team]

On 25 Nov 2015 at 15:31, Kees Cook wrote:

> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index c4bd0e2c173c..772c784ba763 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -256,6 +256,7 @@
>   .rodata           : AT(ADDR(.rodata) - LOAD_OFFSET) {		\
>    VMLINUX_SYMBOL(__start_rodata) = .;			\
>    *(.rodata) *(.rodata.*)					\
> +		*(.data..ro_after_init)	/* Read only after init */	\
   ^^^^^^^^^^^
looks like it's tabs vs. spaces...

> +/*
> + * __read_only is used to mark things that are read-only after init (i.e.
      ^^^^^^^^^^^
i know you liked the old name but probably this one needs to change too :P

> + * after mark_rodata_ro() has been called). These are effectively read-only,
> + * but may get written to during init, so can't live in .rodata (via "const").
> + */
> +#ifndef __ro_after_init
> +#define __ro_after_init __attribute__((__section__(".data..ro_after_init")))
> +#endif

Re: [PATCH v2 2/4] introduce post-init read-only memory

[PaX Team]

On 25 Nov 2015 at 15:31, Kees Cook wrote:

> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index c4bd0e2c173c..772c784ba763 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -256,6 +256,7 @@
>   .rodata           : AT(ADDR(.rodata) - LOAD_OFFSET) {		\
>    VMLINUX_SYMBOL(__start_rodata) = .;			\
>    *(.rodata) *(.rodata.*)					\
> +		*(.data..ro_after_init)	/* Read only after init */	\
   ^^^^^^^^^^^
looks like it's tabs vs. spaces...

> +/*
> + * __read_only is used to mark things that are read-only after init (i.e.
      ^^^^^^^^^^^
i know you liked the old name but probably this one needs to change too :P

> + * after mark_rodata_ro() has been called). These are effectively read-only,
> + * but may get written to during init, so can't live in .rodata (via "const").
> + */
> +#ifndef __ro_after_init
> +#define __ro_after_init __attribute__((__section__(".data..ro_after_init")))
> +#endif

Re: [PATCH v2 2/4] introduce post-init read-only memory

[PaX Team]

On 25 Nov 2015 at 15:31, Kees Cook wrote:

> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index c4bd0e2c173c..772c784ba763 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -256,6 +256,7 @@
>   .rodata           : AT(ADDR(.rodata) - LOAD_OFFSET) {		\
>    VMLINUX_SYMBOL(__start_rodata) = .;			\
>    *(.rodata) *(.rodata.*)					\
> +		*(.data..ro_after_init)	/* Read only after init */	\
   ^^^^^^^^^^^
looks like it's tabs vs. spaces...

> +/*
> + * __read_only is used to mark things that are read-only after init (i.e.
      ^^^^^^^^^^^
i know you liked the old name but probably this one needs to change too :P

> + * after mark_rodata_ro() has been called). These are effectively read-only,
> + * but may get written to during init, so can't live in .rodata (via "const").
> + */
> +#ifndef __ro_after_init
> +#define __ro_after_init __attribute__((__section__(".data..ro_after_init")))
> +#endif

[kernel-hardening] Re: [PATCH v2 2/4] introduce post-init read-only memory

[PaX Team]

On 25 Nov 2015 at 15:31, Kees Cook wrote:

> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
> index c4bd0e2c173c..772c784ba763 100644
> --- a/include/asm-generic/vmlinux.lds.h
> +++ b/include/asm-generic/vmlinux.lds.h
> @@ -256,6 +256,7 @@
>   .rodata           : AT(ADDR(.rodata) - LOAD_OFFSET) {		\
>    VMLINUX_SYMBOL(__start_rodata) = .;			\
>    *(.rodata) *(.rodata.*)					\
> +		*(.data..ro_after_init)	/* Read only after init */	\
   ^^^^^^^^^^^
looks like it's tabs vs. spaces...

> +/*
> + * __read_only is used to mark things that are read-only after init (i.e.
      ^^^^^^^^^^^
i know you liked the old name but probably this one needs to change too :P

> + * after mark_rodata_ro() has been called). These are effectively read-only,
> + * but may get written to during init, so can't live in .rodata (via "const").
> + */
> +#ifndef __ro_after_init
> +#define __ro_after_init __attribute__((__section__(".data..ro_after_init")))
> +#endif

Re: [PATCH v2 1/4] init: create cmdline param to disable readonly

[PaX Team]

On 25 Nov 2015 at 15:31, Kees Cook wrote:

> +	rodata=		[KNL]
> +		on	Mark read-only kernel memory as read-only (default).
> +		off	Leave read-only kernel memory writable for debugging.
> +

> +#ifdef CONFIG_DEBUG_RODATA
> +bool disable_mark_readonly;

__initdata?

> +static int __init set_debug_rodata(char *str)
> +{
> +	if (!str)
> +		return -EINVAL;
> +	if (!strncmp(str, "on", 2))
> +		disable_mark_readonly = false;
> +	else if (!strncmp(str, "off", 3))
> +		disable_mark_readonly = true;

maybe it's just me but the double negatives make my head spin,
perhaps call it enable_rodata instead (so that the variable name
isn't so disconnected from the option name)?

> +
> +	return 0;
> +}
> +__setup("rodata=", set_debug_rodata);
> +
> +static void mark_readonly(void)
> +{
> +	if (disable_mark_readonly)
> +		pr_info("Kernel read-only memory marking disabled.\n");
> +	else
> +		mark_rodata_ro();
> +}

Re: [PATCH v2 1/4] init: create cmdline param to disable readonly

[PaX Team]

On 25 Nov 2015 at 15:31, Kees Cook wrote:

> +	rodata=		[KNL]
> +		on	Mark read-only kernel memory as read-only (default).
> +		off	Leave read-only kernel memory writable for debugging.
> +

> +#ifdef CONFIG_DEBUG_RODATA
> +bool disable_mark_readonly;

__initdata?

> +static int __init set_debug_rodata(char *str)
> +{
> +	if (!str)
> +		return -EINVAL;
> +	if (!strncmp(str, "on", 2))
> +		disable_mark_readonly = false;
> +	else if (!strncmp(str, "off", 3))
> +		disable_mark_readonly = true;

maybe it's just me but the double negatives make my head spin,
perhaps call it enable_rodata instead (so that the variable name
isn't so disconnected from the option name)?

> +
> +	return 0;
> +}
> +__setup("rodata=", set_debug_rodata);
> +
> +static void mark_readonly(void)
> +{
> +	if (disable_mark_readonly)
> +		pr_info("Kernel read-only memory marking disabled.\n");
> +	else
> +		mark_rodata_ro();
> +}

Re: [PATCH v2 1/4] init: create cmdline param to disable readonly

[PaX Team]

On 25 Nov 2015 at 15:31, Kees Cook wrote:

> +	rodata=		[KNL]
> +		on	Mark read-only kernel memory as read-only (default).
> +		off	Leave read-only kernel memory writable for debugging.
> +

> +#ifdef CONFIG_DEBUG_RODATA
> +bool disable_mark_readonly;

__initdata?

> +static int __init set_debug_rodata(char *str)
> +{
> +	if (!str)
> +		return -EINVAL;
> +	if (!strncmp(str, "on", 2))
> +		disable_mark_readonly = false;
> +	else if (!strncmp(str, "off", 3))
> +		disable_mark_readonly = true;

maybe it's just me but the double negatives make my head spin,
perhaps call it enable_rodata instead (so that the variable name
isn't so disconnected from the option name)?

> +
> +	return 0;
> +}
> +__setup("rodata=", set_debug_rodata);
> +
> +static void mark_readonly(void)
> +{
> +	if (disable_mark_readonly)
> +		pr_info("Kernel read-only memory marking disabled.\n");
> +	else
> +		mark_rodata_ro();
> +}

[kernel-hardening] Re: [PATCH v2 1/4] init: create cmdline param to disable readonly

[PaX Team]

On 25 Nov 2015 at 15:31, Kees Cook wrote:

> +	rodata=		[KNL]
> +		on	Mark read-only kernel memory as read-only (default).
> +		off	Leave read-only kernel memory writable for debugging.
> +

> +#ifdef CONFIG_DEBUG_RODATA
> +bool disable_mark_readonly;

__initdata?

> +static int __init set_debug_rodata(char *str)
> +{
> +	if (!str)
> +		return -EINVAL;
> +	if (!strncmp(str, "on", 2))
> +		disable_mark_readonly = false;
> +	else if (!strncmp(str, "off", 3))
> +		disable_mark_readonly = true;

maybe it's just me but the double negatives make my head spin,
perhaps call it enable_rodata instead (so that the variable name
isn't so disconnected from the option name)?

> +
> +	return 0;
> +}
> +__setup("rodata=", set_debug_rodata);
> +
> +static void mark_readonly(void)
> +{
> +	if (disable_mark_readonly)
> +		pr_info("Kernel read-only memory marking disabled.\n");
> +	else
> +		mark_rodata_ro();
> +}

Re: [kernel-hardening] [PATCH v2 1/4] init: create cmdline param to disable readonly

[Greg KH]

On Wed, Nov 25, 2015 at 03:31:23PM -0800, Kees Cook wrote:
> It may be useful to debug writes to the readonly sections of memory,
> so provide a cmdline "rodata=off" to allow for this.
> 
> Suggested-by: H. Peter Anvin <hpa@zytor.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  Documentation/kernel-parameters.txt |  4 ++++
>  init/main.c                         | 31 +++++++++++++++++++++++++++----
>  2 files changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 742f69d18fc8..21cf76dbba90 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -3409,6 +3409,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>  
>  	ro		[KNL] Mount root device read-only on boot
>  
> +	rodata=		[KNL]
> +		on	Mark read-only kernel memory as read-only (default).
> +		off	Leave read-only kernel memory writable for debugging.
> +
>  	root=		[KNL] Root filesystem
>  			See name_to_dev_t comment in init/do_mounts.c.
>  
> diff --git a/init/main.c b/init/main.c
> index 9e64d7097f1a..06200d2fbf08 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -93,9 +93,6 @@ static int kernel_init(void *);
>  extern void init_IRQ(void);
>  extern void fork_init(void);
>  extern void radix_tree_init(void);
> -#ifndef CONFIG_DEBUG_RODATA
> -static inline void mark_rodata_ro(void) { }
> -#endif
>  
>  /*
>   * Debug helper: via this flag we know that we are in 'early bootup code'
> @@ -929,6 +926,32 @@ static int try_to_run_init_process(const char *init_filename)
>  
>  static noinline void __init kernel_init_freeable(void);
>  
> +#ifdef CONFIG_DEBUG_RODATA
> +bool disable_mark_readonly;
> +static int __init set_debug_rodata(char *str)
> +{
> +	if (!str)
> +		return -EINVAL;
> +	if (!strncmp(str, "on", 2))
> +		disable_mark_readonly = false;
> +	else if (!strncmp(str, "off", 3))
> +		disable_mark_readonly = true;

strtobool()?

Re: [PATCH v2 1/4] init: create cmdline param to disable readonly

[Greg KH]

On Wed, Nov 25, 2015 at 03:31:23PM -0800, Kees Cook wrote:
> It may be useful to debug writes to the readonly sections of memory,
> so provide a cmdline "rodata=off" to allow for this.
> 
> Suggested-by: H. Peter Anvin <hpa@zytor.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  Documentation/kernel-parameters.txt |  4 ++++
>  init/main.c                         | 31 +++++++++++++++++++++++++++----
>  2 files changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 742f69d18fc8..21cf76dbba90 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -3409,6 +3409,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>  
>  	ro		[KNL] Mount root device read-only on boot
>  
> +	rodata=		[KNL]
> +		on	Mark read-only kernel memory as read-only (default).
> +		off	Leave read-only kernel memory writable for debugging.
> +
>  	root=		[KNL] Root filesystem
>  			See name_to_dev_t comment in init/do_mounts.c.
>  
> diff --git a/init/main.c b/init/main.c
> index 9e64d7097f1a..06200d2fbf08 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -93,9 +93,6 @@ static int kernel_init(void *);
>  extern void init_IRQ(void);
>  extern void fork_init(void);
>  extern void radix_tree_init(void);
> -#ifndef CONFIG_DEBUG_RODATA
> -static inline void mark_rodata_ro(void) { }
> -#endif
>  
>  /*
>   * Debug helper: via this flag we know that we are in 'early bootup code'
> @@ -929,6 +926,32 @@ static int try_to_run_init_process(const char *init_filename)
>  
>  static noinline void __init kernel_init_freeable(void);
>  
> +#ifdef CONFIG_DEBUG_RODATA
> +bool disable_mark_readonly;
> +static int __init set_debug_rodata(char *str)
> +{
> +	if (!str)
> +		return -EINVAL;
> +	if (!strncmp(str, "on", 2))
> +		disable_mark_readonly = false;
> +	else if (!strncmp(str, "off", 3))
> +		disable_mark_readonly = true;

strtobool()?

Re: [PATCH v2 1/4] init: create cmdline param to disable readonly

[Ingo Molnar]

* Kees Cook <keescook@chromium.org> wrote:

> It may be useful to debug writes to the readonly sections of memory,
> so provide a cmdline "rodata=off" to allow for this.
> 
> Suggested-by: H. Peter Anvin <hpa@zytor.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  Documentation/kernel-parameters.txt |  4 ++++
>  init/main.c                         | 31 +++++++++++++++++++++++++++----
>  2 files changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 742f69d18fc8..21cf76dbba90 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -3409,6 +3409,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>  
>  	ro		[KNL] Mount root device read-only on boot
>  
> +	rodata=		[KNL]
> +		on	Mark read-only kernel memory as read-only (default).
> +		off	Leave read-only kernel memory writable for debugging.
> +
>  	root=		[KNL] Root filesystem
>  			See name_to_dev_t comment in init/do_mounts.c.
>  
> diff --git a/init/main.c b/init/main.c
> index 9e64d7097f1a..06200d2fbf08 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -93,9 +93,6 @@ static int kernel_init(void *);
>  extern void init_IRQ(void);
>  extern void fork_init(void);
>  extern void radix_tree_init(void);
> -#ifndef CONFIG_DEBUG_RODATA
> -static inline void mark_rodata_ro(void) { }
> -#endif
>  
>  /*
>   * Debug helper: via this flag we know that we are in 'early bootup code'
> @@ -929,6 +926,32 @@ static int try_to_run_init_process(const char *init_filename)
>  
>  static noinline void __init kernel_init_freeable(void);
>  
> +#ifdef CONFIG_DEBUG_RODATA

Btw., could you please remove the Kconfig option altogether in an additional patch 
and make read-only sections an always-on feature? It has been default-y for years 
and all distros have it enabled.

The 'debug rodata' naming is purely historic: this started out as a simple 
debugging feature, but meanwhile it has spread and has become an essential kernel 
robustness feature.

The boot option you added can be used if anyone needs to disable it. (Never heard 
of such a case though.)

Thanks,

	Ingo

[kernel-hardening] Re: [PATCH v2 1/4] init: create cmdline param to disable readonly

[Ingo Molnar]

* Kees Cook <keescook@chromium.org> wrote:

> It may be useful to debug writes to the readonly sections of memory,
> so provide a cmdline "rodata=off" to allow for this.
> 
> Suggested-by: H. Peter Anvin <hpa@zytor.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  Documentation/kernel-parameters.txt |  4 ++++
>  init/main.c                         | 31 +++++++++++++++++++++++++++----
>  2 files changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 742f69d18fc8..21cf76dbba90 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -3409,6 +3409,10 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>  
>  	ro		[KNL] Mount root device read-only on boot
>  
> +	rodata=		[KNL]
> +		on	Mark read-only kernel memory as read-only (default).
> +		off	Leave read-only kernel memory writable for debugging.
> +
>  	root=		[KNL] Root filesystem
>  			See name_to_dev_t comment in init/do_mounts.c.
>  
> diff --git a/init/main.c b/init/main.c
> index 9e64d7097f1a..06200d2fbf08 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -93,9 +93,6 @@ static int kernel_init(void *);
>  extern void init_IRQ(void);
>  extern void fork_init(void);
>  extern void radix_tree_init(void);
> -#ifndef CONFIG_DEBUG_RODATA
> -static inline void mark_rodata_ro(void) { }
> -#endif
>  
>  /*
>   * Debug helper: via this flag we know that we are in 'early bootup code'
> @@ -929,6 +926,32 @@ static int try_to_run_init_process(const char *init_filename)
>  
>  static noinline void __init kernel_init_freeable(void);
>  
> +#ifdef CONFIG_DEBUG_RODATA

Btw., could you please remove the Kconfig option altogether in an additional patch 
and make read-only sections an always-on feature? It has been default-y for years 
and all distros have it enabled.

The 'debug rodata' naming is purely historic: this started out as a simple 
debugging feature, but meanwhile it has spread and has become an essential kernel 
robustness feature.

The boot option you added can be used if anyone needs to disable it. (Never heard 
of such a case though.)

Thanks,

	Ingo

Re: [PATCH v2 1/4] init: create cmdline param to disable readonly

[Kees Cook]

On Wed, Nov 25, 2015 at 11:51 PM, Ingo Molnar <mingo@kernel.org> wrote:
> * Kees Cook <keescook@chromium.org> wrote:
>> +#ifdef CONFIG_DEBUG_RODATA
>
> Btw., could you please remove the Kconfig option altogether in an additional patch
> and make read-only sections an always-on feature? It has been default-y for years
> and all distros have it enabled.

Yeah, this is something I've wanted to do for a while, but I would
point out that only a few architectures have actually implemented it,
and for arm and arm64 it was very recent:

$ git grep 'config DEBUG_RODATA'
arch/arm/mm/Kconfig:config DEBUG_RODATA
arch/arm64/Kconfig.debug:config DEBUG_RODATA
arch/parisc/Kconfig.debug:config DEBUG_RODATA
arch/x86/Kconfig.debug:config DEBUG_RODATA

I think s390 already has strict kernel memory permissions, but they
set it up ahead of time. And now, I see in reading the parisc tree,
they do too, and mark_rodata_ro() is effectively a no-op. How does
powerpc handle permissions for kernel rodata?

For parisc (and maybe powerpc and s390) we'll need additional changes
to support __ro_after_init, since they may be making the ro section ro
_before_ init runs. But, that's okay since this series only uses
__ro_after_init on x86 for the moment. ;)

> The 'debug rodata' naming is purely historic: this started out as a simple
> debugging feature, but meanwhile it has spread and has become an essential kernel
> robustness feature.

I agree completely. I suspect I would turn this into
ARCH_HAS_STRICT_KERNMEM or something.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

Re: [PATCH v2 1/4] init: create cmdline param to disable readonly

[Kees Cook]

On Wed, Nov 25, 2015 at 11:51 PM, Ingo Molnar <mingo@kernel.org> wrote:
> * Kees Cook <keescook@chromium.org> wrote:
>> +#ifdef CONFIG_DEBUG_RODATA
>
> Btw., could you please remove the Kconfig option altogether in an additional patch
> and make read-only sections an always-on feature? It has been default-y for years
> and all distros have it enabled.

Yeah, this is something I've wanted to do for a while, but I would
point out that only a few architectures have actually implemented it,
and for arm and arm64 it was very recent:

$ git grep 'config DEBUG_RODATA'
arch/arm/mm/Kconfig:config DEBUG_RODATA
arch/arm64/Kconfig.debug:config DEBUG_RODATA
arch/parisc/Kconfig.debug:config DEBUG_RODATA
arch/x86/Kconfig.debug:config DEBUG_RODATA

I think s390 already has strict kernel memory permissions, but they
set it up ahead of time. And now, I see in reading the parisc tree,
they do too, and mark_rodata_ro() is effectively a no-op. How does
powerpc handle permissions for kernel rodata?

For parisc (and maybe powerpc and s390) we'll need additional changes
to support __ro_after_init, since they may be making the ro section ro
_before_ init runs. But, that's okay since this series only uses
__ro_after_init on x86 for the moment. ;)

> The 'debug rodata' naming is purely historic: this started out as a simple
> debugging feature, but meanwhile it has spread and has become an essential kernel
> robustness feature.

I agree completely. I suspect I would turn this into
ARCH_HAS_STRICT_KERNMEM or something.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

[kernel-hardening] Re: [PATCH v2 1/4] init: create cmdline param to disable readonly

[Kees Cook]

On Wed, Nov 25, 2015 at 11:51 PM, Ingo Molnar <mingo@kernel.org> wrote:
> * Kees Cook <keescook@chromium.org> wrote:
>> +#ifdef CONFIG_DEBUG_RODATA
>
> Btw., could you please remove the Kconfig option altogether in an additional patch
> and make read-only sections an always-on feature? It has been default-y for years
> and all distros have it enabled.

Yeah, this is something I've wanted to do for a while, but I would
point out that only a few architectures have actually implemented it,
and for arm and arm64 it was very recent:

$ git grep 'config DEBUG_RODATA'
arch/arm/mm/Kconfig:config DEBUG_RODATA
arch/arm64/Kconfig.debug:config DEBUG_RODATA
arch/parisc/Kconfig.debug:config DEBUG_RODATA
arch/x86/Kconfig.debug:config DEBUG_RODATA

I think s390 already has strict kernel memory permissions, but they
set it up ahead of time. And now, I see in reading the parisc tree,
they do too, and mark_rodata_ro() is effectively a no-op. How does
powerpc handle permissions for kernel rodata?

For parisc (and maybe powerpc and s390) we'll need additional changes
to support __ro_after_init, since they may be making the ro section ro
_before_ init runs. But, that's okay since this series only uses
__ro_after_init on x86 for the moment. ;)

> The 'debug rodata' naming is purely historic: this started out as a simple
> debugging feature, but meanwhile it has spread and has become an essential kernel
> robustness feature.

I agree completely. I suspect I would turn this into
ARCH_HAS_STRICT_KERNMEM or something.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

Re: [PATCH v2 1/4] init: create cmdline param to disable readonly

[Russell King - ARM Linux]

On Mon, Nov 30, 2015 at 01:52:10PM -0800, Kees Cook wrote:
> On Wed, Nov 25, 2015 at 11:51 PM, Ingo Molnar <mingo@kernel.org> wrote:
> > * Kees Cook <keescook@chromium.org> wrote:
> >> +#ifdef CONFIG_DEBUG_RODATA
> >
> > Btw., could you please remove the Kconfig option altogether in an additional patch
> > and make read-only sections an always-on feature? It has been default-y for years
> > and all distros have it enabled.
> 
> Yeah, this is something I've wanted to do for a while, but I would
> point out that only a few architectures have actually implemented it,
> and for arm and arm64 it was very recent:

I don't think it can entirely be a kernel command line option.  On ARM,
enabling DEBUG_RODATA has a substantial effect on the size of the kernel
image - we have to pad various sections to 1MB boundaries so we can
set the appropriate permissions.

Forcing this layout on everyone won't work.

What we can do is the half-way house: we can have the kernel command
line option which enables and disables the protections, but the layout
of the kernel image would still need to be controlled by DEBUG_RODATA.
I'm left wondering what the advantage of that would be: it'd end up
offering a suboptimal layout, additional memory usage but without the
benefits of memory protections.

The alternative is keeping the kernel in unlinked object form, and
laying out and linking the kernel at boot time, probably in PIC
assembly code.  That's possible but I think is undesirable.

So all in all, I'm in favour of keeping things as they are on ARM.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v2 1/4] init: create cmdline param to disable readonly

[Russell King - ARM Linux]

On Mon, Nov 30, 2015 at 01:52:10PM -0800, Kees Cook wrote:
> On Wed, Nov 25, 2015 at 11:51 PM, Ingo Molnar <mingo@kernel.org> wrote:
> > * Kees Cook <keescook@chromium.org> wrote:
> >> +#ifdef CONFIG_DEBUG_RODATA
> >
> > Btw., could you please remove the Kconfig option altogether in an additional patch
> > and make read-only sections an always-on feature? It has been default-y for years
> > and all distros have it enabled.
> 
> Yeah, this is something I've wanted to do for a while, but I would
> point out that only a few architectures have actually implemented it,
> and for arm and arm64 it was very recent:

I don't think it can entirely be a kernel command line option.  On ARM,
enabling DEBUG_RODATA has a substantial effect on the size of the kernel
image - we have to pad various sections to 1MB boundaries so we can
set the appropriate permissions.

Forcing this layout on everyone won't work.

What we can do is the half-way house: we can have the kernel command
line option which enables and disables the protections, but the layout
of the kernel image would still need to be controlled by DEBUG_RODATA.
I'm left wondering what the advantage of that would be: it'd end up
offering a suboptimal layout, additional memory usage but without the
benefits of memory protections.

The alternative is keeping the kernel in unlinked object form, and
laying out and linking the kernel at boot time, probably in PIC
assembly code.  That's possible but I think is undesirable.

So all in all, I'm in favour of keeping things as they are on ARM.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[kernel-hardening] Re: [PATCH v2 1/4] init: create cmdline param to disable readonly

[Russell King - ARM Linux]

On Mon, Nov 30, 2015 at 01:52:10PM -0800, Kees Cook wrote:
> On Wed, Nov 25, 2015 at 11:51 PM, Ingo Molnar <mingo@kernel.org> wrote:
> > * Kees Cook <keescook@chromium.org> wrote:
> >> +#ifdef CONFIG_DEBUG_RODATA
> >
> > Btw., could you please remove the Kconfig option altogether in an additional patch
> > and make read-only sections an always-on feature? It has been default-y for years
> > and all distros have it enabled.
> 
> Yeah, this is something I've wanted to do for a while, but I would
> point out that only a few architectures have actually implemented it,
> and for arm and arm64 it was very recent:

I don't think it can entirely be a kernel command line option.  On ARM,
enabling DEBUG_RODATA has a substantial effect on the size of the kernel
image - we have to pad various sections to 1MB boundaries so we can
set the appropriate permissions.

Forcing this layout on everyone won't work.

What we can do is the half-way house: we can have the kernel command
line option which enables and disables the protections, but the layout
of the kernel image would still need to be controlled by DEBUG_RODATA.
I'm left wondering what the advantage of that would be: it'd end up
offering a suboptimal layout, additional memory usage but without the
benefits of memory protections.

The alternative is keeping the kernel in unlinked object form, and
laying out and linking the kernel at boot time, probably in PIC
assembly code.  That's possible but I think is undesirable.

So all in all, I'm in favour of keeping things as they are on ARM.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v2 2/4] introduce post-init read-only memory

[H. Peter Anvin]

On 11/25/15 16:15, PaX Team wrote:
> On 25 Nov 2015 at 15:31, Kees Cook wrote:
> 
>> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
>> index c4bd0e2c173c..772c784ba763 100644
>> --- a/include/asm-generic/vmlinux.lds.h
>> +++ b/include/asm-generic/vmlinux.lds.h
>> @@ -256,6 +256,7 @@
>>   .rodata           : AT(ADDR(.rodata) - LOAD_OFFSET) {		\
>>    VMLINUX_SYMBOL(__start_rodata) = .;			\
>>    *(.rodata) *(.rodata.*)					\
>> +		*(.data..ro_after_init)	/* Read only after init */	\
>    ^^^^^^^^^^^
> looks like it's tabs vs. spaces...
> 

One more thing... double dots I think has special meanings to gcc, so it
might be better to avoid.

	-hpa

[kernel-hardening] Re: [PATCH v2 2/4] introduce post-init read-only memory

[H. Peter Anvin]

On 11/25/15 16:15, PaX Team wrote:
> On 25 Nov 2015 at 15:31, Kees Cook wrote:
> 
>> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
>> index c4bd0e2c173c..772c784ba763 100644
>> --- a/include/asm-generic/vmlinux.lds.h
>> +++ b/include/asm-generic/vmlinux.lds.h
>> @@ -256,6 +256,7 @@
>>   .rodata           : AT(ADDR(.rodata) - LOAD_OFFSET) {		\
>>    VMLINUX_SYMBOL(__start_rodata) = .;			\
>>    *(.rodata) *(.rodata.*)					\
>> +		*(.data..ro_after_init)	/* Read only after init */	\
>    ^^^^^^^^^^^
> looks like it's tabs vs. spaces...
> 

One more thing... double dots I think has special meanings to gcc, so it
might be better to avoid.

	-hpa

Re: [PATCH v2 1/4] init: create cmdline param to disable readonly

[Kees Cook]

On Mon, Nov 30, 2015 at 2:24 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Nov 30, 2015 at 01:52:10PM -0800, Kees Cook wrote:
>> On Wed, Nov 25, 2015 at 11:51 PM, Ingo Molnar <mingo@kernel.org> wrote:
>> > * Kees Cook <keescook@chromium.org> wrote:
>> >> +#ifdef CONFIG_DEBUG_RODATA
>> >
>> > Btw., could you please remove the Kconfig option altogether in an additional patch
>> > and make read-only sections an always-on feature? It has been default-y for years
>> > and all distros have it enabled.
>>
>> Yeah, this is something I've wanted to do for a while, but I would
>> point out that only a few architectures have actually implemented it,
>> and for arm and arm64 it was very recent:
>
> I don't think it can entirely be a kernel command line option.  On ARM,
> enabling DEBUG_RODATA has a substantial effect on the size of the kernel
> image - we have to pad various sections to 1MB boundaries so we can
> set the appropriate permissions.
>
> Forcing this layout on everyone won't work.
>
> What we can do is the half-way house: we can have the kernel command
> line option which enables and disables the protections, but the layout
> of the kernel image would still need to be controlled by DEBUG_RODATA.
> I'm left wondering what the advantage of that would be: it'd end up
> offering a suboptimal layout, additional memory usage but without the
> benefits of memory protections.

Right, I think it'll be there just as a debugging assist: something
broke with DEBUG_RODATA, let's boot with rodata=off and see what
happens.

> The alternative is keeping the kernel in unlinked object form, and
> laying out and linking the kernel at boot time, probably in PIC
> assembly code.  That's possible but I think is undesirable.
>
> So all in all, I'm in favour of keeping things as they are on ARM.

I've looked at the implementation in ARM again, and I think I see how
it can be improved slightly. I think I named things incorrectly when I
implemented, and I'll be sending a patch to fix that up. In the end,
though, I agree: the thing that is CONFIG_DEBUG_RODATA may change its
name, but on some architectures, there is a cost to using it, so it
needs to remain a CONFIG.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

Re: [PATCH v2 1/4] init: create cmdline param to disable readonly

[Kees Cook]

On Mon, Nov 30, 2015 at 2:24 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Nov 30, 2015 at 01:52:10PM -0800, Kees Cook wrote:
>> On Wed, Nov 25, 2015 at 11:51 PM, Ingo Molnar <mingo@kernel.org> wrote:
>> > * Kees Cook <keescook@chromium.org> wrote:
>> >> +#ifdef CONFIG_DEBUG_RODATA
>> >
>> > Btw., could you please remove the Kconfig option altogether in an additional patch
>> > and make read-only sections an always-on feature? It has been default-y for years
>> > and all distros have it enabled.
>>
>> Yeah, this is something I've wanted to do for a while, but I would
>> point out that only a few architectures have actually implemented it,
>> and for arm and arm64 it was very recent:
>
> I don't think it can entirely be a kernel command line option.  On ARM,
> enabling DEBUG_RODATA has a substantial effect on the size of the kernel
> image - we have to pad various sections to 1MB boundaries so we can
> set the appropriate permissions.
>
> Forcing this layout on everyone won't work.
>
> What we can do is the half-way house: we can have the kernel command
> line option which enables and disables the protections, but the layout
> of the kernel image would still need to be controlled by DEBUG_RODATA.
> I'm left wondering what the advantage of that would be: it'd end up
> offering a suboptimal layout, additional memory usage but without the
> benefits of memory protections.

Right, I think it'll be there just as a debugging assist: something
broke with DEBUG_RODATA, let's boot with rodata=off and see what
happens.

> The alternative is keeping the kernel in unlinked object form, and
> laying out and linking the kernel at boot time, probably in PIC
> assembly code.  That's possible but I think is undesirable.
>
> So all in all, I'm in favour of keeping things as they are on ARM.

I've looked at the implementation in ARM again, and I think I see how
it can be improved slightly. I think I named things incorrectly when I
implemented, and I'll be sending a patch to fix that up. In the end,
though, I agree: the thing that is CONFIG_DEBUG_RODATA may change its
name, but on some architectures, there is a cost to using it, so it
needs to remain a CONFIG.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

[kernel-hardening] Re: [PATCH v2 1/4] init: create cmdline param to disable readonly

[Kees Cook]

On Mon, Nov 30, 2015 at 2:24 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Nov 30, 2015 at 01:52:10PM -0800, Kees Cook wrote:
>> On Wed, Nov 25, 2015 at 11:51 PM, Ingo Molnar <mingo@kernel.org> wrote:
>> > * Kees Cook <keescook@chromium.org> wrote:
>> >> +#ifdef CONFIG_DEBUG_RODATA
>> >
>> > Btw., could you please remove the Kconfig option altogether in an additional patch
>> > and make read-only sections an always-on feature? It has been default-y for years
>> > and all distros have it enabled.
>>
>> Yeah, this is something I've wanted to do for a while, but I would
>> point out that only a few architectures have actually implemented it,
>> and for arm and arm64 it was very recent:
>
> I don't think it can entirely be a kernel command line option.  On ARM,
> enabling DEBUG_RODATA has a substantial effect on the size of the kernel
> image - we have to pad various sections to 1MB boundaries so we can
> set the appropriate permissions.
>
> Forcing this layout on everyone won't work.
>
> What we can do is the half-way house: we can have the kernel command
> line option which enables and disables the protections, but the layout
> of the kernel image would still need to be controlled by DEBUG_RODATA.
> I'm left wondering what the advantage of that would be: it'd end up
> offering a suboptimal layout, additional memory usage but without the
> benefits of memory protections.

Right, I think it'll be there just as a debugging assist: something
broke with DEBUG_RODATA, let's boot with rodata=off and see what
happens.

> The alternative is keeping the kernel in unlinked object form, and
> laying out and linking the kernel at boot time, probably in PIC
> assembly code.  That's possible but I think is undesirable.
>
> So all in all, I'm in favour of keeping things as they are on ARM.

I've looked at the implementation in ARM again, and I think I see how
it can be improved slightly. I think I named things incorrectly when I
implemented, and I'll be sending a patch to fix that up. In the end,
though, I agree: the thing that is CONFIG_DEBUG_RODATA may change its
name, but on some architectures, there is a cost to using it, so it
needs to remain a CONFIG.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

Re: [PATCH v2 1/4] init: create cmdline param to disable readonly

[Heiko Carstens]

On Mon, Nov 30, 2015 at 01:52:10PM -0800, Kees Cook wrote:
> On Wed, Nov 25, 2015 at 11:51 PM, Ingo Molnar <mingo@kernel.org> wrote:
> > * Kees Cook <keescook@chromium.org> wrote:
> >> +#ifdef CONFIG_DEBUG_RODATA
> >
> > Btw., could you please remove the Kconfig option altogether in an additional patch
> > and make read-only sections an always-on feature? It has been default-y for years
> > and all distros have it enabled.
> 
> Yeah, this is something I've wanted to do for a while, but I would
> point out that only a few architectures have actually implemented it,
> and for arm and arm64 it was very recent:
> 
> $ git grep 'config DEBUG_RODATA'
> arch/arm/mm/Kconfig:config DEBUG_RODATA
> arch/arm64/Kconfig.debug:config DEBUG_RODATA
> arch/parisc/Kconfig.debug:config DEBUG_RODATA
> arch/x86/Kconfig.debug:config DEBUG_RODATA
> 
> I think s390 already has strict kernel memory permissions, but they
> set it up ahead of time. And now, I see in reading the parisc tree,
> they do too, and mark_rodata_ro() is effectively a no-op. How does
> powerpc handle permissions for kernel rodata?
> 
> For parisc (and maybe powerpc and s390) we'll need additional changes
> to support __ro_after_init, since they may be making the ro section ro
> _before_ init runs. But, that's okay since this series only uses
> __ro_after_init on x86 for the moment. ;)

s390 marks the ro sections read-only on paging_init() for the kernel
1:1 mapping before we enable address translation.  Afterwards we
currently do not support modification of the kernel 1:1 mapping.
This also might be larger change, since we may need to split large
2GB mappings into 1MB or 4KB mappings.

Given that s390 has priviledged instructions that can easily bypass
page table based write protection (we use that for ftrace for
example), I certainly have doubts about the security value here.  For
me this is more a debugging help which catches random writes to kernel
text and which makes life for "security" module writers a bit more
difficult who try to modify the system call table.

Anyway, if you remove CONFIG_DEBUG_RODATA you could simply make the
existing mark_rodata_ro() function in kernel/init.c a weak function
and architectures could override it if wanted.

Re: [PATCH v2 1/4] init: create cmdline param to disable readonly

[Heiko Carstens]

On Mon, Nov 30, 2015 at 01:52:10PM -0800, Kees Cook wrote:
> On Wed, Nov 25, 2015 at 11:51 PM, Ingo Molnar <mingo@kernel.org> wrote:
> > * Kees Cook <keescook@chromium.org> wrote:
> >> +#ifdef CONFIG_DEBUG_RODATA
> >
> > Btw., could you please remove the Kconfig option altogether in an additional patch
> > and make read-only sections an always-on feature? It has been default-y for years
> > and all distros have it enabled.
> 
> Yeah, this is something I've wanted to do for a while, but I would
> point out that only a few architectures have actually implemented it,
> and for arm and arm64 it was very recent:
> 
> $ git grep 'config DEBUG_RODATA'
> arch/arm/mm/Kconfig:config DEBUG_RODATA
> arch/arm64/Kconfig.debug:config DEBUG_RODATA
> arch/parisc/Kconfig.debug:config DEBUG_RODATA
> arch/x86/Kconfig.debug:config DEBUG_RODATA
> 
> I think s390 already has strict kernel memory permissions, but they
> set it up ahead of time. And now, I see in reading the parisc tree,
> they do too, and mark_rodata_ro() is effectively a no-op. How does
> powerpc handle permissions for kernel rodata?
> 
> For parisc (and maybe powerpc and s390) we'll need additional changes
> to support __ro_after_init, since they may be making the ro section ro
> _before_ init runs. But, that's okay since this series only uses
> __ro_after_init on x86 for the moment. ;)

s390 marks the ro sections read-only on paging_init() for the kernel
1:1 mapping before we enable address translation.  Afterwards we
currently do not support modification of the kernel 1:1 mapping.
This also might be larger change, since we may need to split large
2GB mappings into 1MB or 4KB mappings.

Given that s390 has priviledged instructions that can easily bypass
page table based write protection (we use that for ftrace for
example), I certainly have doubts about the security value here.  For
me this is more a debugging help which catches random writes to kernel
text and which makes life for "security" module writers a bit more
difficult who try to modify the system call table.

Anyway, if you remove CONFIG_DEBUG_RODATA you could simply make the
existing mark_rodata_ro() function in kernel/init.c a weak function
and architectures could override it if wanted.

[kernel-hardening] Re: [PATCH v2 1/4] init: create cmdline param to disable readonly

[Heiko Carstens]

On Mon, Nov 30, 2015 at 01:52:10PM -0800, Kees Cook wrote:
> On Wed, Nov 25, 2015 at 11:51 PM, Ingo Molnar <mingo@kernel.org> wrote:
> > * Kees Cook <keescook@chromium.org> wrote:
> >> +#ifdef CONFIG_DEBUG_RODATA
> >
> > Btw., could you please remove the Kconfig option altogether in an additional patch
> > and make read-only sections an always-on feature? It has been default-y for years
> > and all distros have it enabled.
> 
> Yeah, this is something I've wanted to do for a while, but I would
> point out that only a few architectures have actually implemented it,
> and for arm and arm64 it was very recent:
> 
> $ git grep 'config DEBUG_RODATA'
> arch/arm/mm/Kconfig:config DEBUG_RODATA
> arch/arm64/Kconfig.debug:config DEBUG_RODATA
> arch/parisc/Kconfig.debug:config DEBUG_RODATA
> arch/x86/Kconfig.debug:config DEBUG_RODATA
> 
> I think s390 already has strict kernel memory permissions, but they
> set it up ahead of time. And now, I see in reading the parisc tree,
> they do too, and mark_rodata_ro() is effectively a no-op. How does
> powerpc handle permissions for kernel rodata?
> 
> For parisc (and maybe powerpc and s390) we'll need additional changes
> to support __ro_after_init, since they may be making the ro section ro
> _before_ init runs. But, that's okay since this series only uses
> __ro_after_init on x86 for the moment. ;)

s390 marks the ro sections read-only on paging_init() for the kernel
1:1 mapping before we enable address translation.  Afterwards we
currently do not support modification of the kernel 1:1 mapping.
This also might be larger change, since we may need to split large
2GB mappings into 1MB or 4KB mappings.

Given that s390 has priviledged instructions that can easily bypass
page table based write protection (we use that for ftrace for
example), I certainly have doubts about the security value here.  For
me this is more a debugging help which catches random writes to kernel
text and which makes life for "security" module writers a bit more
difficult who try to modify the system call table.

Anyway, if you remove CONFIG_DEBUG_RODATA you could simply make the
existing mark_rodata_ro() function in kernel/init.c a weak function
and architectures could override it if wanted.

Re: [PATCH v2 1/4] init: create cmdline param to disable readonly

[Ingo Molnar]

* Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Mon, Nov 30, 2015 at 01:52:10PM -0800, Kees Cook wrote:
> > On Wed, Nov 25, 2015 at 11:51 PM, Ingo Molnar <mingo@kernel.org> wrote:
> > > * Kees Cook <keescook@chromium.org> wrote:
> > >> +#ifdef CONFIG_DEBUG_RODATA
> > >
> > > Btw., could you please remove the Kconfig option altogether in an additional patch
> > > and make read-only sections an always-on feature? It has been default-y for years
> > > and all distros have it enabled.
> > 
> > Yeah, this is something I've wanted to do for a while, but I would
> > point out that only a few architectures have actually implemented it,
> > and for arm and arm64 it was very recent:
> 
> I don't think it can entirely be a kernel command line option.  On ARM,
> enabling DEBUG_RODATA has a substantial effect on the size of the kernel
> image - we have to pad various sections to 1MB boundaries so we can
> set the appropriate permissions.
> 
> Forcing this layout on everyone won't work.

Yeah, so I'd suggest to have it always-on on x86 (after adding the boot option), 
to simplify the x86 code and to make it more obvious that we rely on this.

There's a moderate amount of #ifdeffery around this:

  triton:~/tip> git grep -w CONFIG_DEBUG_RODATA arch/x86/  | grep \# | wc -l
  15

Thanks,

	Ingo

Re: [PATCH v2 1/4] init: create cmdline param to disable readonly

[Ingo Molnar]

* Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Mon, Nov 30, 2015 at 01:52:10PM -0800, Kees Cook wrote:
> > On Wed, Nov 25, 2015 at 11:51 PM, Ingo Molnar <mingo@kernel.org> wrote:
> > > * Kees Cook <keescook@chromium.org> wrote:
> > >> +#ifdef CONFIG_DEBUG_RODATA
> > >
> > > Btw., could you please remove the Kconfig option altogether in an additional patch
> > > and make read-only sections an always-on feature? It has been default-y for years
> > > and all distros have it enabled.
> > 
> > Yeah, this is something I've wanted to do for a while, but I would
> > point out that only a few architectures have actually implemented it,
> > and for arm and arm64 it was very recent:
> 
> I don't think it can entirely be a kernel command line option.  On ARM,
> enabling DEBUG_RODATA has a substantial effect on the size of the kernel
> image - we have to pad various sections to 1MB boundaries so we can
> set the appropriate permissions.
> 
> Forcing this layout on everyone won't work.

Yeah, so I'd suggest to have it always-on on x86 (after adding the boot option), 
to simplify the x86 code and to make it more obvious that we rely on this.

There's a moderate amount of #ifdeffery around this:

  triton:~/tip> git grep -w CONFIG_DEBUG_RODATA arch/x86/  | grep \# | wc -l
  15

Thanks,

	Ingo

[kernel-hardening] Re: [PATCH v2 1/4] init: create cmdline param to disable readonly

[Ingo Molnar]

* Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Mon, Nov 30, 2015 at 01:52:10PM -0800, Kees Cook wrote:
> > On Wed, Nov 25, 2015 at 11:51 PM, Ingo Molnar <mingo@kernel.org> wrote:
> > > * Kees Cook <keescook@chromium.org> wrote:
> > >> +#ifdef CONFIG_DEBUG_RODATA
> > >
> > > Btw., could you please remove the Kconfig option altogether in an additional patch
> > > and make read-only sections an always-on feature? It has been default-y for years
> > > and all distros have it enabled.
> > 
> > Yeah, this is something I've wanted to do for a while, but I would
> > point out that only a few architectures have actually implemented it,
> > and for arm and arm64 it was very recent:
> 
> I don't think it can entirely be a kernel command line option.  On ARM,
> enabling DEBUG_RODATA has a substantial effect on the size of the kernel
> image - we have to pad various sections to 1MB boundaries so we can
> set the appropriate permissions.
> 
> Forcing this layout on everyone won't work.

Yeah, so I'd suggest to have it always-on on x86 (after adding the boot option), 
to simplify the x86 code and to make it more obvious that we rely on this.

There's a moderate amount of #ifdeffery around this:

  triton:~/tip> git grep -w CONFIG_DEBUG_RODATA arch/x86/  | grep \# | wc -l
  15

Thanks,

	Ingo

Re: [PATCH v2 2/4] introduce post-init read-only memory

[Kees Cook]

On Mon, Nov 30, 2015 at 2:24 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 11/25/15 16:15, PaX Team wrote:
>> On 25 Nov 2015 at 15:31, Kees Cook wrote:
>>
>>> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
>>> index c4bd0e2c173c..772c784ba763 100644
>>> --- a/include/asm-generic/vmlinux.lds.h
>>> +++ b/include/asm-generic/vmlinux.lds.h
>>> @@ -256,6 +256,7 @@
>>>   .rodata           : AT(ADDR(.rodata) - LOAD_OFFSET) {              \
>>>    VMLINUX_SYMBOL(__start_rodata) = .;                       \
>>>    *(.rodata) *(.rodata.*)                                   \
>>> +            *(.data..ro_after_init) /* Read only after init */      \
>>    ^^^^^^^^^^^
>> looks like it's tabs vs. spaces...

Not sure what caused that, but I see it as tabs in my tree.

> One more thing... double dots I think has special meanings to gcc, so it
> might be better to avoid.

Hrm, I could find no information about this, so I've left it as-is.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

Re: [PATCH v2 2/4] introduce post-init read-only memory

[Kees Cook]

On Mon, Nov 30, 2015 at 2:24 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 11/25/15 16:15, PaX Team wrote:
>> On 25 Nov 2015 at 15:31, Kees Cook wrote:
>>
>>> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
>>> index c4bd0e2c173c..772c784ba763 100644
>>> --- a/include/asm-generic/vmlinux.lds.h
>>> +++ b/include/asm-generic/vmlinux.lds.h
>>> @@ -256,6 +256,7 @@
>>>   .rodata           : AT(ADDR(.rodata) - LOAD_OFFSET) {              \
>>>    VMLINUX_SYMBOL(__start_rodata) = .;                       \
>>>    *(.rodata) *(.rodata.*)                                   \
>>> +            *(.data..ro_after_init) /* Read only after init */      \
>>    ^^^^^^^^^^^
>> looks like it's tabs vs. spaces...

Not sure what caused that, but I see it as tabs in my tree.

> One more thing... double dots I think has special meanings to gcc, so it
> might be better to avoid.

Hrm, I could find no information about this, so I've left it as-is.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

[kernel-hardening] Re: [PATCH v2 2/4] introduce post-init read-only memory

[Kees Cook]

On Mon, Nov 30, 2015 at 2:24 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 11/25/15 16:15, PaX Team wrote:
>> On 25 Nov 2015 at 15:31, Kees Cook wrote:
>>
>>> diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
>>> index c4bd0e2c173c..772c784ba763 100644
>>> --- a/include/asm-generic/vmlinux.lds.h
>>> +++ b/include/asm-generic/vmlinux.lds.h
>>> @@ -256,6 +256,7 @@
>>>   .rodata           : AT(ADDR(.rodata) - LOAD_OFFSET) {              \
>>>    VMLINUX_SYMBOL(__start_rodata) = .;                       \
>>>    *(.rodata) *(.rodata.*)                                   \
>>> +            *(.data..ro_after_init) /* Read only after init */      \
>>    ^^^^^^^^^^^
>> looks like it's tabs vs. spaces...

Not sure what caused that, but I see it as tabs in my tree.

> One more thing... double dots I think has special meanings to gcc, so it
> might be better to avoid.

Hrm, I could find no information about this, so I've left it as-is.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security