[PATCH 0/3] perf: hw_breakpoint safety improvements

[PATCH 0/3] perf: hw_breakpoint safety improvements

[Andy Lutomirski]

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(-)

-- 
2.4.3

[PATCH 1/3] x86/perf/hw_breakpoint: Disallow kernel breakpoints unless kprobe-safe

[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 <luto@kernel.org>
---
 arch/x86/kernel/hw_breakpoint.c | 15 +++++++++++++++
 include/linux/kprobes.h         |  2 ++
 kernel/kprobes.c                |  2 +-
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
index 7114ba220fd4..78f3e90c5659 100644
--- a/arch/x86/kernel/hw_breakpoint.c
+++ b/arch/x86/kernel/hw_breakpoint.c
@@ -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;
+#else
+			return -EINVAL;
+#endif
+		}
+
 		info->type = X86_BREAKPOINT_EXECUTE;
 		/*
 		 * x86 inst breakpoints need to have a specific undefined len.
diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index 1ab54754a86d..8f6849084248 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -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);
 
+extern bool within_kprobe_blacklist(unsigned long addr);
+
 struct kprobe_insn_cache {
 	struct mutex mutex;
 	void *(*alloc)(void);	/* allocate insn page */
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index c90e417bb963..d10ab6b9b5e0 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1332,7 +1332,7 @@ bool __weak arch_within_kprobe_blacklist(unsigned long addr)
 	       addr < (unsigned long)__kprobes_text_end;
 }
 
-static bool within_kprobe_blacklist(unsigned long addr)
+bool within_kprobe_blacklist(unsigned long addr)
 {
 	struct kprobe_blacklist_entry *ent;
 
-- 
2.4.3

[PATCH 2/3] x86/perf/hw_breakpoint: Improve range breakpoint validation

[Andy Lutomirski]

Range breakpoints will do the wrong thing if the address isn't
aligned.  While we're there, add comments about why it's safe for
instruction breakpoints.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/kernel/hw_breakpoint.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
index 78f3e90c5659..6f345d302cf6 100644
--- a/arch/x86/kernel/hw_breakpoint.c
+++ b/arch/x86/kernel/hw_breakpoint.c
@@ -291,8 +291,18 @@ static int arch_build_bp_info(struct perf_event *bp)
 		break;
 #endif
 	default:
+		/* AMD range breakpoint */
 		if (!is_power_of_2(bp->attr.bp_len))
 			return -EINVAL;
+		if (bp->attr.bp_addr & (bp->attr.bp_len - 1))
+			return -EINVAL;
+		/*
+		 * It's impossible to use a range breakpoint to fake out
+		 * user vs kernel detection because bp_len - 1 can't
+		 * have the high bit set.  If we ever allow range instruction
+		 * breakpoints, then we'll have to check for kprobe-blacklisted
+		 * addresses anywhere in the range.
+		 */
 		if (!cpu_has_bpext)
 			return -EOPNOTSUPP;
 		info->mask = bp->attr.bp_len - 1;
-- 
2.4.3

[PATCH 3/3] x86/perf/hw_breakpoint: Fix check for kernelspace breakpoints

[Andy Lutomirski]

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 <luto@kernel.org>
---
 arch/x86/kernel/hw_breakpoint.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
index 6f345d302cf6..50a3fad5b89f 100644
--- a/arch/x86/kernel/hw_breakpoint.c
+++ b/arch/x86/kernel/hw_breakpoint.c
@@ -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,
-- 
2.4.3

Re: [PATCH 0/3] perf: hw_breakpoint safety improvements

[Ingo Molnar]

* Andy Lutomirski <luto@kernel.org> wrote:

> 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(-)

Looks good to me at first glance. Cc:-ed Frederic who wrote and maintains these 
bits. Frederic, what do you think?

Thanks,

	Ingo

Re: [PATCH 0/3] perf: hw_breakpoint safety improvements

[Peter Zijlstra]

On Thu, Jul 30, 2015 at 08:32:39PM -0700, Andy Lutomirski wrote:
> Hi, Peter-
> 
> Here are some baby steps toward eliminating nested NMIs.  What do
> you think?

They work for me, Thanks!

[tip:perf/core] perf/x86/hw_breakpoints: Disallow kernel breakpoints unless kprobe-safe

[tip-bot for Andy Lutomirski]

Commit-ID:  e5779e8e12299f77c2421a707855d8d124171d85
Gitweb:     http://git.kernel.org/tip/e5779e8e12299f77c2421a707855d8d124171d85
Author:     Andy Lutomirski <luto@kernel.org>
AuthorDate: Thu, 30 Jul 2015 20:32:40 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 4 Aug 2015 10:16:54 +0200

perf/x86/hw_breakpoints: Disallow kernel breakpoints unless kprobe-safe

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 <luto@kernel.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/e14b152af99640448d895e3c2a8c2d5ee19a1325.1438312874.git.luto@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/hw_breakpoint.c | 15 +++++++++++++++
 include/linux/kprobes.h         |  2 ++
 kernel/kprobes.c                |  2 +-
 3 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
index 7114ba2..78f3e90 100644
--- a/arch/x86/kernel/hw_breakpoint.c
+++ b/arch/x86/kernel/hw_breakpoint.c
@@ -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;
+#else
+			return -EINVAL;
+#endif
+		}
+
 		info->type = X86_BREAKPOINT_EXECUTE;
 		/*
 		 * x86 inst breakpoints need to have a specific undefined len.
diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index 1ab5475..8f68490 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -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);
 
+extern bool within_kprobe_blacklist(unsigned long addr);
+
 struct kprobe_insn_cache {
 	struct mutex mutex;
 	void *(*alloc)(void);	/* allocate insn page */
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index c90e417..d10ab6b 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1332,7 +1332,7 @@ bool __weak arch_within_kprobe_blacklist(unsigned long addr)
 	       addr < (unsigned long)__kprobes_text_end;
 }
 
