[PATCH 3/3 V13] RO/NX protection for loadable kernel

[PATCH 3/3 V13] RO/NX protection for loadable kernel

[matthieu castet]

[-- Attachment #1: Type: text/plain, Size: 2064 bytes --]

This patch is a logical extension of the protection provided by
CONFIG_DEBUG_RODATA to LKMs. The protection is provided by splitting
module_core and module_init into three logical parts each and setting
appropriate page access permissions for each individual section:

 1. Code: RO+X
 2. RO data: RO+NX
 3. RW data: RW+NX

In order to achieve proper protection, layout_sections() have been
modified to align each of the three parts mentioned above onto page
boundary. Next, the corresponding page access permissions are set
right before successful exit from load_module(). Further, free_module()
and sys_init_module have been modified to set module_core and
module_init as RW+NX right before calling module_free().

By default, the original section layout and access flags are preserved.
When compiled with CONFIG_DEBUG_SET_MODULE_RONX=y, the patch
will page-align each group of sections to ensure that each page contains
only one type of content and will enforce RO/NX for each group of pages.

V1: Initial proof-of-concept patch.
V2: The patch have been re-written to reduce the number of #ifdefs and
to make it architecture-agnostic. Code formatting have been corrected also.
V3: Opportunistic RO/NX protectiuon is now unconditional. Section
page-alignment is enabled when CONFIG_DEBUG_RODATA=y.
V4: Removed most macros and improved coding style.
V5: Changed page-alignment and RO/NX section size calculation
V6: Fixed comments. Restricted RO/NX enforcement to x86 only
V7: Introduced CONFIG_DEBUG_SET_MODULE_RONX, added calls to
set_all_modules_text_rw() and set_all_modules_text_ro() in ftrace
V8: updated for compatibility with linux 2.6.33-rc5
V9: coding style fixes
V10: more coding style fixes
V11: minor adjutments for -tip
V12: minor adjutments for v2.6.35-rc2-tip
V13: minor adjutments for v2.6.37-rc1-tip

Signed-off-by: Siarhei Liakh <sliakh.lkml@gmail.com>
Signed-off-by: Xuxian Jiang <jiang@cs.ncsu.edu>
Acked-by: Arjan van de Ven <arjan@linux.intel.com>
Reviewed-by: James Morris <jmorris@namei.org>
Signed-off-by: H. Peter Anvin <hpa@zytor.com>


[-- Attachment #2: x86_nx_module_data.diff --]
[-- Type: text/x-diff, Size: 9838 bytes --]

diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
index 3be21d5..316350a 100644
--- a/arch/x86/Kconfig.debug
+++ b/arch/x86/Kconfig.debug
@@ -117,6 +117,17 @@ config DEBUG_RODATA_TEST
 	  feature as well as for the change_page_attr() infrastructure.
 	  If in doubt, say "N"
 
+config DEBUG_SET_MODULE_RONX
+	bool "Set loadable kernel module data as NX and text as RO"
+	default n
+	depends on X86 && MODULES
+	---help---
+	  This option helps to catch unintended modifications to loadable
+	  kernel module's text and read-only data. It also prevents execution
+	  of LKM's data. Such protection may interfere with run-time code
+	  patching and dynamic kernel tracing.
+	  If in doubt, say "N".
+
 config DEBUG_NX_TEST
 	tristate "Testcase for the NX non-executable stack feature"
 	depends on DEBUG_KERNEL && m
diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 3afb33f..2984486 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -19,6 +19,7 @@
 #include <linux/sched.h>
 #include <linux/init.h>
 #include <linux/list.h>
+#include <linux/module.h>
 
 #include <trace/syscall.h>
 
@@ -49,6 +50,7 @@ static DEFINE_PER_CPU(int, save_modifying_code);
 int ftrace_arch_code_modify_prepare(void)
 {
 	set_kernel_text_rw();
+	set_all_modules_text_rw();
 	modifying_code = 1;
 	return 0;
 }
@@ -56,6 +58,7 @@ int ftrace_arch_code_modify_prepare(void)
 int ftrace_arch_code_modify_post_process(void)
 {
 	modifying_code = 0;
+	set_all_modules_text_ro();
 	set_kernel_text_ro();
 	return 0;
 }
diff --git a/include/linux/module.h b/include/linux/module.h
index b29e745..4d9ce95 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -308,6 +308,9 @@ struct module
 	/* The size of the executable code in each section.  */
 	unsigned int init_text_size, core_text_size;
 
+	/* Size of RO sections of the module (text+rodata) */
+	unsigned int init_ro_size, core_ro_size;
+
 	/* Arch-specific module values */
 	struct mod_arch_specific arch;
 
@@ -548,6 +551,9 @@ extern void print_modules(void);
 extern void module_update_tracepoints(void);
 extern int module_get_iter_tracepoints(struct tracepoint_iter *iter);
 
+void set_all_modules_text_rw(void);
+void set_all_modules_text_ro(void);
+
 #else /* !CONFIG_MODULES... */
 #define EXPORT_SYMBOL(sym)
 #define EXPORT_SYMBOL_GPL(sym)
@@ -673,6 +679,13 @@ static inline int module_get_iter_tracepoints(struct tracepoint_iter *iter)
 	return 0;
 }
 
+static inline void set_all_modules_text_rw(void)
+{
+}
+
+static inline void set_all_modules_text_ro(void)
+{
+}
 #endif /* CONFIG_MODULES */
 
 #ifdef CONFIG_SYSFS
diff --git a/kernel/module.c b/kernel/module.c
index 437a74a..70d2d41 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -56,6 +56,7 @@
 #include <linux/percpu.h>
 #include <linux/kmemleak.h>
 #include <linux/jump_label.h>
+#include <linux/pfn.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/module.h>
@@ -70,6 +71,26 @@
 #define ARCH_SHF_SMALL 0
 #endif
 
+/*
+ * Modules' sections will be aligned on page boundaries
+ * to ensure complete separation of code and data, but
+ * only when CONFIG_DEBUG_SET_MODULE_RONX=y
+ */
+#ifdef CONFIG_DEBUG_SET_MODULE_RONX
+#define debug_align(X) ALIGN(X, PAGE_SIZE)
+#else
+#define debug_align(X) (X)
+#endif
+
+/*
+ * Given BASE and SIZE this macro calculates the number of pages the
+ * memory regions occupies
+ */
+#define NUMBER_OF_PAGES(BASE, SIZE) (((SIZE) > 0) ?		\
+		(PFN_DOWN((unsigned long)(BASE) + (SIZE) - 1) -	\
+			 PFN_DOWN((unsigned long)BASE) + 1)	\
+		: (0UL))
+
 /* If this is set, the section belongs in the init part of the module */
 #define INIT_OFFSET_MASK (1UL << (BITS_PER_LONG-1))
 
@@ -1542,6 +1563,121 @@ static int __unlink_module(void *_mod)
 	return 0;
 }
 
+#ifdef CONFIG_DEBUG_SET_MODULE_RONX
+/*
+ * LKM RO/NX protection: protect module's text/ro-data
+ * from modification and any data from execution.
+ */
+void set_page_attributes(void *start, void *end,
+		int (*set)(unsigned long start, int num_pages))
+{
+	unsigned long begin_pfn = PFN_DOWN((unsigned long)start);
+	unsigned long end_pfn = PFN_DOWN((unsigned long)end);
+	if (end_pfn > begin_pfn)
+		set(begin_pfn << PAGE_SHIFT, end_pfn - begin_pfn);
+}
+
+static void set_section_ro_nx(void *base,
+			unsigned long text_size,
+			unsigned long ro_size,
+			unsigned long total_size)
+{
+	/* begin and end PFNs of the current subsection */
+	unsigned long begin_pfn;
+	unsigned long end_pfn;
+
+	/*
+	 * Set RO for module text and RO-data:
+	 * - Always protect first page.
+	 * - Do not protect last partial page.
+	 */
+	if (ro_size > 0)
+		set_page_attributes(base, base + ro_size, set_memory_ro);
+
+	/*
+	 * Set NX permissions for module data:
+	 * - Do not protect first partial page.
+	 * - Always protect last page.
+	 */
+	if (total_size > text_size) {
+		begin_pfn = PFN_UP((unsigned long)base + text_size);
+		end_pfn = PFN_UP((unsigned long)base + total_size);
+		if (end_pfn > begin_pfn)
+			set_memory_nx(begin_pfn << PAGE_SHIFT, end_pfn - begin_pfn);
+	}
+}
+
+/* Setting memory back to RW+NX before releasing it */
+void unset_section_ro_nx(struct module *mod, void *module_region)
+{
+	unsigned long total_pages;
+
+	if (mod->module_core == module_region) {
+		/* Set core as NX+RW */
+		total_pages = NUMBER_OF_PAGES(mod->module_core, mod->core_size);
+		set_memory_nx((unsigned long)mod->module_core, total_pages);
+		set_memory_rw((unsigned long)mod->module_core, total_pages);
+
+	} else if (mod->module_init == module_region) {
+		/* Set init as NX+RW */
+		total_pages = NUMBER_OF_PAGES(mod->module_init, mod->init_size);
+		set_memory_nx((unsigned long)mod->module_init, total_pages);
+		set_memory_rw((unsigned long)mod->module_init, total_pages);
+	}
+}
+
+/* Iterate through all modules and set each module's text as RW */
+void set_all_modules_text_rw()
+{
+	struct module *mod;
+
+	mutex_lock(&module_mutex);
+	list_for_each_entry_rcu(mod, &modules, list) {
+		if ((mod->module_core) && (mod->core_text_size)) {
+			set_page_attributes(mod->module_core,
+						mod->module_core + mod->core_text_size,
+						set_memory_rw);
+		}
+		if ((mod->module_init) && (mod->init_text_size)) {
+			set_page_attributes(mod->module_init,
+						mod->module_init + mod->init_text_size,
+						set_memory_rw);
+		}
+	}
+	mutex_unlock(&module_mutex);
+}
+
+/* Iterate through all modules and set each module's text as RO */
+void set_all_modules_text_ro()
+{
+	struct module *mod;
+
+	mutex_lock(&module_mutex);
+	list_for_each_entry_rcu(mod, &modules, list) {
+		if ((mod->module_core) && (mod->core_text_size)) {
+			set_page_attributes(mod->module_core,
+						mod->module_core + mod->core_text_size,
+						set_memory_ro);
+		}
+		if ((mod->module_init) && (mod->init_text_size)) {
+			set_page_attributes(mod->module_init,
+						mod->module_init + mod->init_text_size,
+						set_memory_ro);
+		}
+	}
+	mutex_unlock(&module_mutex);
+}
+#else
+static void set_section_ro_nx(void *base,
+			unsigned long text_size,
+			unsigned long ro_size,
+			unsigned long total_size) { }
+
+void unset_section_ro_nx(struct module *mod, void *module_region) { }
+void set_all_modules_text_rw() { }
+void set_all_modules_text_ro() { }
+#endif
+
 /* Free a module, remove from lists, etc. */
 static void free_module(struct module *mod)
 {
@@ -1566,6 +1702,7 @@ static void free_module(struct module *mod)
 	destroy_params(mod->kp, mod->num_kp);
 
 	/* This may be NULL, but that's OK */
+	unset_section_ro_nx(mod, mod->module_init);
 	module_free(mod, mod->module_init);
 	kfree(mod->args);
 	percpu_modfree(mod);
@@ -1574,6 +1711,7 @@ static void free_module(struct module *mod)
 	lockdep_free_key_range(mod->module_core, mod->core_size);
 
 	/* Finally, free the core (containing the module structure) */
+	unset_section_ro_nx(mod, mod->module_core);
 	module_free(mod, mod->module_core);
 
 #ifdef CONFIG_MPU
@@ -1777,8 +1915,19 @@ static void layout_sections(struct module *mod, struct load_info *info)
 			s->sh_entsize = get_offset(mod, &mod->core_size, s, i);
 			DEBUGP("\t%s\n", name);
 		}
