Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1649635 > unrolled thread
| Started by | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| First post | 2017-05-24 15:50 +0200 |
| Last post | 2017-05-26 03:40 +0200 |
| Articles | 20 on this page of 26 — 7 participants |
Back to article view | Back to linux.kernel
[PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX Thomas Gleixner <tglx@linutronix.de> - 2017-05-24 15:50 +0200
Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX Masami Hiramatsu <mhiramat@kernel.org> - 2017-05-24 16:40 +0200
Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX Steven Rostedt <rostedt@goodmis.org> - 2017-05-24 17:10 +0200
Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-05-24 20:20 +0200
Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX Thomas Gleixner <tglx@linutronix.de> - 2017-05-24 21:00 +0200
Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-05-24 21:40 +0200
Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX Thomas Gleixner <tglx@linutronix.de> - 2017-05-24 21:20 +0200
Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX Steven Rostedt <rostedt@goodmis.org> - 2017-05-25 00:30 +0200
Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-05-25 01:20 +0200
Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX Thomas Gleixner <tglx@linutronix.de> - 2017-05-25 08:30 +0200
[PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX Thomas Gleixner <tglx@linutronix.de> - 2017-05-25 11:00 +0200
Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX Steven Rostedt <rostedt@goodmis.org> - 2017-05-25 17:20 +0200
Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-05-25 19:50 +0200
Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX Kees Cook <keescook@chromium.org> - 2017-05-25 22:00 +0200
Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX Thomas Gleixner <tglx@linutronix.de> - 2017-05-26 09:10 +0200
Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX Heiko Carstens <heiko.carstens@de.ibm.com> - 2017-05-26 11:40 +0200
Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX Thomas Gleixner <tglx@linutronix.de> - 2017-05-26 12:00 +0200
Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX Michael Ellerman <mpe@ellerman.id.au> - 2017-05-26 13:50 +0200
Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX Masami Hiramatsu <mhiramat@kernel.org> - 2017-05-26 11:50 +0200
Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX Steven Rostedt <rostedt@goodmis.org> - 2017-05-26 15:40 +0200
Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX Steven Rostedt <rostedt@goodmis.org> - 2017-05-26 16:00 +0200
Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX Thomas Gleixner <tglx@linutronix.de> - 2017-05-26 16:00 +0200
Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX Masami Hiramatsu <mhiramat@kernel.org> - 2017-05-25 11:10 +0200
Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX Masami Hiramatsu <mhiramat@kernel.org> - 2017-05-25 12:40 +0200
Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX Steven Rostedt <rostedt@goodmis.org> - 2017-05-25 17:20 +0200
Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX Masami Hiramatsu <mhiramat@kernel.org> - 2017-05-26 03:40 +0200
Page 1 of 2 [1] 2 Next page →
| From | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| Date | 2017-05-24 15:50 +0200 |
| Subject | [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX |
| Message-ID | <tKEPN-2YG-33@gated-at.bofh.it> |
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);
[toc] | [next] | [standalone]
| From | Masami Hiramatsu <mhiramat@kernel.org> |
|---|---|
| Date | 2017-05-24 16:40 +0200 |
| Subject | Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX |
| Message-ID | <tKFCb-3vO-27@gated-at.bofh.it> |
| In reply to | #1649635 |
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>
[toc] | [prev] | [next] | [standalone]
| From | Steven Rostedt <rostedt@goodmis.org> |
|---|---|
| Date | 2017-05-24 17:10 +0200 |
| Subject | Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX |
| Message-ID | <tKG5c-3Wx-43@gated-at.bofh.it> |
| In reply to | #1649635 |
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);
[toc] | [prev] | [next] | [standalone]
| From | "Luis R. Rodriguez" <mcgrof@kernel.org> |
|---|---|
| Date | 2017-05-24 20:20 +0200 |
| Message-ID | <tKJ34-5LE-11@gated-at.bofh.it> |
| In reply to | #1649635 |
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
[toc] | [prev] | [next] | [standalone]
| From | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| Date | 2017-05-24 21:00 +0200 |
| Subject | Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX |
| Message-ID | <tKJFM-5Zr-9@gated-at.bofh.it> |
| In reply to | #1649852 |
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
[toc] | [prev] | [next] | [standalone]
| From | "Luis R. Rodriguez" <mcgrof@kernel.org> |
|---|---|
| Date | 2017-05-24 21:40 +0200 |
| Message-ID | <tKKit-6rO-3@gated-at.bofh.it> |
| In reply to | #1649865 |
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
[toc] | [prev] | [next] | [standalone]
| From | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| Date | 2017-05-24 21:20 +0200 |
| Subject | Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX |
| Message-ID | <tKJZ8-6lc-13@gated-at.bofh.it> |
| In reply to | #1649635 |
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
[toc] | [prev] | [next] | [standalone]
| From | Steven Rostedt <rostedt@goodmis.org> |
|---|---|
| Date | 2017-05-25 00:30 +0200 |
| Subject | Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX |
| Message-ID | <tKMWZ-8aU-5@gated-at.bofh.it> |
| In reply to | #1649872 |
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;
}
[toc] | [prev] | [next] | [standalone]
| From | "Luis R. Rodriguez" <mcgrof@kernel.org> |
|---|---|
| Date | 2017-05-25 01:20 +0200 |
| Message-ID | <tKNJn-dX-13@gated-at.bofh.it> |
| In reply to | #1650000 |
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
[toc] | [prev] | [next] | [standalone]
| From | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| Date | 2017-05-25 08:30 +0200 |
| Subject | Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX |
| Message-ID | <tKUrw-4D7-3@gated-at.bofh.it> |
| In reply to | #1650000 |
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
[toc] | [prev] | [next] | [standalone]
| From | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| Date | 2017-05-25 11:00 +0200 |
| Subject | [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX |
| Message-ID | <tKWMG-5YF-15@gated-at.bofh.it> |
| In reply to | #1650211 |
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;
}
[toc] | [prev] | [next] | [standalone]
| From | Steven Rostedt <rostedt@goodmis.org> |
|---|---|
| Date | 2017-05-25 17:20 +0200 |
| Subject | Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX |
| Message-ID | <tL2Iq-1qK-23@gated-at.bofh.it> |
| In reply to | #1650336 |
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
[toc] | [prev] | [next] | [standalone]
| From | "Luis R. Rodriguez" <mcgrof@kernel.org> |
|---|---|
| Date | 2017-05-25 19:50 +0200 |
| Subject | Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX |
| Message-ID | <tL53z-2KJ-7@gated-at.bofh.it> |
| In reply to | #1650336 |
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
[toc] | [prev] | [next] | [standalone]
| From | Kees Cook <keescook@chromium.org> |
|---|---|
| Date | 2017-05-25 22:00 +0200 |
| Subject | Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX |
| Message-ID | <tL75o-40J-7@gated-at.bofh.it> |
| In reply to | #1650701 |
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
[toc] | [prev] | [next] | [standalone]
| From | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| Date | 2017-05-26 09:10 +0200 |
| Subject | Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX |
| Message-ID | <tLhxL-2Oy-3@gated-at.bofh.it> |
| In reply to | #1650781 |
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
[toc] | [prev] | [next] | [standalone]
| From | Heiko Carstens <heiko.carstens@de.ibm.com> |
|---|---|
| Date | 2017-05-26 11:40 +0200 |
| Subject | Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX |
| Message-ID | <tLjSW-490-15@gated-at.bofh.it> |
| In reply to | #1651118 |
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).
[toc] | [prev] | [next] | [standalone]
| From | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| Date | 2017-05-26 12:00 +0200 |
| Subject | Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX |
| Message-ID | <tLkch-4g5-3@gated-at.bofh.it> |
| In reply to | #1651264 |
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
[toc] | [prev] | [next] | [standalone]
| From | Michael Ellerman <mpe@ellerman.id.au> |
|---|---|
| Date | 2017-05-26 13:50 +0200 |
| Subject | Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX |
| Message-ID | <tLlUJ-5nB-13@gated-at.bofh.it> |
| In reply to | #1651264 |
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
[toc] | [prev] | [next] | [standalone]
| From | Masami Hiramatsu <mhiramat@kernel.org> |
|---|---|
| Date | 2017-05-26 11:50 +0200 |
| Subject | Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX |
| Message-ID | <tLk2B-4cR-1@gated-at.bofh.it> |
| In reply to | #1650781 |
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>
[toc] | [prev] | [next] | [standalone]
| From | Steven Rostedt <rostedt@goodmis.org> |
|---|---|
| Date | 2017-05-26 15:40 +0200 |
| Subject | Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX |
| Message-ID | <tLnDb-6sK-5@gated-at.bofh.it> |
| In reply to | #1650336 |
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
[toc] | [prev] | [next] | [standalone]
Page 1 of 2 [1] 2 Next page →
Back to top | Article view | linux.kernel
csiph-web