-static bool within_kprobe_blacklist(unsigned long addr)
+bool within_kprobe_blacklist(unsigned long addr)
 {
 	struct kprobe_blacklist_entry *ent;
 

[tip:perf/core] perf/x86/hw_breakpoints: Improve range breakpoint validation

[tip-bot for Andy Lutomirski]

Commit-ID:  ab513927ab449af00cc70b0269e15ee80dd537f9
Gitweb:     http://git.kernel.org/tip/ab513927ab449af00cc70b0269e15ee80dd537f9
Author:     Andy Lutomirski <luto@kernel.org>
AuthorDate: Thu, 30 Jul 2015 20:32:41 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 4 Aug 2015 10:16:54 +0200

perf/x86/hw_breakpoints: Improve range breakpoint validation

Range breakpoints will do the wrong thing if the address isn't
aligned.  While we're there, add comments about why it's safe for
instruction breakpoints.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/ae25d14d61f2f43b78e0a247e469f3072df7e201.1438312874.git.luto@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/hw_breakpoint.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
index 78f3e90..6f345d3 100644
--- a/arch/x86/kernel/hw_breakpoint.c
+++ b/arch/x86/kernel/hw_breakpoint.c
@@ -291,8 +291,18 @@ static int arch_build_bp_info(struct perf_event *bp)
 		break;
 #endif
 	default:
+		/* AMD range breakpoint */
 		if (!is_power_of_2(bp->attr.bp_len))
 			return -EINVAL;
+		if (bp->attr.bp_addr & (bp->attr.bp_len - 1))
+			return -EINVAL;
+		/*
+		 * It's impossible to use a range breakpoint to fake out
+		 * user vs kernel detection because bp_len - 1 can't
+		 * have the high bit set.  If we ever allow range instruction
+		 * breakpoints, then we'll have to check for kprobe-blacklisted
+		 * addresses anywhere in the range.
+		 */
 		if (!cpu_has_bpext)
 			return -EOPNOTSUPP;
 		info->mask = bp->attr.bp_len - 1;

[tip:perf/core] perf/x86/hw_breakpoints: Fix check for kernel-space breakpoints

[tip-bot for Andy Lutomirski]

Commit-ID:  27747f8bc355a2808ca9e490ab6866acd85b4c16
Gitweb:     http://git.kernel.org/tip/27747f8bc355a2808ca9e490ab6866acd85b4c16
Author:     Andy Lutomirski <luto@kernel.org>
AuthorDate: Thu, 30 Jul 2015 20:32:42 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 4 Aug 2015 10:16:55 +0200

perf/x86/hw_breakpoints: Fix check for kernel-space breakpoints

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 <luto@kernel.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/136be387950e78f18cea60e9d1bef74465d0ee8f.1438312874.git.luto@kernel.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/hw_breakpoint.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
index 6f345d3..50a3fad 100644
--- a/arch/x86/kernel/hw_breakpoint.c
+++ b/arch/x86/kernel/hw_breakpoint.c
@@ -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,

Re: [PATCH 1/3] x86/perf/hw_breakpoint: Disallow kernel breakpoints unless kprobe-safe

[Frederic Weisbecker]

On Thu, Jul 30, 2015 at 08:32:40PM -0700, Andy Lutomirski wrote:
> 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 <luto@kernel.org>
> ---
>  arch/x86/kernel/hw_breakpoint.c | 15 +++++++++++++++
>  include/linux/kprobes.h         |  2 ++
>  kernel/kprobes.c                |  2 +-
>  3 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
> index 7114ba220fd4..78f3e90c5659 100644
> --- a/arch/x86/kernel/hw_breakpoint.c
> +++ b/arch/x86/kernel/hw_breakpoint.c
> @@ -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;
> +#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.

>  		info->type = X86_BREAKPOINT_EXECUTE;
>  		/*
>  		 * x86 inst breakpoints need to have a specific undefined len.
> diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
> index 1ab54754a86d..8f6849084248 100644
> --- a/include/linux/kprobes.h
> +++ b/include/linux/kprobes.h
> @@ -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);
>  
> +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().

Thanks.

Re: [PATCH 3/3] x86/perf/hw_breakpoint: Fix check for kernelspace breakpoints

[Frederic Weisbecker]

On Thu, Jul 30, 2015 at 08:32:42PM -0700, Andy Lutomirski wrote:
> 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 <luto@kernel.org>

Indeed, in fact Oleg posted the same patch a long while ago but I eventually forgot to
track them.

See https://lkml.org/lkml/2013/11/24/44

Arm, arm64 and sh show the same issue and powerpc uses some obscure is_kernel_addr().

Eventually we should move this function to kernel/event/hw_breakpoint.c with a weak
tag to let archs override it like powerpc does. Although it seems not to care about the
breakpoint length so perhaps it's not correct.

In fact we should have some sort of generic in_kernel_va_range(start, length). This
reminds me something I'm sure it already exists :-)

> ---
>  arch/x86/kernel/hw_breakpoint.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
> index 6f345d302cf6..50a3fad5b89f 100644
> --- a/arch/x86/kernel/hw_breakpoint.c
> +++ b/arch/x86/kernel/hw_breakpoint.c
> @@ -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,
> -- 
> 2.4.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH 1/3] x86/perf/hw_breakpoint: Disallow kernel breakpoints unless kprobe-safe

[平松雅巳 / HIRAMATU，MASAMI]

Hi,

> From: Frederic Weisbecker [mailto:fweisbec@gmail.com]
> 
> On Thu, Jul 30, 2015 at 08:32:40PM -0700, Andy Lutomirski wrote:
> > 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 <luto@kernel.org>
> > ---
> >  arch/x86/kernel/hw_breakpoint.c | 15 +++++++++++++++
> >  include/linux/kprobes.h         |  2 ++
> >  kernel/kprobes.c                |  2 +-
> >  3 files changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
> > index 7114ba220fd4..78f3e90c5659 100644
> > --- a/arch/x86/kernel/hw_breakpoint.c
> > +++ b/arch/x86/kernel/hw_breakpoint.c
> > @@ -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;
> > +#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.

> 
> >  		info->type = X86_BREAKPOINT_EXECUTE;
> >  		/*
> >  		 * x86 inst breakpoints need to have a specific undefined len.
> > diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
> > index 1ab54754a86d..8f6849084248 100644
> > --- a/include/linux/kprobes.h
> > +++ b/include/linux/kprobes.h
> > @@ -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);
> >
> > +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().

No, there is the "kprobe_blacklist", that function means 
"whether the address is within kprobe_blacklist or not?" like within_module_core.

Thank you,

> 

Re: [PATCH 1/3] x86/perf/hw_breakpoint: Disallow kernel breakpoints unless kprobe-safe

[Frederic Weisbecker]

2015-08-05 1:29 GMT+02:00 平松雅巳 / HIRAMATU，MASAMI
<masami.hiramatsu.pt@hitachi.com>:
> Hi,
>
>> From: Frederic Weisbecker [mailto:fweisbec@gmail.com]
>>
>> On Thu, Jul 30, 2015 at 08:32:40PM -0700, Andy Lutomirski wrote:
>> > 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 <luto@kernel.org>
>> > ---
>> >  arch/x86/kernel/hw_breakpoint.c | 15 +++++++++++++++
>> >  include/linux/kprobes.h         |  2 ++
>> >  kernel/kprobes.c                |  2 +-
>> >  3 files changed, 18 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
>> > index 7114ba220fd4..78f3e90c5659 100644
>> > --- a/arch/x86/kernel/hw_breakpoint.c
>> > +++ b/arch/x86/kernel/hw_breakpoint.c
>> > @@ -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;
>> > +#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.

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

Re: [PATCH 1/3] x86/perf/hw_breakpoint: Disallow kernel breakpoints unless kprobe-safe

[Peter Zijlstra]

On Tue, Sep 01, 2015 at 12:57:11PM +0200, Frederic Weisbecker wrote:

> > Agreed, kprobes also does it in generic code.
> 
> Well, the patchset got applied anyway and the reviews ignored...

Bugger, sorry about that, I meant to change it and things slipped, how
about I atone by doing the patch.

---
Subject: perf,hwbreakpoint,kprobe: Put kprobe test in generic code

Place the kprobe blacklist test for breakpoints in generic code; as in
general any place we should not kprobe we should not break on either, as
the reason for marking things nokprobe is that the code in question
cannot deal with interrupts of this kind.

Suggested-by: Frederic Weisbecker <fweisbec@gmail.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/x86/kernel/hw_breakpoint.c | 14 --------------
 include/linux/kprobes.h         |  7 ++++++-
 kernel/events/hw_breakpoint.c   |  8 ++++++++
 kernel/kprobes.c                |  4 ++--
 4 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/arch/x86/kernel/hw_breakpoint.c b/arch/x86/kernel/hw_breakpoint.c
index 50a3fad5b89f..82b4a86b1e94 100644
--- a/arch/x86/kernel/hw_breakpoint.c
+++ b/arch/x86/kernel/hw_breakpoint.c
@@ -248,20 +248,6 @@ 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;
-#else
-			return -EINVAL;
-#endif
-		}
-
 		info->type = X86_BREAKPOINT_EXECUTE;
 		/*
 		 * x86 inst breakpoints need to have a specific undefined len.
diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index 8f6849084248..54bb483fbfe4 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -267,7 +267,7 @@ 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);
 
-extern bool within_kprobe_blacklist(unsigned long addr);
+extern bool kprobe_blacklisted(unsigned long addr);
 
 struct kprobe_insn_cache {
 	struct mutex mutex;
@@ -391,6 +391,11 @@ void dump_kprobe(struct kprobe *kp);
 
 #else /* !CONFIG_KPROBES: */
 
+static inline bool kprobe_blacklisted(unsigned long addr)
+{
+	return false;
+}
+
 static inline int kprobes_built_in(void)
 {
 	return 0;
diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index 92ce5f4ccc26..7c08e6d1175c 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -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;
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index d10ab6b9b5e0..adf646b17016 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1332,7 +1332,7 @@ bool __weak arch_within_kprobe_blacklist(unsigned long addr)
 	       addr < (unsigned long)__kprobes_text_end;
 }
 
-bool within_kprobe_blacklist(unsigned long addr)
+bool kprobe_blacklisted(unsigned long addr)
 {
 	struct kprobe_blacklist_entry *ent;
 
@@ -1442,7 +1442,7 @@ static int check_kprobe_address_safe(struct kprobe *p,
 
 	/* Ensure it is not in reserved area nor out of text */
 	if (!kernel_text_address((unsigned long) p->addr) ||
-	    within_kprobe_blacklist((unsigned long) p->addr) ||
+	    kprobe_blacklisted((unsigned long) p->addr) ||
 	    jump_label_text_reserved(p->addr, p->addr)) {
 		ret = -EINVAL;
 		goto out;

Re: [PATCH 1/3] x86/perf/hw_breakpoint: Disallow kernel breakpoints unless kprobe-safe

[Peter Zijlstra]

On Tue, Sep 01, 2015 at 01:36:08PM +0200, Peter Zijlstra wrote:
> +static inline bool kprobe_blacklisted(unsigned long addr)
> +{
> +	return false;

Ah, I think that wants to be true instead, to match the below comment.

> +}
> +
>  static inline int kprobes_built_in(void)
>  {
>  	return 0;
> diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
> index 92ce5f4ccc26..7c08e6d1175c 100644
> --- a/kernel/events/hw_breakpoint.c
> +++ b/kernel/events/hw_breakpoint.c
> @@ -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;