perf: hw_breakpoint safety improvements
Subsystems (3)
cover 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(-)
Signature · 1 line
2.4.3
PATCH 1/3 x86/perf/hw_breakpoint: 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.
--- 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 +15 -0
@@ -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.
include/linux/kprobes.h +2 -0
@@ -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 */
kernel/kprobes.c +1 -1
@@ -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;
Signature · 1 line
2.4.3
tip-bot for Andy Lutomirski
merge bot[tip:perf/core] perf/x86/hw_breakpoints: Disallow kernel breakpoints unless kprobe-safe
perf/x86/hw_breakpoints: Disallow kernel breakpoints unless kprobe-safe
Commit-ID: e5779e8e12299f77c2421a707855d8d124171d85 Gitweb: http://git.kernel.org/tip/e5779e8e12299f77c2421a707855d8d124171d85 Author: Andy Lutomirski <[email protected]> AuthorDate: Thu, 30 Jul 2015 20:32:40 -0700 Committer: Ingo Molnar <[email protected]> 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.
--- 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 +15 -0
@@ -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.
include/linux/kprobes.h +2 -0
@@ -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 */
kernel/kprobes.c +1 -1
@@ -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;
Re: [PATCH 1/3] x86/perf/hw_breakpoint: Disallow kernel breakpoints unless kprobe-safe
In reply to Andy LutomirskiQuoted 37 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 +5
-0@@ -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.
arch/x86/kernel/hw_breakpoint.c static int arch_build_bp_info(struct perf_event *bp) · line 260
Quoted 10 lines
arch/x86/kernel/hw_breakpoint.c +10
-0+*/ +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.
include/linux/kprobes.h extern void show_registers(struct pt_regs *regs); · line 270
Quoted 12 lines
arch/x86/kernel/hw_breakpoint.c +0
-0info->type =X86_BREAKPOINT_EXECUTE ;/* * x86 inst breakpoints need to have a specific undefined len. include/linux/kprobes.h +1
-0@@ -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 1/3] x86/perf/hw_breakpoint: Disallow kernel breakpoints unless kprobe-safe
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@@ -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 thearch_check_bp_in_kernelspace()check.
Agreed, kprobes also does it in generic code.
Quoted 12 lines
Quoted 12 lines
arch/x86/kernel/hw_breakpoint.c +0
-0info->type =X86_BREAKPOINT_EXECUTE ;/* * x86 inst breakpoints need to have a specific undefined len. include/linux/kprobes.h +0
-0@@ -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);
Quoted 4 lines
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().
No, there is the "kprobe_blacklist", that function means
"whether the address is within kprobe_blacklist or not?" like within_module_core.
Thank you,
Quoted 1 line
Re: [PATCH 1/3] x86/perf/hw_breakpoint: Disallow kernel breakpoints unless kprobe-safe
2015-08-05 1:29 GMT+02:00 平松雅巳 / HIRAMATU，MASAMI
<[email protected]>:
Quoted 56 lines
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@@ -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 thearch_check_bp_in_kernelspace()check.
Quoted 1 line
Agreed, kprobes also does it in generic code.
Well, the patchset got applied anyway and the reviews ignored...
Peter Zijlstra
maintainerRe: [PATCH 1/3] x86/perf/hw_breakpoint: Disallow kernel breakpoints unless kprobe-safe
Quoted 3 lines · Frederic Weisbecker
Quoted 1 line
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.
--- 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(-)
arch/x86/kernel/hw_breakpoint.c +0 -14
@@ -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.
include/linux/kprobes.h +6 -1
@@ -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 ;
kernel/events/hw_breakpoint.c +8 -0
@@ -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 ;
kernel/kprobes.c +2 -2
@@ -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;
Peter Zijlstra
maintainerRe: [PATCH 1/3] x86/perf/hw_breakpoint: Disallow kernel breakpoints unless kprobe-safe
include/linux/kprobes.h void dump_kprobe(struct kprobe *kp); · line 396
Quoted 3 lines · Peter Zijlstra
include/linux/kprobes.h +3
-0+static inline bool kprobe_blacklisted (unsigned long addr) +{ +return false;
Ah, I think that wants to be true instead, to match the below comment.
Quoted 24 lines
include/linux/kprobes.h +2
-0+} +static inline int kprobes_built_in (void ) {return 0 ;kernel/events/hw_breakpoint.c +8
-0@@ -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 ;
PATCH 2/3 x86/perf/hw_breakpoint: Improve range breakpoint validation
In reply to Andy LutomirskiRange 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.
--- arch/x86/kernel/hw_breakpoint.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
arch/x86/kernel/hw_breakpoint.c +10 -0
@@ -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 ;
Signature · 1 line
2.4.3
tip-bot for Andy Lutomirski
merge bot[tip:perf/core] perf/x86/hw_breakpoints: Improve range breakpoint validation
perf/x86/hw_breakpoints: Improve range breakpoint validation
Commit-ID: ab513927ab449af00cc70b0269e15ee80dd537f9 Gitweb: http://git.kernel.org/tip/ab513927ab449af00cc70b0269e15ee80dd537f9 Author: Andy Lutomirski <[email protected]> AuthorDate: Thu, 30 Jul 2015 20:32:41 -0700 Committer: Ingo Molnar <[email protected]> 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.
--- arch/x86/kernel/hw_breakpoint.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
arch/x86/kernel/hw_breakpoint.c +10 -0
@@ -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 ;
PATCH 3/3 x86/perf/hw_breakpoint: Fix check for kernelspace breakpoints
In reply to Andy LutomirskiThe 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.
--- arch/x86/kernel/hw_breakpoint.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
arch/x86/kernel/hw_breakpoint.c +5 -1
@@ -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 · 1 line
2.4.3
tip-bot for Andy Lutomirski
merge bot[tip:perf/core] perf/x86/hw_breakpoints: Fix check for kernel-space breakpoints
perf/x86/hw_breakpoints: Fix check for kernel-space breakpoints
Commit-ID: 27747f8bc355a2808ca9e490ab6866acd85b4c16 Gitweb: http://git.kernel.org/tip/27747f8bc355a2808ca9e490ab6866acd85b4c16 Author: Andy Lutomirski <[email protected]> AuthorDate: Thu, 30 Jul 2015 20:32:42 -0700 Committer: Ingo Molnar <[email protected]> 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.
--- arch/x86/kernel/hw_breakpoint.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
arch/x86/kernel/hw_breakpoint.c +5 -1
@@ -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 3/3] x86/perf/hw_breakpoint: Fix check for kernelspace breakpoints
In reply to Andy LutomirskiQuoted 10 lines · Andy Lutomirski
The check looked wrong, although I think it was actually safe.
TASK_SIZEis 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]>
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 :-)
Quoted 29 lines
--- arch/x86/kernel/hw_breakpoint.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)arch/x86/kernel/hw_breakpoint.c +5
-1@@ -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/
Quoted 10 lines · 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
Quoted 4 lines
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
Peter Zijlstra
maintainerQuoted 4 lines · Andy Lutomirski
Hi, Peter-
Here are some baby steps toward eliminating nested NMIs. What do you think?
They work for me, Thanks!