-		if (m == 0)
+		switch (m) {
+		case 0: /* executable */
+			mod->core_size = debug_align(mod->core_size);
 			mod->core_text_size = mod->core_size;
+			break;
+		case 1: /* RO: text and ro-data */
+			mod->core_size = debug_align(mod->core_size);
+			mod->core_ro_size = mod->core_size;
+			break;
+		case 3: /* whole core */
+			mod->core_size = debug_align(mod->core_size);
+			break;
+		}
 	}
 
 	DEBUGP("Init section allocation order:\n");
@@ -1796,8 +1945,19 @@ static void layout_sections(struct module *mod, struct load_info *info)
 					 | INIT_OFFSET_MASK);
 			DEBUGP("\t%s\n", sname);
 		}
-		if (m == 0)
+		switch (m) {
+		case 0: /* executable */
+			mod->init_size = debug_align(mod->init_size);
 			mod->init_text_size = mod->init_size;
+			break;
+		case 1: /* RO: text and ro-data */
+			mod->init_size = debug_align(mod->init_size);
+			mod->init_ro_size = mod->init_size;
+			break;
+		case 3: /* whole init */
+			mod->init_size = debug_align(mod->init_size);
+			break;
+		}
 	}
 }
 
@@ -2650,6 +2810,18 @@ static struct module *load_module(void __user *umod,
 	kfree(info.strmap);
 	free_copy(&info);
 
+	/* Set RO and NX regions for core */
+	set_section_ro_nx(mod->module_core,
+				mod->core_text_size,
+				mod->core_ro_size,
+				mod->core_size);
+
+	/* Set RO and NX regions for init */
+	set_section_ro_nx(mod->module_init,
+				mod->init_text_size,
+				mod->init_ro_size,
+				mod->init_size);
+
 	/* Done! */
 	trace_module_load(mod);
 	return mod;
@@ -2753,6 +2925,7 @@ SYSCALL_DEFINE3(init_module, void __user *, umod,
 	mod->symtab = mod->core_symtab;
 	mod->strtab = mod->core_strtab;
 #endif
+	unset_section_ro_nx(mod, mod->module_init);
 	module_free(mod, mod->module_init);
 	mod->module_init = NULL;
 	mod->init_size = 0;

[tip:x86/security] x86: Add RO/NX protection for loadable kernel modules

[tip-bot for matthieu castet]

Commit-ID:  84e1c6bb38eb318e456558b610396d9f1afaabf0
Gitweb:     http://git.kernel.org/tip/84e1c6bb38eb318e456558b610396d9f1afaabf0
Author:     matthieu castet <castet.matthieu@free.fr>
AuthorDate: Tue, 16 Nov 2010 22:35:16 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Thu, 18 Nov 2010 13:32:56 +0100

x86: Add RO/NX protection for loadable kernel modules

This patch is a logical extension of the protection provided by
CONFIG_DEBUG_RODATA to LKMs. The protection is provided by
splitting module_core and module_init into three logical parts
each and setting appropriate page access permissions for each
individual section:

 1. Code: RO+X
 2. RO data: RO+NX
 3. RW data: RW+NX

In order to achieve proper protection, layout_sections() have
been modified to align each of the three parts mentioned above
onto page boundary. Next, the corresponding page access
permissions are set right before successful exit from
load_module(). Further, free_module() and sys_init_module have
been modified to set module_core and module_init as RW+NX right
before calling module_free().

By default, the original section layout and access flags are
preserved. When compiled with CONFIG_DEBUG_SET_MODULE_RONX=y,
the patch will page-align each group of sections to ensure that
each page contains only one type of content and will enforce
RO/NX for each group of pages.

  -v1: Initial proof-of-concept patch.
  -v2: The patch have been re-written to reduce the number of #ifdefs
       and to make it architecture-agnostic. Code formatting has also
       been corrected.
  -v3: Opportunistic RO/NX protection is now unconditional. Section
       page-alignment is enabled when CONFIG_DEBUG_RODATA=y.
  -v4: Removed most macros and improved coding style.
  -v5: Changed page-alignment and RO/NX section size calculation
  -v6: Fixed comments. Restricted RO/NX enforcement to x86 only
  -v7: Introduced CONFIG_DEBUG_SET_MODULE_RONX, added
       calls to set_all_modules_text_rw() and set_all_modules_text_ro()
       in ftrace
  -v8: updated for compatibility with linux 2.6.33-rc5
  -v9: coding style fixes
 -v10: more coding style fixes
 -v11: minor adjustments for -tip
 -v12: minor adjustments for v2.6.35-rc2-tip
 -v13: minor adjustments for v2.6.37-rc1-tip

Signed-off-by: Siarhei Liakh <sliakh.lkml@gmail.com>
Signed-off-by: Xuxian Jiang <jiang@cs.ncsu.edu>
Acked-by: Arjan van de Ven <arjan@linux.intel.com>
Reviewed-by: James Morris <jmorris@namei.org>
Signed-off-by: H. Peter Anvin <hpa@zytor.com>
Cc: Andi Kleen <ak@muc.de>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Stephen Rothwell <sfr@canb.auug.org.au>
Cc: Dave Jones <davej@redhat.com>
Cc: Kees Cook <kees.cook@canonical.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
LKML-Reference: <4CE2F914.9070106@free.fr>
[ minor cleanliness edits, -v14: build failure fix ]
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/Kconfig.debug   |   11 +++
 arch/x86/kernel/ftrace.c |    3 +
 include/linux/module.h   |   11 +++-
 kernel/module.c          |  171 +++++++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 193 insertions(+), 3 deletions(-)

diff --git a/arch/x86/Kconfig.debug b/arch/x86/Kconfig.debug
index b59ee76..45143bb 100644
--- a/arch/x86/Kconfig.debug
+++ b/arch/x86/Kconfig.debug
@@ -117,6 +117,17 @@ config DEBUG_RODATA_TEST
 	  feature as well as for the change_page_attr() infrastructure.
 	  If in doubt, say "N"
 
+config DEBUG_SET_MODULE_RONX
+	bool "Set loadable kernel module data as NX and text as RO"
+	depends on MODULES
+	---help---
+	  This option helps catch unintended modifications to loadable
+	  kernel module's text and read-only data. It also prevents execution
+	  of module data. Such protection may interfere with run-time code
+	  patching and dynamic kernel tracing - and they might also protect
+	  against certain classes of kernel exploits.
+	  If in doubt, say "N".
+
 config DEBUG_NX_TEST
 	tristate "Testcase for the NX non-executable stack feature"
 	depends on DEBUG_KERNEL && m
diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 3afb33f..2984486 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -19,6 +19,7 @@
 #include <linux/sched.h>
 #include <linux/init.h>
 #include <linux/list.h>
+#include <linux/module.h>
 
 #include <trace/syscall.h>
 
@@ -49,6 +50,7 @@ static DEFINE_PER_CPU(int, save_modifying_code);
 int ftrace_arch_code_modify_prepare(void)
 {
 	set_kernel_text_rw();
+	set_all_modules_text_rw();
 	modifying_code = 1;
 	return 0;
 }
@@ -56,6 +58,7 @@ int ftrace_arch_code_modify_prepare(void)
 int ftrace_arch_code_modify_post_process(void)
 {
 	modifying_code = 0;
+	set_all_modules_text_ro();
 	set_kernel_text_ro();
 	return 0;
 }
diff --git a/include/linux/module.h b/include/linux/module.h
index b29e745..ddaa689 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -308,6 +308,9 @@ struct module
 	/* The size of the executable code in each section.  */
 	unsigned int init_text_size, core_text_size;
 
+	/* Size of RO sections of the module (text+rodata) */
+	unsigned int init_ro_size, core_ro_size;
+
 	/* Arch-specific module values */
 	struct mod_arch_specific arch;
 
@@ -672,7 +675,6 @@ static inline int module_get_iter_tracepoints(struct tracepoint_iter *iter)
 {
 	return 0;
 }
-
 #endif /* CONFIG_MODULES */
 
 #ifdef CONFIG_SYSFS
@@ -687,6 +689,13 @@ extern int module_sysfs_initialized;
 
 #define __MODULE_STRING(x) __stringify(x)
 
+#ifdef CONFIG_DEBUG_SET_MODULE_RONX
+extern void set_all_modules_text_rw(void);
+extern void set_all_modules_text_ro(void);
+#else
+static inline void set_all_modules_text_rw(void) { }
+static inline void set_all_modules_text_ro(void) { }
+#endif
 
 #ifdef CONFIG_GENERIC_BUG
 void module_bug_finalize(const Elf_Ehdr *, const Elf_Shdr *,
diff --git a/kernel/module.c b/kernel/module.c
index 437a74a..ba421e6 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -56,6 +56,7 @@
 #include <linux/percpu.h>
 #include <linux/kmemleak.h>
 #include <linux/jump_label.h>
+#include <linux/pfn.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/module.h>
@@ -70,6 +71,26 @@
 #define ARCH_SHF_SMALL 0
 #endif
 
+/*
+ * Modules' sections will be aligned on page boundaries
+ * to ensure complete separation of code and data, but
+ * only when CONFIG_DEBUG_SET_MODULE_RONX=y
+ */
+#ifdef CONFIG_DEBUG_SET_MODULE_RONX
+# define debug_align(X) ALIGN(X, PAGE_SIZE)
+#else
+# define debug_align(X) (X)
+#endif
+
+/*
+ * Given BASE and SIZE this macro calculates the number of pages the
+ * memory regions occupies
+ */
+#define MOD_NUMBER_OF_PAGES(BASE, SIZE) (((SIZE) > 0) ?		\
+		(PFN_DOWN((unsigned long)(BASE) + (SIZE) - 1) -	\
+			 PFN_DOWN((unsigned long)BASE) + 1)	\
+		: (0UL))
+
 /* If this is set, the section belongs in the init part of the module */
 #define INIT_OFFSET_MASK (1UL << (BITS_PER_LONG-1))
 
@@ -1542,6 +1563,115 @@ static int __unlink_module(void *_mod)
 	return 0;
 }
 
+#ifdef CONFIG_DEBUG_SET_MODULE_RONX
+/*
+ * LKM RO/NX protection: protect module's text/ro-data
+ * from modification and any data from execution.
+ */
+void set_page_attributes(void *start, void *end, int (*set)(unsigned long start, int num_pages))
+{
+	unsigned long begin_pfn = PFN_DOWN((unsigned long)start);
+	unsigned long end_pfn = PFN_DOWN((unsigned long)end);
+
+	if (end_pfn > begin_pfn)
+		set(begin_pfn << PAGE_SHIFT, end_pfn - begin_pfn);
+}
+
+static void set_section_ro_nx(void *base,
+			unsigned long text_size,
+			unsigned long ro_size,
+			unsigned long total_size)
+{
+	/* begin and end PFNs of the current subsection */
+	unsigned long begin_pfn;
+	unsigned long end_pfn;
+
+	/*
+	 * Set RO for module text and RO-data:
+	 * - Always protect first page.
+	 * - Do not protect last partial page.
+	 */
+	if (ro_size > 0)
+		set_page_attributes(base, base + ro_size, set_memory_ro);
+
+	/*
+	 * Set NX permissions for module data:
+	 * - Do not protect first partial page.
+	 * - Always protect last page.
+	 */
+	if (total_size > text_size) {
+		begin_pfn = PFN_UP((unsigned long)base + text_size);
+		end_pfn = PFN_UP((unsigned long)base + total_size);
+		if (end_pfn > begin_pfn)
+			set_memory_nx(begin_pfn << PAGE_SHIFT, end_pfn - begin_pfn);
+	}
+}
+
+/* Setting memory back to RW+NX before releasing it */
+void unset_section_ro_nx(struct module *mod, void *module_region)
+{
+	unsigned long total_pages;
+
+	if (mod->module_core == module_region) {
+		/* Set core as NX+RW */
+		total_pages = MOD_NUMBER_OF_PAGES(mod->module_core, mod->core_size);
+		set_memory_nx((unsigned long)mod->module_core, total_pages);
+		set_memory_rw((unsigned long)mod->module_core, total_pages);
+
+	} else if (mod->module_init == module_region) {
+		/* Set init as NX+RW */
+		total_pages = MOD_NUMBER_OF_PAGES(mod->module_init, mod->init_size);
+		set_memory_nx((unsigned long)mod->module_init, total_pages);
+		set_memory_rw((unsigned long)mod->module_init, total_pages);
+	}
+}
+
+/* Iterate through all modules and set each module's text as RW */
+void set_all_modules_text_rw()
+{
+	struct module *mod;
+
+	mutex_lock(&module_mutex);
+	list_for_each_entry_rcu(mod, &modules, list) {
+		if ((mod->module_core) && (mod->core_text_size)) {
+			set_page_attributes(mod->module_core,
+						mod->module_core + mod->core_text_size,
+						set_memory_rw);
+		}
+		if ((mod->module_init) && (mod->init_text_size)) {
+			set_page_attributes(mod->module_init,
+						mod->module_init + mod->init_text_size,
+						set_memory_rw);
+		}
+	}
+	mutex_unlock(&module_mutex);
+}
+
+/* Iterate through all modules and set each module's text as RO */
+void set_all_modules_text_ro()
+{
+	struct module *mod;
+
+	mutex_lock(&module_mutex);
+	list_for_each_entry_rcu(mod, &modules, list) {
+		if ((mod->module_core) && (mod->core_text_size)) {
+			set_page_attributes(mod->module_core,
+						mod->module_core + mod->core_text_size,
+						set_memory_ro);
+		}
+		if ((mod->module_init) && (mod->init_text_size)) {
+			set_page_attributes(mod->module_init,
+						mod->module_init + mod->init_text_size,
+						set_memory_ro);
+		}
+	}
+	mutex_unlock(&module_mutex);
+}
+#else
+static inline void set_section_ro_nx(void *base, unsigned long text_size, unsigned long ro_size, unsigned long total_size) { }
+static inline void unset_section_ro_nx(struct module *mod, void *module_region) { }
+#endif
+
 /* Free a module, remove from lists, etc. */
 static void free_module(struct module *mod)
 {
@@ -1566,6 +1696,7 @@ static void free_module(struct module *mod)
 	destroy_params(mod->kp, mod->num_kp);
 
 	/* This may be NULL, but that's OK */
+	unset_section_ro_nx(mod, mod->module_init);
 	module_free(mod, mod->module_init);
 	kfree(mod->args);
 	percpu_modfree(mod);
@@ -1574,6 +1705,7 @@ static void free_module(struct module *mod)
 	lockdep_free_key_range(mod->module_core, mod->core_size);
 
 	/* Finally, free the core (containing the module structure) */
+	unset_section_ro_nx(mod, mod->module_core);
 	module_free(mod, mod->module_core);
 
 #ifdef CONFIG_MPU
@@ -1777,8 +1909,19 @@ static void layout_sections(struct module *mod, struct load_info *info)
 			s->sh_entsize = get_offset(mod, &mod->core_size, s, i);
 			DEBUGP("\t%s\n", name);
 		}
-		if (m == 0)
+		switch (m) {
+		case 0: /* executable */
+			mod->core_size = debug_align(mod->core_size);
 			mod->core_text_size = mod->core_size;
+			break;
+		case 1: /* RO: text and ro-data */
+			mod->core_size = debug_align(mod->core_size);
+			mod->core_ro_size = mod->core_size;
+			break;
+		case 3: /* whole core */
+			mod->core_size = debug_align(mod->core_size);
+			break;
+		}
 	}
 
 	DEBUGP("Init section allocation order:\n");
