On Mon, 27 Jun 2016 15:54:35 +0200
Petr Mladek <pmla...@suse.com> wrote:
> Ftrace modifies the code on many locations. It is paranoid
> and avoid a kernel crash using probe_kernel_read() and
> probe_kernel_write(). The only exception is update_ftrace_func()
> where where we read the old code using memcpy().
>
> It is true that this function is used only to modify well
> defined functions that are part of the ftrace API. But
> it might still make sense to be paranoid and be consistent
> with the writing side.
I'm not so sure I'm too hot on this patch. I left it with the memcpy()
because it was a well known location. If this is wrong, then we should
crash the kernel.
I'm not totally against it. But a comment should be added stating
something like:
/*
* ip points to the ftrace infrastructure. If this fails,
* then something is totally messed up.
*/
and perhaps even add a WARN_ON() here too.
-- Steve
>
> Signed-off-by: Petr Mladek <pmla...@suse.com>
> ---
> arch/x86/kernel/ftrace.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> index 42ea69d35dfd..8305c6792ad2 100644
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -233,7 +233,8 @@ static int update_ftrace_func(unsigned long ip, void *new)
> unsigned char old[MCOUNT_INSN_SIZE];
> int ret;
>
> - memcpy(old, (void *)ip, MCOUNT_INSN_SIZE);
> + if (probe_kernel_read(old, (void *)ip, MCOUNT_INSN_SIZE))
> + return -EFAULT;
>
> /*
> * Make sure that we replace 5-byte instruction that
