Skip to contentSkip to conversation

x86/kprobes: Fix optprobe to detect padding int3 correctly

PATCH Applied Masami Hiramatsu 5 participants 5 messages Dec 11, 2020 – Dec 12, 2020

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 x86/kprobes: Fix optprobe to detect padding int3 correctly

Fix optprobe to detect padding int3 correctly.

Since commit 7705dc855797 ("x86/vmlinux: Use INT3 instead of NOP for linker fill bytes") changed the padding bytes between functions from nop to int3, when optprobe decodes a target function it finds int3 and gives up the jump optimization.

Instead of giving up any int3 detection, this checks whether the rest of bytes to the end of the function are int3 or not. If all of those are int3, those come from the linker. In that case, optprobe continues jump optimization.

Fixes 7705dc855797 ("x86/vmlinux: Use INT3 instead of NOP for linker fill bytes")
---
 arch/x86/kernel/kprobes/opt.c |   22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)
arch/x86/kernel/kprobes/opt.c +20 -2
index 041f0b50bc27..b5cf39f1a855 100644
@@ -272,6 +272,19 @@ 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)
 {
@@ -310,9 +323,14 @@ static int can_optimize(unsigned long paddr)
 			return 0;
 		kernel_insn_init(&insn, (void *)recovered_insn, MAX_INSN_SIZE);
 		insn_get_length(&insn);
-		/* Another subsystem puts a breakpoint */
+		/*
+		 * 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 0;
+			return is_padding_int3(addr, paddr - offset + size) ? 1 : 0;
+
 		/* Recover address */
 		insn.kaddr = (void *)addr;
 		insn.next_byte = (void *)(addr + insn.length);
Reviewed-by on v1 1/1
#1 lore raw
Quoted 12 lines · On Fri, 11 Dec 2020 16:04:17 +0900 Masami Hiramatsu <[email protected]> wrote:

Fix optprobe to detect padding int3 correctly.

Since commit 7705dc855797 ("x86/vmlinux: Use INT3 instead of NOP for linker fill bytes") changed the padding bytes between functions from nop to int3, when optprobe decodes a target function it finds int3 and gives up the jump optimization.

Instead of giving up any int3 detection, this checks whether the rest of bytes to the end of the function are int3 or not. If all of those are int3, those come from the linker. In that case, optprobe continues jump optimization.

Quoted 4 lines
Fixes 7705dc855797 ("x86/vmlinux: Use INT3 instead of NOP for linker fill bytes")

This should probably go in via tip.

-- Steve

Quoted 45 lines
---
 arch/x86/kernel/kprobes/opt.c |   22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)
