From:   Chen Zhongjin <chenzhongjin@huawei.com>
To:     <linux-trace-kernel@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
CC:     <chenzhongjin@huawei.com>, <rostedt@goodmis.org>,
        <mhiramat@kernel.org>, <mark.rutland@arm.com>
Subject: [PATCH] ftrace: Add ftrace_page to list after the index is calculated
Date:   Wed, 8 Mar 2023 15:08:44 +0800
Message-ID: <20230308070844.58180-1-chenzhongjin@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

KASAN reported follow problem:

 BUG: KASAN: use-after-free in lookup_rec
 Read of size 8 at addr ffff000199270ff0 by task modprobe

 CPU: 2 Comm: modprobe
 Hardware name: QEMU KVM Virtual Machine
 Call trace:
  kasan_report
  __asan_load8
  lookup_rec
  ftrace_location
  arch_check_ftrace_location
  check_kprobe_address_safe
  register_kprobe.part.0
  register_kprobe

This happened when lookup_rec accessing pg->records[pg->index - 1].
The accessed position is not a valid records address, it has -16 offset
to the last allocated records page.

In ftrace_process_locs, ftrace_page will be added to ftrace_pages_start
list fist, then its pg->index will be calculated. Before pg->index++,
pg->index == 0.

lookup_rec iterates the whole list if it doesn't find a record. When
there is a page with pg->index == 0, getting pg->records[-1].ip causes
this problem.

Add ftrace_page to the ftrace_pages_start list after pg->index is
calculated, to fix this.

Fixes: 3208230983a0 ("ftrace: Remove usage of "freed" records")
Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com>
---
 kernel/trace/ftrace.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 29baa97d0d53..a258c48ad91e 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -6804,28 +6804,14 @@ static int ftrace_process_locs(struct module *mod,
 
 	mutex_lock(&ftrace_lock);
 
+	if (WARN_ON(mod && !ftrace_pages))
+		goto out;
+
 	/*
 	 * Core and each module needs their own pages, as
 	 * modules will free them when they are removed.
 	 * Force a new page to be allocated for modules.
 	 */
-	if (!mod) {
-		WARN_ON(ftrace_pages || ftrace_pages_start);
-		/* First initialization */
-		ftrace_pages = ftrace_pages_start = start_pg;
-	} else {
-		if (!ftrace_pages)
-			goto out;
-
-		if (WARN_ON(ftrace_pages->next)) {
-			/* Hmm, we have free pages? */
-			while (ftrace_pages->next)
-				ftrace_pages = ftrace_pages->next;
-		}
-
-		ftrace_pages->next = start_pg;
-	}
-
 	p = start;
 	pg = start_pg;
 	while (p < end) {
@@ -6855,6 +6841,21 @@ static int ftrace_process_locs(struct module *mod,
 	/* We should have used all pages */
 	WARN_ON(pg->next);
 
+	/* Add pages to ftrace_pages list */
+	if (!mod) {
+		WARN_ON(ftrace_pages || ftrace_pages_start);
+		/* First initialization */
+		ftrace_pages_start = start_pg;
+	} else {
+		if (WARN_ON(ftrace_pages->next)) {
+			/* Hmm, we have free pages? */
+			while (ftrace_pages->next)
+				ftrace_pages = ftrace_pages->next;
+		}
+
+		ftrace_pages->next = start_pg;
+	}
+
 	/* Assign the last page to ftrace_pages */
 	ftrace_pages = pg;
 
-- 
2.17.1