[PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX

[PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX

[Thomas Gleixner]

ftrace uses module_alloc() to allocate trampoline pages. The mapping of
module_alloc() is RWX, which makes sense as the memory is written to right
after allocation. But nothing makes these pages RO after writing to them.

This problem exists since ftrace uses trampolines on x86, but it went
unnoticed because the W=X sanity check only triggers when the tracer
builtin selftests are enabled. Though the mappings are also created W+X w/o
the self tests when the tracer is used after booting.

Add proper set_memory_rw/ro() calls to [un]protect the trampolines before
and after modification.

Fixes: f3bea49115b2 ("ftrace/x86: Add dynamic allocated trampoline for ftrace_ops")
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/ftrace.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -839,7 +839,7 @@ void arch_ftrace_update_trampoline(struc
 	unsigned long offset;
 	unsigned long ip;
 	unsigned int size;
-	int ret;
+	int ret, npages;
 
 	if (ops->trampoline) {
 		/*
@@ -848,11 +848,14 @@ void arch_ftrace_update_trampoline(struc
 		 */
 		if (!(ops->flags & FTRACE_OPS_FL_ALLOC_TRAMP))
 			return;
+		npages = PAGE_ALIGN(ops->trampoline_size) >> PAGE_SHIFT;
+		set_memory_rw(ops->trampoline, npages);
 	} else {
 		ops->trampoline = create_trampoline(ops, &size);
 		if (!ops->trampoline)
 			return;
 		ops->trampoline_size = size;
+		npages = PAGE_ALIGN(size) >> PAGE_SHIFT;
 	}
 
 	offset = calc_trampoline_call_offset(ops->flags & FTRACE_OPS_FL_SAVE_REGS);
@@ -863,6 +866,7 @@ void arch_ftrace_update_trampoline(struc
 	/* Do a safe modify in case the trampoline is executing */
 	new = ftrace_call_replace(ip, (unsigned long)func);
 	ret = update_ftrace_func(ip, new);
+	set_memory_ro(ops->trampoline, npages);
 
 	/* The update should never fail */
 	WARN_ON(ret);

Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX

[Masami Hiramatsu]

On Wed, 24 May 2017 15:47:17 +0200 (CEST)
Thomas Gleixner <tglx@linutronix.de> wrote:

> ftrace uses module_alloc() to allocate trampoline pages. The mapping of
> module_alloc() is RWX, which makes sense as the memory is written to right
> after allocation. But nothing makes these pages RO after writing to them.
> 
> This problem exists since ftrace uses trampolines on x86, but it went
> unnoticed because the W=X sanity check only triggers when the tracer
> builtin selftests are enabled. Though the mappings are also created W+X w/o
> the self tests when the tracer is used after booting.
> 
> Add proper set_memory_rw/ro() calls to [un]protect the trampolines before
> and after modification.
> 

This looks good to me. (same as I did for kprobes trampoline pages)

Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>

Thank you,

> Fixes: f3bea49115b2 ("ftrace/x86: Add dynamic allocated trampoline for ftrace_ops")
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  arch/x86/kernel/ftrace.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -839,7 +839,7 @@ void arch_ftrace_update_trampoline(struc
>  	unsigned long offset;
>  	unsigned long ip;
>  	unsigned int size;
> -	int ret;
> +	int ret, npages;
>  
>  	if (ops->trampoline) {
>  		/*
> @@ -848,11 +848,14 @@ void arch_ftrace_update_trampoline(struc
>  		 */
>  		if (!(ops->flags & FTRACE_OPS_FL_ALLOC_TRAMP))
>  			return;
> +		npages = PAGE_ALIGN(ops->trampoline_size) >> PAGE_SHIFT;
> +		set_memory_rw(ops->trampoline, npages);
>  	} else {
>  		ops->trampoline = create_trampoline(ops, &size);
>  		if (!ops->trampoline)
>  			return;
>  		ops->trampoline_size = size;
> +		npages = PAGE_ALIGN(size) >> PAGE_SHIFT;
>  	}
>  
>  	offset = calc_trampoline_call_offset(ops->flags & FTRACE_OPS_FL_SAVE_REGS);
> @@ -863,6 +866,7 @@ void arch_ftrace_update_trampoline(struc
>  	/* Do a safe modify in case the trampoline is executing */
>  	new = ftrace_call_replace(ip, (unsigned long)func);
>  	ret = update_ftrace_func(ip, new);
> +	set_memory_ro(ops->trampoline, npages);
>  
>  	/* The update should never fail */
>  	WARN_ON(ret);


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX

[Steven Rostedt]

On Wed, 24 May 2017 15:47:17 +0200 (CEST)
Thomas Gleixner <tglx@linutronix.de> wrote:

> ftrace uses module_alloc() to allocate trampoline pages. The mapping
> of module_alloc() is RWX, which makes sense as the memory is written
> to right after allocation. But nothing makes these pages RO after
> writing to them.
> 
> This problem exists since ftrace uses trampolines on x86, but it went
> unnoticed because the W=X sanity check only triggers when the tracer
> builtin selftests are enabled. Though the mappings are also created
> W+X w/o the self tests when the tracer is used after booting.
> 
> Add proper set_memory_rw/ro() calls to [un]protect the trampolines
> before and after modification.
> 
> Fixes: f3bea49115b2 ("ftrace/x86: Add dynamic allocated trampoline
> for ftrace_ops") Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Thanks! I was thinking that this was the issue after your last reply.
I'll send this to my box at home and run my tests against it. I'll let
you know tomorrow the results.

-- Steve


> ---
>  arch/x86/kernel/ftrace.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -839,7 +839,7 @@ void arch_ftrace_update_trampoline(struc
>  	unsigned long offset;
>  	unsigned long ip;
>  	unsigned int size;
> -	int ret;
> +	int ret, npages;
>  
>  	if (ops->trampoline) {
>  		/*
> @@ -848,11 +848,14 @@ void arch_ftrace_update_trampoline(struc
>  		 */
>  		if (!(ops->flags & FTRACE_OPS_FL_ALLOC_TRAMP))
>  			return;
> +		npages = PAGE_ALIGN(ops->trampoline_size) >>
> PAGE_SHIFT;
> +		set_memory_rw(ops->trampoline, npages);
>  	} else {
>  		ops->trampoline = create_trampoline(ops, &size);
>  		if (!ops->trampoline)
>  			return;
>  		ops->trampoline_size = size;
> +		npages = PAGE_ALIGN(size) >> PAGE_SHIFT;
>  	}
>  
>  	offset = calc_trampoline_call_offset(ops->flags &
> FTRACE_OPS_FL_SAVE_REGS); @@ -863,6 +866,7 @@ void
> arch_ftrace_update_trampoline(struc /* Do a safe modify in case the
> trampoline is executing */ new = ftrace_call_replace(ip, (unsigned
> long)func); ret = update_ftrace_func(ip, new);
> +	set_memory_ro(ops->trampoline, npages);
>  
>  	/* The update should never fail */
>  	WARN_ON(ret);

Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX

[Steven Rostedt]

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

OK, it crashed on one of my tests. I removed the patch and it boots
fine, and crashes when I add it back.

Here's the crash:

Running tests on all trace events:
Testing all events: OK
Testing ftrace filter: OK
trace_kprobe: Testing kprobe tracing: 
BUG: unable to handle kernel paging request at ffff880214f5c000
IP: new_slab+0x1e8/0x2b4
PGD 338c067 
P4D 338c067 
PUD 338f067 
PMD 212022063 
PTE 8000000214f5c161

Oops: 0003 [#1] SMP
Modules linked in:
CPU: 3 PID: 1 Comm: swapper/0 Not tainted 4.12.0-rc2-test+ #42
Hardware name: MSI MS-7823/CSM-H87M-G43 (MS-7823), BIOS V1.6 02/22/2014
task: ffff8802153a8000 task.stack: ffffc90000c74000
RIP: 0010:new_slab+0x1e8/0x2b4
RSP: 0000:ffffc90000c77b28 EFLAGS: 00010282
RAX: 0000000040040000 RBX: ffff880216003f00 RCX: ffff880214f5c058
RDX: 0000000000000000 RSI: ffff880214f5c000 RDI: ffff880216003f00
RBP: ffffc90000c77b70 R08: 000000000000002a R09: 0000000000000000
R10: 00000000000201e2 R11: 0000000000020190 R12: ffff880214f5c000
R13: 000000000000002e R14: 0000000000000001 R15: ffffea000853d700
FS:  0000000000000000(0000) GS:ffff88021eb80000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: ffff880214f5c000 CR3: 000000000221d000 CR4: 00000000001406e0
Call Trace:
 ? interleave_nodes+0x29/0x40
 ___slab_alloc+0x2e8/0x49e
 ? trace_create_new_event+0x1f/0x63
 ? trace_add_event_call+0x27/0x91
 ? mark_lock+0x24/0x1ec
 ? trace_create_new_event+0x1f/0x63
 __slab_alloc+0x46/0x6b
 ? __slab_alloc+0x46/0x6b
 ? trace_create_new_event+0x1f/0x63
 kmem_cache_alloc+0x59/0x1b2
 trace_create_new_event+0x1f/0x63
 __trace_add_new_event+0xd/0x29
 trace_add_event_call+0x66/0x91
 create_trace_kprobe+0x722/0x7f5
 ? argv_split+0x6c/0xce
 ? probes_open+0x3b/0x3b
 ? set_debug_rodata+0x17/0x17
 traceprobe_command+0x42/0x57
 kprobe_trace_self_tests_init+0x3c/0x338
 ? kprobe_trace_selftest_target+0x13/0x13
 ? set_debug_rodata+0x17/0x17
 do_one_initcall+0x90/0x131
 ? set_debug_rodata+0x17/0x17
 kernel_init_freeable+0x1f4/0x27c
 ? rest_init+0x12d/0x12d
 kernel_init+0xe/0xfa
 ret_from_fork+0x2e/0x40
Code: ff ff 48 8b 53 48 48 85 d2 74 05 4c 89 e7 ff d2 66 41 8b 57 1a 81 e2 ff 7f 00 00 44 39 f2 48 63 53 20 7e 0d 48 63 4b 18 4c 01 e1 <49> 89 0c 14 eb 08 49 c7 04 14 00 00 00 00 48 63 53 18 41 ff c6 
RIP: new_slab+0x1e8/0x2b4 RSP: ffffc90000c77b28
CR2: ffff880214f5c000
---[ end trace e8a2200699d40525 ]---
Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000009

Kernel Offset: disabled
---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000009

sched: Unexpected reschedule of offline CPU#1!
------------[ cut here ]------------
WARNING: CPU: 3 PID: 1 at /work/git/linux-trace.git/arch/x86/kernel/smp.c:128 native_smp_send_reschedule+0x23/0x3b
Modules linked in:
CPU: 3 PID: 1 Comm: swapper/0 Tainted: G      D         4.12.0-rc2-test+ #42
Hardware name: MSI MS-7823/CSM-H87M-G43 (MS-7823), BIOS V1.6 02/22/2014
task: ffff8802153a8000 task.stack: ffffc90000c74000
RIP: 0010:native_smp_send_reschedule+0x23/0x3b
RSP: 0000:ffff88021eb83e60 EFLAGS: 00010086
RAX: 000000000000002e RBX: 0000000000000001 RCX: ffff8802153a8000
RDX: ffffffff8226ba60 RSI: ffffffff8109ff35 RDI: ffffffff8108e779
RBP: ffff88021eb83e60 R08: 0000000000000001 R09: 0000000000000001
R10: ffff88021eb83de8 R11: ffffffff832b238c R12: 0000000000000003
R13: 0000000000000003 R14: ffff880216002270 R15: 0000000000000001
FS:  0000000000000000(0000) GS:ffff88021eb80000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: ffff880214f5c000 CR3: 000000000221d000 CR4: 00000000001406e0
Call Trace:
 <IRQ>
 trigger_load_balance+0x2dc/0x2e5
 scheduler_tick+0x92/0x9b
 update_process_times+0x47/0x54
 tick_sched_handle+0x44/0x53
 tick_sched_timer+0x39/0x5f
 __hrtimer_run_queues+0x141/0x2c8
 ? tick_sched_do_timer+0x2e/0x2e
 hrtimer_interrupt+0x6c/0x12e
 local_apic_timer_interrupt+0x4b/0x4e
 smp_apic_timer_interrupt+0x29/0x39
 apic_timer_interrupt+0x90/0xa0
RIP: 0010:panic+0x1e1/0x21f
RSP: 0000:ffffc90000c77e70 EFLAGS: 00000246 ORIG_RAX: ffffffffffffff10
RAX: 00000003810a049e RBX: 0000000000000000 RCX: ffff8802153a8000
RDX: ffffc90000c77e00 RSI: ffffffff81133f1a RDI: ffffffff81091058
RBP: ffffc90000c77ee0 R08: 0000000000000001 R09: 0000000000000001
R10: ffffc90000c77d78 R11: ffffffff8265a60a R12: 0000000000000000
R13: 0000000000000000 R14: 0000000000000001 R15: 0000000000000009
 </IRQ>
 ? panic+0x1de/0x21f
 ? trace_hardirqs_on+0xd/0xf
 do_exit+0x568/0xa40
 rewind_stack_do_exit+0x17/0x20
Code: ff 90 a0 00 00 00 5d c3 0f 1f 44 00 00 55 89 f8 48 89 e5 48 0f a3 05 ec 05 41 01 72 12 89 fe 48 c7 c7 5d 0b fe 81 e8 7a 2a 10 00 <0f> ff eb 12 48 8b 05 31 62 0e 01 be fd 00 00 00 ff 90 a0 00 00 
---[ end trace e8a2200699d40526 ]---


I attached the config. I don't currently have time to look deeper into
it. But I should have some time later today.

-- Steve

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 29237 bytes --]

Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX

[Luis R. Rodriguez]

On Wed, May 24, 2017 at 01:47:28PM -0400, Steven Rostedt wrote:
> OK, it crashed on one of my tests. I removed the patch and it boots
> fine, and crashes when I add it back.
> 
> Here's the crash:
> 
> Running tests on all trace events:
> Testing all events: OK
> Testing ftrace filter: OK
> trace_kprobe: Testing kprobe tracing: 
> BUG: unable to handle kernel paging request at ffff880214f5c000
> IP: new_slab+0x1e8/0x2b4
> PGD 338c067 
> P4D 338c067 
> PUD 338f067 
> PMD 212022063 
> PTE 8000000214f5c161

The good news is that this also fixes the other issue I reported with
CONFIG_KPROBES_SANITY_TEST=y and the same WARN complaint without a crash [0].
Please note that the issue I reported actually is present since the
infrastructure to report on these warnings was added through commit
e1a58320a38dfa72be48a0f1a3a92273663ba6db ("x86/mm: Warn on W^X mappings")
which was added through kernel 4.3.0-rc3.

Thomas's patch says: "Fixes: f3bea49115b2 ("ftrace/x86: Add dynamic allocated
trampoline for ftrace_ops"), and this was added through kernel v3.19-rc1 so it
may well be the same issue given the traces on my issue also reflect a
module_alloc() trace after arch_ftrace_update_trampoline() which matches what
Thomas also saw when inspecting his buggy page with kmemleak echo dump [1].

[0] https://lkml.kernel.org/r/20170524175508.GZ8951@wotan.suse.de
[1] https://lkml.kernel.org/r/alpine.DEB.2.20.1705232243520.2409@nanos

BUT I'm also able to reprorduce a crash too though easily, as follows where
as before this commit it was passing all 45 tests just fine:

mcgrof@piggy ~/linux-next (git::next-20170519)$ sudo tools/testing/selftests/ftrace/ftracetest
=== Ftrace unit tests ===                                                       
[1] Basic trace file check      [PASS]                                          
[2] Basic test for tracers      [PASS]                                          
[3] Basic trace clock test      [PASS]                                          
[4] Basic event tracing check   [PASS]                                          
[5] event tracing - enable/disable with event level files       [PASS]          
[6] event tracing - restricts events based on pid       [PASS]                  
[7] event tracing - enable/disable with subsystem level files   [PASS]          
[8] event tracing - enable/disable with top level files [PASS]                  
[9] ftrace - function graph filters with stack tracer   [PASS]                  
[10] ftrace - function graph filters    [PASS]                                  
[11] ftrace - test for function event triggers 

<it hangs there>

I managed to get the kernel log:

[  429.416279] in mmio_trace_init
[  429.504315] mmiotrace: Disabling non-boot CPUs...
[  429.545572] Unregister pv shared memory for cpu 1
[  429.551725] smpboot: CPU 1 is now offline
[  429.553044] mmiotrace: CPU1 is down.
[  429.572874] Unregister pv shared memory for cpu 2
[  429.576361] smpboot: CPU 2 is now offline
[  429.578355] mmiotrace: CPU2 is down.
[  429.596755] Unregister pv shared memory for cpu 3
[  429.601946] smpboot: CPU 3 is now offline
[  429.602768] mmiotrace: CPU3 is down.
[  429.603418] mmiotrace: enabled.
[  429.605001] in mmio_trace_reset
[  429.605003] mmiotrace: Re-enabling CPUs...
[  429.605933] x86: Booting SMP configuration:
[  429.606505] smpboot: Booting Node 0 Processor 1 APIC 0x1
[  429.617989] kvm-clock: cpu 1, msr 1:3fff0041, secondary cpu clock
[  429.640600] KVM setup async PF for cpu 1
[  429.640603] kvm-stealtime: cpu 1, msr 13fc8da40
[  429.642384] mmiotrace: enabled CPU1.
[  429.643391] smpboot: Booting Node 0 Processor 2 APIC 0x2
[  429.654788] kvm-clock: cpu 2, msr 1:3fff0081, secondary cpu clock
[  429.675512] KVM setup async PF for cpu 2
[  429.675921] kvm-stealtime: cpu 2, msr 13fd0da40
[  429.676895] mmiotrace: enabled CPU2.
[  429.678531] smpboot: Booting Node 0 Processor 3 APIC 0x3
[  429.689999] kvm-clock: cpu 3, msr 1:3fff00c1, secondary cpu clock
[  429.710808] KVM setup async PF for cpu 3
[  429.711183] kvm-stealtime: cpu 3, msr 13fd8da40
[  429.712123] mmiotrace: enabled CPU3.
[  429.728330] mmiotrace: disabled.
[  440.537183] BUG: unable to handle kernel paging request at ffff964cf9376000
[  440.538899] IP: tlb_next_batch.isra.54+0x41/0x70
[  440.540022] PGD b736f067 
[  440.540024] P4D b736f067 
[  440.540707] PUD b7373067 
[  440.541318] PMD 12ff82063 
[  440.541953] PTE 8000000139376161

[  440.543555] Oops: 0003 [#1] SMP
[  440.543966] Modules linked in: ppdev(E) joydev(E) evdev(E) serio_raw(E) pcspkr(E) bochs_drm(E) drm_kms_helper(E) ttm(E) drm(E) i2c_piix4(E) sg(E) parport_pc(E) parport(E) acpi_cpufreq(E) button(E) sch_fq_codel(E) autofs4(E) crc32c_generic(E) ext4(E) crc16(E) jbd2(E) mbcache(E) sr_mod(E) cdrom(E) sd_mod(E) ata_generic(E) psmouse(E) floppy(E) ata_piix(E) e1000(E) libata(E) scsi_mod(E)
[  440.548057] CPU: 2 PID: 1765 Comm: ftracetest Tainted: G            E   4.12.0-rc1-next-20170519+ #155
[  440.549159] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.10.2-0-g5f4c7b1-prebuilt.qemu-project.org 04/01/2014
[  440.550329] task: ffff964cf14e2600 task.stack: ffffbe7c80b58000
[  440.550921] RIP: 0010:tlb_next_batch.isra.54+0x41/0x70
[  440.551385] RSP: 0018:ffffbe7c80b5bc40 EFLAGS: 00010202
[  440.551855] RAX: ffff964cf9376000 RBX: ffffbe7c80b5bdf0 RCX: 000000000001dd18
[  440.552493] RDX: 0000000000000000 RSI: 0000000000000011 RDI: 0000000000000246
[  440.553134] RBP: ffffbe7c80b5bc50 R08: ffff964cf0b6d958 R09: 0000000000000000
[  440.553750] R10: 0000000000000000 R11: 0000000000000000 R12: ffffbe7c80b5be48
[  440.554386] R13: ffffbe7c80b5bdd0 R14: 00005560d7a30000 R15: ffff964cf035c178
[  440.555025] FS:  0000000000000000(0000) GS:ffff964cffd00000(0000) knlGS:0000000000000000
[  440.555743] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  440.556255] CR2: ffff964cf9376000 CR3: 0000000139688000 CR4: 00000000000006e0
[  440.556895] Call Trace:
[  440.557107]  unmap_page_range+0x722/0x930
[  440.557443]  ? cpumask_any_but+0x24/0x40
[  440.557773]  unmap_single_vma+0x59/0xe0
[  440.558088]  ? lru_deactivate_file_fn+0x360/0x360
[  440.558485]  unmap_vmas+0x51/0xa0
[  440.558769]  exit_mmap+0xa7/0x170
[  440.559054]  mmput+0x53/0x140
[  440.559309]  do_exit+0x286/0xb30
[  440.559582]  ? __do_page_fault+0x266/0x4e0
[  440.559928]  do_group_exit+0x43/0xb0
[  440.560230]  SyS_exit_group+0x14/0x20
[  440.560539]  entry_SYSCALL_64_fastpath+0x1e/0xa9
[  440.560963] RIP: 0033:0x7f9533db24d8
[  440.561287] RSP: 002b:00007ffda4899cf8 EFLAGS: 00000246 ORIG_RAX: 00000000000000e7
[  440.561963] RAX: ffffffffffffffda RBX: 00005560d821c110 RCX: 00007f9533db24d8
[  440.562601] RDX: 0000000000000000 RSI: 000000000000003c RDI: 0000000000000000
[  440.563253] RBP: 00007f9534092b00 R08: 00000000000000e7 R09: ffffffffffffff98
[  440.563863] R10: 00005560d82208a0 R11: 0000000000000246 R12: 0000000000000000
[  440.564437] R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
[  440.565023] Code: 00 00 c3 83 3e 13 74 4f 55 48 89 e5 41 54 53 49 89 f4 48 89 fb 31 f6 bf 00 02 00 01 e8 d9 f0 fc ff 48 85 c0 74 2d 41 83 04 24 01 <48> c7 00 00 00 00 00 c7 40 08 00 00 00 00 c7 40 0c fe 01 00 00 
[  440.566681] RIP: tlb_next_batch.isra.54+0x41/0x70 RSP: ffffbe7c80b5bc40
[  440.567283] CR2: ffff964cf9376000
[  440.567592] ---[ end trace 382f2593927e722f ]---
[  440.567991] Fixing recursive fault but reboot is needed!

  Luis

Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX

[Thomas Gleixner]

On Wed, 24 May 2017, Luis R. Rodriguez wrote:
> [  440.548057] CPU: 2 PID: 1765 Comm: ftracetest Tainted: G            E

4.12.0-rc1-next-20170519+ #155

Can you please try that patch against plain 4.12-rc2?

Thanks,

	tglx

Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX

[Thomas Gleixner]

On Wed, 24 May 2017, Steven Rostedt wrote:

> OK, it crashed on one of my tests. I removed the patch and it boots
> fine, and crashes when I add it back.

It reproduces here with your config.

> BUG: unable to handle kernel paging request at ffff880214f5c000
> IP: new_slab+0x1e8/0x2b4
> PGD 338c067 
> P4D 338c067 
> PUD 338f067 
> PMD 212022063 
> PTE 8000000214f5c161
> 
> Oops: 0003 [#1] SMP
> Modules linked in:
> CPU: 3 PID: 1 Comm: swapper/0 Not tainted 4.12.0-rc2-test+ #42
> Hardware name: MSI MS-7823/CSM-H87M-G43 (MS-7823), BIOS V1.6 02/22/2014
> task: ffff8802153a8000 task.stack: ffffc90000c74000
> RIP: 0010:new_slab+0x1e8/0x2b4
> RSP: 0000:ffffc90000c77b28 EFLAGS: 00010282
> RAX: 0000000040040000 RBX: ffff880216003f00 RCX: ffff880214f5c058
> RDX: 0000000000000000 RSI: ffff880214f5c000 RDI: ffff880216003f00
> RBP: ffffc90000c77b70 R08: 000000000000002a R09: 0000000000000000
> R10: 00000000000201e2 R11: 0000000000020190 R12: ffff880214f5c000
> R13: 000000000000002e R14: 0000000000000001 R15: ffffea000853d700
> FS:  0000000000000000(0000) GS:ffff88021eb80000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: ffff880214f5c000 CR3: 000000000221d000 CR4: 00000000001406e0
> Call Trace:
>  ? interleave_nodes+0x29/0x40
>  ___slab_alloc+0x2e8/0x49e

That does not make any sense, but I'm digging into it.

Thanks,

	tglx

Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX

[Luis R. Rodriguez]

On Wed, May 24, 2017 at 08:53:00PM +0200, Thomas Gleixner wrote:
> On Wed, 24 May 2017, Luis R. Rodriguez wrote:
> > [  440.548057] CPU: 2 PID: 1765 Comm: ftracetest Tainted: G            E
> 
> 4.12.0-rc1-next-20170519+ #155
> 
> Can you please try that patch against plain 4.12-rc2?

Yup, just tried it. Same trace. Could we need to do something
special at __unregister_ftrace_function() and friends ?

  Luis

Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX

[Steven Rostedt]

On Wed, 24 May 2017 21:13:27 +0200 (CEST)
Thomas Gleixner <tglx@linutronix.de> wrote:


> > Oops: 0003 [#1] SMP
> > Modules linked in:
> > CPU: 3 PID: 1 Comm: swapper/0 Not tainted 4.12.0-rc2-test+ #42
> > Hardware name: MSI MS-7823/CSM-H87M-G43 (MS-7823), BIOS V1.6
> > 02/22/2014 task: ffff8802153a8000 task.stack: ffffc90000c74000
> > RIP: 0010:new_slab+0x1e8/0x2b4
> > RSP: 0000:ffffc90000c77b28 EFLAGS: 00010282
> > RAX: 0000000040040000 RBX: ffff880216003f00 RCX: ffff880214f5c058
> > RDX: 0000000000000000 RSI: ffff880214f5c000 RDI: ffff880216003f00
> > RBP: ffffc90000c77b70 R08: 000000000000002a R09: 0000000000000000
> > R10: 00000000000201e2 R11: 0000000000020190 R12: ffff880214f5c000
> > R13: 000000000000002e R14: 0000000000000001 R15: ffffea000853d700
> > FS:  0000000000000000(0000) GS:ffff88021eb80000(0000)
> > knlGS:0000000000000000 CS:  0010 DS: 0000 ES: 0000 CR0:
> > 0000000080050033 CR2: ffff880214f5c000 CR3: 000000000221d000 CR4:
> > 00000000001406e0 Call Trace:
> >  ? interleave_nodes+0x29/0x40
> >  ___slab_alloc+0x2e8/0x49e  
> 
> That does not make any sense, but I'm digging into it.

The trampolines uses the module allocation, and it appears, that needs
to become rw before freeing again.

I applied this patch, and it appears to fix the bug for me.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 663a35d..5e93a9a 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -689,8 +689,12 @@ static inline void *alloc_tramp(unsigned long size)
 {
 	return module_alloc(size);
 }
-static inline void tramp_free(void *tramp)
+static inline void tramp_free(void *tramp, int size)
 {
+	int npages;
+
+	npages = PAGE_ALIGN(size) >> PAGE_SHIFT;
+	set_memory_rw((unsigned long)tramp, npages);
 	module_memfree(tramp);
 }
 #else
@@ -699,7 +703,7 @@ static inline void *alloc_tramp(unsigned long size)
 {
 	return NULL;
 }
-static inline void tramp_free(void *tramp) { }
+static inline void tramp_free(void *tramp, int size) { }
 #endif
 
 /* Defined as markers to the end of the ftrace default trampolines */
@@ -771,7 +775,7 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
 	/* Copy ftrace_caller onto the trampoline memory */
 	ret = probe_kernel_read(trampoline, (void *)start_offset, size);
 	if (WARN_ON(ret < 0)) {
-		tramp_free(trampoline);
+		tramp_free(trampoline, *tramp_size);
 		return 0;
 	}
 
@@ -797,7 +801,7 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
 
 	/* Are we pointing to the reference? */
 	if (WARN_ON(memcmp(op_ptr.op, op_ref, 3) != 0)) {
-		tramp_free(trampoline);
+		tramp_free(trampoline, *tramp_size);
 		return 0;
 	}
 
@@ -943,7 +947,7 @@ void arch_ftrace_trampoline_free(struct ftrace_ops *ops)
 	if (!ops || !(ops->flags & FTRACE_OPS_FL_ALLOC_TRAMP))
 		return;
 
-	tramp_free((void *)ops->trampoline);
+	tramp_free((void *)ops->trampoline, ops->trampoline_size);
 	ops->trampoline = 0;
 }
 

Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX

[Luis R. Rodriguez]

On Wed, May 24, 2017 at 06:25:47PM -0400, Steven Rostedt wrote:
> On Wed, 24 May 2017 21:13:27 +0200 (CEST)
> Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> 
> > > Oops: 0003 [#1] SMP
> > > Modules linked in:
> > > CPU: 3 PID: 1 Comm: swapper/0 Not tainted 4.12.0-rc2-test+ #42
> > > Hardware name: MSI MS-7823/CSM-H87M-G43 (MS-7823), BIOS V1.6
> > > 02/22/2014 task: ffff8802153a8000 task.stack: ffffc90000c74000
> > > RIP: 0010:new_slab+0x1e8/0x2b4
> > > RSP: 0000:ffffc90000c77b28 EFLAGS: 00010282
> > > RAX: 0000000040040000 RBX: ffff880216003f00 RCX: ffff880214f5c058
> > > RDX: 0000000000000000 RSI: ffff880214f5c000 RDI: ffff880216003f00
> > > RBP: ffffc90000c77b70 R08: 000000000000002a R09: 0000000000000000
> > > R10: 00000000000201e2 R11: 0000000000020190 R12: ffff880214f5c000
> > > R13: 000000000000002e R14: 0000000000000001 R15: ffffea000853d700
> > > FS:  0000000000000000(0000) GS:ffff88021eb80000(0000)
> > > knlGS:0000000000000000 CS:  0010 DS: 0000 ES: 0000 CR0:
> > > 0000000080050033 CR2: ffff880214f5c000 CR3: 000000000221d000 CR4:
> > > 00000000001406e0 Call Trace:
> > >  ? interleave_nodes+0x29/0x40
> > >  ___slab_alloc+0x2e8/0x49e  
> > 
> > That does not make any sense, but I'm digging into it.
> 
> The trampolines uses the module allocation, and it appears, that needs
> to become rw before freeing again.
> 
> I applied this patch, and it appears to fix the bug for me.
> 
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

Awesome, this also cures my panic:

mcgrof@piggy ~/linux-next (git::20170524-fixwarn)$ sudo tools/testing/selftests/ftrace/ftracetest 
...
# of passed:  45
# of failed:  0
# of unresolved:  0
# of untested:  0
# of unsupported:  0
# of xfailed:  0
# of undefined(test bug):  0

So the combination of both:

Tested-by: Luis R. Rodriguez <mcgrof@kernel.org>

  Luis

Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX

[Thomas Gleixner]

On Wed, 24 May 2017, Steven Rostedt wrote:
> The trampolines uses the module allocation, and it appears, that needs
> to become rw before freeing again.

Indeed. I realized that when enabling more debug options, which led to a
reliable triple fault.

How intuitive.

> I applied this patch, and it appears to fix the bug for me.

It fixes the bug, but ...

> -static inline void tramp_free(void *tramp)
> +static inline void tramp_free(void *tramp, int size)
>  {
> +	int npages;
> +
> +	npages = PAGE_ALIGN(size) >> PAGE_SHIFT;

For correctness sake this wants

    	set_memory_nx(...);

as well.

> +	set_memory_rw((unsigned long)tramp, npages);
>  	module_memfree(tramp);
>  }

I'll clean that up and post a V2.

Thanks,

	tglx

[PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX

[Thomas Gleixner]

ftrace use module_alloc() to allocate trampoline pages. The mapping of
module_alloc() is RWX, which makes sense as the memory is written to right
after allocation. But nothing makes these pages RO after writing to them.

Add proper set_memory_rw/ro() calls to protect the trampolines after
modification.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/ftrace.c |   20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -689,8 +689,12 @@ static inline void *alloc_tramp(unsigned
 {
 	return module_alloc(size);
 }
-static inline void tramp_free(void *tramp)
+static inline void tramp_free(void *tramp, int size)
 {
+	int npages = PAGE_ALIGN(size) >> PAGE_SHIFT;
+
+	set_memory_nx((unsigned long)tramp, npages);
+	set_memory_rw((unsigned long)tramp, npages);
 	module_memfree(tramp);
 }
 #else
@@ -699,7 +703,7 @@ static inline void *alloc_tramp(unsigned
 {
 	return NULL;
 }
-static inline void tramp_free(void *tramp) { }
+static inline void tramp_free(void *tramp, int size) { }
 #endif
 
 /* Defined as markers to the end of the ftrace default trampolines */
@@ -771,7 +775,7 @@ create_trampoline(struct ftrace_ops *ops
 	/* Copy ftrace_caller onto the trampoline memory */
 	ret = probe_kernel_read(trampoline, (void *)start_offset, size);
 	if (WARN_ON(ret < 0)) {
-		tramp_free(trampoline);
+		tramp_free(trampoline, *tramp_size);
 		return 0;
 	}
 
@@ -797,7 +801,7 @@ create_trampoline(struct ftrace_ops *ops
 
 	/* Are we pointing to the reference? */
 	if (WARN_ON(memcmp(op_ptr.op, op_ref, 3) != 0)) {
-		tramp_free(trampoline);
+		tramp_free(trampoline, *tramp_size);
 		return 0;
 	}
 
@@ -839,7 +843,7 @@ void arch_ftrace_update_trampoline(struc
 	unsigned long offset;
 	unsigned long ip;
 	unsigned int size;
-	int ret;
+	int ret, npages;
 
 	if (ops->trampoline) {
 		/*
@@ -848,11 +852,14 @@ void arch_ftrace_update_trampoline(struc
 		 */
 		if (!(ops->flags & FTRACE_OPS_FL_ALLOC_TRAMP))
 			return;
+		npages = PAGE_ALIGN(ops->trampoline_size) >> PAGE_SHIFT;
+		set_memory_rw(ops->trampoline, npages);
 	} else {
 		ops->trampoline = create_trampoline(ops, &size);
 		if (!ops->trampoline)
 			return;
 		ops->trampoline_size = size;
+		npages = PAGE_ALIGN(size) >> PAGE_SHIFT;
 	}
 
 	offset = calc_trampoline_call_offset(ops->flags & FTRACE_OPS_FL_SAVE_REGS);
@@ -863,6 +870,7 @@ void arch_ftrace_update_trampoline(struc
 	/* Do a safe modify in case the trampoline is executing */
 	new = ftrace_call_replace(ip, (unsigned long)func);
 	ret = update_ftrace_func(ip, new);
+	set_memory_ro(ops->trampoline, npages);
 
 	/* The update should never fail */
 	WARN_ON(ret);
@@ -939,7 +947,7 @@ void arch_ftrace_trampoline_free(struct
 	if (!ops || !(ops->flags & FTRACE_OPS_FL_ALLOC_TRAMP))
 		return;
 
-	tramp_free((void *)ops->trampoline);
+	tramp_free((void *)ops->trampoline, ops->trampoline_size);
 	ops->trampoline = 0;
 }
 

Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX

[Masami Hiramatsu]

On Wed, 24 May 2017 18:25:47 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 24 May 2017 21:13:27 +0200 (CEST)
> Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> 
> > > Oops: 0003 [#1] SMP
> > > Modules linked in:
> > > CPU: 3 PID: 1 Comm: swapper/0 Not tainted 4.12.0-rc2-test+ #42
> > > Hardware name: MSI MS-7823/CSM-H87M-G43 (MS-7823), BIOS V1.6
> > > 02/22/2014 task: ffff8802153a8000 task.stack: ffffc90000c74000
> > > RIP: 0010:new_slab+0x1e8/0x2b4
> > > RSP: 0000:ffffc90000c77b28 EFLAGS: 00010282
> > > RAX: 0000000040040000 RBX: ffff880216003f00 RCX: ffff880214f5c058
> > > RDX: 0000000000000000 RSI: ffff880214f5c000 RDI: ffff880216003f00
> > > RBP: ffffc90000c77b70 R08: 000000000000002a R09: 0000000000000000
> > > R10: 00000000000201e2 R11: 0000000000020190 R12: ffff880214f5c000
> > > R13: 000000000000002e R14: 0000000000000001 R15: ffffea000853d700
> > > FS:  0000000000000000(0000) GS:ffff88021eb80000(0000)
> > > knlGS:0000000000000000 CS:  0010 DS: 0000 ES: 0000 CR0:
> > > 0000000080050033 CR2: ffff880214f5c000 CR3: 000000000221d000 CR4:
> > > 00000000001406e0 Call Trace:
> > >  ? interleave_nodes+0x29/0x40
> > >  ___slab_alloc+0x2e8/0x49e  
> > 
> > That does not make any sense, but I'm digging into it.
> 
> The trampolines uses the module allocation, and it appears, that needs
> to become rw before freeing again.

Oops, I also have to fix kprobes side, because it has same issue.

Thanks!

> 
> I applied this patch, and it appears to fix the bug for me.
> 
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> 
> -- Steve
> 
> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> index 663a35d..5e93a9a 100644
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -689,8 +689,12 @@ static inline void *alloc_tramp(unsigned long size)
>  {
>  	return module_alloc(size);
>  }
> -static inline void tramp_free(void *tramp)
> +static inline void tramp_free(void *tramp, int size)
>  {
> +	int npages;
> +
> +	npages = PAGE_ALIGN(size) >> PAGE_SHIFT;
> +	set_memory_rw((unsigned long)tramp, npages);
>  	module_memfree(tramp);
>  }
>  #else
> @@ -699,7 +703,7 @@ static inline void *alloc_tramp(unsigned long size)
>  {
>  	return NULL;
>  }
> -static inline void tramp_free(void *tramp) { }
> +static inline void tramp_free(void *tramp, int size) { }
>  #endif
>  
>  /* Defined as markers to the end of the ftrace default trampolines */
> @@ -771,7 +775,7 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
>  	/* Copy ftrace_caller onto the trampoline memory */
>  	ret = probe_kernel_read(trampoline, (void *)start_offset, size);
>  	if (WARN_ON(ret < 0)) {
> -		tramp_free(trampoline);
> +		tramp_free(trampoline, *tramp_size);
>  		return 0;
>  	}
>  
> @@ -797,7 +801,7 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
>  
>  	/* Are we pointing to the reference? */
>  	if (WARN_ON(memcmp(op_ptr.op, op_ref, 3) != 0)) {
> -		tramp_free(trampoline);
> +		tramp_free(trampoline, *tramp_size);
>  		return 0;
>  	}
>  
> @@ -943,7 +947,7 @@ void arch_ftrace_trampoline_free(struct ftrace_ops *ops)
>  	if (!ops || !(ops->flags & FTRACE_OPS_FL_ALLOC_TRAMP))
>  		return;
>  
> -	tramp_free((void *)ops->trampoline);
> +	tramp_free((void *)ops->trampoline, ops->trampoline_size);
>  	ops->trampoline = 0;
>  }
>  


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX

[Masami Hiramatsu]

On Thu, 25 May 2017 18:09:10 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> On Wed, 24 May 2017 18:25:47 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Wed, 24 May 2017 21:13:27 +0200 (CEST)
> > Thomas Gleixner <tglx@linutronix.de> wrote:
> > 
> > 
> > > > Oops: 0003 [#1] SMP
> > > > Modules linked in:
> > > > CPU: 3 PID: 1 Comm: swapper/0 Not tainted 4.12.0-rc2-test+ #42
> > > > Hardware name: MSI MS-7823/CSM-H87M-G43 (MS-7823), BIOS V1.6
> > > > 02/22/2014 task: ffff8802153a8000 task.stack: ffffc90000c74000
> > > > RIP: 0010:new_slab+0x1e8/0x2b4
> > > > RSP: 0000:ffffc90000c77b28 EFLAGS: 00010282
> > > > RAX: 0000000040040000 RBX: ffff880216003f00 RCX: ffff880214f5c058
> > > > RDX: 0000000000000000 RSI: ffff880214f5c000 RDI: ffff880216003f00
> > > > RBP: ffffc90000c77b70 R08: 000000000000002a R09: 0000000000000000
> > > > R10: 00000000000201e2 R11: 0000000000020190 R12: ffff880214f5c000
> > > > R13: 000000000000002e R14: 0000000000000001 R15: ffffea000853d700
> > > > FS:  0000000000000000(0000) GS:ffff88021eb80000(0000)
> > > > knlGS:0000000000000000 CS:  0010 DS: 0000 ES: 0000 CR0:
> > > > 0000000080050033 CR2: ffff880214f5c000 CR3: 000000000221d000 CR4:
> > > > 00000000001406e0 Call Trace:
> > > >  ? interleave_nodes+0x29/0x40
> > > >  ___slab_alloc+0x2e8/0x49e  
> > > 
> > > That does not make any sense, but I'm digging into it.
> > 
> > The trampolines uses the module allocation, and it appears, that needs
> > to become rw before freeing again.
> 
> Oops, I also have to fix kprobes side, because it has same issue.

OK, I've ensured that following command hits same BUG.

grep t /proc/kallsyms  | cut -f3 -d" " | grep -v .*\\..* | \
head -n 256 | while read i; do echo p $i+5 ; done > tracing/kprobe_events

echo 1 > tracing/events/kprobes/enable 
echo 0 > tracing/events/kprobes/enable
echo > tracing/kprobe_events
sleep 5

I'll send a bugfix asap.

Thank,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX

[Steven Rostedt]

On Thu, 25 May 2017 10:57:51 +0200 (CEST)
Thomas Gleixner <tglx@linutronix.de> wrote:

> ftrace use module_alloc() to allocate trampoline pages. The mapping of
> module_alloc() is RWX, which makes sense as the memory is written to
> right after allocation. But nothing makes these pages RO after
> writing to them.
> 
> Add proper set_memory_rw/ro() calls to protect the trampolines after
> modification.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

OK, I pulled this in and I'm currently running it through my test
suite. I'm about to board a flight, hopefully it runs smoothly and will
finish by the time I get home.

-- Steve

Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX

[Steven Rostedt]

On Thu, 25 May 2017 19:34:43 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:


> OK, I've ensured that following command hits same BUG.
> 
> grep t /proc/kallsyms  | cut -f3 -d" " | grep -v .*\\..* | \
> head -n 256 | while read i; do echo p $i+5 ; done >
> tracing/kprobe_events
> 
> echo 1 > tracing/events/kprobes/enable 
> echo 0 > tracing/events/kprobes/enable
> echo > tracing/kprobe_events
> sleep 5

Hi Masami,

Can you add a selftest that checks this too?

Thanks!

-- Steve

> 
> I'll send a bugfix asap.
> 
> Thank,
> 

Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX

[Luis R. Rodriguez]

On Thu, May 25, 2017 at 10:57:51AM +0200, Thomas Gleixner wrote:
> ftrace use module_alloc() to allocate trampoline pages. The mapping of
> module_alloc() is RWX, which makes sense as the memory is written to right
> after allocation. But nothing makes these pages RO after writing to them.
> 
> Add proper set_memory_rw/ro() calls to protect the trampolines after
> modification.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
>  arch/x86/kernel/ftrace.c |   20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -689,8 +689,12 @@ static inline void *alloc_tramp(unsigned
>  {
>  	return module_alloc(size);
>  }
> -static inline void tramp_free(void *tramp)
> +static inline void tramp_free(void *tramp, int size)
>  {
> +	int npages = PAGE_ALIGN(size) >> PAGE_SHIFT;
> +
> +	set_memory_nx((unsigned long)tramp, npages);
> +	set_memory_rw((unsigned long)tramp, npages);
>  	module_memfree(tramp);
>  }

Can/should module_memfree() just do this for users? With Masami's fix that'd
be 2 users already.

   Luis

Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX

[Kees Cook]

On Thu, May 25, 2017 at 10:46 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> On Thu, May 25, 2017 at 10:57:51AM +0200, Thomas Gleixner wrote:
>> ftrace use module_alloc() to allocate trampoline pages. The mapping of
>> module_alloc() is RWX, which makes sense as the memory is written to right
>> after allocation. But nothing makes these pages RO after writing to them.
>>
>> Add proper set_memory_rw/ro() calls to protect the trampolines after
>> modification.
>>
>> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>> ---
>>  arch/x86/kernel/ftrace.c |   20 ++++++++++++++------
>>  1 file changed, 14 insertions(+), 6 deletions(-)
>>
>> --- a/arch/x86/kernel/ftrace.c
>> +++ b/arch/x86/kernel/ftrace.c
>> @@ -689,8 +689,12 @@ static inline void *alloc_tramp(unsigned
>>  {
>>       return module_alloc(size);
>>  }
>> -static inline void tramp_free(void *tramp)
>> +static inline void tramp_free(void *tramp, int size)
>>  {
>> +     int npages = PAGE_ALIGN(size) >> PAGE_SHIFT;
>> +
>> +     set_memory_nx((unsigned long)tramp, npages);
>> +     set_memory_rw((unsigned long)tramp, npages);
>>       module_memfree(tramp);
>>  }
>
> Can/should module_memfree() just do this for users? With Masami's fix that'd
> be 2 users already.

It seems like it really should. That would put it in a single place
and avoid this mistake again in the future. Does module_memfree() have
access to the allocation size, or does that need to get plumbed?

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX

[Masami Hiramatsu]

On Thu, 25 May 2017 11:18:02 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 25 May 2017 19:34:43 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> 
> > OK, I've ensured that following command hits same BUG.
> > 
> > grep t /proc/kallsyms  | cut -f3 -d" " | grep -v .*\\..* | \
> > head -n 256 | while read i; do echo p $i+5 ; done >
> > tracing/kprobe_events
> > 
> > echo 1 > tracing/events/kprobes/enable 
> > echo 0 > tracing/events/kprobes/enable
> > echo > tracing/kprobe_events
> > sleep 5
> 
> Hi Masami,
> 
> Can you add a selftest that checks this too?

Yes, however, it should be included after my fix is
accepted, or it can cause a kernel panic...

Thanks,



-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX

[Thomas Gleixner]

On Thu, 25 May 2017, Kees Cook wrote:
> On Thu, May 25, 2017 at 10:46 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > On Thu, May 25, 2017 at 10:57:51AM +0200, Thomas Gleixner wrote:
> >> ftrace use module_alloc() to allocate trampoline pages. The mapping of
> >> module_alloc() is RWX, which makes sense as the memory is written to right
> >> after allocation. But nothing makes these pages RO after writing to them.
> >>
> >> Add proper set_memory_rw/ro() calls to protect the trampolines after
> >> modification.
> >>
> >> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> >> ---
> >>  arch/x86/kernel/ftrace.c |   20 ++++++++++++++------
> >>  1 file changed, 14 insertions(+), 6 deletions(-)
> >>
> >> --- a/arch/x86/kernel/ftrace.c
> >> +++ b/arch/x86/kernel/ftrace.c
> >> @@ -689,8 +689,12 @@ static inline void *alloc_tramp(unsigned
> >>  {
> >>       return module_alloc(size);
> >>  }
> >> -static inline void tramp_free(void *tramp)
> >> +static inline void tramp_free(void *tramp, int size)
> >>  {
> >> +     int npages = PAGE_ALIGN(size) >> PAGE_SHIFT;
> >> +
> >> +     set_memory_nx((unsigned long)tramp, npages);
> >> +     set_memory_rw((unsigned long)tramp, npages);
> >>       module_memfree(tramp);
> >>  }
> >
> > Can/should module_memfree() just do this for users? With Masami's fix that'd
> > be 2 users already.
> 
> It seems like it really should. That would put it in a single place
> and avoid this mistake again in the future. Does module_memfree() have
> access to the allocation size, or does that need to get plumbed?

No, it doesn't. But the number of instances is pretty limited.

Btw, looking at BPF. It allocates memory via module_alloc() which means
it's RWX. There is nothing in that BPF code which changes the permissions
afterwards ....

Thanks,

	tglx

Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX

[Heiko Carstens]

On Fri, May 26, 2017 at 09:03:13AM +0200, Thomas Gleixner wrote:
> > It seems like it really should. That would put it in a single place
> > and avoid this mistake again in the future. Does module_memfree() have
> > access to the allocation size, or does that need to get plumbed?
> 
> No, it doesn't. But the number of instances is pretty limited.
> 
> Btw, looking at BPF. It allocates memory via module_alloc() which means
> it's RWX. There is nothing in that BPF code which changes the permissions
> afterwards ....

For BPF you're probably referring to bpf_jit_binary_alloc()? Permissions
are changed with bpf_jit_binary_lock_ro() within each architecure backend.

Well, except for powerpc (cc'ed Michael).

Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX

[Masami Hiramatsu]

On Thu, 25 May 2017 12:51:21 -0700
Kees Cook <keescook@chromium.org> wrote:

> On Thu, May 25, 2017 at 10:46 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > On Thu, May 25, 2017 at 10:57:51AM +0200, Thomas Gleixner wrote:
> >> ftrace use module_alloc() to allocate trampoline pages. The mapping of
> >> module_alloc() is RWX, which makes sense as the memory is written to right
> >> after allocation. But nothing makes these pages RO after writing to them.
> >>
> >> Add proper set_memory_rw/ro() calls to protect the trampolines after
> >> modification.
> >>
> >> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> >> ---
> >>  arch/x86/kernel/ftrace.c |   20 ++++++++++++++------
> >>  1 file changed, 14 insertions(+), 6 deletions(-)
> >>
> >> --- a/arch/x86/kernel/ftrace.c
> >> +++ b/arch/x86/kernel/ftrace.c
> >> @@ -689,8 +689,12 @@ static inline void *alloc_tramp(unsigned
> >>  {
> >>       return module_alloc(size);
> >>  }
> >> -static inline void tramp_free(void *tramp)
> >> +static inline void tramp_free(void *tramp, int size)
> >>  {
> >> +     int npages = PAGE_ALIGN(size) >> PAGE_SHIFT;
> >> +
> >> +     set_memory_nx((unsigned long)tramp, npages);
> >> +     set_memory_rw((unsigned long)tramp, npages);
> >>       module_memfree(tramp);
> >>  }
> >
> > Can/should module_memfree() just do this for users? With Masami's fix that'd
> > be 2 users already.
> 
> It seems like it really should. That would put it in a single place
> and avoid this mistake again in the future. Does module_memfree() have
> access to the allocation size, or does that need to get plumbed?

module_memfree() is just a wrapper of vfree, so find_vm_area()
will help us to get the size.

Thank you,


> 
> -Kees
> 
> -- 
> Kees Cook
> Pixel Security


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX

[Thomas Gleixner]

On Fri, 26 May 2017, Heiko Carstens wrote:

> On Fri, May 26, 2017 at 09:03:13AM +0200, Thomas Gleixner wrote:
> > > It seems like it really should. That would put it in a single place
> > > and avoid this mistake again in the future. Does module_memfree() have
> > > access to the allocation size, or does that need to get plumbed?
> > 
> > No, it doesn't. But the number of instances is pretty limited.
> > 
> > Btw, looking at BPF. It allocates memory via module_alloc() which means
> > it's RWX. There is nothing in that BPF code which changes the permissions
> > afterwards ....
> 
> For BPF you're probably referring to bpf_jit_binary_alloc()? Permissions
> are changed with bpf_jit_binary_lock_ro() within each architecure backend.
> 
> Well, except for powerpc (cc'ed Michael).

The problem starts elsewhere. module_alloc() should not allocate RWX memory
in the first place. It should allocated RW and then the usage sites should
set it to RX when the code is ready to go.

Thanks,

	tglx

Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX

[Michael Ellerman]

Heiko Carstens <heiko.carstens@de.ibm.com> writes:

> On Fri, May 26, 2017 at 09:03:13AM +0200, Thomas Gleixner wrote:
>> > It seems like it really should. That would put it in a single place
>> > and avoid this mistake again in the future. Does module_memfree() have
>> > access to the allocation size, or does that need to get plumbed?
>> 
>> No, it doesn't. But the number of instances is pretty limited.
>> 
>> Btw, looking at BPF. It allocates memory via module_alloc() which means
>> it's RWX. There is nothing in that BPF code which changes the permissions
>> afterwards ....
>
> For BPF you're probably referring to bpf_jit_binary_alloc()? Permissions
> are changed with bpf_jit_binary_lock_ro() within each architecure backend.
>
> Well, except for powerpc (cc'ed Michael).

[hangs head in shame]

Thanks, we are working on this stuff (stricter RWX perms) at the moment,
so will add this to the list. It's complicated somewhat by the variety
of MMUs we support, but still.

cheers

Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX

[Steven Rostedt]

On Thu, 25 May 2017 10:57:51 +0200 (CEST)
Thomas Gleixner <tglx@linutronix.de> wrote:

> ftrace use module_alloc() to allocate trampoline pages. The mapping of
> module_alloc() is RWX, which makes sense as the memory is written to right
> after allocation. But nothing makes these pages RO after writing to them.
> 
> Add proper set_memory_rw/ro() calls to protect the trampolines after
> modification.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---

Unfortunately, this was triggered in my tests:

ftrace: allocating 54840 entries in 215 pages
Starting tracer 'function'
------------[ cut here ]------------
kernel BUG at /work/autotest/nobackup/linux-test.git/arch/x86/mm/pageattr.c:189!
invalid opcode: 0000 [#1] SMP
Modules linked in:
CPU: 0 PID: 0 Comm: swapper Not tainted 4.12.0-rc2-test+ #3
Hardware name: MSI MS-7823/CSM-H87M-G43 (MS-7823), BIOS V1.6 02/22/2014
task: ffffffffb4222500 task.stack: ffffffffb4200000
RIP: 0010:change_page_attr_set_clr+0x269/0x302
RSP: 0000:ffffffffb4203c88 EFLAGS: 00010046
RAX: 0000000000000046 RBX: 0000000000000000 RCX: 00000001b6000000
RDX: ffffffffb4203d40 RSI: 0000000000000000 RDI: ffffffffb4240d60
RBP: ffffffffb4203d18 R08: 00000001b6000000 R09: 0000000000000001
R10: ffffffffb4203aa8 R11: 0000000000000003 R12: ffffffffc029b000
R13: ffffffffb4203d40 R14: 0000000000000001 R15: 0000000000000000
FS:  0000000000000000(0000) GS:ffff9a639ea00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: ffff9a636b384000 CR3: 00000001ea21d000 CR4: 00000000000406b0
Call Trace:
 change_page_attr_clear+0x1f/0x21
 set_memory_ro+0x1e/0x20
 arch_ftrace_update_trampoline+0x207/0x21c
 ? ftrace_caller+0x64/0x64
 ? 0xffffffffc029b000
 ftrace_startup+0xf4/0x198
 register_ftrace_function+0x26/0x3c
 function_trace_init+0x5e/0x73
 tracer_init+0x1e/0x23
 tracing_set_tracer+0x127/0x15a
 register_tracer+0x19b/0x1bc
 init_function_trace+0x90/0x92
 early_trace_init+0x236/0x2b3
 start_kernel+0x200/0x3f5
 x86_64_start_reservations+0x29/0x2b
 x86_64_start_kernel+0x17c/0x18f
 secondary_startup_64+0x9f/0x9f
 ? secondary_startup_64+0x9f/0x9f
Code: 89 df e8 79 f4 ff ff 48 85 c0 74 12 f6 00 01 74 0d be 00 10 00 00 48 89 df e8 84 e8 ff ff 49 ff c4 eb a4 9c 58 0f ba e0 09 72 02 <0f> 0b 49 8d 84 24 ff 0f 00 00 48 25 00 f0 ff ff 49 39 c4 74 02
RIP: change_page_attr_set_clr+0x269/0x302 RSP: ffffffffb4203c88
---[ end trace 418d67f4f812a298 ]---
Kernel panic - not syncing: Attempted to kill the idle task!
---[ end Kernel panic - not syncing: Attempted to kill the idle task!

It appears that if you enable function tracing at boot up, calling
set_memory_ro() with interrupts disabled can cause this. As pageattr.c
at line 189 has:

	BUG_ON(irqs_disabled());

in cpa_flush_range()

-- Steve

Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX

[Thomas Gleixner]

On Fri, 26 May 2017, Steven Rostedt wrote:

> On Thu, 25 May 2017 10:57:51 +0200 (CEST)
> Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > ftrace use module_alloc() to allocate trampoline pages. The mapping of
> > module_alloc() is RWX, which makes sense as the memory is written to right
> > after allocation. But nothing makes these pages RO after writing to them.
> > 
> > Add proper set_memory_rw/ro() calls to protect the trampolines after
> > modification.
> > 
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > ---
> 
> Unfortunately, this was triggered in my tests:
> 
> ftrace: allocating 54840 entries in 215 pages
> Starting tracer 'function'
> ------------[ cut here ]------------
> kernel BUG at /work/autotest/nobackup/linux-test.git/arch/x86/mm/pageattr.c:189!
>
> It appears that if you enable function tracing at boot up, calling
> set_memory_ro() with interrupts disabled can cause this. As pageattr.c
> at line 189 has:
> 
> 	BUG_ON(irqs_disabled());

That's very early boot, right? So interrupts have to be disabled.

So this wants to be:

   BUG_ON(irqs_disabled() && !early_boot_irqs_disabled);

Thanks,

	tglx

Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX

[Steven Rostedt]

On Fri, 26 May 2017 15:50:38 +0200 (CEST)
Thomas Gleixner <tglx@linutronix.de> wrote:


> That's very early boot, right? So interrupts have to be disabled.
> 
> So this wants to be:
> 
>    BUG_ON(irqs_disabled() && !early_boot_irqs_disabled);
> 

I was thinking the same thing. I'll add a patch and retest.

Thanks!

-- Steve