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

From: Masami Hiramatsu
Date: Wed May 24 2017 - 10:33:49 EST


On Wed, 24 May 2017 15:47:17 +0200 (CEST)
Thomas Gleixner <tglx@xxxxxxxxxxxxx> 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@xxxxxxxxxx>

Thank you,

> Fixes: f3bea49115b2 ("ftrace/x86: Add dynamic allocated trampoline for ftrace_ops")
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>