Mailing List Archive

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,
Valdis.Kletnieks@vt.edu a écrit :

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

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


Matthieu

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

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

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

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

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

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

This patch breaks function tracer:

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


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

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

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

Below is the patch that fixes this bug.

-- Steve

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

diff --git a/kernel/module.c b/kernel/module.c
index ba421e6..5ccf52c 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2804,18 +2804,6 @@ static struct module *load_module(void __user *umod,
kfree(info.strmap);
free_copy(&info);

- /* Set RO and NX regions for core */
- set_section_ro_nx(mod->module_core,
- mod->core_text_size,
- mod->core_ro_size,
- mod->core_size);
-
- /* Set RO and NX regions for init */
- set_section_ro_nx(mod->module_init,
- mod->init_text_size,
- mod->init_ro_size,
- mod->init_size);
-
/* Done! */
trace_module_load(mod);
return mod;
@@ -2876,6 +2864,18 @@ SYSCALL_DEFINE3(init_module, void __user *, umod,
blocking_notifier_call_chain(&module_notify_list,
MODULE_STATE_COMING, mod);

+ /* Set RO and NX regions for core */
+ set_section_ro_nx(mod->module_core,
+ mod->core_text_size,
+ mod->core_ro_size,
+ mod->core_size);
+
+ /* Set RO and NX regions for init */
+ set_section_ro_nx(mod->module_init,
+ mod->init_text_size,
+ mod->init_ro_size,
+ mod->init_size);
+
do_mod_ctors(mod);
/* Start the module */
if (mod->init != NULL)


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Hi,

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

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

That's look fine for me.

But I wonder why ftrace_arch_code_modify_prepare isn't called ?

It is only called when we start/stop tracing ?

Matthieu
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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

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

Thanks,
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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

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

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

Can this please get committed to tip?

Thanks,

-Kees

--
Kees Cook
Ubuntu Security Team
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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

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

Matthieu

[1]
> > Here we set the text read only before we call the notifiers. The
> > function tracer changes the calls to mcount into nops via a notifier
> > call so this must be done after the module notifiers.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Hi,

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

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

-Kees

--
Kees Cook
Ubuntu Security Team
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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

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

What's the status of this bug?

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

Thanks,

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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

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

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

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

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

Steve, Andrew?

Thanks,

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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

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

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

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

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

Thanks!

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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

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

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

Thanks,

-- Steve


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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

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

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

This seems to be fixed by VirtualBox 4.

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

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

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

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

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

-Tobias

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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

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

Thanks
Xiaotian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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

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

That would be commit:

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

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

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

Thanks,

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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

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

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

But this section is read only.

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

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


Matthieu
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/