linux-kernel - [PATCH 2/2] x86/kprobes: Fix optprobe optimization check with CONFIG

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:   Wed,  7 Sep 2022 09:55:31 +0900
From:   "Masami Hiramatsu (Google)" <mhiramat@...nel.org>
To:     Steven Rostedt <rostedt@...dmis.org>,
        Peter Zijlstra <peterz@...radead.org>
Cc:     Masami Hiramatsu <mhiramat@...nel.org>,
        Ingo Molnar <mingo@...nel.org>,
        Suleiman Souhlal <suleiman@...gle.com>,
        bpf <bpf@...r.kernel.org>, linux-kernel@...r.kernel.org,
        Borislav Petkov <bp@...e.de>,
        Josh Poimboeuf <jpoimboe@...nel.org>, x86@...nel.org
Subject: [PATCH 2/2] x86/kprobes: Fix optprobe optimization check with CONFIG_RETHUNK

From: Masami Hiramatsu (Google) <mhiramat@...nel.org>

Since the CONFIG_RETHUNK and CONFIG_SLS will use INT3 for padding after
RET instruction, kprobe jump optimization always fails on the functions
with INT3 padding inside the function body. (It already checks the INT3
padding between functions, but not inside the function)

To avoid this issue, when it finds an INT3, read following bytes and
find the next non-INT3 instruction, and decode the function again to
search a branch which jumps to that address. If it can not find such
branch instruction, it thinks that INT3 does not come from RETHUNK or
SLS.

Signed-off-by: Masami Hiramatsu (Google) <mhiramat@...nel.org>
Suggested-by: Peter Zijlstra <peterz@...radead.org>
Fixes: 15e67227c49a ("x86: Undo return-thunk damage")
Cc: stable@...r.kernel.org
---
 arch/x86/kernel/kprobes/opt.c |   70 +++++++++++++++++++----------------------
 1 file changed, 33 insertions(+), 37 deletions(-)

diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c
index 2e41850cab06..ed77eeeef4ed 100644
--- a/arch/x86/kernel/kprobes/opt.c
+++ b/arch/x86/kernel/kprobes/opt.c
@@ -260,25 +260,12 @@ static int insn_is_indirect_jump(struct insn *insn)
 	return ret;
 }
 
-static bool is_padding_int3(unsigned long addr, unsigned long eaddr)
-{
-	unsigned char ops;
-
-	for (; addr < eaddr; addr++) {
-		if (get_kernel_nofault(ops, (void *)addr) < 0 ||
-		    ops != INT3_INSN_OPCODE)
-			return false;
-	}
-
-	return true;
-}
-
 /* Decode whole function to ensure any instructions don't jump into target */
 static int can_optimize(unsigned long paddr)
 {
-	unsigned long addr, size = 0, offset = 0;
-	struct insn insn;
 	kprobe_opcode_t buf[MAX_INSN_SIZE];
+	unsigned long size = 0, offset = 0;
+	struct insn insn;
 
 	/* Lookup symbol including addr */
 	if (!kallsyms_lookup_size_offset(paddr, &size, &offset))
@@ -296,11 +283,9 @@ static int can_optimize(unsigned long paddr)
 	if (size - offset < JMP32_INSN_SIZE)
 		return 0;
 
-	/* Decode instructions */
-	addr = paddr - offset;
-	while (addr < paddr - offset + size) { /* Decode until function end */
-		unsigned long recovered_insn;
-		int ret;
+	/* Decode all instructions in the function */
+	for_each_insn(&insn, paddr - offset, paddr - offset + size, buf) {
+		unsigned long addr = (unsigned long)insn.kaddr;
 
 		if (search_exception_tables(addr))
 			/*
@@ -308,31 +293,42 @@ static int can_optimize(unsigned long paddr)
 			 * we can't optimize kprobe in this function.
 			 */
 			return 0;
-		recovered_insn = recover_probed_instruction(buf, addr);
-		if (!recovered_insn)
-			return 0;
 
-		ret = insn_decode_kernel(&insn, (void *)recovered_insn);
-		if (ret < 0)
+		if (insn.opcode.bytes[0] == INT3_INSN_OPCODE) {
+			addr = skip_padding_int3(addr);
+			if (!addr)
+				return 0;
+			/*
+			 * If addr becomes the next function entry, this is
+			 * the INT3 padding between functions.
+			 */
+			if (addr - 1 == paddr - offset + size)
+				return 1;
+
+			/*
+			 * This can be padding INT3 for CONFIG_RETHUNK or
+			 * CONFIG_SLS. If a branch jumps to the address next
+			 * to the INT3 sequence, this is just for padding,
+			 * then we can continue decoding.
+			 */
+			for_each_insn(&insn, paddr - offset, addr, buf) {
+				if (insn_get_branch_addr(&insn) == addr)
+					goto found;
+			}
+
+			/* This INT3 can not be decoded safely. */
 			return 0;
+found:
+			/* Set loop cursor */
+			insn.next_byte = (void *)addr;
+			continue;
+		}
 
-		/*
-		 * In the case of detecting unknown breakpoint, this could be
-		 * a padding INT3 between functions. Let's check that all the
-		 * rest of the bytes are also INT3.
-		 */
-		if (insn.opcode.bytes[0] == INT3_INSN_OPCODE)
-			return is_padding_int3(addr, paddr - offset + size) ? 1 : 0;
-
-		/* Recover address */
-		insn.kaddr = (void *)addr;
-		insn.next_byte = (void *)(addr + insn.length);
 		/* Check any instructions don't jump into target */
 		if (insn_is_indirect_jump(&insn) ||
 		    insn_jump_into_range(&insn, paddr + INT3_INSN_SIZE,
 					 DISP32_SIZE))
 			return 0;
-		addr += insn.length;
 	}
 
 	return 1;