On Wed, 24 May 2017 15:47:17 +0200 (CEST)
Thomas Gleixner <[email protected]> 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 <[email protected]>

Thank you,

> Fixes: f3bea49115b2 ("ftrace/x86: Add dynamic allocated trampoline for 
> ftrace_ops")
> Signed-off-by: Thomas Gleixner <[email protected]>
> ---
>  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 <[email protected]>

Reply via email to