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

[Masami Hiramatsu <mhiramat@kernel.org>]

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>