@@ -1796,8 +1939,19 @@ static void layout_sections(struct module *mod, struct load_info *info)
 					 | INIT_OFFSET_MASK);
 			DEBUGP("\t%s\n", sname);
 		}
-		if (m == 0)
+		switch (m) {
+		case 0: /* executable */
+			mod->init_size = debug_align(mod->init_size);
 			mod->init_text_size = mod->init_size;
+			break;
+		case 1: /* RO: text and ro-data */
+			mod->init_size = debug_align(mod->init_size);
+			mod->init_ro_size = mod->init_size;
+			break;
+		case 3: /* whole init */
+			mod->init_size = debug_align(mod->init_size);
+			break;
+		}
 	}
 }
 
@@ -2650,6 +2804,18 @@ static struct module *load_module(void __user *umod,
 	kfree(info.strmap);
 	free_copy(&info);
 
+	/* Set RO and NX regions for core */
+	set_section_ro_nx(mod->module_core,
+				mod->core_text_size,
+				mod->core_ro_size,
+				mod->core_size);
+
+	/* Set RO and NX regions for init */
+	set_section_ro_nx(mod->module_init,
+				mod->init_text_size,
+				mod->init_ro_size,
+				mod->init_size);
+
 	/* Done! */
 	trace_module_load(mod);
 	return mod;
@@ -2753,6 +2919,7 @@ SYSCALL_DEFINE3(init_module, void __user *, umod,
 	mod->symtab = mod->core_symtab;
 	mod->strtab = mod->core_strtab;
 #endif
+	unset_section_ro_nx(mod, mod->module_init);
 	module_free(mod, mod->module_init);
 	mod->module_init = NULL;
 	mod->init_size = 0;

Re: [PATCH 3/3 V13] RO/NX protection for loadable kernel

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 3438 bytes --]

On Tue, 16 Nov 2010 22:35:16 +0100, matthieu castet said:

> This patch is a logical extension of the protection provided by
> CONFIG_DEBUG_RODATA to LKMs. The protection is provided by splitting
> module_core and module_init into three logical parts each and setting
> appropriate page access permissions for each individual section:
> 
>  1. Code: RO+X
>  2. RO data: RO+NX
>  3. RW data: RW+NX

This is incompatible with CONFIG_JUMP_LABEL:

[  252.093624] BUG: unable to handle kernel paging request at ffffffffa0680764
[  252.094008] IP: [<ffffffff81225ee0>] generic_swap+0xa/0x1a
[  252.094008] PGD 1a1e067 PUD 1a22063 PMD 1093ac067 PTE 8000000109786161
[  252.094008] Oops: 0003 [#1] PREEMPT SMP 

[  252.094008] Pid: 3740, comm: modprobe Not tainted 2.6.37-rc3-mmotm1123 #1 0X564R/Latitude E6500                  
[  252.094008] RIP: 0010:[<ffffffff81225ee0>]  [<ffffffff81225ee0>] generic_swap+0xa/0x1a
[  252.094008] RSP: 0018:ffff88011a217d98  EFLAGS: 00010206
[  252.094008] RAX: 00000000000000d9 RBX: 0000000000000030 RCX: 000000000000007c
[  252.094008] RDX: 0000000000000017 RSI: ffffffffa0680794 RDI: ffffffffa0680764
[  252.094008] RBP: ffff88011a217d98 R08: 0000000000000000 R09: ffff88011a217d38
[  252.094008] R10: 0000000000000000 R11: 000000000000013d R12: ffffffffa0680764
[  252.094008] R13: 0000000000000018 R14: 0000000000000018 R15: 0000000000000018
[  252.094008] FS:  00007f6ecb897720(0000) GS:ffff8800df100000(0000) knlGS:0000000000000000
[  252.094008] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[  252.094008] CR2: ffffffffa0680764 CR3: 000000011911e000 CR4: 00000000000406e0
[  252.094008] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  252.094008] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[  252.094008] Process modprobe (pid: 3740, threadinfo ffff88011a216000, task ffff88011861e300)
[  252.094008] Stack:
[  252.094008]  ffff88011a217e28 ffffffff81226007 ffffffff81a31158 0000000000000000
[  252.094008]  0000000000000030 0000000000000048 ffffffff8105fa34 ffffffff81225ed6
[  252.094008]  ffffffff00000018 ffffffff00000018 0000000000000018 00000030ffffffff
[  252.094008] Call Trace:
[  252.094008]  [<ffffffff81226007>] sort+0x117/0x1b0
[  252.094008]  [<ffffffff8105fa34>] ? jump_label_cmp+0x0/0x1b
[  252.094008]  [<ffffffff81225ed6>] ? generic_swap+0x0/0x1a
[  252.094008]  [<ffffffff8105faa6>] sort_jump_label_entries+0x2b/0x2d
[  252.094008]  [<ffffffff8105feda>] jump_label_module_notify+0x58/0x253
[  252.094008]  [<ffffffff8155e816>] notifier_call_chain+0x54/0x81
[  252.094008]  [<ffffffff8105d9b2>] __blocking_notifier_call_chain+0x5c/0x79
[  252.094008]  [<ffffffff8105d9de>] blocking_notifier_call_chain+0xf/0x11
[  252.094008]  [<ffffffff810778cc>] sys_init_module+0x76/0x1f5
[  252.094008]  [<ffffffff8100277b>] system_call_fastpath+0x16/0x1b
[  252.094008] Code: 05 ff c0 48 29 f7 48 39 f7 73 f6 48 89 3a c9 c3 90 90 90 8b 07 8b 16 55 89 17 48 89 e5 89 06 c9 c3 55 48 89 e5 8a 07 8a 0e ff ca <88> 0f 48 ff c7 88 06 48 ff c6 85 d2 7f ec c9 c3 55 89 d0 48 89
[  252.094008] RIP  [<ffffffff81225ee0>] generic_swap+0xa/0x1a
[  252.094008]  RSP <ffff88011a217d98>
[  252.094008] CR2: ffffffffa0680764
[  252.094008] ---[ end trace f88479be6b01e4c4 ]---


> +config DEBUG_SET_MODULE_RONX
> +	bool "Set loadable kernel module data as NX and text as RO"
> +	default n
> +	depends on X86 && MODULES

	depends on X86 && MODULES && !JUMP_LABEL

[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

Re: [PATCH 3/3 V13] RO/NX protection for loadable kernel

[mat]

[-- Attachment #1: Type: text/plain, Size: 3837 bytes --]

Le Wed, 24 Nov 2010 22:41:07 -0500,
Valdis.Kletnieks@vt.edu a écrit :

> On Tue, 16 Nov 2010 22:35:16 +0100, matthieu castet said:
> 
> > This patch is a logical extension of the protection provided by
> > CONFIG_DEBUG_RODATA to LKMs. The protection is provided by splitting
> > module_core and module_init into three logical parts each and
> > setting appropriate page access permissions for each individual
> > section:
> > 
> >  1. Code: RO+X
> >  2. RO data: RO+NX
> >  3. RW data: RW+NX
> 
> This is incompatible with CONFIG_JUMP_LABEL:
> 
> [  252.093624] BUG: unable to handle kernel paging request at
> ffffffffa0680764 [  252.094008] IP: [<ffffffff81225ee0>]
> generic_swap+0xa/0x1a [  252.094008] PGD 1a1e067 PUD 1a22063 PMD
> 1093ac067 PTE 8000000109786161 [  252.094008] Oops: 0003 [#1] PREEMPT
> SMP 
> 
> [  252.094008] Pid: 3740, comm: modprobe Not tainted
> 2.6.37-rc3-mmotm1123 #1 0X564R/Latitude E6500 [  252.094008] RIP:
> 0010:[<ffffffff81225ee0>]  [<ffffffff81225ee0>] generic_swap+0xa/0x1a
> [  252.094008] RSP: 0018:ffff88011a217d98  EFLAGS: 00010206
> [  252.094008] RAX: 00000000000000d9 RBX: 0000000000000030 RCX:
> 000000000000007c [  252.094008] RDX: 0000000000000017 RSI:
> ffffffffa0680794 RDI: ffffffffa0680764 [  252.094008] RBP:
> ffff88011a217d98 R08: 0000000000000000 R09: ffff88011a217d38
> [  252.094008] R10: 0000000000000000 R11: 000000000000013d R12:
> ffffffffa0680764 [  252.094008] R13: 0000000000000018 R14:
> 0000000000000018 R15: 0000000000000018 [  252.094008] FS:
> 00007f6ecb897720(0000) GS:ffff8800df100000(0000)
> knlGS:0000000000000000 [  252.094008] CS:  0010 DS: 0000 ES: 0000
> CR0: 000000008005003b [  252.094008] CR2: ffffffffa0680764 CR3:
> 000000011911e000 CR4: 00000000000406e0 [  252.094008] DR0:
> 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [  252.094008] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7:
> 0000000000000400 [  252.094008] Process modprobe (pid: 3740,
> threadinfo ffff88011a216000, task ffff88011861e300) [  252.094008]
> Stack: [  252.094008]  ffff88011a217e28 ffffffff81226007
> ffffffff81a31158 0000000000000000 [  252.094008]  0000000000000030
> 0000000000000048 ffffffff8105fa34 ffffffff81225ed6 [  252.094008]
> ffffffff00000018 ffffffff00000018 0000000000000018 00000030ffffffff
> [  252.094008] Call Trace: [  252.094008]  [<ffffffff81226007>]
> sort+0x117/0x1b0 [  252.094008]  [<ffffffff8105fa34>] ?
> jump_label_cmp+0x0/0x1b [  252.094008]  [<ffffffff81225ed6>] ?
> generic_swap+0x0/0x1a [  252.094008]  [<ffffffff8105faa6>]
> sort_jump_label_entries+0x2b/0x2d [  252.094008]
> [<ffffffff8105feda>] jump_label_module_notify+0x58/0x253
> [  252.094008]  [<ffffffff8155e816>] notifier_call_chain+0x54/0x81
> [  252.094008]  [<ffffffff8105d9b2>]
> __blocking_notifier_call_chain+0x5c/0x79 [  252.094008]
> [<ffffffff8105d9de>] blocking_notifier_call_chain+0xf/0x11
> [  252.094008]  [<ffffffff810778cc>] sys_init_module+0x76/0x1f5
> [  252.094008]  [<ffffffff8100277b>] system_call_fastpath+0x16/0x1b
> [  252.094008] Code: 05 ff c0 48 29 f7 48 39 f7 73 f6 48 89 3a c9 c3
> 90 90 90 8b 07 8b 16 55 89 17 48 89 e5 89 06 c9 c3 55 48 89 e5 8a 07
> 8a 0e ff ca <88> 0f 48 ff c7 88 06 48 ff c6 85 d2 7f ec c9 c3 55 89
> d0 48 89 [  252.094008] RIP  [<ffffffff81225ee0>]
> generic_swap+0xa/0x1a [  252.094008]  RSP <ffff88011a217d98>
> [  252.094008] CR2: ffffffffa0680764 [  252.094008] ---[ end trace
> f88479be6b01e4c4 ]---
> 
> 
> > +config DEBUG_SET_MODULE_RONX
> > +	bool "Set loadable kernel module data as NX and text as RO"
> > +	default n
> > +	depends on X86 && MODULES
> 
> 	depends on X86 && MODULES && !JUMP_LABEL
could you try the attached patch ?

on module load, we sort the __jump_table section. So we should make it
writable.


Matthieu

[-- Attachment #2: jmp_label.diff --]
[-- Type: text/x-patch, Size: 481 bytes --]

diff --git a/arch/x86/include/asm/jump_label.h b/arch/x86/include/asm/jump_label.h
index f52d42e..574dbc2 100644
--- a/arch/x86/include/asm/jump_label.h
+++ b/arch/x86/include/asm/jump_label.h
@@ -14,7 +14,7 @@
 	do {							\
 		asm goto("1:"					\
 			JUMP_LABEL_INITIAL_NOP			\
-			".pushsection __jump_table,  \"a\" \n\t"\
+			".pushsection __jump_table,  \"aw\" \n\t"\
 			_ASM_PTR "1b, %l[" #label "], %c0 \n\t" \
 			".popsection \n\t"			\
 			: :  "i" (key) :  : label);		\

Re: [PATCH 3/3 V13] RO/NX protection for loadable kernel

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 1239 bytes --]

On Fri, 26 Nov 2010 18:23:55 +0100, mat said:

> Le Wed, 24 Nov 2010 22:41:07 -0500,
> Valdis.Kletnieks@vt.edu a =E9crit :

> > This is incompatible with CONFIG_JUMP_LABEL:
> >
> > [  252.093624] BUG: unable to handle kernel paging request at
> > ffffffffa0680764 [  252.094008] IP: [<ffffffff81225ee0>]
> > generic_swap+0xa/0x1a [  252.094008] PGD 1a1e067 PUD 1a22063 PMD
> > 1093ac067 PTE 8000000109786161 [  252.094008] Oops: 0003 [#1] PREEMPT
> > SMP

> > > +config DEBUG_SET_MODULE_RONX
> > > +	bool "Set loadable kernel module data as NX and text as RO"
> > > +	default n
> > > +	depends on X86 && MODULES
> >
> > 	depends on X86 && MODULES && !JUMP_LABEL
> could you try the attached patch ?
> 
> on module load, we sort the __jump_table section. So we should make it
> writable.

> diff --git a/arch/x86/include/asm/jump_label.h b/arch/x86/include/asm/jump_la
bel.h
> index f52d42e..574dbc2 100644
> --- a/arch/x86/include/asm/jump_label.h
> +++ b/arch/x86/include/asm/jump_label.h
> @@ -14,7 +14,7 @@
>  	do {							\
>  		asm goto("1:"					\
>  			JUMP_LABEL_INITIAL_NOP			\
> -			".pushsection __jump_table,  \"a\" \n\t"\
> +			".pushsection __jump_table,  \"aw\" \n\t"\

Confirming that fixes the issue I was seeing, thanks...

[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

Re: [PATCH 3/3 V13] RO/NX protection for loadable kernel

[Steven Rostedt]

This patch breaks function tracer:

------------[ cut here ]------------
WARNING: at kernel/trace/ftrace.c:1014 ftrace_bug+0x114/0x171()
Hardware name: Precision WorkStation 470    
Modules linked in: i2c_core(+)
Pid: 86, comm: modprobe Not tainted 2.6.37-rc2+ #68
Call Trace:
 [<ffffffff8104e957>] warn_slowpath_common+0x85/0x9d
 [<ffffffffa00026db>] ? __process_new_adapter+0x7/0x34 [i2c_core]
 [<ffffffffa00026db>] ? __process_new_adapter+0x7/0x34 [i2c_core]
 [<ffffffff8104e989>] warn_slowpath_null+0x1a/0x1c
 [<ffffffff810a9dfe>] ftrace_bug+0x114/0x171
 [<ffffffffa00026db>] ? __process_new_adapter+0x7/0x34 [i2c_core]
 [<ffffffff810aa0db>] ftrace_process_locs+0x1ae/0x274
 [<ffffffffa00026db>] ? __process_new_adapter+0x7/0x34 [i2c_core]
 [<ffffffff810aa29e>] ftrace_module_notify+0x39/0x44
 [<ffffffff814405cf>] notifier_call_chain+0x37/0x63
 [<ffffffff8106e054>] __blocking_notifier_call_chain+0x46/0x5b
 [<ffffffff8106e07d>] blocking_notifier_call_chain+0x14/0x16
 [<ffffffff8107ffde>] sys_init_module+0x73/0x1f3
 [<ffffffff8100acf2>] system_call_fastpath+0x16/0x1b
---[ end trace 2aff4f4ca53ec746 ]---
ftrace faulted on writing [<ffffffffa00026db>]
__process_new_adapter+0x7/0x34 [i2c_core]


On Tue, Nov 16, 2010 at 10:35:16PM +0100, matthieu castet wrote:
>  
> @@ -2650,6 +2810,18 @@ static struct module *load_module(void __user *umod,
>  	kfree(info.strmap);
>  	free_copy(&info);
>  
> +	/* Set RO and NX regions for core */
> +	set_section_ro_nx(mod->module_core,
> +				mod->core_text_size,
> +				mod->core_ro_size,
> +				mod->core_size);
> +
> +	/* Set RO and NX regions for init */
> +	set_section_ro_nx(mod->module_init,
> +				mod->init_text_size,
> +				mod->init_ro_size,
> +				mod->init_size);
> +

Here we set the text read only before we call the notifiers. The
function tracer changes the calls to mcount into nops via a notifier
call so this must be done after the module notifiers.

>  	/* Done! */
>  	trace_module_load(mod);
>  	return mod;
> @@ -2753,6 +2925,7 @@ SYSCALL_DEFINE3(init_module, void __user *, umod,
>  	mod->symtab = mod->core_symtab;
>  	mod->strtab = mod->core_strtab;
>  #endif
> +	unset_section_ro_nx(mod, mod->module_init);
>  	module_free(mod, mod->module_init);
>  	mod->module_init = NULL;
>  	mod->init_size = 0;

Below is the patch that fixes this bug.

-- Steve

Reported-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

diff --git a/kernel/module.c b/kernel/module.c
index ba421e6..5ccf52c 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2804,18 +2804,6 @@ static struct module *load_module(void __user *umod,
 	kfree(info.strmap);
 	free_copy(&info);
 
-	/* Set RO and NX regions for core */
-	set_section_ro_nx(mod->module_core,
-				mod->core_text_size,
-				mod->core_ro_size,
-				mod->core_size);
-
-	/* Set RO and NX regions for init */
-	set_section_ro_nx(mod->module_init,
-				mod->init_text_size,
-				mod->init_ro_size,
-				mod->init_size);
-
 	/* Done! */
 	trace_module_load(mod);
 	return mod;
@@ -2876,6 +2864,18 @@ SYSCALL_DEFINE3(init_module, void __user *, umod,
 	blocking_notifier_call_chain(&module_notify_list,
 			MODULE_STATE_COMING, mod);
 
+	/* Set RO and NX regions for core */
+	set_section_ro_nx(mod->module_core,
+				mod->core_text_size,
+				mod->core_ro_size,
+				mod->core_size);
+
+	/* Set RO and NX regions for init */
+	set_section_ro_nx(mod->module_init,
+				mod->init_text_size,
+				mod->init_ro_size,
+				mod->init_size);
+
 	do_mod_ctors(mod);
 	/* Start the module */
 	if (mod->init != NULL)

Re: [PATCH 3/3 V13] RO/NX protection for loadable kernel

[Rusty Russell]

On Tue, 30 Nov 2010 04:45:42 am Steven Rostedt wrote:
> This patch breaks function tracer:
...
> Here we set the text read only before we call the notifiers. The
> function tracer changes the calls to mcount into nops via a notifier
> call so this must be done after the module notifiers.

That seems fine.

I note that both before and after this patch we potentially execute code
in the module (via parse_args) before we set it ro & nx.  But fixing this
last bit of coverage is probably not worth the pain...

Cheers,
Rusty.

Re: [PATCH 3/3 V13] RO/NX protection for loadable kernel

[Steven Rostedt]

On Tue, 2010-11-30 at 10:05 +1030, Rusty Russell wrote:
> On Tue, 30 Nov 2010 04:45:42 am Steven Rostedt wrote:
> > This patch breaks function tracer:
> ...
> > Here we set the text read only before we call the notifiers. The
> > function tracer changes the calls to mcount into nops via a notifier
> > call so this must be done after the module notifiers.
> 
> That seems fine.
> 
> I note that both before and after this patch we potentially execute code
> in the module (via parse_args) before we set it ro & nx.  But fixing this
> last bit of coverage is probably not worth the pain...

Rusty, can I take this as an "Acked-by"?

Thanks,

-- Steve

Re: [PATCH 3/3 V13] RO/NX protection for loadable kernel

[mat]

Hi,

Le Mon, 29 Nov 2010 13:15:42 -0500,
Steven Rostedt <rostedt@goodmis.org> a écrit :

> This patch breaks function tracer:
> 
> ------------[ cut here ]------------
> WARNING: at kernel/trace/ftrace.c:1014 ftrace_bug+0x114/0x171()
> Hardware name: Precision WorkStation 470    
> Modules linked in: i2c_core(+)
> Pid: 86, comm: modprobe Not tainted 2.6.37-rc2+ #68
> Call Trace:
>  [<ffffffff8104e957>] warn_slowpath_common+0x85/0x9d
>  [<ffffffffa00026db>] ? __process_new_adapter+0x7/0x34 [i2c_core]
>  [<ffffffffa00026db>] ? __process_new_adapter+0x7/0x34 [i2c_core]
>  [<ffffffff8104e989>] warn_slowpath_null+0x1a/0x1c
>  [<ffffffff810a9dfe>] ftrace_bug+0x114/0x171
>  [<ffffffffa00026db>] ? __process_new_adapter+0x7/0x34 [i2c_core]
>  [<ffffffff810aa0db>] ftrace_process_locs+0x1ae/0x274
>  [<ffffffffa00026db>] ? __process_new_adapter+0x7/0x34 [i2c_core]
>  [<ffffffff810aa29e>] ftrace_module_notify+0x39/0x44
>  [<ffffffff814405cf>] notifier_call_chain+0x37/0x63
>  [<ffffffff8106e054>] __blocking_notifier_call_chain+0x46/0x5b
>  [<ffffffff8106e07d>] blocking_notifier_call_chain+0x14/0x16
>  [<ffffffff8107ffde>] sys_init_module+0x73/0x1f3
>  [<ffffffff8100acf2>] system_call_fastpath+0x16/0x1b
> ---[ end trace 2aff4f4ca53ec746 ]---
> ftrace faulted on writing [<ffffffffa00026db>]
> __process_new_adapter+0x7/0x34 [i2c_core]
> 
> 
> On Tue, Nov 16, 2010 at 10:35:16PM +0100, matthieu castet wrote:
> >  
> > @@ -2650,6 +2810,18 @@ static struct module *load_module(void
> > __user *umod, kfree(info.strmap);
> >  	free_copy(&info);
> >  
> > +	/* Set RO and NX regions for core */
> > +	set_section_ro_nx(mod->module_core,
> > +				mod->core_text_size,
> > +				mod->core_ro_size,
> > +				mod->core_size);
> > +
> > +	/* Set RO and NX regions for init */
> > +	set_section_ro_nx(mod->module_init,
> > +				mod->init_text_size,
> > +				mod->init_ro_size,
> > +				mod->init_size);
> > +
> 
> Here we set the text read only before we call the notifiers. The
> function tracer changes the calls to mcount into nops via a notifier
> call so this must be done after the module notifiers.
> 
> >  	/* Done! */
> >  	trace_module_load(mod);
> >  	return mod;
> > @@ -2753,6 +2925,7 @@ SYSCALL_DEFINE3(init_module, void __user *,
> > umod, mod->symtab = mod->core_symtab;
> >  	mod->strtab = mod->core_strtab;
> >  #endif
> > +	unset_section_ro_nx(mod, mod->module_init);
> >  	module_free(mod, mod->module_init);
> >  	mod->module_init = NULL;
> >  	mod->init_size = 0;
> 
> Below is the patch that fixes this bug.
> 
> -- Steve
> 
> Reported-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index ba421e6..5ccf52c 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2804,18 +2804,6 @@ static struct module *load_module(void __user
> *umod, kfree(info.strmap);
>  	free_copy(&info);
>  
> -	/* Set RO and NX regions for core */
> -	set_section_ro_nx(mod->module_core,
> -				mod->core_text_size,
> -				mod->core_ro_size,
> -				mod->core_size);
> -
> -	/* Set RO and NX regions for init */
> -	set_section_ro_nx(mod->module_init,
> -				mod->init_text_size,
> -				mod->init_ro_size,
> -				mod->init_size);
> -
>  	/* Done! */
>  	trace_module_load(mod);
>  	return mod;
> @@ -2876,6 +2864,18 @@ SYSCALL_DEFINE3(init_module, void __user *,
> umod, blocking_notifier_call_chain(&module_notify_list,
>  			MODULE_STATE_COMING, mod);
>  
> +	/* Set RO and NX regions for core */
> +	set_section_ro_nx(mod->module_core,
> +				mod->core_text_size,
> +				mod->core_ro_size,
> +				mod->core_size);
> +
> +	/* Set RO and NX regions for init */
> +	set_section_ro_nx(mod->module_init,
> +				mod->init_text_size,
> +				mod->init_ro_size,
> +				mod->init_size);
> +
>  	do_mod_ctors(mod);
>  	/* Start the module */
>  	if (mod->init != NULL)
> 
> 

That's look fine for me.

But I wonder why ftrace_arch_code_modify_prepare isn't called ?

It is only called when we start/stop tracing ?

Matthieu

Re: [PATCH 3/3 V13] RO/NX protection for loadable kernel

[Steven Rostedt]

On Tue, 2010-11-30 at 22:20 +0100, mat wrote:

> That's look fine for me.
> 
> But I wonder why ftrace_arch_code_modify_prepare isn't called ?
> 
> It is only called when we start/stop tracing ?

Correct. There's no reason to use it for the changing of mcount callers
to nops. For core kernel code, it happens before SMP is enabled. For
modules, it happens before the module code is executed. Except for what
Rusty stated with the module parameters, but if you are executing
complex code with that, you deserve what you get ;-)

The initial changes are made outside of stop machine. The code is not
being executed by anyone else.

-- Steve

Re: [PATCH 3/3 V13] RO/NX protection for loadable kernel

[Rusty Russell]

On Wed, 1 Dec 2010 01:16:23 am Steven Rostedt wrote:
> On Tue, 2010-11-30 at 10:05 +1030, Rusty Russell wrote:
> > On Tue, 30 Nov 2010 04:45:42 am Steven Rostedt wrote:
> > > This patch breaks function tracer:
> > ...
> > > Here we set the text read only before we call the notifiers. The
> > > function tracer changes the calls to mcount into nops via a notifier
> > > call so this must be done after the module notifiers.
> > 
> > That seems fine.
> > 
> > I note that both before and after this patch we potentially execute code
> > in the module (via parse_args) before we set it ro & nx.  But fixing this
> > last bit of coverage is probably not worth the pain...
> 
> Rusty, can I take this as an "Acked-by"?

Yep... Acked-by: Rusty Russell <rusty@rustcorp.com.au>

Thanks,
Rusty.

Re: [PATCH 3/3 V13] RO/NX protection for loadable kernel

[Kees Cook]

On Fri, Nov 26, 2010 at 06:23:55PM +0100, mat wrote:
> could you try the attached patch ?
> 
> on module load, we sort the __jump_table section. So we should make it
> writable.
> 
> 
> Matthieu

> diff --git a/arch/x86/include/asm/jump_label.h b/arch/x86/include/asm/jump_label.h
> index f52d42e..574dbc2 100644
> --- a/arch/x86/include/asm/jump_label.h
> +++ b/arch/x86/include/asm/jump_label.h
> @@ -14,7 +14,7 @@
>  	do {							\
>  		asm goto("1:"					\
>  			JUMP_LABEL_INITIAL_NOP			\
> -			".pushsection __jump_table,  \"a\" \n\t"\
> +			".pushsection __jump_table,  \"aw\" \n\t"\
>  			_ASM_PTR "1b, %l[" #label "], %c0 \n\t" \
>  			".popsection \n\t"			\
>  			: :  "i" (key) :  : label);		\

Acked-by: Kees Cook <kees.cook@canonical.com>

Can this please get committed to tip?

Thanks,

-Kees

-- 
Kees Cook
Ubuntu Security Team

Re: [PATCH 3/3 V13] RO/NX protection for loadable kernel

[mat]

Le Wed, 8 Dec 2010 14:19:51 -0800,
Kees Cook <kees.cook@canonical.com> a écrit :

> On Fri, Nov 26, 2010 at 06:23:55PM +0100, mat wrote:
> > could you try the attached patch ?
> > 
> > on module load, we sort the __jump_table section. So we should make
> > it writable.
> > 
> > 
> > Matthieu
> 
> > diff --git a/arch/x86/include/asm/jump_label.h
> > b/arch/x86/include/asm/jump_label.h index f52d42e..574dbc2 100644
> > --- a/arch/x86/include/asm/jump_label.h
> > +++ b/arch/x86/include/asm/jump_label.h
> > @@ -14,7 +14,7 @@
> >  	do
> > {							\ asm
> > goto("1:"					\
> > JUMP_LABEL_INITIAL_NOP			\
> > -			".pushsection __jump_table,  \"a\" \n\t"\
> > +			".pushsection __jump_table,  \"aw\" \n\t"\
> >  			_ASM_PTR "1b, %l[" #label "], %c0 \n\t" \
> >  			".popsection \n\t"			\
> >  			: :  "i" (key) :  : label);
> > \
> 
> Acked-by: Kees Cook <kees.cook@canonical.com>
> 
> Can this please get committed to tip?
I think it is not need anymore with  Steven Rostedt patch [1]

Matthieu

[1]
> > Here we set the text read only before we call the notifiers. The
> > function tracer changes the calls to mcount into nops via a notifier
> > call so this must be done after the module notifiers.

Re: [PATCH 3/3 V13] RO/NX protection for loadable kernel

[Kees Cook]

Hi,

On Sat, Dec 11, 2010 at 12:18:57AM +0100, mat wrote:
> Le Wed, 8 Dec 2010 14:19:51 -0800,
> Kees Cook <kees.cook@canonical.com> a écrit :
> 
> > On Fri, Nov 26, 2010 at 06:23:55PM +0100, mat wrote:
> > > could you try the attached patch ?
> > > 
> > > on module load, we sort the __jump_table section. So we should make
> > > it writable.
> > > 
> > > 
> > > Matthieu
> > 
> > > diff --git a/arch/x86/include/asm/jump_label.h
> > > b/arch/x86/include/asm/jump_label.h index f52d42e..574dbc2 100644
> > > --- a/arch/x86/include/asm/jump_label.h
> > > +++ b/arch/x86/include/asm/jump_label.h
> > > @@ -14,7 +14,7 @@
> > >  	do
> > > {							\ asm
> > > goto("1:"					\
> > > JUMP_LABEL_INITIAL_NOP			\
> > > -			".pushsection __jump_table,  \"a\" \n\t"\
> > > +			".pushsection __jump_table,  \"aw\" \n\t"\
> > >  			_ASM_PTR "1b, %l[" #label "], %c0 \n\t" \
> > >  			".popsection \n\t"			\
> > >  			: :  "i" (key) :  : label);
> > > \
> > 
> > Acked-by: Kees Cook <kees.cook@canonical.com>
> > 
> > Can this please get committed to tip?
> I think it is not need anymore with  Steven Rostedt patch [1]
> 
> Matthieu
> 
> [1]
> > > Here we set the text read only before we call the notifiers. The
> > > function tracer changes the calls to mcount into nops via a notifier
> > > call so this must be done after the module notifiers.

Which patch was that? (Or, rather, what's a good url to see it?)

-Kees

-- 
Kees Cook
Ubuntu Security Team

Re: [PATCH 3/3 V13] RO/NX protection for loadable kernel

[Kees Cook]

On Sat, Dec 11, 2010 at 11:57:35AM +0100, mat wrote:
> http://marc.info/?l=linux-security-module&m=129105456828505&w=2

Ah-ha! Thanks for the pointer. So, instead of the other explicit jump_table
fix, can this get pushed into tip?

Thanks!

-Kees

-- 
Kees Cook
Ubuntu Security Team

Re: [PATCH 3/3 V13] RO/NX protection for loadable kernel

[Ingo Molnar]

* mat <castet.matthieu@free.fr> wrote:

> Le Wed, 8 Dec 2010 14:19:51 -0800,
> Kees Cook <kees.cook@canonical.com> a écrit :
> 
> > On Fri, Nov 26, 2010 at 06:23:55PM +0100, mat wrote:
> > > could you try the attached patch ?
> > > 
> > > on module load, we sort the __jump_table section. So we should make
> > > it writable.
> > > 
> > > 
> > > Matthieu
> > 
> > > diff --git a/arch/x86/include/asm/jump_label.h
> > > b/arch/x86/include/asm/jump_label.h index f52d42e..574dbc2 100644
> > > --- a/arch/x86/include/asm/jump_label.h
> > > +++ b/arch/x86/include/asm/jump_label.h
> > > @@ -14,7 +14,7 @@
> > >  	do
> > > {							\ asm
> > > goto("1:"					\
> > > JUMP_LABEL_INITIAL_NOP			\
> > > -			".pushsection __jump_table,  \"a\" \n\t"\
> > > +			".pushsection __jump_table,  \"aw\" \n\t"\
> > >  			_ASM_PTR "1b, %l[" #label "], %c0 \n\t" \
> > >  			".popsection \n\t"			\
> > >  			: :  "i" (key) :  : label);
> > > \
> > 
> > Acked-by: Kees Cook <kees.cook@canonical.com>
> > 
> > Can this please get committed to tip?
> I think it is not need anymore with  Steven Rostedt patch [1]
> 
> Matthieu
> 
> [1]
> > > Here we set the text read only before we call the notifiers. The
> > > function tracer changes the calls to mcount into nops via a notifier
> > > call so this must be done after the module notifiers.

What's the status of this bug?

If we still need the patch then please submit it standalone with a proper subject 
line, with acks/signoffs added, etc.

Thanks,

	Ingo

Re: [PATCH 3/3 V13] RO/NX protection for loadable kernel

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 1904 bytes --]

On Wed, 22 Dec 2010 13:40:19 +0100, Ingo Molnar said:
> 
> * mat <castet.matthieu@free.fr> wrote:
> 
> > Le Wed, 8 Dec 2010 14:19:51 -0800,
> > Kees Cook <kees.cook@canonical.com> a écrit :
> > 
> > > On Fri, Nov 26, 2010 at 06:23:55PM +0100, mat wrote:
> > > > could you try the attached patch ?
> > > > 
> > > > on module load, we sort the __jump_table section. So we should make
> > > > it writable.
> > > > 
> > > > 
> > > > Matthieu
> > > 
> > > > diff --git a/arch/x86/include/asm/jump_label.h
> > > > b/arch/x86/include/asm/jump_label.h index f52d42e..574dbc2 100644
> > > > --- a/arch/x86/include/asm/jump_label.h
> > > > +++ b/arch/x86/include/asm/jump_label.h
> > > > @@ -14,7 +14,7 @@
> > > >  	do
> > > > {							\ asm
> > > > goto("1:"					\
> > > > JUMP_LABEL_INITIAL_NOP			\
> > > > -			".pushsection __jump_table,  \"a\" \n\t"\
> > > > +			".pushsection __jump_table,  \"aw\" \n\t"\
> > > >  			_ASM_PTR "1b, %l[" #label "], %c0 \n\t" \
> > > >  			".popsection \n\t"			\
> > > >  			: :  "i" (key) :  : label);
> > > > \
> > > 
> > > Acked-by: Kees Cook <kees.cook@canonical.com>
> > > 
> > > Can this please get committed to tip?
> > I think it is not need anymore with  Steven Rostedt patch [1]
> > 
> > Matthieu
> > 
> > [1]
> > > > Here we set the text read only before we call the notifiers. The
> > > > function tracer changes the calls to mcount into nops via a notifier
> > > > call so this must be done after the module notifiers.
> 
> What's the status of this bug?
> 
> If we still need the patch then please submit it standalone with a proper subject 
> line, with acks/signoffs added, etc.

Steve Rostedt's patch that moves the setting of the page permissions seems to
make this patch no longer necessary.  I tripped over this same issue, but the
version in the latest -mmotm does not need it, as it includes Steve's fix.

[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

Re: [PATCH 3/3 V13] RO/NX protection for loadable kernel

[Ingo Molnar]

* Valdis.Kletnieks@vt.edu <Valdis.Kletnieks@vt.edu> wrote:

> On Wed, 22 Dec 2010 13:40:19 +0100, Ingo Molnar said:
> > 
> > * mat <castet.matthieu@free.fr> wrote:
> > 
> > > Le Wed, 8 Dec 2010 14:19:51 -0800,
> > > Kees Cook <kees.cook@canonical.com> a écrit :
> > > 
> > > > On Fri, Nov 26, 2010 at 06:23:55PM +0100, mat wrote:
> > > > > could you try the attached patch ?
> > > > > 
> > > > > on module load, we sort the __jump_table section. So we should make
> > > > > it writable.
> > > > > 
> > > > > 
> > > > > Matthieu
> > > > 
> > > > > diff --git a/arch/x86/include/asm/jump_label.h
> > > > > b/arch/x86/include/asm/jump_label.h index f52d42e..574dbc2 100644
> > > > > --- a/arch/x86/include/asm/jump_label.h
> > > > > +++ b/arch/x86/include/asm/jump_label.h
> > > > > @@ -14,7 +14,7 @@
> > > > >  	do
> > > > > {							\ asm
> > > > > goto("1:"					\
> > > > > JUMP_LABEL_INITIAL_NOP			\
> > > > > -			".pushsection __jump_table,  \"a\" \n\t"\
> > > > > +			".pushsection __jump_table,  \"aw\" \n\t"\
> > > > >  			_ASM_PTR "1b, %l[" #label "], %c0 \n\t" \
> > > > >  			".popsection \n\t"			\
> > > > >  			: :  "i" (key) :  : label);
> > > > > \
> > > > 
> > > > Acked-by: Kees Cook <kees.cook@canonical.com>
> > > > 
> > > > Can this please get committed to tip?
> > > I think it is not need anymore with  Steven Rostedt patch [1]
> > > 
> > > Matthieu
> > > 
> > > [1]
> > > > > Here we set the text read only before we call the notifiers. The
> > > > > function tracer changes the calls to mcount into nops via a notifier
> > > > > call so this must be done after the module notifiers.
> > 
> > What's the status of this bug?
> > 
> > If we still need the patch then please submit it standalone with a proper subject 
> > line, with acks/signoffs added, etc.
> 
> Steve Rostedt's patch that moves the setting of the page permissions seems to make 
> this patch no longer necessary.  I tripped over this same issue, but the version 
> in the latest -mmotm does not need it, as it includes Steve's fix.

It would be nice to see that fix submitted so that it gets into the tree that 
introduced the bug.

Steve, Andrew?

Thanks,

	Ingo

Re: [PATCH 3/3 V13] RO/NX protection for loadable kernel

[Steven Rostedt]

On Wed, 2010-12-22 at 22:57 +0100, Ingo Molnar wrote:

> It would be nice to see that fix submitted so that it gets into the tree that 
> introduced the bug.
> 
> Steve, Andrew?

Hi Ingo,

Which tree/branch should I base this on? I could do write up formal
patch and push it out.

-- Steve

Re: [PATCH 3/3 V13] RO/NX protection for loadable kernel

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 2010-12-22 at 22:57 +0100, Ingo Molnar wrote:
> 
> > It would be nice to see that fix submitted so that it gets into the tree that 
> > introduced the bug.
> > 
> > Steve, Andrew?
> 
> Hi Ingo,
> 
> Which tree/branch should I base this on?

Please use tip:x86/security, it contains the RO/NX patches:

 691513f70d39: x86: Resume trampoline must be executable
 84e1c6bb38eb: x86: Add RO/NX protection for loadable kernel modules
 5bd5a452662b: x86: Add NX protection for kernel data
 64edc8ed5ffa: x86: Fix improper large page preservation

> [...] I could do write up formal patch and push it out.

Thanks!

	Ingo

Re: [PATCH 3/3 V13] RO/NX protection for loadable kernel

[Steven Rostedt]

On Wed, 2010-12-22 at 16:35 -0500, Valdis.Kletnieks@vt.edu wrote:

> Steve Rostedt's patch that moves the setting of the page permissions seems to
> make this patch no longer necessary.  I tripped over this same issue, but the
> version in the latest -mmotm does not need it, as it includes Steve's fix.

Can I add your "Tested-by: ..." to the commit?

Thanks,

-- Steve

Re: [PATCH 3/3 V13] RO/NX protection for loadable kernel

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 659 bytes --]

On Thu, 23 Dec 2010 10:01:48 EST, Steven Rostedt said:
> On Wed, 2010-12-22 at 16:35 -0500, Valdis.Kletnieks@vt.edu wrote:
> 
> > Steve Rostedt's patch that moves the setting of the page permissions seems to
> > make this patch no longer necessary.  I tripped over this same issue, but the
> > version in the latest -mmotm does not need it, as it includes Steve's fix.
> 
> Can I add your "Tested-by: ..." to the commit?

Sure.  I can verify that the patch series (a) doesn't break well-behaved
modules and (b) it *does* trigger for misbehaving modules (I sent the
VirtualBox crew a bug report for a module that tried to write to an area
marked executable).


[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

Re: [PATCH 3/3 V13] RO/NX protection for loadable kernel

[Xiaotian Feng]

On Thu, Dec 23, 2010 at 5:35 AM,  <Valdis.Kletnieks@vt.edu> wrote:
> On Wed, 22 Dec 2010 13:40:19 +0100, Ingo Molnar said:
>>
>> * mat <castet.matthieu@free.fr> wrote:
>>
>> > Le Wed, 8 Dec 2010 14:19:51 -0800,
>> > Kees Cook <kees.cook@canonical.com> a écrit :
>> >
>> > > On Fri, Nov 26, 2010 at 06:23:55PM +0100, mat wrote:
>> > > > could you try the attached patch ?
>> > > >
>> > > > on module load, we sort the __jump_table section. So we should make
>> > > > it writable.
>> > > >
>> > > >
>> > > > Matthieu
>> > >
>> > > > diff --git a/arch/x86/include/asm/jump_label.h
>> > > > b/arch/x86/include/asm/jump_label.h index f52d42e..574dbc2 100644
>> > > > --- a/arch/x86/include/asm/jump_label.h
>> > > > +++ b/arch/x86/include/asm/jump_label.h
>> > > > @@ -14,7 +14,7 @@
>> > > >         do
>> > > > {                                                       \ asm
>> > > > goto("1:"                                       \
>> > > > JUMP_LABEL_INITIAL_NOP                  \
>> > > > -                       ".pushsection __jump_table,  \"a\" \n\t"\
>> > > > +                       ".pushsection __jump_table,  \"aw\" \n\t"\
>> > > >                         _ASM_PTR "1b, %l[" #label "], %c0 \n\t" \
>> > > >                         ".popsection \n\t"                      \
>> > > >                         : :  "i" (key) :  : label);
>> > > > \
>> > >
>> > > Acked-by: Kees Cook <kees.cook@canonical.com>
>> > >
>> > > Can this please get committed to tip?
>> > I think it is not need anymore with  Steven Rostedt patch [1]
>> >
>> > Matthieu
>> >
>> > [1]
>> > > > Here we set the text read only before we call the notifiers. The
>> > > > function tracer changes the calls to mcount into nops via a notifier
>> > > > call so this must be done after the module notifiers.
>>
>> What's the status of this bug?
>>
>> If we still need the patch then please submit it standalone with a proper subject
>> line, with acks/signoffs added, etc.
>
> Steve Rostedt's patch that moves the setting of the page permissions seems to
> make this patch no longer necessary.  I tripped over this same issue, but the
> version in the latest -mmotm does not need it, as it includes Steve's fix.
>

I'm facing a boot failure (panic'ed on remove_jump_label_module_init)
on 2.6.37 (latest commit 3c0cb7c), which is 100% reproducible.
With this patch applied, I can boot my machine successfully, so I do
think this patch is needed.

Thanks
Xiaotian

Re: [PATCH 3/3 V13] RO/NX protection for loadable kernel

[Ingo Molnar]

* Xiaotian Feng <xtfeng@gmail.com> wrote:

> On Thu, Dec 23, 2010 at 5:35 AM,  <Valdis.Kletnieks@vt.edu> wrote:
> > On Wed, 22 Dec 2010 13:40:19 +0100, Ingo Molnar said:
> >>
> >> * mat <castet.matthieu@free.fr> wrote:
> >>
> >> > Le Wed, 8 Dec 2010 14:19:51 -0800,
> >> > Kees Cook <kees.cook@canonical.com> a écrit :
> >> >
> >> > > On Fri, Nov 26, 2010 at 06:23:55PM +0100, mat wrote:
> >> > > > could you try the attached patch ?
> >> > > >
> >> > > > on module load, we sort the __jump_table section. So we should make
> >> > > > it writable.
> >> > > >
> >> > > >
> >> > > > Matthieu
> >> > >
> >> > > > diff --git a/arch/x86/include/asm/jump_label.h
> >> > > > b/arch/x86/include/asm/jump_label.h index f52d42e..574dbc2 100644
> >> > > > --- a/arch/x86/include/asm/jump_label.h
> >> > > > +++ b/arch/x86/include/asm/jump_label.h
> >> > > > @@ -14,7 +14,7 @@
> >> > > >         do
> >> > > > {                                                       \ asm
> >> > > > goto("1:"                                       \
> >> > > > JUMP_LABEL_INITIAL_NOP                  \
> >> > > > -                       ".pushsection __jump_table,  \"a\" \n\t"\
> >> > > > +                       ".pushsection __jump_table,  \"aw\" \n\t"\
> >> > > >                         _ASM_PTR "1b, %l[" #label "], %c0 \n\t" \
> >> > > >                         ".popsection \n\t"                      \
> >> > > >                         : :  "i" (key) :  : label);
> >> > > > \
> >> > >
> >> > > Acked-by: Kees Cook <kees.cook@canonical.com>
> >> > >
> >> > > Can this please get committed to tip?
> >> > I think it is not need anymore with  Steven Rostedt patch [1]
> >> >
> >> > Matthieu
> >> >
> >> > [1]
> >> > > > Here we set the text read only before we call the notifiers. The
> >> > > > function tracer changes the calls to mcount into nops via a notifier
> >> > > > call so this must be done after the module notifiers.
> >>
> >> What's the status of this bug?
> >>
> >> If we still need the patch then please submit it standalone with a proper subject
> >> line, with acks/signoffs added, etc.
> >
> > Steve Rostedt's patch that moves the setting of the page permissions seems to
> > make this patch no longer necessary.  I tripped over this same issue, but the
> > version in the latest -mmotm does not need it, as it includes Steve's fix.
> >
> 
> I'm facing a boot failure (panic'ed on remove_jump_label_module_init)
> on 2.6.37 (latest commit 3c0cb7c), which is 100% reproducible.
> With this patch applied, I can boot my machine successfully, so I do
> think this patch is needed.

That would be commit:

 94462ad3b147: module: Move RO/NX module protection to after ftrace module update

So if commit 3c0cb7c is still broken, it has 94462ad3b147 included already, and 
there's some other bug. Kees, Steve, any ideas?

Xiaotian, please post as much about the crash as you can - a log/picture of the boot 
crash that occurs would be good.

Thanks,

	Ingo

Re: [PATCH 3/3 V13] RO/NX protection for loadable kernel

[matthieu castet]

Le Fri, 7 Jan 2011 14:04:26 +0100,
Ingo Molnar <mingo@elte.hu> a écrit :

> 
> * Xiaotian Feng <xtfeng@gmail.com> wrote:
> 
> > 
> > I'm facing a boot failure (panic'ed on
> > remove_jump_label_module_init) on 2.6.37 (latest commit 3c0cb7c),
> > which is 100% reproducible. With this patch applied, I can boot my
> > machine successfully, so I do think this patch is needed.
> 
> That would be commit:
> 
>  94462ad3b147: module: Move RO/NX module protection to after ftrace
> module update
> 
> So if commit 3c0cb7c is still broken, it has 94462ad3b147 included
> already, and there's some other bug. Kees, Steve, any ideas?
> 
The problem comes from remove_jump_label_module_init that does :
if (within_module_init(iter->code, mod))
                        iter->key = 0;

This mean if there are jump label in the module init, we will
invalidate them by writing the the jump label section.

But this section is read only.

The solution is either to make the section read write, either we avoid
this write.

For avoid the write a solution could be to do something like
trim_init_extable :
/*
 * If the exception table is sorted, any referring to the module init
 * will be at the beginning or the end.
 */


Matthieu

Re: [PATCH 3/3 V13] RO/NX protection for loadable kernel

[Kees Cook]

Hi,

I was just shown this[1] on Xen from an Ubuntu bug report[2].

[    1.230382] NX-protecting the kernel data: 3884k
[    1.231002] BUG: unable to handle kernel paging request at c1782ae0
...
[    1.231145] Call Trace:
[    1.231152]  [<c0138481>] ? __change_page_attr+0x2c1/0x370
[    1.231161]  [<c02163a1>] ? __purge_vmap_area_lazy+0xc1/0x180
[    1.231169]  [<c013857c>] ? __change_page_attr_set_clr+0x4c/0xb0
[    1.231176]  [<c0138838>] ? change_page_attr_set_clr+0x128/0x300
[    1.231183]  [<c010798e>] ? __raw_callee_save_xen_restore_fl+0x6/0x8
[    1.231192]  [<c0159ca1>] ? vprintk+0x171/0x3f0
[    1.231198]  [<c0138bdf>] ? set_memory_nx+0x5f/0x70


Does Xen have different size page table allocations or something weird?

-Kees

[1] http://launchpadlibrarian.net/61853131/natty-failed-boot-ec2.txt
[2] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/699828

-- 
Kees Cook
Ubuntu Security Team

Re: [PATCH 3/3 V13] RO/NX protection for loadable kernel

[matthieu castet]

Kees Cook a écrit :
> Hi,
> 
> I was just shown this[1] on Xen from an Ubuntu bug report[2].
> 
> [    1.230382] NX-protecting the kernel data: 3884k
> [    1.231002] BUG: unable to handle kernel paging request at c1782ae0
> ...
> [    1.231145] Call Trace:
> [    1.231152]  [<c0138481>] ? __change_page_attr+0x2c1/0x370
> [    1.231161]  [<c02163a1>] ? __purge_vmap_area_lazy+0xc1/0x180
> [    1.231169]  [<c013857c>] ? __change_page_attr_set_clr+0x4c/0xb0
> [    1.231176]  [<c0138838>] ? change_page_attr_set_clr+0x128/0x300
> [    1.231183]  [<c010798e>] ? __raw_callee_save_xen_restore_fl+0x6/0x8
> [    1.231192]  [<c0159ca1>] ? vprintk+0x171/0x3f0
> [    1.231198]  [<c0138bdf>] ? set_memory_nx+0x5f/0x70
> 
> 
> Does Xen have different size page table allocations or something weird?
> 
Note that this one isn't related to this one but to "Add NX protection for kernel data".

But have no idea how xen page table work.

Matthieu

Re: [PATCH 3/3 V13] RO/NX protection for loadable kernel

[matthieu castet]

[-- Attachment #1: Type: text/plain, Size: 2455 bytes --]

Xiaotian Feng a écrit :
> On Thu, Dec 23, 2010 at 5:35 AM,  <Valdis.Kletnieks@vt.edu> wrote:
>> On Wed, 22 Dec 2010 13:40:19 +0100, Ingo Molnar said:
>>> * mat <castet.matthieu@free.fr> wrote:
>>>
>>>> Le Wed, 8 Dec 2010 14:19:51 -0800,
>>>> Kees Cook <kees.cook@canonical.com> a écrit :
>>>>
>>>>> On Fri, Nov 26, 2010 at 06:23:55PM +0100, mat wrote:
>>>>>> could you try the attached patch ?
>>>>>>
>>>>>> on module load, we sort the __jump_table section. So we should make
>>>>>> it writable.
>>>>>>
>>>>>>
>>>>>> Matthieu
>>>>>> diff --git a/arch/x86/include/asm/jump_label.h
>>>>>> b/arch/x86/include/asm/jump_label.h index f52d42e..574dbc2 100644
>>>>>> --- a/arch/x86/include/asm/jump_label.h
>>>>>> +++ b/arch/x86/include/asm/jump_label.h
>>>>>> @@ -14,7 +14,7 @@
>>>>>>         do
>>>>>> {                                                       \ asm
>>>>>> goto("1:"                                       \
>>>>>> JUMP_LABEL_INITIAL_NOP                  \
>>>>>> -                       ".pushsection __jump_table,  \"a\" \n\t"\
>>>>>> +                       ".pushsection __jump_table,  \"aw\" \n\t"\
>>>>>>                         _ASM_PTR "1b, %l[" #label "], %c0 \n\t" \
>>>>>>                         ".popsection \n\t"                      \
>>>>>>                         : :  "i" (key) :  : label);
>>>>>> \
>>>>> Acked-by: Kees Cook <kees.cook@canonical.com>
>>>>>
>>>>> Can this please get committed to tip?
>>>> I think it is not need anymore with  Steven Rostedt patch [1]
>>>>
>>>> Matthieu
>>>>
>>>> [1]
>>>>>> Here we set the text read only before we call the notifiers. The
>>>>>> function tracer changes the calls to mcount into nops via a notifier
>>>>>> call so this must be done after the module notifiers.
>>> What's the status of this bug?
>>>
>>> If we still need the patch then please submit it standalone with a proper subject
>>> line, with acks/signoffs added, etc.
>> Steve Rostedt's patch that moves the setting of the page permissions seems to
>> make this patch no longer necessary.  I tripped over this same issue, but the
>> version in the latest -mmotm does not need it, as it includes Steve's fix.
>>
> 
> I'm facing a boot failure (panic'ed on remove_jump_label_module_init)
> on 2.6.37 (latest commit 3c0cb7c), which is 100% reproducible.
> With this patch applied, I can boot my machine successfully, so I do
> think this patch is needed.
> 
Could you confirm that this patch fix the problem ?


Matthieu

[-- Attachment #2: 0001-Fix-jump-table-in-module-init-section.patch --]
[-- Type: text/x-diff, Size: 1145 bytes --]

>From 0eeba453aaba0ebff86f0a7ad9bfb8afbde0c0dc Mon Sep 17 00:00:00 2001
From: Matthieu CASTET <castet.matthieu@free.fr>
Date: Thu, 20 Jan 2011 21:25:03 +0100
Subject: [PATCH] Fix jump table in module init section

If we use jump table in module init, there are marked
as removed in __jump_table section after init is done.

But we already applied ro permissions on the module, so
we can't modify a read only section (crash in
remove_jump_label_module_init).

Make the __jump_table section rw.

Signed-off-by: Matthieu CASTET <castet.matthieu@free.fr>
---
 arch/x86/include/asm/jump_label.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/include/asm/jump_label.h b/arch/x86/include/asm/jump_label.h
index f52d42e..574dbc2 100644
--- a/arch/x86/include/asm/jump_label.h
+++ b/arch/x86/include/asm/jump_label.h
@@ -14,7 +14,7 @@
 	do {							\
 		asm goto("1:"					\
 			JUMP_LABEL_INITIAL_NOP			\
-			".pushsection __jump_table,  \"a\" \n\t"\
+			".pushsection __jump_table,  \"aw\" \n\t"\
 			_ASM_PTR "1b, %l[" #label "], %c0 \n\t" \
 			".popsection \n\t"			\
 			: :  "i" (key) :  : label);		\
-- 
1.7.2.3

Re: [PATCH 3/3 V13] RO/NX protection for loadable kernel

[Xiaotian Feng]

On Fri, Jan 21, 2011 at 4:32 AM, matthieu castet
<castet.matthieu@free.fr> wrote:
> Xiaotian Feng a écrit :
>>
>> On Thu, Dec 23, 2010 at 5:35 AM,  <Valdis.Kletnieks@vt.edu> wrote:
>>>
>>> On Wed, 22 Dec 2010 13:40:19 +0100, Ingo Molnar said:
>>>>
>>>> * mat <castet.matthieu@free.fr> wrote:
>>>>
>>>>> Le Wed, 8 Dec 2010 14:19:51 -0800,
>>>>> Kees Cook <kees.cook@canonical.com> a écrit :
>>>>>
>>>>>> On Fri, Nov 26, 2010 at 06:23:55PM +0100, mat wrote:
>>>>>>>
>>>>>>> could you try the attached patch ?
>>>>>>>
>>>>>>> on module load, we sort the __jump_table section. So we should make
>>>>>>> it writable.
>>>>>>>
>>>>>>>
>>>>>>> Matthieu
>>>>>>> diff --git a/arch/x86/include/asm/jump_label.h
>>>>>>> b/arch/x86/include/asm/jump_label.h index f52d42e..574dbc2 100644
>>>>>>> --- a/arch/x86/include/asm/jump_label.h
>>>>>>> +++ b/arch/x86/include/asm/jump_label.h
>>>>>>> @@ -14,7 +14,7 @@
>>>>>>>        do
>>>>>>> {                                                       \ asm
>>>>>>> goto("1:"                                       \
>>>>>>> JUMP_LABEL_INITIAL_NOP                  \
>>>>>>> -                       ".pushsection __jump_table,  \"a\" \n\t"\
>>>>>>> +                       ".pushsection __jump_table,  \"aw\" \n\t"\
>>>>>>>                        _ASM_PTR "1b, %l[" #label "], %c0 \n\t" \
>>>>>>>                        ".popsection \n\t"                      \
>>>>>>>                        : :  "i" (key) :  : label);
>>>>>>> \
>>>>>>
>>>>>> Acked-by: Kees Cook <kees.cook@canonical.com>
>>>>>>
>>>>>> Can this please get committed to tip?
>>>>>
>>>>> I think it is not need anymore with  Steven Rostedt patch [1]
>>>>>
>>>>> Matthieu
>>>>>
>>>>> [1]
>>>>>>>
>>>>>>> Here we set the text read only before we call the notifiers. The
>>>>>>> function tracer changes the calls to mcount into nops via a notifier
>>>>>>> call so this must be done after the module notifiers.
>>>>
>>>> What's the status of this bug?
>>>>
>>>> If we still need the patch then please submit it standalone with a
>>>> proper subject
>>>> line, with acks/signoffs added, etc.
>>>
>>> Steve Rostedt's patch that moves the setting of the page permissions
>>> seems to
>>> make this patch no longer necessary.  I tripped over this same issue, but
>>> the
>>> version in the latest -mmotm does not need it, as it includes Steve's
>>> fix.
>>>
>>
>> I'm facing a boot failure (panic'ed on remove_jump_label_module_init)
>> on 2.6.37 (latest commit 3c0cb7c), which is 100% reproducible.
>> With this patch applied, I can boot my machine successfully, so I do
>> think this patch is needed.
>>
> Could you confirm that this patch fix the problem ?
>

Yes, I already applied this patch, and my system works fine now.

>
> Matthieu
>

Re: [PATCH 3/3 V13] RO/NX protection for loadable kernel

[Tobias Karnat]

> Sure.  I can verify that the patch series (a) doesn't break well-behaved
> modules and (b) it *does* trigger for misbehaving modules (I sent the
> VirtualBox crew a bug report for a module that tried to write to an area
> marked executable).

This seems to be fixed by VirtualBox 4.

I applied the patchset to 2.6.36.
('All in one' patch, http://pastebin.com/raw.php?i=6CDnAkjP )

I have however noticed that after enabling;
CONFIG_DEBUG_KERNEL, CONFIG_DEBUG_RODATA and CONFIG_DEBUG_SET_MODULE_RONX
there are three "Freeing unused kernel memory" messages, is this correct?

Before applying patch:
[    1.449499] Freeing initrd memory: 16328k freed
[    3.336464] Freeing unused kernel memory: 844k freed

After applying patch:
[    1.449262] Freeing initrd memory: 16328k freed
[    3.297901] Freeing unused kernel memory: 844k freed
[    3.311849] Write protecting the kernel read-only data: 8192k
[    3.327592] Freeing unused kernel memory: 448k freed
[    3.342651] Freeing unused kernel memory: 276k freed

And why does CONFIG_DEBUG_RODATA depend on CONFIG_DEBUG_KERNEL;
Isn't it primarily a security enhancement?

-Tobias