arch/x86/kernel/kprobes/opt.c +20 -2
index 041f0b50bc27..b5cf39f1a855 100644
@@ -272,6 +272,19 @@ 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)
 {
@@ -310,9 +323,14 @@ static int can_optimize(unsigned long paddr)
 			return 0;
 		kernel_insn_init(&insn, (void *)recovered_insn, MAX_INSN_SIZE);
 		insn_get_length(&insn);
-		/* Another subsystem puts a breakpoint */
+		/*
+		 * 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 0;
+			return is_padding_int3(addr, paddr - offset + size) ? 1 : 0;
+
 		/* Recover address */
 		insn.kaddr = (void *)addr;
 		insn.next_byte = (void *)(addr + insn.length);
Reviewed-by on v1 1/1
#2 lore raw
In reply to Masami Hiramatsu
Quoted 12 lines · Masami Hiramatsu

Fix optprobe to detect padding int3 correctly.

Since commit 7705dc855797 ("x86/vmlinux: Use INT3 instead of NOP for linker fill bytes") changed the padding bytes between functions from nop to int3, when optprobe decodes a target function it finds int3 and gives up the jump optimization.

Instead of giving up any int3 detection, this checks whether the rest of bytes to the end of the function are int3 or not. If all of those are int3, those come from the linker. In that case, optprobe continues jump optimization.

Quoted 4 lines
Fixes 7705dc855797 ("x86/vmlinux: Use INT3 instead of NOP for linker fill bytes")
Signature · 1 line
Kees Cook
#3 lore raw

Hi,

I've applied this patch on the top of kernel 5.9.7 and verified it works fine:

ffffffff813050e0 <ftrace_enable_sysctl>:
...
...

ffffffff813052f1:       e9 fe fe ff ff          jmpq   ffffffff813051f4 <ftrace_enable_sysctl+0x114>
ffffffff813052f6:       cc                      int3
ffffffff813052f7:       cc                      int3
ffffffff813052f8:       cc                      int3
ffffffff813052f9:       cc                      int3
ffffffff813052fa:       cc                      int3
ffffffff813052fb:       cc                      int3
ffffffff813052fc:       cc                      int3
ffffffff813052fd:       cc                      int3
ffffffff813052fe:       cc                      int3
ffffffff813052ff:       cc                      int3

When I install KRETPROBE on this function, it is correctly optimized:

root@pi3:~/off-debug/git/lkrg# cat /sys/kernel/debug/kprobes/list|grep ftrace_enable_sysctl
ffffffff813050e0  r  ftrace_enable_sysctl+0x0    [OPTIMIZED]
root@pi3:~/off-debug/git/lkrg# 

gef➤ x/2i ftrace_enable_sysctl
0xffffffff813050e0 <ftrace_enable_sysctl>: jmp 0xffffffffc062807a
0xffffffff813050e5 <ftrace_enable_sysctl+5>: push r14

Thanks,
Adam

Quoted 22 lines · Kees Cook
Quoted 12 lines · Masami Hiramatsu

Fix optprobe to detect padding int3 correctly.

Since commit 7705dc855797 ("x86/vmlinux: Use INT3 instead of NOP for linker fill bytes") changed the padding bytes between functions from nop to int3, when optprobe decodes a target function it finds int3 and gives up the jump optimization.

Instead of giving up any int3 detection, this checks whether the rest of bytes to the end of the function are int3 or not. If all of those are int3, those come from the linker. In that case, optprobe continues jump optimization.

Quoted 4 lines
Fixes 7705dc855797 ("x86/vmlinux: Use INT3 instead of NOP for linker fill bytes")
Signature · 1 line
Kees Cook
Signature · 2 lines
pi3 (pi3ki31ny) - pi3 (at) itsec pl
http://pi3.com.pl
#4 lore raw

[tip: x86/urgent] x86/kprobes: Fix optprobe to detect INT3 padding correctly

In reply to Masami Hiramatsu
The following commit has been merged into the x86/urgent branch of tip:

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     0d07c0ec4381f630c801539c79ad8dcc627f6e4a
Gitweb:        https://git.kernel.org/tip/0d07c0ec4381f630c801539c79ad8dcc627f6e4a
Author:        Masami Hiramatsu <[email protected]>
AuthorDate:    Fri, 11 Dec 2020 16:04:17 +09:00
Committer:     Borislav Petkov <[email protected]>
CommitterDate: Sat, 12 Dec 2020 15:25:17 +01:00

x86/kprobes: Fix optprobe to detect INT3 padding correctly

Commit

7705dc855797 ("x86/vmlinux: Use INT3 instead of NOP for linker fill bytes")

changed the padding bytes between functions from NOP to INT3. However, when optprobe decodes a target function it finds INT3 and gives up the jump optimization.

Instead of giving up any INT3 detection, check whether the rest of the bytes to the end of the function are INT3. If all of them are INT3, those come from the linker. In that case, continue the optprobe jump optimization.

[ bp: Massage commit message. ]

---
 arch/x86/kernel/kprobes/opt.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)
arch/x86/kernel/kprobes/opt.c +20 -2
index 041f0b5..08eb230 100644
@@ -272,6 +272,19 @@ 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)
 {
@@ -310,9 +323,14 @@ static int can_optimize(unsigned long paddr)
 			return 0;
 		kernel_insn_init(&insn, (void *)recovered_insn, MAX_INSN_SIZE);
 		insn_get_length(&insn);
-		/* Another subsystem puts a breakpoint */
+		/*
+		 * 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 0;
+			return is_padding_int3(addr, paddr - offset + size) ? 1 : 0;
+
 		/* Recover address */
 		insn.kaddr = (void *)addr;
 		insn.next_byte = (void *)(addr + insn.length);