RO/NX protection for loadable kernel

PATCH In review matthieu castet 7 participants 29 messages Nov 16, 2010 – Jan 21, 2011 1 message not in the archive

Subsystems (5)

x86 Linux-next tree ftrace Module support +1 more subsystems

lore

Share & tools

lore

matthieu castet

author

2010-11-16 21:35 UTC #0 lore raw

PATCH v13 3/3 RO/NX protection for loadable kernel

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

arch/x86/Kconfig.debug +11 -0

index 3be21d5..316350a 100644
@@ -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

arch/x86/kernel/ftrace.c +3 -0

index 3afb33f..2984486 100644
@@ -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;
 }

include/linux/module.h +13 -0

index b29e745..4d9ce95 100644
@@ -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

kernel/module.c +175 -2

233 lines not on this page · raw message

tip-bot for matthieu castet

author

2010-11-18 14:14 UTC #1 lore raw

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

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

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

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

arch/x86/Kconfig.debug +11 -0

index b59ee76..45143bb 100644
@@ -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

arch/x86/kernel/ftrace.c +3 -0

index 3afb33f..2984486 100644
@@ -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;
 }

include/linux/module.h +10 -1

index b29e745..ddaa689 100644
@@ -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 *,

kernel/module.c +169 -2

227 lines not on this page · raw message

[email protected]

2010-11-25 03:43 UTC #2 lore raw

In reply to matthieu castet

Quoted 8 lines · matthieu castet

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

arch/x86/Kconfig.debug config DEBUG_RODATA_TEST · line 123

Quoted 4 lines

arch/x86/Kconfig.debug +4 -0

+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

mat

author

2010-11-26 17:24 UTC #3 lore raw

Quoted 64 lines · Le Wed, 24 Nov 2010 22:41:07 -0500, [email protected] a écrit :

Quoted 9 lines · matthieu castet
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 ]---

Quoted 6 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+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

arch/x86/include/asm/jump_label.h +1 -1

index f52d42e..574dbc2 100644
@@ -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);		\

[email protected]

2010-11-29 17:00 UTC #4 lore raw

arch/x86/include/asm/jump_la

Quoted 23 lines · mat

Le Wed, 24 Nov 2010 22:41:07 -0500,
[email protected] a =E9crit :
Quoted 8 lines
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
Quoted 6 lines
Review
Quoted 4 lines
(quoted diff) +4 -0
+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.
arch/x86/include/asm/jump_la renamed from arch/x86/include/asm/jump_label.h +0 -0

bel.h

arch/x86/include/asm/jump_label.h line 17

Quoted 9 lines

index f52d42e..574dbc2 100644

arch/x86/include/asm/jump_label.h +1 -1

@@ -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...

Kees Cook

Acked-by

2010-12-08 22:21 UTC #5 lore raw

In reply to mat

Quoted 13 lines · mat

could you try the attached patch ?
on module load, we sort the __jump_table section. So we should make it writable.
Matthieu
arch/x86/include/asm/jump_label.h +0 -0
index f52d42e..574dbc2 100644

arch/x86/include/asm/jump_label.h line 20

Quoted 8 lines

arch/x86/include/asm/jump_label.h +1 -1

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

Signature · 2 lines

Kees Cook
Ubuntu Security Team

mat

author

2010-12-10 23:18 UTC #6 lore raw

Quoted 27 lines · Kees Cook

