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 <[email protected]>
Signed-off-by: Xuxian Jiang <[email protected]>
Acked-by: Arjan van de Ven <[email protected]>
Reviewed-by: James Morris <[email protected]>
Signed-off-by: H. Peter Anvin <[email protected]>
Commit-ID: 84e1c6bb38eb318e456558b610396d9f1afaabf0
Gitweb: http://git.kernel.org/tip/84e1c6bb38eb318e456558b610396d9f1afaabf0
Author: matthieu castet <[email protected]>
AuthorDate: Tue, 16 Nov 2010 22:35:16 +0100
Committer: Ingo Molnar <[email protected]>
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 <[email protected]>
Signed-off-by: Xuxian Jiang <[email protected]>
Acked-by: Arjan van de Ven <[email protected]>
Reviewed-by: James Morris <[email protected]>
Signed-off-by: H. Peter Anvin <[email protected]>
Cc: Andi Kleen <[email protected]>
Cc: Rusty Russell <[email protected]>
Cc: Stephen Rothwell <[email protected]>
Cc: Dave Jones <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Linus Torvalds <[email protected]>
LKML-Reference: <[email protected]>
[ minor cleanliness edits, -v14: build failure fix ]
Signed-off-by: Ingo Molnar <[email protected]>
---
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;
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
Le Wed, 24 Nov 2010 22:41:07 -0500,
[email protected] 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
On Fri, 26 Nov 2010 18:23:55 +0100, mat said:
> Le Wed, 24 Nov 2010 22:41:07 -0500,
> [email protected] 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...
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 <[email protected]>
Signed-off-by: Steven Rostedt <[email protected]>
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)
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.
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
Hi,
Le Mon, 29 Nov 2010 13:15:42 -0500,
Steven Rostedt <[email protected]> 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 <[email protected]>
> Signed-off-by: Steven Rostedt <[email protected]>
>
> 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
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
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 <[email protected]>
Thanks,
Rusty.
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 <[email protected]>
Can this please get committed to tip?
Thanks,
-Kees
--
Kees Cook
Ubuntu Security Team
Le Wed, 8 Dec 2010 14:19:51 -0800,
Kees Cook <[email protected]> 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 <[email protected]>
>
> 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.
Hi,
On Sat, Dec 11, 2010 at 12:18:57AM +0100, mat wrote:
> Le Wed, 8 Dec 2010 14:19:51 -0800,
> Kees Cook <[email protected]> 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 <[email protected]>
> >
> > 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
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
* mat <[email protected]> wrote:
> Le Wed, 8 Dec 2010 14:19:51 -0800,
> Kees Cook <[email protected]> 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 <[email protected]>
> >
> > 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
On Wed, 22 Dec 2010 13:40:19 +0100, Ingo Molnar said:
>
> * mat <[email protected]> wrote:
>
> > Le Wed, 8 Dec 2010 14:19:51 -0800,
> > Kees Cook <[email protected]> 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 <[email protected]>
> > >
> > > 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.
* [email protected] <[email protected]> wrote:
> On Wed, 22 Dec 2010 13:40:19 +0100, Ingo Molnar said:
> >
> > * mat <[email protected]> wrote:
> >
> > > Le Wed, 8 Dec 2010 14:19:51 -0800,
> > > Kees Cook <[email protected]> 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 <[email protected]>
> > > >
> > > > 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
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
* Steven Rostedt <[email protected]> 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
On Wed, 2010-12-22 at 16:35 -0500, [email protected] 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
On Thu, 23 Dec 2010 10:01:48 EST, Steven Rostedt said:
> On Wed, 2010-12-22 at 16:35 -0500, [email protected] 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).