linux-kernel - Re: [PATCH 2/4] ftrace/x86: Do not crash when reading wrong ftrace func

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 28 Jun 2016 12:00:48 +0200
From:	Petr Mladek <pmladek@...e.com>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	Ingo Molnar <mingo@...hat.com>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	Jiri Kosina <jkosina@...e.cz>, linux-kernel@...r.kernel.org,
	x86@...nel.org
Subject: Re: [PATCH 2/4] ftrace/x86: Do not crash when reading wrong ftrace
 func

On Mon 2016-06-27 10:48:58, Steven Rostedt wrote:
> On Mon, 27 Jun 2016 15:54:35 +0200
> Petr Mladek <pmladek@...e.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 had a suspicion that you had had a reason for using memcpy(). This
is why I put it into a separate patch ;-)

Well, I still would feel better if the kernel survive. An error here
means that there is a bug in the core ftrace infrastructure but
it does not need to be fatal for the running system.


> 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.

Yup, WARN_ON() seems appropriate. I like it. Please,
find the updated patch below.


>From 168e3edcf44f0e9a31284b40818cfa693fee78a2 Mon Sep 17 00:00:00 2001
From: Petr Mladek <pmladek@...e.com>
Date: Wed, 22 Jun 2016 15:49:06 +0200
Subject: [PATCH 2/4] ftrace/x86: Do not crash when reading wrong ftrace func

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.

Signed-off-by: Petr Mladek <pmladek@...e.com>
---
 arch/x86/kernel/ftrace.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 42ea69d35dfd..951c4bd639c4 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -233,7 +233,13 @@ 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);
+	/*
+	 * ip points to the ftrace infrastructure. If this fails,
+	 * then something is totally messed up.
+	 */
+	ret = probe_kernel_read(old, (void *)ip, MCOUNT_INSN_SIZE);
+	if (WARN_ON(ret))
+		return -EFAULT;
 
 	/*
 	 * Make sure that we replace 5-byte instruction that
-- 
1.8.5.6