Quoted 22 lines · mat
could you try the attached patch ?
on module load, we sort the __jump_table section. So we should make it writable.
Matthieu
(quoted diff) +0 -0
b/arch/x86/include/asm/jump_label.h index f52d42e..574dbc2 100644
arch/x86/include/asm/jump_label.h +0 -0
@@ -14,7 +14,7 @@
 	do
{ \ asm
goto("1:" \
JUMP_LABEL_INITIAL_NOP \
arch/x86/include/asm/jump_label.h +1 -1
-			".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);
Quoted 1 line
\
Acked-by Kees Cook <[email protected]>

Quoted 1 line

Can this please get committed to tip?

I think it is not need anymore with Steven Rostedt patch [1]

Matthieu

[1]

Quoted 3 lines

Quoted 3 lines
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.

Kees Cook

2010-12-11 00:28 UTC #7 lore raw

Hi,

Quoted 30 lines · mat

Quoted 27 lines · Kees Cook
Quoted 22 lines · mat
could you try the attached patch ?
on module load, we sort the __jump_table section. So we should make it writable.
Matthieu
(quoted diff) +0 -0
b/arch/x86/include/asm/jump_label.h index f52d42e..574dbc2 100644
arch/x86/include/asm/jump_label.h +0 -0
@@ -14,7 +14,7 @@
 	do
{ \ asm
goto("1:" \
JUMP_LABEL_INITIAL_NOP \
arch/x86/include/asm/jump_label.h +1 -1
-			".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);
Quoted 1 line
\
Acked-by Kees Cook <[email protected]>

Quoted 9 lines

Quoted 1 line
Can this please get committed to tip?
I think it is not need anymore with Steven Rostedt patch [1]
Matthieu
[1]
Quoted 3 lines
Quoted 3 lines
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

Signature · 2 lines

Kees Cook
Ubuntu Security Team

A message that is not in the archive (sent off-list or not yet received): 20101211115735.21b616fe@mat-laptop

Kees Cook

2010-12-11 23:15 UTC #9 lore raw

Quoted 1 line · mat

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

Signature · 2 lines

Kees Cook
Ubuntu Security Team

Ingo Molnar

2010-12-22 12:40 UTC #10 lore raw

In reply to mat

Quoted 30 lines · mat

Quoted 27 lines · Kees Cook
Quoted 22 lines · mat
could you try the attached patch ?
on module load, we sort the __jump_table section. So we should make it writable.
Matthieu
(quoted diff) +0 -0
b/arch/x86/include/asm/jump_label.h index f52d42e..574dbc2 100644
arch/x86/include/asm/jump_label.h +0 -0
@@ -14,7 +14,7 @@
 	do
{ \ asm
goto("1:" \
JUMP_LABEL_INITIAL_NOP \
arch/x86/include/asm/jump_label.h +1 -1
-			".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);
Quoted 1 line
\
Acked-by Kees Cook <[email protected]>

Quoted 9 lines

Quoted 1 line
Can this please get committed to tip?
I think it is not need anymore with Steven Rostedt patch [1]
Matthieu
[1]
Quoted 3 lines
Quoted 3 lines
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

[email protected]

2010-12-22 21:36 UTC #11 lore raw

Quoted 39 lines · Ingo Molnar

Quoted 36 lines · mat
Quoted 27 lines · Kees Cook
Quoted 22 lines · mat
could you try the attached patch ?
on module load, we sort the __jump_table section. So we should make it writable.
Matthieu
(quoted diff) +0 -0
b/arch/x86/include/asm/jump_label.h index f52d42e..574dbc2 100644
arch/x86/include/asm/jump_label.h +0 -0
@@ -14,7 +14,7 @@
 	do
{ \ asm
goto("1:" \
JUMP_LABEL_INITIAL_NOP \
arch/x86/include/asm/jump_label.h +1 -1
-			".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);
Quoted 1 line
\
Acked-by Kees Cook <[email protected]>
Quoted 1 line
Can this please get committed to tip?
I think it is not need anymore with Steven Rostedt patch [1]
Matthieu
[1]

Quoted 8 lines

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
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.

Ingo Molnar

2010-12-22 21:57 UTC #12 lore raw

Quoted 44 lines · * [email protected] <[email protected]> wrote:

Quoted 43 lines · Ingo Molnar
Quoted 39 lines · mat
Quoted 27 lines · Kees Cook
Quoted 22 lines · mat
could you try the attached patch ?
on module load, we sort the __jump_table section. So we should make it writable.
Matthieu
(quoted diff) +0 -0
b/arch/x86/include/asm/jump_label.h index f52d42e..574dbc2 100644
arch/x86/include/asm/jump_label.h +0 -0
@@ -14,7 +14,7 @@
 	do
{ \ asm
goto("1:" \
JUMP_LABEL_INITIAL_NOP \
arch/x86/include/asm/jump_label.h +1 -1
-			".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);
Quoted 1 line
\
Acked-by Kees Cook <[email protected]>
Quoted 1 line
Can this please get committed to tip?
I think it is not need anymore with Steven Rostedt patch [1]
Matthieu
[1]
Quoted 3 lines
Quoted 3 lines
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.

Quoted 8 lines

Quoted 4 lines
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Steven Rostedt

maintainer

2010-12-23 01:17 UTC #13 lore raw

Quoted 4 lines · Ingo Molnar

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

Ingo Molnar

2010-12-23 08:49 UTC #14 lore raw

Quoted 10 lines · Steven Rostedt

Quoted 4 lines · Ingo Molnar
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

Quoted 1 line

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

Thanks!

Ingo

Steven Rostedt

maintainer

2010-12-23 15:01 UTC #15 lore raw

In reply to [email protected]

Quoted 3 lines · 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

[email protected]

2010-12-24 01:45 UTC #16 lore raw

Quoted 7 lines · Steven Rostedt

Quoted 3 lines · 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).

Xiaotian Feng

2011-01-07 09:34 UTC #17 lore raw

In reply to [email protected]

Quoted 44 lines · On Thu, Dec 23, 2010 at 5:35 AM, <[email protected]> wrote:

Quoted 43 lines · Ingo Molnar
Quoted 39 lines · mat
Quoted 27 lines · Kees Cook
Quoted 15 lines · mat
could you try the attached patch ?
on module load, we sort the __jump_table section. So we should make it writable.
Matthieu
(quoted diff) +0 -0
b/arch/x86/include/asm/jump_label.h index f52d42e..574dbc2 100644
arch/x86/include/asm/jump_label.h +0 -0
@@ -14,7 +14,7 @@
do
{ \ asm
Quoted 8 lines
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]>
Quoted 1 line
Can this please get committed to tip?
I think it is not need anymore with Steven Rostedt patch [1]
Matthieu
[1]
Quoted 3 lines
Quoted 3 lines
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.

Quoted 9 lines

Quoted 4 lines
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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Ingo Molnar

2011-01-07 13:04 UTC #18 lore raw

Quoted 55 lines · Xiaotian Feng

Quoted 44 lines · On Thu, Dec 23, 2010 at 5:35 AM, <[email protected]> wrote:
Quoted 43 lines · Ingo Molnar
Quoted 39 lines · mat
Quoted 27 lines · Kees Cook
Quoted 15 lines · mat
could you try the attached patch ?
on module load, we sort the __jump_table section. So we should make it writable.
Matthieu
(quoted diff) +0 -0
b/arch/x86/include/asm/jump_label.h index f52d42e..574dbc2 100644
arch/x86/include/asm/jump_label.h +0 -0
@@ -14,7 +14,7 @@
do
{ \ asm
Quoted 8 lines
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]>
Quoted 1 line
Can this please get committed to tip?
I think it is not need anymore with Steven Rostedt patch [1]
Matthieu
[1]
Quoted 3 lines
Quoted 3 lines
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.
Quoted 9 lines
Quoted 4 lines
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.

Quoted 4 lines

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

matthieu castet

author

2011-01-08 11:25 UTC #19 lore raw

Quoted 9 lines · Ingo Molnar

Quoted 5 lines · Xiaotian Feng
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.

Quoted 8 lines

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

Kees Cook

2011-01-10 23:50 UTC #20 lore raw

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

Signature · 2 lines

Kees Cook
Ubuntu Security Team

matthieu castet

author

2011-01-11 22:42 UTC #21 lore raw

Quoted 17 lines · 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

Quoted 2 lines

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
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

matthieu castet

author

2011-01-20 20:32 UTC #22 lore raw

In reply to Xiaotian Feng

Quoted 50 lines · Xiaotian Feng

Quoted 40 lines · On Thu, Dec 23, 2010 at 5:35 AM, <[email protected]> wrote:
Quoted 39 lines · Ingo Molnar
Quoted 37 lines · mat
Quoted 25 lines · Kees Cook
Quoted 21 lines · mat
could you try the attached patch ?
on module load, we sort the __jump_table section. So we should make it writable.
Matthieu
(quoted diff) +0 -0
b/arch/x86/include/asm/jump_label.h index f52d42e..574dbc2 100644
arch/x86/include/asm/jump_label.h +0 -0
@@ -14,7 +14,7 @@
        do
{ \ asm
goto("1:" \
JUMP_LABEL_INITIAL_NOP \
arch/x86/include/asm/jump_label.h +1 -1
-                       ".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);
Quoted 1 line
\
Acked-by Kees Cook <[email protected]>
Quoted 1 line
Can this please get committed to tip?
I think it is not need anymore with Steven Rostedt patch [1]
Matthieu
[1]
Quoted 3 lines
Quoted 3 lines
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.
Quoted 8 lines
Quoted 4 lines
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.

Quoted 5 lines

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

Quoted 1 line

From 0eeba453aaba0ebff86f0a7ad9bfb8afbde0c0dc Mon Sep 17 00:00:00 2001

From: Matthieu CASTET <[email protected]>
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 <[email protected]>

---
 arch/x86/include/asm/jump_label.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

arch/x86/include/asm/jump_label.h +1 -1

index f52d42e..574dbc2 100644
@@ -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);		\

Signature · 1 line

1.7.2.3

Xiaotian Feng

2011-01-21 02:35 UTC #23 lore raw

Quoted 64 lines · matthieu castet

Quoted 63 lines · Xiaotian Feng
Quoted 60 lines · On Thu, Dec 23, 2010 at 5:35 AM, <[email protected]> wrote:
Quoted 45 lines · Ingo Molnar
Quoted 30 lines · mat
Quoted 27 lines · Kees Cook
Quoted 15 lines · mat
could you try the attached patch ?
on module load, we sort the __jump_table section. So we should make it writable.
Matthieu
(quoted diff) +0 -0
b/arch/x86/include/asm/jump_label.h index f52d42e..574dbc2 100644
arch/x86/include/asm/jump_label.h +0 -0
@@ -14,7 +14,7 @@
do
{ \ asm
Quoted 8 lines
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]>
Quoted 11 lines
Quoted 1 line
Can this please get committed to tip?
I think it is not need anymore with Steven Rostedt patch [1]
Matthieu
[1]
Quoted 4 lines
Quoted 4 lines
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.
Quoted 5 lines
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.

Quoted 7 lines

Quoted 5 lines
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.

Quoted 3 lines

Matthieu

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Steven Rostedt

maintainer

2010-11-29 18:15 UTC #24 lore raw

In reply to matthieu castet

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]

Quoted 7 lines · matthieu castet

(quoted diff) +2 -0

@@ -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,

Review static struct module *load_module(void __user *umod, · line 2824

Quoted 10 lines

(quoted diff) +10 -0

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

Review SYSCALL_DEFINE3(init_module, void __user *, umod, · line 2931

Quoted 11 lines

(quoted diff) +1 -0

 	/* 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]>

kernel/module.c +12 ~~-12~~

index ba421e6..5ccf52c 100644
@@ -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)

Rusty Russell

2010-11-29 23:35 UTC #25 lore raw

Quoted 1 line · Steven Rostedt

This patch breaks function tracer:

...

Quoted 3 lines

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.

Steven Rostedt

maintainer

2010-11-30 14:46 UTC #26 lore raw

Quoted 7 lines · Rusty Russell

Quoted 1 line · Steven Rostedt
This patch breaks function tracer:
...
Quoted 3 lines
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.

Quoted 5 lines

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

Rusty Russell

2010-12-01 13:36 UTC #27 lore raw

Quoted 8 lines · Steven Rostedt

Quoted 7 lines · Rusty Russell
Quoted 1 line · Steven Rostedt
This patch breaks function tracer:
...
Quoted 3 lines
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.

Quoted 7 lines

Quoted 5 lines
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.

mat

author

2010-11-30 21:20 UTC #28 lore raw

In reply to Steven Rostedt

Hi,

Quoted 103 lines · 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]

Quoted 7 lines · matthieu castet

kernel/module.c +2 -0

@@ -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,

kernel/module.c static struct module *load_module(void · line 2824

Quoted 10 lines

kernel/module.c +10 -0

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

kernel/module.c SYSCALL_DEFINE3(init_module, void __user *, · line 2931

Quoted 11 lines

kernel/module.c +1 -0

 	/* 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]>

kernel/module.c +5 ~~-12~~

index ba421e6..5ccf52c 100644
@@ -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);

kernel/module.c SYSCALL_DEFINE3(init_module, void __user *, · line 2881

Quoted 10 lines

kernel/module.c +7 -0

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

Steven Rostedt

maintainer

2010-12-01 00:38 UTC #29 lore raw

Quoted 5 lines · mat

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