perf: hw_breakpoint safety improvements

Hi, Peter-

Here are some baby steps toward eliminating nested NMIs. What do you think?

Andy Lutomirski (3):
x86/perf/hw_breakpoint: Disallow kernel breakpoints unless kprobe-safe x86/perf/hw_breakpoint: Improve range breakpoint validation x86/perf/hw_breakpoint: Fix check for kernelspace breakpoints

 arch/x86/kernel/hw_breakpoint.c | 31 ++++++++++++++++++++++++++++++-
 include/linux/kprobes.h         |  2 ++
 kernel/kprobes.c                |  2 +-
 3 files changed, 33 insertions(+), 2 deletions(-)

Hi, Peter-

Here are some baby steps toward eliminating nested NMIs. What do you think?

Code on the kprobe blacklist doesn't want unexpected int3 exceptions. It probably doesn't want unexpected debug exceptions either. Be safe: disallow breakpoints in nokprobes code.

On non-CONFIG_KPROBES kernels, there is no kprobe blacklist. In that case, disallow kernel breakpoints entirely.

It will be particularly important to keep hw breakpoints out of the entry and NMI code once we move debug exceptions off the IST stack.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/kernel/hw_breakpoint.c | 15 +++++++++++++++
 include/linux/kprobes.h         |  2 ++
 kernel/kprobes.c                |  2 +-
 3 files changed, 18 insertions(+), 1 deletion(-)

arch/x86/kernel/hw_breakpoint.c +5 -0

index 7114ba220fd4..78f3e90c5659 100644
@@ -32,6 +32,7 @@
 #include <linux/irqflags.h>
 #include <linux/notifier.h>
 #include <linux/kallsyms.h>
+#include <linux/kprobes.h>
 #include <linux/percpu.h>
 #include <linux/kdebug.h>
 #include <linux/kernel.h>

@@ -243,6 +244,20 @@ static int arch_build_bp_info(struct perf_event *bp)
 		info->type = X86_BREAKPOINT_RW;
 		break;
 	case HW_BREAKPOINT_X:
+		/*
+		 * We don't allow kernel breakpoints in places that are not
+		 * acceptable for kprobes.  On non-kprobes kernels, we don't
+		 * allow kernel breakpoints at all.

The check looked wrong, although I think it was actually safe. TASK_SIZE is unnecessarily small for compat tasks, and it wasn't possible to make a range breakpoint so large it started in user space and ended in kernel space.

Nonetheless, let's fix up the check for the benefit of future readers. A breakpoint is in the kernel if either end is in the kernel.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/kernel/hw_breakpoint.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

arch/x86/kernel/hw_breakpoint.c +5 -1

index 6f345d302cf6..50a3fad5b89f 100644
@@ -180,7 +180,11 @@ int arch_check_bp_in_kernelspace(struct perf_event *bp)
 	va = info->address;
 	len = bp->attr.bp_len;
 
-	return (va >= TASK_SIZE) && ((va + len - 1) >= TASK_SIZE);
+	/*
+	 * We don't need to worry about va + len - 1 overflowing:
+	 * we already require that va is aligned to a multiple of len.
+	 */
+	return (va >= TASK_SIZE_MAX) || ((va + len - 1) >= TASK_SIZE_MAX);
 }
 
 int arch_bp_generic_fields(int x86_len, int x86_type,

Signature · 7 lines

2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

From: Frederic Weisbecker [mailto:[email protected]]

Quoted 42 lines · Andy Lutomirski

Code on the kprobe blacklist doesn't want unexpected int3 exceptions. It probably doesn't want unexpected debug exceptions either. Be safe: disallow breakpoints in nokprobes code.

On non-CONFIG_KPROBES kernels, there is no kprobe blacklist. In that case, disallow kernel breakpoints entirely.

It will be particularly important to keep hw breakpoints out of the entry and NMI code once we move debug exceptions off the IST stack.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/kernel/hw_breakpoint.c | 15 +++++++++++++++
 include/linux/kprobes.h         |  2 ++
 kernel/kprobes.c                |  2 +-
 3 files changed, 18 insertions(+), 1 deletion(-)

arch/x86/kernel/hw_breakpoint.c +10 -0

index 7114ba220fd4..78f3e90c5659 100644
@@ -32,6 +32,7 @@
 #include <linux/irqflags.h>
 #include <linux/notifier.h>
 #include <linux/kallsyms.h>
+#include <linux/kprobes.h>
 #include <linux/percpu.h>
 #include <linux/kdebug.h>
 #include <linux/kernel.h>

@@ -243,6 +244,20 @@ static int arch_build_bp_info(struct perf_event *bp)
 		info->type = X86_BREAKPOINT_RW;
 		break;
 	case HW_BREAKPOINT_X:
+		/*
+		 * We don't allow kernel breakpoints in places that are not
+		 * acceptable for kprobes.  On non-kprobes kernels, we don't
+		 * allow kernel breakpoints at all.
+		 */
+		if (bp->attr.bp_addr >= TASK_SIZE_MAX) {
+#ifdef CONFIG_KPROBES
+			if (within_kprobe_blacklist(bp->attr.bp_addr))
+				return -EINVAL;

arch/x86/kernel/hw_breakpoint.c static int arch_build_bp_info(struct perf_event *bp) · line 260

Quoted 5 lines

arch/x86/kernel/hw_breakpoint.c +5 -0

+#else
+			return -EINVAL;
+#endif
+		}
+

It should be done on generic code I think. In validate_hw_breakpoint() under the arch_check_bp_in_kernelspace() check.

Quoted 12 lines

arch/x86/kernel/hw_breakpoint.c +0 -0

 		info->type = X86_BREAKPOINT_EXECUTE;
 		/*
 		 * x86 inst breakpoints need to have a specific undefined len.

include/linux/kprobes.h +0 -0

index 1ab54754a86d..8f6849084248 100644
@@ -267,6 +267,8 @@ extern void show_registers(struct pt_regs *regs);
 extern void kprobes_inc_nmissed_count(struct kprobe *p);
 extern bool arch_within_kprobe_blacklist(unsigned long addr);
 

include/linux/kprobes.h extern void show_registers(struct pt_regs *regs); · line 270

Quoted 1 line

include/linux/kprobes.h +1 -0

+extern bool within_kprobe_blacklist(unsigned long addr);

The name was fine for a kprobe's private function. But if you make it public, maybe standardize the prefix like kprobes_within_blacklist().

Hi,

Quoted 45 lines

From: Frederic Weisbecker [mailto:[email protected]]

Quoted 42 lines · Andy Lutomirski

Code on the kprobe blacklist doesn't want unexpected int3 exceptions. It probably doesn't want unexpected debug exceptions either. Be safe: disallow breakpoints in nokprobes code.

On non-CONFIG_KPROBES kernels, there is no kprobe blacklist. In that case, disallow kernel breakpoints entirely.

It will be particularly important to keep hw breakpoints out of the entry and NMI code once we move debug exceptions off the IST stack.

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/kernel/hw_breakpoint.c | 15 +++++++++++++++
 include/linux/kprobes.h         |  2 ++
 kernel/kprobes.c                |  2 +-
 3 files changed, 18 insertions(+), 1 deletion(-)

arch/x86/kernel/hw_breakpoint.c +10 -0

index 7114ba220fd4..78f3e90c5659 100644
@@ -32,6 +32,7 @@
 #include <linux/irqflags.h>
 #include <linux/notifier.h>
 #include <linux/kallsyms.h>
+#include <linux/kprobes.h>
 #include <linux/percpu.h>
 #include <linux/kdebug.h>
 #include <linux/kernel.h>

@@ -243,6 +244,20 @@ static int arch_build_bp_info(struct perf_event *bp)
            info->type = X86_BREAKPOINT_RW;
            break;
    case HW_BREAKPOINT_X:
+           /*
+            * We don't allow kernel breakpoints in places that are not
+            * acceptable for kprobes.  On non-kprobes kernels, we don't
+            * allow kernel breakpoints at all.
+            */
+           if (bp->attr.bp_addr >= TASK_SIZE_MAX) {
+#ifdef CONFIG_KPROBES
+                   if (within_kprobe_blacklist(bp->attr.bp_addr))
+                           return -EINVAL;

Quoted 8 lines

arch/x86/kernel/hw_breakpoint.c static int arch_build_bp_info(struct perf_event *bp) · line 260

Quoted 5 lines

arch/x86/kernel/hw_breakpoint.c +5 -0

+#else
+                   return -EINVAL;
+#endif
+           }
+

It should be done on generic code I think. In validate_hw_breakpoint() under the arch_check_bp_in_kernelspace() check.

Agreed, kprobes also does it in generic code.

Quoted 1 line

Agreed, kprobes also does it in generic code.

Well, the patchset got applied anyway and the reviews ignored...

include/linux/kprobes.h +2 -0

+}
+
 static inline int kprobes_built_in(void)
 {
 	return 0;

kernel/events/hw_breakpoint.c +8 -0

index 92ce5f4ccc26..7c08e6d1175c 100644
@@ -384,6 +384,14 @@ static int validate_hw_breakpoint(struct perf_event *bp)
 		 */
 		if (!capable(CAP_SYS_ADMIN))
 			return -EPERM;
+
+		/*
+		 * We don't allow kernel breakpoints in places that are not
+		 * acceptable for kprobes.  On non-kprobes kernels, we don't
+		 * allow kernel breakpoints at all.
+		 */
+		if (kprobe_blacklisted(bp->attr.bp_addr))
+			return -EINVAL;
 	}
 
 	return 0;