* [PATCH 01/10] arm64: debug: Don't propagate UNKNOWN FAR into si_code for debug signals
2019-03-01 13:27 [PATCH 00/10] Rework debug exception handling code Will Deacon
@ 2019-03-01 13:28 ` Will Deacon
2019-03-01 13:28 ` Will Deacon
` (9 subsequent siblings)
10 siblings, 0 replies; 28+ messages in thread
From: Will Deacon @ 2019-03-01 13:28 UTC (permalink / raw)
To: linux-arm-kernel; +Cc: catalin.marinas, mark.rutland, Will Deacon, stable
FAR_EL1 is UNKNOWN for all debug exceptions other than those caused by
taking a hardware watchpoint. Unfortunately, if a debug handler returns
a non-zero value, then we will propagate the UNKNOWN FAR value to
userspace via the si_addr field of the SIGTRAP siginfo_t.
Instead, let's set si_addr to take on the PC of the faulting instruction,
which we have available in the current pt_regs.
Cc: <stable@vger.kernel.org>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
arch/arm64/mm/fault.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index efb7b2cbead5..ef46925096f0 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -824,11 +824,12 @@ void __init hook_debug_fault_code(int nr,
debug_fault_info[nr].name = name;
}
-asmlinkage int __exception do_debug_exception(unsigned long addr,
+asmlinkage int __exception do_debug_exception(unsigned long addr_if_watchpoint,
unsigned int esr,
struct pt_regs *regs)
{
const struct fault_info *inf = esr_to_debug_fault_info(esr);
+ unsigned long pc = instruction_pointer(regs);
int rv;
/*
@@ -838,14 +839,14 @@ asmlinkage int __exception do_debug_exception(unsigned long addr,
if (interrupts_enabled(regs))
trace_hardirqs_off();
- if (user_mode(regs) && !is_ttbr0_addr(instruction_pointer(regs)))
+ if (user_mode(regs) && !is_ttbr0_addr(pc))
arm64_apply_bp_hardening();
- if (!inf->fn(addr, esr, regs)) {
+ if (!inf->fn(addr_if_watchpoint, esr, regs)) {
rv = 1;
} else {
arm64_notify_die(inf->name, regs,
- inf->sig, inf->code, (void __user *)addr, esr);
+ inf->sig, inf->code, (void __user *)pc, esr);
rv = 0;
}
--
2.11.0
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH 01/10] arm64: debug: Don't propagate UNKNOWN FAR into si_code for debug signals
@ 2019-03-01 13:28 ` Will Deacon
0 siblings, 0 replies; 28+ messages in thread
From: Will Deacon @ 2019-03-01 13:28 UTC (permalink / raw)
To: linux-arm-kernel; +Cc: mark.rutland, catalin.marinas, Will Deacon, stable
FAR_EL1 is UNKNOWN for all debug exceptions other than those caused by
taking a hardware watchpoint. Unfortunately, if a debug handler returns
a non-zero value, then we will propagate the UNKNOWN FAR value to
userspace via the si_addr field of the SIGTRAP siginfo_t.
Instead, let's set si_addr to take on the PC of the faulting instruction,
which we have available in the current pt_regs.
Cc: <stable@vger.kernel.org>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
arch/arm64/mm/fault.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index efb7b2cbead5..ef46925096f0 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -824,11 +824,12 @@ void __init hook_debug_fault_code(int nr,
debug_fault_info[nr].name = name;
}
-asmlinkage int __exception do_debug_exception(unsigned long addr,
+asmlinkage int __exception do_debug_exception(unsigned long addr_if_watchpoint,
unsigned int esr,
struct pt_regs *regs)
{
const struct fault_info *inf = esr_to_debug_fault_info(esr);
+ unsigned long pc = instruction_pointer(regs);
int rv;
/*
@@ -838,14 +839,14 @@ asmlinkage int __exception do_debug_exception(unsigned long addr,
if (interrupts_enabled(regs))
trace_hardirqs_off();
- if (user_mode(regs) && !is_ttbr0_addr(instruction_pointer(regs)))
+ if (user_mode(regs) && !is_ttbr0_addr(pc))
arm64_apply_bp_hardening();
- if (!inf->fn(addr, esr, regs)) {
+ if (!inf->fn(addr_if_watchpoint, esr, regs)) {
rv = 1;
} else {
arm64_notify_die(inf->name, regs,
- inf->sig, inf->code, (void __user *)addr, esr);
+ inf->sig, inf->code, (void __user *)pc, esr);
rv = 0;
}
--
2.11.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 01/10] arm64: debug: Don't propagate UNKNOWN FAR into si_code for debug signals
2019-03-01 13:28 ` Will Deacon
@ 2019-03-01 13:45 ` Mark Rutland
-1 siblings, 0 replies; 28+ messages in thread
From: Mark Rutland @ 2019-03-01 13:45 UTC (permalink / raw)
To: Will Deacon; +Cc: linux-arm-kernel, catalin.marinas, stable
On Fri, Mar 01, 2019 at 01:28:00PM +0000, Will Deacon wrote:
> FAR_EL1 is UNKNOWN for all debug exceptions other than those caused by
> taking a hardware watchpoint. Unfortunately, if a debug handler returns
> a non-zero value, then we will propagate the UNKNOWN FAR value to
> userspace via the si_addr field of the SIGTRAP siginfo_t.
>
> Instead, let's set si_addr to take on the PC of the faulting instruction,
> which we have available in the current pt_regs.
>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Mark.
> ---
> arch/arm64/mm/fault.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index efb7b2cbead5..ef46925096f0 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -824,11 +824,12 @@ void __init hook_debug_fault_code(int nr,
> debug_fault_info[nr].name = name;
> }
>
> -asmlinkage int __exception do_debug_exception(unsigned long addr,
> +asmlinkage int __exception do_debug_exception(unsigned long addr_if_watchpoint,
> unsigned int esr,
> struct pt_regs *regs)
> {
> const struct fault_info *inf = esr_to_debug_fault_info(esr);
> + unsigned long pc = instruction_pointer(regs);
> int rv;
>
> /*
> @@ -838,14 +839,14 @@ asmlinkage int __exception do_debug_exception(unsigned long addr,
> if (interrupts_enabled(regs))
> trace_hardirqs_off();
>
> - if (user_mode(regs) && !is_ttbr0_addr(instruction_pointer(regs)))
> + if (user_mode(regs) && !is_ttbr0_addr(pc))
> arm64_apply_bp_hardening();
>
> - if (!inf->fn(addr, esr, regs)) {
> + if (!inf->fn(addr_if_watchpoint, esr, regs)) {
> rv = 1;
> } else {
> arm64_notify_die(inf->name, regs,
> - inf->sig, inf->code, (void __user *)addr, esr);
> + inf->sig, inf->code, (void __user *)pc, esr);
> rv = 0;
> }
>
> --
> 2.11.0
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 01/10] arm64: debug: Don't propagate UNKNOWN FAR into si_code for debug signals
@ 2019-03-01 13:45 ` Mark Rutland
0 siblings, 0 replies; 28+ messages in thread
From: Mark Rutland @ 2019-03-01 13:45 UTC (permalink / raw)
To: Will Deacon; +Cc: catalin.marinas, stable, linux-arm-kernel
On Fri, Mar 01, 2019 at 01:28:00PM +0000, Will Deacon wrote:
> FAR_EL1 is UNKNOWN for all debug exceptions other than those caused by
> taking a hardware watchpoint. Unfortunately, if a debug handler returns
> a non-zero value, then we will propagate the UNKNOWN FAR value to
> userspace via the si_addr field of the SIGTRAP siginfo_t.
>
> Instead, let's set si_addr to take on the PC of the faulting instruction,
> which we have available in the current pt_regs.
>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Mark.
> ---
> arch/arm64/mm/fault.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index efb7b2cbead5..ef46925096f0 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -824,11 +824,12 @@ void __init hook_debug_fault_code(int nr,
> debug_fault_info[nr].name = name;
> }
>
> -asmlinkage int __exception do_debug_exception(unsigned long addr,
> +asmlinkage int __exception do_debug_exception(unsigned long addr_if_watchpoint,
> unsigned int esr,
> struct pt_regs *regs)
> {
> const struct fault_info *inf = esr_to_debug_fault_info(esr);
> + unsigned long pc = instruction_pointer(regs);
> int rv;
>
> /*
> @@ -838,14 +839,14 @@ asmlinkage int __exception do_debug_exception(unsigned long addr,
> if (interrupts_enabled(regs))
> trace_hardirqs_off();
>
> - if (user_mode(regs) && !is_ttbr0_addr(instruction_pointer(regs)))
> + if (user_mode(regs) && !is_ttbr0_addr(pc))
> arm64_apply_bp_hardening();
>
> - if (!inf->fn(addr, esr, regs)) {
> + if (!inf->fn(addr_if_watchpoint, esr, regs)) {
> rv = 1;
> } else {
> arm64_notify_die(inf->name, regs,
> - inf->sig, inf->code, (void __user *)addr, esr);
> + inf->sig, inf->code, (void __user *)pc, esr);
> rv = 0;
> }
>
> --
> 2.11.0
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 01/10] arm64: debug: Don't propagate UNKNOWN FAR into si_code for debug signals
2019-03-01 13:28 ` Will Deacon
(?)
(?)
@ 2019-03-05 13:35 ` Sasha Levin
-1 siblings, 0 replies; 28+ messages in thread
From: Sasha Levin @ 2019-03-05 13:35 UTC (permalink / raw)
To: Sasha Levin, Will Deacon, linux-arm-kernel
Cc: mark.rutland, catalin.marinas, stable
Hi,
[This is an automated email]
This commit has been processed because it contains a -stable tag.
The stable tag indicates that it's relevant for the following trees: all
The bot has tested the following trees: v4.20.13, v4.19.26, v4.14.104, v4.9.161, v4.4.176, v3.18.136.
v4.20.13: Failed to apply! Possible dependencies:
356607f21e60 ("kasan, arm64: fix up fault handling logic")
v4.19.26: Failed to apply! Possible dependencies:
356607f21e60 ("kasan, arm64: fix up fault handling logic")
359048f91db4 ("arm64/mm: Define esr_to_debug_fault_info()")
6fa998e83ef9 ("signal/arm64: Push siginfo generation into arm64_notify_die")
dbfe3828a6f3 ("arm64/mm: Reorganize arguments for is_el1_permission_fault()")
v4.14.104: Failed to apply! Possible dependencies:
1fc5dce78ad1 ("arm64/sve: Low-level SVE architectural state manipulation functions")
2923f5ea7738 ("nds32: Exception handling")
2c9120f3a86a ("arm64: signal: Make force_signal_inject more robust")
356607f21e60 ("kasan, arm64: fix up fault handling logic")
359048f91db4 ("arm64/mm: Define esr_to_debug_fault_info()")
3eb0f5193b49 ("signal: Ensure every siginfo we send has all bits initialized")
3f7c86b2382e ("arm64: Update fault_info table with new exception types")
42dbf54e8890 ("arm64: consistently log ESR and page table")
526c3ddb6aa2 ("signal/arm64: Document conflicts with SI_USER and SIGFPE,SIGTRAP,SIGBUS")
532826f3712b ("arm64: Mirror arm for unimplemented compat syscalls")
6fa998e83ef9 ("signal/arm64: Push siginfo generation into arm64_notify_die")
92ff0674f5d8 ("arm64: mm: Rework unhandled user pagefaults to call arm64_force_sig_info")
94ef7ecbdf6f ("arm64: fpsimd: Correctly annotate exception helpers called from asm")
969e61ba87f9 ("arm64: make is_permission_fault() name clearer")
af40ff687bc9 ("arm64: signal: Ensure si_code is valid for all fault signals")
bc0ee4760364 ("arm64/sve: Core task context handling")
cc19846079a7 ("arm64: fault: Don't leak data in ESR context for user fault on kernel VA")
dbfe3828a6f3 ("arm64/mm: Reorganize arguments for is_el1_permission_fault()")
v4.9.161: Failed to apply! Possible dependencies:
0e3a9026396c ("arm64: mm: Update perf accounting to handle poison faults")
1f9b8936f36f ("arm64: Decode information from ESR upon mem faults")
32015c235603 ("arm64: exception: handle Synchronous External Abort")
356607f21e60 ("kasan, arm64: fix up fault handling logic")
359048f91db4 ("arm64/mm: Define esr_to_debug_fault_info()")
3eb0f5193b49 ("signal: Ensure every siginfo we send has all bits initialized")
532826f3712b ("arm64: Mirror arm for unimplemented compat syscalls")
67ce16ec15ce ("arm64: mm: print out correct page table entries")
6fa998e83ef9 ("signal/arm64: Push siginfo generation into arm64_notify_die")
786889636ad7 ("arm64: Handle faults caused by inadvertent user access with PAN enabled")
7edda0886bc3 ("acpi: apei: handle SEA notification type for ARMv8")
83016b204225 ("arm64: mm: print file name of faulting vma")
92ff0674f5d8 ("arm64: mm: Rework unhandled user pagefaults to call arm64_force_sig_info")
969e61ba87f9 ("arm64: make is_permission_fault() name clearer")
a8ada146f517 ("arm64: Update the synchronous external abort fault description")
b824b9306823 ("arm64: print a fault message when attempting to write RO memory")
c07ab957d9af ("arm64: Call __show_regs directly")
cc19846079a7 ("arm64: fault: Don't leak data in ESR context for user fault on kernel VA")
dbfe3828a6f3 ("arm64/mm: Reorganize arguments for is_el1_permission_fault()")
e7c600f149b8 ("arm64: hwpoison: add VM_FAULT_HWPOISON[_LARGE] handling")
v4.4.176: Failed to apply! Possible dependencies:
09a6adf53d42 ("arm64: mm: unaligned access by user-land should be received as SIGBUS")
0a8ea52c3eb1 ("arm64: Add HAVE_REGS_AND_STACK_ACCESS_API feature")
1f9b8936f36f ("arm64: Decode information from ESR upon mem faults")
2dd0e8d2d2a1 ("arm64: Kprobes with single stepping support")
32015c235603 ("arm64: exception: handle Synchronous External Abort")
356607f21e60 ("kasan, arm64: fix up fault handling logic")
359048f91db4 ("arm64/mm: Define esr_to_debug_fault_info()")
57f4959bad0a ("arm64: kernel: Add support for User Access Override")
6afedcd23cfd ("arm64: mm: Add trace_irqflags annotations to do_debug_exception()")
7dd01aef0557 ("arm64: trap userspace "dc cvau" cache operation on errata-affected core")
9dbd5bb25c56 ("arm64: Refactor sysinstr exception handling")
a8ada146f517 ("arm64: Update the synchronous external abort fault description")
bbb1681ee365 ("arm64: mm: mark fault_info table const")
d5370f754875 ("arm64: prefetch: add alternative pattern for CPUs without a prefetcher")
d59bee887231 ("arm64: Add more test functions to insn.c")
e04a28d45ff3 ("arm64: debug: re-enable irqs before sending breakpoint SIGTRAP")
e7227d0e528f ("arm64: Cleanup SCTLR flags")
v3.18.136: Failed to apply! Possible dependencies:
09a6adf53d42 ("arm64: mm: unaligned access by user-land should be received as SIGBUS")
2dd0e8d2d2a1 ("arm64: Kprobes with single stepping support")
31dde116cb08 ("arm64: Replace set_arch_dma_coherent_ops with arch_setup_dma_ops")
3505f30fb6a9 ("ARM64 / ACPI: If we chose to boot from acpi then disable FDT")
359048f91db4 ("arm64/mm: Define esr_to_debug_fault_info()")
37655163ce1a ("ARM64 / ACPI: Get RSDP and ACPI boot-time tables")
587064b610c7 ("arm64: Add framework for legacy instruction emulation")
6933de0ca0b7 ("ARM64 / ACPI: Select ACPI_REDUCED_HARDWARE_ONLY if ACPI is enabled on ARM64")
7c59a3df15df ("ARM64 / ACPI: Get PSCI flags in FADT for PSCI init")
86dca36e6ba0 ("arm64: use private ratelimit state along with show_unhandled_signals")
876945dbf649 ("arm64: Hook up IOMMU dma_ops")
9b79f52d1a70 ("arm64: Add support for hooks to handle undefined instructions")
b10d79f76085 ("ARM64 / ACPI: Introduce early_param "acpi=" to enable/disable ACPI")
b6197b93fa4b ("arm64 : Introduce support for ACPI _CCA object")
bbb1681ee365 ("arm64: mm: mark fault_info table const")
d8f4f161e31f ("ACPI: move arm64 GSI IRQ model to generic GSI IRQ layer")
de7ee503f279 ("arm64: introduce is_device_dma_coherent")
f871d2680707 ("arm64: Fix show_unhandled_signal_ratelimited usage")
fbe61ec71ac9 ("ARM64 / ACPI: Introduce ACPI_IRQ_MODEL_GIC and register device's gsi")
fccb9a81fd08 ("ARM64 / ACPI: Parse MADT for SMP initialization")
How should we proceed with this patch?
--
Thanks,
Sasha
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 02/10] arm64: debug: Ensure debug handlers check triggering exception level
2019-03-01 13:27 [PATCH 00/10] Rework debug exception handling code Will Deacon
@ 2019-03-01 13:28 ` Will Deacon
2019-03-01 13:28 ` Will Deacon
` (9 subsequent siblings)
10 siblings, 0 replies; 28+ messages in thread
From: Will Deacon @ 2019-03-01 13:28 UTC (permalink / raw)
To: linux-arm-kernel; +Cc: catalin.marinas, mark.rutland, Will Deacon, stable
Debug exception handlers may be called for exceptions generated both by
user and kernel code. In many cases, this is checked explicitly, but
in other cases things either happen to work by happy accident or they
go slightly wrong. For example, executing 'brk #4' from userspace will
enter the kprobes code and be ignored, but the instruction will be
retried forever in userspace instead of delivering a SIGTRAP.
Fix this issue in the most stable-friendly fashion by simply adding
explicit checks of the triggering exception level to all of our debug
exception handlers.
Cc: <stable@vger.kernel.org>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
arch/arm64/kernel/kgdb.c | 14 ++++++++++----
arch/arm64/kernel/probes/kprobes.c | 6 ++++++
2 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
index ce46c4cdf368..691854b77c7f 100644
--- a/arch/arm64/kernel/kgdb.c
+++ b/arch/arm64/kernel/kgdb.c
@@ -244,27 +244,33 @@ int kgdb_arch_handle_exception(int exception_vector, int signo,
static int kgdb_brk_fn(struct pt_regs *regs, unsigned int esr)
{
+ if (user_mode(regs))
+ return DBG_HOOK_ERROR;
+
kgdb_handle_exception(1, SIGTRAP, 0, regs);
- return 0;
+ return DBG_HOOK_HANDLED;
}
NOKPROBE_SYMBOL(kgdb_brk_fn)
static int kgdb_compiled_brk_fn(struct pt_regs *regs, unsigned int esr)
{
+ if (user_mode(regs))
+ return DBG_HOOK_ERROR;
+
compiled_break = 1;
kgdb_handle_exception(1, SIGTRAP, 0, regs);
- return 0;
+ return DBG_HOOK_HANDLED;
}
NOKPROBE_SYMBOL(kgdb_compiled_brk_fn);
static int kgdb_step_brk_fn(struct pt_regs *regs, unsigned int esr)
{
- if (!kgdb_single_step)
+ if (user_mode(regs) || !kgdb_single_step)
return DBG_HOOK_ERROR;
kgdb_handle_exception(1, SIGTRAP, 0, regs);
- return 0;
+ return DBG_HOOK_HANDLED;
}
NOKPROBE_SYMBOL(kgdb_step_brk_fn);
diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
index f17afb99890c..7fb6f3aa5ceb 100644
--- a/arch/arm64/kernel/probes/kprobes.c
+++ b/arch/arm64/kernel/probes/kprobes.c
@@ -450,6 +450,9 @@ kprobe_single_step_handler(struct pt_regs *regs, unsigned int esr)
struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
int retval;
+ if (user_mode(regs))
+ return DBG_HOOK_ERROR;
+
/* return error if this is not our step */
retval = kprobe_ss_hit(kcb, instruction_pointer(regs));
@@ -466,6 +469,9 @@ kprobe_single_step_handler(struct pt_regs *regs, unsigned int esr)
int __kprobes
kprobe_breakpoint_handler(struct pt_regs *regs, unsigned int esr)
{
+ if (user_mode(regs))
+ return DBG_HOOK_ERROR;
+
kprobe_handler(regs);
return DBG_HOOK_HANDLED;
}
--
2.11.0
^ permalink raw reply related [flat|nested] 28+ messages in thread* [PATCH 02/10] arm64: debug: Ensure debug handlers check triggering exception level
@ 2019-03-01 13:28 ` Will Deacon
0 siblings, 0 replies; 28+ messages in thread
From: Will Deacon @ 2019-03-01 13:28 UTC (permalink / raw)
To: linux-arm-kernel; +Cc: mark.rutland, catalin.marinas, Will Deacon, stable
Debug exception handlers may be called for exceptions generated both by
user and kernel code. In many cases, this is checked explicitly, but
in other cases things either happen to work by happy accident or they
go slightly wrong. For example, executing 'brk #4' from userspace will
enter the kprobes code and be ignored, but the instruction will be
retried forever in userspace instead of delivering a SIGTRAP.
Fix this issue in the most stable-friendly fashion by simply adding
explicit checks of the triggering exception level to all of our debug
exception handlers.
Cc: <stable@vger.kernel.org>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
arch/arm64/kernel/kgdb.c | 14 ++++++++++----
arch/arm64/kernel/probes/kprobes.c | 6 ++++++
2 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
index ce46c4cdf368..691854b77c7f 100644
--- a/arch/arm64/kernel/kgdb.c
+++ b/arch/arm64/kernel/kgdb.c
@@ -244,27 +244,33 @@ int kgdb_arch_handle_exception(int exception_vector, int signo,
static int kgdb_brk_fn(struct pt_regs *regs, unsigned int esr)
{
+ if (user_mode(regs))
+ return DBG_HOOK_ERROR;
+
kgdb_handle_exception(1, SIGTRAP, 0, regs);
- return 0;
+ return DBG_HOOK_HANDLED;
}
NOKPROBE_SYMBOL(kgdb_brk_fn)
static int kgdb_compiled_brk_fn(struct pt_regs *regs, unsigned int esr)
{
+ if (user_mode(regs))
+ return DBG_HOOK_ERROR;
+
compiled_break = 1;
kgdb_handle_exception(1, SIGTRAP, 0, regs);
- return 0;
+ return DBG_HOOK_HANDLED;
}
NOKPROBE_SYMBOL(kgdb_compiled_brk_fn);
static int kgdb_step_brk_fn(struct pt_regs *regs, unsigned int esr)
{
- if (!kgdb_single_step)
+ if (user_mode(regs) || !kgdb_single_step)
return DBG_HOOK_ERROR;
kgdb_handle_exception(1, SIGTRAP, 0, regs);
- return 0;
+ return DBG_HOOK_HANDLED;
}
NOKPROBE_SYMBOL(kgdb_step_brk_fn);
diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
index f17afb99890c..7fb6f3aa5ceb 100644
--- a/arch/arm64/kernel/probes/kprobes.c
+++ b/arch/arm64/kernel/probes/kprobes.c
@@ -450,6 +450,9 @@ kprobe_single_step_handler(struct pt_regs *regs, unsigned int esr)
struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
int retval;
+ if (user_mode(regs))
+ return DBG_HOOK_ERROR;
+
/* return error if this is not our step */
retval = kprobe_ss_hit(kcb, instruction_pointer(regs));
@@ -466,6 +469,9 @@ kprobe_single_step_handler(struct pt_regs *regs, unsigned int esr)
int __kprobes
kprobe_breakpoint_handler(struct pt_regs *regs, unsigned int esr)
{
+ if (user_mode(regs))
+ return DBG_HOOK_ERROR;
+
kprobe_handler(regs);
return DBG_HOOK_HANDLED;
}
--
2.11.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 02/10] arm64: debug: Ensure debug handlers check triggering exception level
2019-03-01 13:28 ` Will Deacon
@ 2019-03-01 13:46 ` Mark Rutland
-1 siblings, 0 replies; 28+ messages in thread
From: Mark Rutland @ 2019-03-01 13:46 UTC (permalink / raw)
To: Will Deacon; +Cc: linux-arm-kernel, catalin.marinas, stable
On Fri, Mar 01, 2019 at 01:28:01PM +0000, Will Deacon wrote:
> Debug exception handlers may be called for exceptions generated both by
> user and kernel code. In many cases, this is checked explicitly, but
> in other cases things either happen to work by happy accident or they
> go slightly wrong. For example, executing 'brk #4' from userspace will
> enter the kprobes code and be ignored, but the instruction will be
> retried forever in userspace instead of delivering a SIGTRAP.
>
> Fix this issue in the most stable-friendly fashion by simply adding
> explicit checks of the triggering exception level to all of our debug
> exception handlers.
>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
It might be worth noting in the commit message that this also makes the
functions consistentluy use the DBG_HOOK_* mnemonics, but either way:
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Mark.
> ---
> arch/arm64/kernel/kgdb.c | 14 ++++++++++----
> arch/arm64/kernel/probes/kprobes.c | 6 ++++++
> 2 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
> index ce46c4cdf368..691854b77c7f 100644
> --- a/arch/arm64/kernel/kgdb.c
> +++ b/arch/arm64/kernel/kgdb.c
> @@ -244,27 +244,33 @@ int kgdb_arch_handle_exception(int exception_vector, int signo,
>
> static int kgdb_brk_fn(struct pt_regs *regs, unsigned int esr)
> {
> + if (user_mode(regs))
> + return DBG_HOOK_ERROR;
> +
> kgdb_handle_exception(1, SIGTRAP, 0, regs);
> - return 0;
> + return DBG_HOOK_HANDLED;
> }
> NOKPROBE_SYMBOL(kgdb_brk_fn)
>
> static int kgdb_compiled_brk_fn(struct pt_regs *regs, unsigned int esr)
> {
> + if (user_mode(regs))
> + return DBG_HOOK_ERROR;
> +
> compiled_break = 1;
> kgdb_handle_exception(1, SIGTRAP, 0, regs);
>
> - return 0;
> + return DBG_HOOK_HANDLED;
> }
> NOKPROBE_SYMBOL(kgdb_compiled_brk_fn);
>
> static int kgdb_step_brk_fn(struct pt_regs *regs, unsigned int esr)
> {
> - if (!kgdb_single_step)
> + if (user_mode(regs) || !kgdb_single_step)
> return DBG_HOOK_ERROR;
>
> kgdb_handle_exception(1, SIGTRAP, 0, regs);
> - return 0;
> + return DBG_HOOK_HANDLED;
> }
> NOKPROBE_SYMBOL(kgdb_step_brk_fn);
>
> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> index f17afb99890c..7fb6f3aa5ceb 100644
> --- a/arch/arm64/kernel/probes/kprobes.c
> +++ b/arch/arm64/kernel/probes/kprobes.c
> @@ -450,6 +450,9 @@ kprobe_single_step_handler(struct pt_regs *regs, unsigned int esr)
> struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
> int retval;
>
> + if (user_mode(regs))
> + return DBG_HOOK_ERROR;
> +
> /* return error if this is not our step */
> retval = kprobe_ss_hit(kcb, instruction_pointer(regs));
>
> @@ -466,6 +469,9 @@ kprobe_single_step_handler(struct pt_regs *regs, unsigned int esr)
> int __kprobes
> kprobe_breakpoint_handler(struct pt_regs *regs, unsigned int esr)
> {
> + if (user_mode(regs))
> + return DBG_HOOK_ERROR;
> +
> kprobe_handler(regs);
> return DBG_HOOK_HANDLED;
> }
> --
> 2.11.0
>
^ permalink raw reply [flat|nested] 28+ messages in thread* Re: [PATCH 02/10] arm64: debug: Ensure debug handlers check triggering exception level
@ 2019-03-01 13:46 ` Mark Rutland
0 siblings, 0 replies; 28+ messages in thread
From: Mark Rutland @ 2019-03-01 13:46 UTC (permalink / raw)
To: Will Deacon; +Cc: catalin.marinas, stable, linux-arm-kernel
On Fri, Mar 01, 2019 at 01:28:01PM +0000, Will Deacon wrote:
> Debug exception handlers may be called for exceptions generated both by
> user and kernel code. In many cases, this is checked explicitly, but
> in other cases things either happen to work by happy accident or they
> go slightly wrong. For example, executing 'brk #4' from userspace will
> enter the kprobes code and be ignored, but the instruction will be
> retried forever in userspace instead of delivering a SIGTRAP.
>
> Fix this issue in the most stable-friendly fashion by simply adding
> explicit checks of the triggering exception level to all of our debug
> exception handlers.
>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
It might be worth noting in the commit message that this also makes the
functions consistentluy use the DBG_HOOK_* mnemonics, but either way:
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Mark.
> ---
> arch/arm64/kernel/kgdb.c | 14 ++++++++++----
> arch/arm64/kernel/probes/kprobes.c | 6 ++++++
> 2 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
> index ce46c4cdf368..691854b77c7f 100644
> --- a/arch/arm64/kernel/kgdb.c
> +++ b/arch/arm64/kernel/kgdb.c
> @@ -244,27 +244,33 @@ int kgdb_arch_handle_exception(int exception_vector, int signo,
>
> static int kgdb_brk_fn(struct pt_regs *regs, unsigned int esr)
> {
> + if (user_mode(regs))
> + return DBG_HOOK_ERROR;
> +
> kgdb_handle_exception(1, SIGTRAP, 0, regs);
> - return 0;
> + return DBG_HOOK_HANDLED;
> }
> NOKPROBE_SYMBOL(kgdb_brk_fn)
>
> static int kgdb_compiled_brk_fn(struct pt_regs *regs, unsigned int esr)
> {
> + if (user_mode(regs))
> + return DBG_HOOK_ERROR;
> +
> compiled_break = 1;
> kgdb_handle_exception(1, SIGTRAP, 0, regs);
>
> - return 0;
> + return DBG_HOOK_HANDLED;
> }
> NOKPROBE_SYMBOL(kgdb_compiled_brk_fn);
>
> static int kgdb_step_brk_fn(struct pt_regs *regs, unsigned int esr)
> {
> - if (!kgdb_single_step)
> + if (user_mode(regs) || !kgdb_single_step)
> return DBG_HOOK_ERROR;
>
> kgdb_handle_exception(1, SIGTRAP, 0, regs);
> - return 0;
> + return DBG_HOOK_HANDLED;
> }
> NOKPROBE_SYMBOL(kgdb_step_brk_fn);
>
> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> index f17afb99890c..7fb6f3aa5ceb 100644
> --- a/arch/arm64/kernel/probes/kprobes.c
> +++ b/arch/arm64/kernel/probes/kprobes.c
> @@ -450,6 +450,9 @@ kprobe_single_step_handler(struct pt_regs *regs, unsigned int esr)
> struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
> int retval;
>
> + if (user_mode(regs))
> + return DBG_HOOK_ERROR;
> +
> /* return error if this is not our step */
> retval = kprobe_ss_hit(kcb, instruction_pointer(regs));
>
> @@ -466,6 +469,9 @@ kprobe_single_step_handler(struct pt_regs *regs, unsigned int esr)
> int __kprobes
> kprobe_breakpoint_handler(struct pt_regs *regs, unsigned int esr)
> {
> + if (user_mode(regs))
> + return DBG_HOOK_ERROR;
> +
> kprobe_handler(regs);
> return DBG_HOOK_HANDLED;
> }
> --
> 2.11.0
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 02/10] arm64: debug: Ensure debug handlers check triggering exception level
2019-03-01 13:28 ` Will Deacon
(?)
(?)
@ 2019-03-05 13:35 ` Sasha Levin
-1 siblings, 0 replies; 28+ messages in thread
From: Sasha Levin @ 2019-03-05 13:35 UTC (permalink / raw)
To: Sasha Levin, Will Deacon, linux-arm-kernel
Cc: mark.rutland, catalin.marinas, stable
Hi,
[This is an automated email]
This commit has been processed because it contains a -stable tag.
The stable tag indicates that it's relevant for the following trees: all
The bot has tested the following trees: v4.20.13, v4.19.26, v4.14.104, v4.9.161, v4.4.176, v3.18.136.
v4.20.13: Build OK!
v4.19.26: Build OK!
v4.14.104: Build OK!
v4.9.161: Failed to apply! Possible dependencies:
b66c9870e974 ("arm64: kgdb_step_brk_fn: ignore other's exception")
v4.4.176: Failed to apply! Possible dependencies:
0a8ea52c3eb1 ("arm64: Add HAVE_REGS_AND_STACK_ACCESS_API feature")
2dd0e8d2d2a1 ("arm64: Kprobes with single stepping support")
b66c9870e974 ("arm64: kgdb_step_brk_fn: ignore other's exception")
d59bee887231 ("arm64: Add more test functions to insn.c")
e04a28d45ff3 ("arm64: debug: re-enable irqs before sending breakpoint SIGTRAP")
v3.18.136: Failed to apply! Possible dependencies:
2dd0e8d2d2a1 ("arm64: Kprobes with single stepping support")
31dde116cb08 ("arm64: Replace set_arch_dma_coherent_ops with arch_setup_dma_ops")
3505f30fb6a9 ("ARM64 / ACPI: If we chose to boot from acpi then disable FDT")
37655163ce1a ("ARM64 / ACPI: Get RSDP and ACPI boot-time tables")
587064b610c7 ("arm64: Add framework for legacy instruction emulation")
6933de0ca0b7 ("ARM64 / ACPI: Select ACPI_REDUCED_HARDWARE_ONLY if ACPI is enabled on ARM64")
6e0a0ea12962 ("ACPI / sleep: Introduce CONFIG_ACPI_SYSTEM_POWER_STATES_SUPPORT")
7c59a3df15df ("ARM64 / ACPI: Get PSCI flags in FADT for PSCI init")
876945dbf649 ("arm64: Hook up IOMMU dma_ops")
b10d79f76085 ("ARM64 / ACPI: Introduce early_param "acpi=" to enable/disable ACPI")
b6197b93fa4b ("arm64 : Introduce support for ACPI _CCA object")
b66c9870e974 ("arm64: kgdb_step_brk_fn: ignore other's exception")
d8f4f161e31f ("ACPI: move arm64 GSI IRQ model to generic GSI IRQ layer")
de7ee503f279 ("arm64: introduce is_device_dma_coherent")
fbe61ec71ac9 ("ARM64 / ACPI: Introduce ACPI_IRQ_MODEL_GIC and register device's gsi")
fccb9a81fd08 ("ARM64 / ACPI: Parse MADT for SMP initialization")
How should we proceed with this patch?
--
Thanks,
Sasha
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 03/10] arm64: debug: Remove unused return value from do_debug_exception()
2019-03-01 13:27 [PATCH 00/10] Rework debug exception handling code Will Deacon
2019-03-01 13:28 ` Will Deacon
2019-03-01 13:28 ` Will Deacon
@ 2019-03-01 13:28 ` Will Deacon
2019-03-01 13:48 ` Mark Rutland
2019-03-01 13:28 ` [PATCH 04/10] arm64: debug: Rename addr parameter for non-watchpoint exception hooks Will Deacon
` (7 subsequent siblings)
10 siblings, 1 reply; 28+ messages in thread
From: Will Deacon @ 2019-03-01 13:28 UTC (permalink / raw)
To: linux-arm-kernel; +Cc: mark.rutland, catalin.marinas, Will Deacon
do_debug_exception() goes out of its way to return a value that isn't
ever used, so just make the thing void.
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
arch/arm64/mm/fault.c | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index ef46925096f0..f684f92d517c 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -824,13 +824,12 @@ void __init hook_debug_fault_code(int nr,
debug_fault_info[nr].name = name;
}
-asmlinkage int __exception do_debug_exception(unsigned long addr_if_watchpoint,
- unsigned int esr,
- struct pt_regs *regs)
+asmlinkage void __exception do_debug_exception(unsigned long addr_if_watchpoint,
+ unsigned int esr,
+ struct pt_regs *regs)
{
const struct fault_info *inf = esr_to_debug_fault_info(esr);
unsigned long pc = instruction_pointer(regs);
- int rv;
/*
* Tell lockdep we disabled irqs in entry.S. Do nothing if they were
@@ -842,17 +841,12 @@ asmlinkage int __exception do_debug_exception(unsigned long addr_if_watchpoint,
if (user_mode(regs) && !is_ttbr0_addr(pc))
arm64_apply_bp_hardening();
- if (!inf->fn(addr_if_watchpoint, esr, regs)) {
- rv = 1;
- } else {
+ if (inf->fn(addr_if_watchpoint, esr, regs)) {
arm64_notify_die(inf->name, regs,
inf->sig, inf->code, (void __user *)pc, esr);
- rv = 0;
}
if (interrupts_enabled(regs))
trace_hardirqs_on();
-
- return rv;
}
NOKPROBE_SYMBOL(do_debug_exception);
--
2.11.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 03/10] arm64: debug: Remove unused return value from do_debug_exception()
2019-03-01 13:28 ` [PATCH 03/10] arm64: debug: Remove unused return value from do_debug_exception() Will Deacon
@ 2019-03-01 13:48 ` Mark Rutland
0 siblings, 0 replies; 28+ messages in thread
From: Mark Rutland @ 2019-03-01 13:48 UTC (permalink / raw)
To: Will Deacon; +Cc: catalin.marinas, linux-arm-kernel
On Fri, Mar 01, 2019 at 01:28:02PM +0000, Will Deacon wrote:
> do_debug_exception() goes out of its way to return a value that isn't
> ever used, so just make the thing void.
>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Mark.
> ---
> arch/arm64/mm/fault.c | 14 ++++----------
> 1 file changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index ef46925096f0..f684f92d517c 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -824,13 +824,12 @@ void __init hook_debug_fault_code(int nr,
> debug_fault_info[nr].name = name;
> }
>
> -asmlinkage int __exception do_debug_exception(unsigned long addr_if_watchpoint,
> - unsigned int esr,
> - struct pt_regs *regs)
> +asmlinkage void __exception do_debug_exception(unsigned long addr_if_watchpoint,
> + unsigned int esr,
> + struct pt_regs *regs)
> {
> const struct fault_info *inf = esr_to_debug_fault_info(esr);
> unsigned long pc = instruction_pointer(regs);
> - int rv;
>
> /*
> * Tell lockdep we disabled irqs in entry.S. Do nothing if they were
> @@ -842,17 +841,12 @@ asmlinkage int __exception do_debug_exception(unsigned long addr_if_watchpoint,
> if (user_mode(regs) && !is_ttbr0_addr(pc))
> arm64_apply_bp_hardening();
>
> - if (!inf->fn(addr_if_watchpoint, esr, regs)) {
> - rv = 1;
> - } else {
> + if (inf->fn(addr_if_watchpoint, esr, regs)) {
> arm64_notify_die(inf->name, regs,
> inf->sig, inf->code, (void __user *)pc, esr);
> - rv = 0;
> }
>
> if (interrupts_enabled(regs))
> trace_hardirqs_on();
> -
> - return rv;
> }
> NOKPROBE_SYMBOL(do_debug_exception);
> --
> 2.11.0
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 04/10] arm64: debug: Rename addr parameter for non-watchpoint exception hooks
2019-03-01 13:27 [PATCH 00/10] Rework debug exception handling code Will Deacon
` (2 preceding siblings ...)
2019-03-01 13:28 ` [PATCH 03/10] arm64: debug: Remove unused return value from do_debug_exception() Will Deacon
@ 2019-03-01 13:28 ` Will Deacon
2019-03-01 13:49 ` Mark Rutland
2019-03-01 13:28 ` [PATCH 05/10] arm64: debug: Remove meaningless comment Will Deacon
` (6 subsequent siblings)
10 siblings, 1 reply; 28+ messages in thread
From: Will Deacon @ 2019-03-01 13:28 UTC (permalink / raw)
To: linux-arm-kernel; +Cc: mark.rutland, catalin.marinas, Will Deacon
Since the 'addr' parameter contains an UNKNOWN value for non-watchpoint
debug exceptions, rename it to 'unused' for those hooks so we don't get
tempted to use it in the future.
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
arch/arm64/kernel/debug-monitors.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index d7bb6aefae0a..c4c263d0cf0f 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -222,7 +222,7 @@ static void send_user_sigtrap(int si_code)
"User debug trap");
}
-static int single_step_handler(unsigned long addr, unsigned int esr,
+static int single_step_handler(unsigned long unused, unsigned int esr,
struct pt_regs *regs)
{
bool handler_found = false;
@@ -302,7 +302,7 @@ static int call_break_hook(struct pt_regs *regs, unsigned int esr)
}
NOKPROBE_SYMBOL(call_break_hook);
-static int brk_handler(unsigned long addr, unsigned int esr,
+static int brk_handler(unsigned long unused, unsigned int esr,
struct pt_regs *regs)
{
bool handler_found = false;
--
2.11.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 04/10] arm64: debug: Rename addr parameter for non-watchpoint exception hooks
2019-03-01 13:28 ` [PATCH 04/10] arm64: debug: Rename addr parameter for non-watchpoint exception hooks Will Deacon
@ 2019-03-01 13:49 ` Mark Rutland
0 siblings, 0 replies; 28+ messages in thread
From: Mark Rutland @ 2019-03-01 13:49 UTC (permalink / raw)
To: Will Deacon; +Cc: catalin.marinas, linux-arm-kernel
On Fri, Mar 01, 2019 at 01:28:03PM +0000, Will Deacon wrote:
> Since the 'addr' parameter contains an UNKNOWN value for non-watchpoint
> debug exceptions, rename it to 'unused' for those hooks so we don't get
> tempted to use it in the future.
>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Mark.
> ---
> arch/arm64/kernel/debug-monitors.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> index d7bb6aefae0a..c4c263d0cf0f 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -222,7 +222,7 @@ static void send_user_sigtrap(int si_code)
> "User debug trap");
> }
>
> -static int single_step_handler(unsigned long addr, unsigned int esr,
> +static int single_step_handler(unsigned long unused, unsigned int esr,
> struct pt_regs *regs)
> {
> bool handler_found = false;
> @@ -302,7 +302,7 @@ static int call_break_hook(struct pt_regs *regs, unsigned int esr)
> }
> NOKPROBE_SYMBOL(call_break_hook);
>
> -static int brk_handler(unsigned long addr, unsigned int esr,
> +static int brk_handler(unsigned long unused, unsigned int esr,
> struct pt_regs *regs)
> {
> bool handler_found = false;
> --
> 2.11.0
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 05/10] arm64: debug: Remove meaningless comment
2019-03-01 13:27 [PATCH 00/10] Rework debug exception handling code Will Deacon
` (3 preceding siblings ...)
2019-03-01 13:28 ` [PATCH 04/10] arm64: debug: Rename addr parameter for non-watchpoint exception hooks Will Deacon
@ 2019-03-01 13:28 ` Will Deacon
2019-03-01 14:08 ` Mark Rutland
2019-03-01 13:28 ` [PATCH 06/10] arm64: debug: Separate debug hooks based on target exception level Will Deacon
` (5 subsequent siblings)
10 siblings, 1 reply; 28+ messages in thread
From: Will Deacon @ 2019-03-01 13:28 UTC (permalink / raw)
To: linux-arm-kernel; +Cc: mark.rutland, catalin.marinas, Will Deacon
The comment next to the definition of our 'break_hook' list head is
at best wrong but mainly just meaningless. Rip it out.
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
arch/arm64/kernel/debug-monitors.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index c4c263d0cf0f..744229d10ca8 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -264,11 +264,6 @@ static int single_step_handler(unsigned long unused, unsigned int esr,
}
NOKPROBE_SYMBOL(single_step_handler);
-/*
- * Breakpoint handler is re-entrant as another breakpoint can
- * hit within breakpoint handler, especically in kprobes.
- * Use reader/writer locks instead of plain spinlock.
- */
static LIST_HEAD(break_hook);
static DEFINE_SPINLOCK(break_hook_lock);
--
2.11.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 05/10] arm64: debug: Remove meaningless comment
2019-03-01 13:28 ` [PATCH 05/10] arm64: debug: Remove meaningless comment Will Deacon
@ 2019-03-01 14:08 ` Mark Rutland
0 siblings, 0 replies; 28+ messages in thread
From: Mark Rutland @ 2019-03-01 14:08 UTC (permalink / raw)
To: Will Deacon; +Cc: catalin.marinas, linux-arm-kernel
On Fri, Mar 01, 2019 at 01:28:04PM +0000, Will Deacon wrote:
> The comment next to the definition of our 'break_hook' list head is
> at best wrong but mainly just meaningless. Rip it out.
>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Mark.
> ---
> arch/arm64/kernel/debug-monitors.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> index c4c263d0cf0f..744229d10ca8 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -264,11 +264,6 @@ static int single_step_handler(unsigned long unused, unsigned int esr,
> }
> NOKPROBE_SYMBOL(single_step_handler);
>
> -/*
> - * Breakpoint handler is re-entrant as another breakpoint can
> - * hit within breakpoint handler, especically in kprobes.
> - * Use reader/writer locks instead of plain spinlock.
> - */
> static LIST_HEAD(break_hook);
> static DEFINE_SPINLOCK(break_hook_lock);
>
> --
> 2.11.0
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 06/10] arm64: debug: Separate debug hooks based on target exception level
2019-03-01 13:27 [PATCH 00/10] Rework debug exception handling code Will Deacon
` (4 preceding siblings ...)
2019-03-01 13:28 ` [PATCH 05/10] arm64: debug: Remove meaningless comment Will Deacon
@ 2019-03-01 13:28 ` Will Deacon
2019-03-01 14:07 ` Mark Rutland
2019-03-01 13:28 ` [PATCH 07/10] arm64: kprobes: Avoid calling kprobes debug handlers explicitly Will Deacon
` (4 subsequent siblings)
10 siblings, 1 reply; 28+ messages in thread
From: Will Deacon @ 2019-03-01 13:28 UTC (permalink / raw)
To: linux-arm-kernel; +Cc: mark.rutland, catalin.marinas, Will Deacon
Mixing kernel and user debug hooks together is highly error-prone as it
relies on all of the hooks to figure out whether the exception came from
kernel or user, and then to act accordingly.
Make our debug hook code a little more robust by maintaining separate
hook lists for user and kernel, with separate registration functions
to force callers to be explicit about the exception levels that they
care about.
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
arch/arm64/include/asm/debug-monitors.h | 17 ++++---
arch/arm64/kernel/debug-monitors.c | 82 +++++++++++++++++++++++----------
arch/arm64/kernel/kgdb.c | 22 ++++-----
arch/arm64/kernel/probes/uprobes.c | 7 ++-
arch/arm64/kernel/traps.c | 15 ++----
5 files changed, 86 insertions(+), 57 deletions(-)
diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
index a44cf5225429..2b136f0f6a35 100644
--- a/arch/arm64/include/asm/debug-monitors.h
+++ b/arch/arm64/include/asm/debug-monitors.h
@@ -94,18 +94,23 @@ struct step_hook {
int (*fn)(struct pt_regs *regs, unsigned int esr);
};
-void register_step_hook(struct step_hook *hook);
-void unregister_step_hook(struct step_hook *hook);
+void register_user_step_hook(struct step_hook *hook);
+void unregister_user_step_hook(struct step_hook *hook);
+
+void register_kernel_step_hook(struct step_hook *hook);
+void unregister_kernel_step_hook(struct step_hook *hook);
struct break_hook {
struct list_head node;
- u32 esr_val;
- u32 esr_mask;
int (*fn)(struct pt_regs *regs, unsigned int esr);
+ u16 imm;
};
-void register_break_hook(struct break_hook *hook);
-void unregister_break_hook(struct break_hook *hook);
+void register_user_break_hook(struct break_hook *hook);
+void unregister_user_break_hook(struct break_hook *hook);
+
+void register_kernel_break_hook(struct break_hook *hook);
+void unregister_kernel_break_hook(struct break_hook *hook);
u8 debug_monitors_arch(void);
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index 744229d10ca8..51946ecaf8e5 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -163,25 +163,46 @@ static void clear_regs_spsr_ss(struct pt_regs *regs)
}
NOKPROBE_SYMBOL(clear_regs_spsr_ss);
-/* EL1 Single Step Handler hooks */
-static LIST_HEAD(step_hook);
-static DEFINE_SPINLOCK(step_hook_lock);
+static DEFINE_SPINLOCK(debug_hook_lock);
+static LIST_HEAD(user_step_hook);
+static LIST_HEAD(kernel_step_hook);
-void register_step_hook(struct step_hook *hook)
+static void register_debug_hook(struct list_head *node, struct list_head *list)
{
- spin_lock(&step_hook_lock);
- list_add_rcu(&hook->node, &step_hook);
- spin_unlock(&step_hook_lock);
+ spin_lock(&debug_hook_lock);
+ list_add_rcu(node, list);
+ spin_unlock(&debug_hook_lock);
+
}
-void unregister_step_hook(struct step_hook *hook)
+static void unregister_debug_hook(struct list_head *node)
{
- spin_lock(&step_hook_lock);
- list_del_rcu(&hook->node);
- spin_unlock(&step_hook_lock);
+ spin_lock(&debug_hook_lock);
+ list_del_rcu(node);
+ spin_unlock(&debug_hook_lock);
synchronize_rcu();
}
+void register_user_step_hook(struct step_hook *hook)
+{
+ register_debug_hook(&hook->node, &user_step_hook);
+}
+
+void unregister_user_step_hook(struct step_hook *hook)
+{
+ unregister_debug_hook(&hook->node);
+}
+
+void register_kernel_step_hook(struct step_hook *hook)
+{
+ register_debug_hook(&hook->node, &kernel_step_hook);
+}
+
+void unregister_kernel_step_hook(struct step_hook *hook)
+{
+ unregister_debug_hook(&hook->node);
+}
+
/*
* Call registered single step handlers
* There is no Syndrome info to check for determining the handler.
@@ -191,11 +212,14 @@ void unregister_step_hook(struct step_hook *hook)
static int call_step_hook(struct pt_regs *regs, unsigned int esr)
{
struct step_hook *hook;
+ struct list_head *list;
int retval = DBG_HOOK_ERROR;
+ list = user_mode(regs) ? &user_step_hook : &kernel_step_hook;
+
rcu_read_lock();
- list_for_each_entry_rcu(hook, &step_hook, node) {
+ list_for_each_entry_rcu(hook, list, node) {
retval = hook->fn(regs, esr);
if (retval == DBG_HOOK_HANDLED)
break;
@@ -264,32 +288,40 @@ static int single_step_handler(unsigned long unused, unsigned int esr,
}
NOKPROBE_SYMBOL(single_step_handler);
-static LIST_HEAD(break_hook);
-static DEFINE_SPINLOCK(break_hook_lock);
+static LIST_HEAD(user_break_hook);
+static LIST_HEAD(kernel_break_hook);
-void register_break_hook(struct break_hook *hook)
+void register_user_break_hook(struct break_hook *hook)
{
- spin_lock(&break_hook_lock);
- list_add_rcu(&hook->node, &break_hook);
- spin_unlock(&break_hook_lock);
+ register_debug_hook(&hook->node, &user_break_hook);
}
-void unregister_break_hook(struct break_hook *hook)
+void unregister_user_break_hook(struct break_hook *hook)
{
- spin_lock(&break_hook_lock);
- list_del_rcu(&hook->node);
- spin_unlock(&break_hook_lock);
- synchronize_rcu();
+ unregister_debug_hook(&hook->node);
+}
+
+void register_kernel_break_hook(struct break_hook *hook)
+{
+ register_debug_hook(&hook->node, &kernel_break_hook);
+}
+
+void unregister_kernel_break_hook(struct break_hook *hook)
+{
+ unregister_debug_hook(&hook->node);
}
static int call_break_hook(struct pt_regs *regs, unsigned int esr)
{
struct break_hook *hook;
+ struct list_head *list;
int (*fn)(struct pt_regs *regs, unsigned int esr) = NULL;
+ list = user_mode(regs) ? &user_break_hook : &kernel_break_hook;
+
rcu_read_lock();
- list_for_each_entry_rcu(hook, &break_hook, node)
- if ((esr & hook->esr_mask) == hook->esr_val)
+ list_for_each_entry_rcu(hook, list, node)
+ if ((esr & BRK64_ESR_MASK) == hook->imm)
fn = hook->fn;
rcu_read_unlock();
diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
index 691854b77c7f..4c01f299aeb2 100644
--- a/arch/arm64/kernel/kgdb.c
+++ b/arch/arm64/kernel/kgdb.c
@@ -275,15 +275,13 @@ static int kgdb_step_brk_fn(struct pt_regs *regs, unsigned int esr)
NOKPROBE_SYMBOL(kgdb_step_brk_fn);
static struct break_hook kgdb_brkpt_hook = {
- .esr_mask = 0xffffffff,
- .esr_val = (u32)ESR_ELx_VAL_BRK64(KGDB_DYN_DBG_BRK_IMM),
- .fn = kgdb_brk_fn
+ .fn = kgdb_brk_fn,
+ .imm = KGDB_DYN_DBG_BRK_IMM,
};
static struct break_hook kgdb_compiled_brkpt_hook = {
- .esr_mask = 0xffffffff,
- .esr_val = (u32)ESR_ELx_VAL_BRK64(KGDB_COMPILED_DBG_BRK_IMM),
- .fn = kgdb_compiled_brk_fn
+ .fn = kgdb_compiled_brk_fn,
+ .imm = KGDB_COMPILED_DBG_BRK_IMM,
};
static struct step_hook kgdb_step_hook = {
@@ -332,9 +330,9 @@ int kgdb_arch_init(void)
if (ret != 0)
return ret;
- register_break_hook(&kgdb_brkpt_hook);
- register_break_hook(&kgdb_compiled_brkpt_hook);
- register_step_hook(&kgdb_step_hook);
+ register_kernel_break_hook(&kgdb_brkpt_hook);
+ register_kernel_break_hook(&kgdb_compiled_brkpt_hook);
+ register_kernel_step_hook(&kgdb_step_hook);
return 0;
}
@@ -345,9 +343,9 @@ int kgdb_arch_init(void)
*/
void kgdb_arch_exit(void)
{
- unregister_break_hook(&kgdb_brkpt_hook);
- unregister_break_hook(&kgdb_compiled_brkpt_hook);
- unregister_step_hook(&kgdb_step_hook);
+ unregister_kernel_break_hook(&kgdb_brkpt_hook);
+ unregister_kernel_break_hook(&kgdb_compiled_brkpt_hook);
+ unregister_kernel_step_hook(&kgdb_step_hook);
unregister_die_notifier(&kgdb_notifier);
}
diff --git a/arch/arm64/kernel/probes/uprobes.c b/arch/arm64/kernel/probes/uprobes.c
index 636ca0119c0e..7d6ea88796a6 100644
--- a/arch/arm64/kernel/probes/uprobes.c
+++ b/arch/arm64/kernel/probes/uprobes.c
@@ -195,8 +195,7 @@ static int uprobe_single_step_handler(struct pt_regs *regs,
/* uprobe breakpoint handler hook */
static struct break_hook uprobes_break_hook = {
- .esr_mask = BRK64_ESR_MASK,
- .esr_val = BRK64_ESR_UPROBES,
+ .imm = BRK64_ESR_UPROBES,
.fn = uprobe_breakpoint_handler,
};
@@ -207,8 +206,8 @@ static struct step_hook uprobes_step_hook = {
static int __init arch_init_uprobes(void)
{
- register_break_hook(&uprobes_break_hook);
- register_step_hook(&uprobes_step_hook);
+ register_user_break_hook(&uprobes_break_hook);
+ register_user_step_hook(&uprobes_step_hook);
return 0;
}
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 4e2fb877f8d5..4be52bdcede6 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -965,9 +965,8 @@ static int bug_handler(struct pt_regs *regs, unsigned int esr)
}
static struct break_hook bug_break_hook = {
- .esr_val = 0xf2000000 | BUG_BRK_IMM,
- .esr_mask = 0xffffffff,
.fn = bug_handler,
+ .imm = BUG_BRK_IMM,
};
#ifdef CONFIG_KASAN_SW_TAGS
@@ -1012,13 +1011,9 @@ static int kasan_handler(struct pt_regs *regs, unsigned int esr)
return DBG_HOOK_HANDLED;
}
-#define KASAN_ESR_VAL (0xf2000000 | KASAN_BRK_IMM)
-#define KASAN_ESR_MASK 0xffffff00
-
static struct break_hook kasan_break_hook = {
- .esr_val = KASAN_ESR_VAL,
- .esr_mask = KASAN_ESR_MASK,
.fn = kasan_handler,
+ .imm = KASAN_BRK_IMM,
};
#endif
@@ -1030,7 +1025,7 @@ int __init early_brk64(unsigned long addr, unsigned int esr,
struct pt_regs *regs)
{
#ifdef CONFIG_KASAN_SW_TAGS
- if ((esr & KASAN_ESR_MASK) == KASAN_ESR_VAL)
+ if ((esr & BRK64_ESR_MASK) == KASAN_BRK_IMM)
return kasan_handler(regs, esr) != DBG_HOOK_HANDLED;
#endif
return bug_handler(regs, esr) != DBG_HOOK_HANDLED;
@@ -1039,8 +1034,8 @@ int __init early_brk64(unsigned long addr, unsigned int esr,
/* This registration must happen early, before debug_traps_init(). */
void __init trap_init(void)
{
- register_break_hook(&bug_break_hook);
+ register_kernel_break_hook(&bug_break_hook);
#ifdef CONFIG_KASAN_SW_TAGS
- register_break_hook(&kasan_break_hook);
+ register_kernel_break_hook(&kasan_break_hook);
#endif
}
--
2.11.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 06/10] arm64: debug: Separate debug hooks based on target exception level
2019-03-01 13:28 ` [PATCH 06/10] arm64: debug: Separate debug hooks based on target exception level Will Deacon
@ 2019-03-01 14:07 ` Mark Rutland
0 siblings, 0 replies; 28+ messages in thread
From: Mark Rutland @ 2019-03-01 14:07 UTC (permalink / raw)
To: Will Deacon; +Cc: catalin.marinas, linux-arm-kernel
On Fri, Mar 01, 2019 at 01:28:05PM +0000, Will Deacon wrote:
> Mixing kernel and user debug hooks together is highly error-prone as it
> relies on all of the hooks to figure out whether the exception came from
> kernel or user, and then to act accordingly.
>
> Make our debug hook code a little more robust by maintaining separate
> hook lists for user and kernel, with separate registration functions
> to force callers to be explicit about the exception levels that they
> care about.
>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
[...]
> struct break_hook {
> struct list_head node;
> - u32 esr_val;
> - u32 esr_mask;
> int (*fn)(struct pt_regs *regs, unsigned int esr);
> + u16 imm;
> };
It's really nice to see the break_hook data reduced down to the BRK
immediate! Unfortunately, I don't thnk that's sufficient for KASAN;
please see below.
[...]
> static int call_break_hook(struct pt_regs *regs, unsigned int esr)
> {
> struct break_hook *hook;
> + struct list_head *list;
> int (*fn)(struct pt_regs *regs, unsigned int esr) = NULL;
>
> + list = user_mode(regs) ? &user_break_hook : &kernel_break_hook;
> +
> rcu_read_lock();
> - list_for_each_entry_rcu(hook, &break_hook, node)
> - if ((esr & hook->esr_mask) == hook->esr_val)
> + list_for_each_entry_rcu(hook, list, node)
> + if ((esr & BRK64_ESR_MASK) == hook->imm)
> fn = hook->fn;
Could we please fix up the existing coding style bug by placing braces
around the loop body?
Coding style says we should, since it's more than a single simple
statement, and it would better match what we do in call_step_hook().
[...]
> -#define KASAN_ESR_VAL (0xf2000000 | KASAN_BRK_IMM)
> -#define KASAN_ESR_MASK 0xffffff00
> -
> static struct break_hook kasan_break_hook = {
> - .esr_val = KASAN_ESR_VAL,
> - .esr_mask = KASAN_ESR_MASK,
> .fn = kasan_handler,
> + .imm = KASAN_BRK_IMM,
> };
> #endif
>
> @@ -1030,7 +1025,7 @@ int __init early_brk64(unsigned long addr, unsigned int esr,
> struct pt_regs *regs)
> {
> #ifdef CONFIG_KASAN_SW_TAGS
> - if ((esr & KASAN_ESR_MASK) == KASAN_ESR_VAL)
> + if ((esr & BRK64_ESR_MASK) == KASAN_BRK_IMM)
According to <asm/brk-imm.h>, KASAN BRKs can use the whole 0x900-0x9ff
range of immediates, and these changes mean that we'll only match 0x900.
We probably need an imm_mask field on struct break_hook to cater for
that.
Thanks,
Mark.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 07/10] arm64: kprobes: Avoid calling kprobes debug handlers explicitly
2019-03-01 13:27 [PATCH 00/10] Rework debug exception handling code Will Deacon
` (5 preceding siblings ...)
2019-03-01 13:28 ` [PATCH 06/10] arm64: debug: Separate debug hooks based on target exception level Will Deacon
@ 2019-03-01 13:28 ` Will Deacon
2019-03-01 14:12 ` Mark Rutland
2019-03-01 13:28 ` [PATCH 08/10] arm64: debug: Remove redundant user_mode(regs) checks from debug handlers Will Deacon
` (3 subsequent siblings)
10 siblings, 1 reply; 28+ messages in thread
From: Will Deacon @ 2019-03-01 13:28 UTC (permalink / raw)
To: linux-arm-kernel; +Cc: mark.rutland, catalin.marinas, Will Deacon
Kprobes bypasses our debug hook registration code so that it doesn't
get tangled up with recursive debug exceptions from things like lockdep:
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-February/324385.html
However, since then, (a) the hook list has become RCU protected and (b)
the kprobes hooks were found not to filter out exceptions from userspace
correctly. On top of that, the step handler is invoked directly from
single_step_handler(), which *does* use the debug hook list, so it's
clearly not the end of the world.
For now, have kprobes use the debug hook registration API like everybody
else. We can revisit this in the future if this is found to limit
coverage significantly.
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
arch/arm64/include/asm/kprobes.h | 2 --
arch/arm64/kernel/debug-monitors.c | 10 ----------
arch/arm64/kernel/probes/kprobes.c | 16 ++++++++++++++--
3 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/arch/arm64/include/asm/kprobes.h b/arch/arm64/include/asm/kprobes.h
index d5a44cf859e9..21721fbf44e7 100644
--- a/arch/arm64/include/asm/kprobes.h
+++ b/arch/arm64/include/asm/kprobes.h
@@ -54,8 +54,6 @@ void arch_remove_kprobe(struct kprobe *);
int kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr);
int kprobe_exceptions_notify(struct notifier_block *self,
unsigned long val, void *data);
-int kprobe_breakpoint_handler(struct pt_regs *regs, unsigned int esr);
-int kprobe_single_step_handler(struct pt_regs *regs, unsigned int esr);
void kretprobe_trampoline(void);
void __kprobes *trampoline_probe_handler(struct pt_regs *regs);
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index 51946ecaf8e5..d9616c34a270 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -258,10 +258,6 @@ static int single_step_handler(unsigned long unused, unsigned int esr,
if (!reinstall_suspended_bps(regs))
return 0;
-#ifdef CONFIG_KPROBES
- if (kprobe_single_step_handler(regs, esr) == DBG_HOOK_HANDLED)
- handler_found = true;
-#endif
if (!handler_found && call_step_hook(regs, esr) == DBG_HOOK_HANDLED)
handler_found = true;
@@ -334,12 +330,6 @@ static int brk_handler(unsigned long unused, unsigned int esr,
{
bool handler_found = false;
-#ifdef CONFIG_KPROBES
- if ((esr & BRK64_ESR_MASK) == BRK64_ESR_KPROBES) {
- if (kprobe_breakpoint_handler(regs, esr) == DBG_HOOK_HANDLED)
- handler_found = true;
- }
-#endif
if (!handler_found && call_break_hook(regs, esr) == DBG_HOOK_HANDLED)
handler_found = true;
diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
index 7fb6f3aa5ceb..3066ffd70cf5 100644
--- a/arch/arm64/kernel/probes/kprobes.c
+++ b/arch/arm64/kernel/probes/kprobes.c
@@ -444,7 +444,7 @@ kprobe_ss_hit(struct kprobe_ctlblk *kcb, unsigned long addr)
return DBG_HOOK_ERROR;
}
-int __kprobes
+static int __kprobes
kprobe_single_step_handler(struct pt_regs *regs, unsigned int esr)
{
struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
@@ -466,7 +466,11 @@ kprobe_single_step_handler(struct pt_regs *regs, unsigned int esr)
return retval;
}
-int __kprobes
+static struct step_hook kprobes_step_hook = {
+ .fn = kprobe_single_step_handler,
+};
+
+static int __kprobes
kprobe_breakpoint_handler(struct pt_regs *regs, unsigned int esr)
{
if (user_mode(regs))
@@ -476,6 +480,11 @@ kprobe_breakpoint_handler(struct pt_regs *regs, unsigned int esr)
return DBG_HOOK_HANDLED;
}
+static struct break_hook kprobes_break_hook = {
+ .imm = BRK64_ESR_KPROBES,
+ .fn = kprobe_breakpoint_handler,
+};
+
bool arch_within_kprobe_blacklist(unsigned long addr)
{
if ((addr >= (unsigned long)__kprobes_text_start &&
@@ -593,5 +602,8 @@ int __kprobes arch_trampoline_kprobe(struct kprobe *p)
int __init arch_init_kprobes(void)
{
+ register_kernel_break_hook(&kprobes_break_hook);
+ register_kernel_step_hook(&kprobes_step_hook);
+
return 0;
}
--
2.11.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 07/10] arm64: kprobes: Avoid calling kprobes debug handlers explicitly
2019-03-01 13:28 ` [PATCH 07/10] arm64: kprobes: Avoid calling kprobes debug handlers explicitly Will Deacon
@ 2019-03-01 14:12 ` Mark Rutland
0 siblings, 0 replies; 28+ messages in thread
From: Mark Rutland @ 2019-03-01 14:12 UTC (permalink / raw)
To: Will Deacon; +Cc: catalin.marinas, linux-arm-kernel
On Fri, Mar 01, 2019 at 01:28:06PM +0000, Will Deacon wrote:
> Kprobes bypasses our debug hook registration code so that it doesn't
> get tangled up with recursive debug exceptions from things like lockdep:
>
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-February/324385.html
>
> However, since then, (a) the hook list has become RCU protected and (b)
> the kprobes hooks were found not to filter out exceptions from userspace
> correctly. On top of that, the step handler is invoked directly from
> single_step_handler(), which *does* use the debug hook list, so it's
> clearly not the end of the world.
>
> For now, have kprobes use the debug hook registration API like everybody
> else. We can revisit this in the future if this is found to limit
> coverage significantly.
>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Mark.
> ---
> arch/arm64/include/asm/kprobes.h | 2 --
> arch/arm64/kernel/debug-monitors.c | 10 ----------
> arch/arm64/kernel/probes/kprobes.c | 16 ++++++++++++++--
> 3 files changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/arch/arm64/include/asm/kprobes.h b/arch/arm64/include/asm/kprobes.h
> index d5a44cf859e9..21721fbf44e7 100644
> --- a/arch/arm64/include/asm/kprobes.h
> +++ b/arch/arm64/include/asm/kprobes.h
> @@ -54,8 +54,6 @@ void arch_remove_kprobe(struct kprobe *);
> int kprobe_fault_handler(struct pt_regs *regs, unsigned int fsr);
> int kprobe_exceptions_notify(struct notifier_block *self,
> unsigned long val, void *data);
> -int kprobe_breakpoint_handler(struct pt_regs *regs, unsigned int esr);
> -int kprobe_single_step_handler(struct pt_regs *regs, unsigned int esr);
> void kretprobe_trampoline(void);
> void __kprobes *trampoline_probe_handler(struct pt_regs *regs);
>
> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> index 51946ecaf8e5..d9616c34a270 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -258,10 +258,6 @@ static int single_step_handler(unsigned long unused, unsigned int esr,
> if (!reinstall_suspended_bps(regs))
> return 0;
>
> -#ifdef CONFIG_KPROBES
> - if (kprobe_single_step_handler(regs, esr) == DBG_HOOK_HANDLED)
> - handler_found = true;
> -#endif
> if (!handler_found && call_step_hook(regs, esr) == DBG_HOOK_HANDLED)
> handler_found = true;
>
> @@ -334,12 +330,6 @@ static int brk_handler(unsigned long unused, unsigned int esr,
> {
> bool handler_found = false;
>
> -#ifdef CONFIG_KPROBES
> - if ((esr & BRK64_ESR_MASK) == BRK64_ESR_KPROBES) {
> - if (kprobe_breakpoint_handler(regs, esr) == DBG_HOOK_HANDLED)
> - handler_found = true;
> - }
> -#endif
> if (!handler_found && call_break_hook(regs, esr) == DBG_HOOK_HANDLED)
> handler_found = true;
>
> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> index 7fb6f3aa5ceb..3066ffd70cf5 100644
> --- a/arch/arm64/kernel/probes/kprobes.c
> +++ b/arch/arm64/kernel/probes/kprobes.c
> @@ -444,7 +444,7 @@ kprobe_ss_hit(struct kprobe_ctlblk *kcb, unsigned long addr)
> return DBG_HOOK_ERROR;
> }
>
> -int __kprobes
> +static int __kprobes
> kprobe_single_step_handler(struct pt_regs *regs, unsigned int esr)
> {
> struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
> @@ -466,7 +466,11 @@ kprobe_single_step_handler(struct pt_regs *regs, unsigned int esr)
> return retval;
> }
>
> -int __kprobes
> +static struct step_hook kprobes_step_hook = {
> + .fn = kprobe_single_step_handler,
> +};
> +
> +static int __kprobes
> kprobe_breakpoint_handler(struct pt_regs *regs, unsigned int esr)
> {
> if (user_mode(regs))
> @@ -476,6 +480,11 @@ kprobe_breakpoint_handler(struct pt_regs *regs, unsigned int esr)
> return DBG_HOOK_HANDLED;
> }
>
> +static struct break_hook kprobes_break_hook = {
> + .imm = BRK64_ESR_KPROBES,
> + .fn = kprobe_breakpoint_handler,
> +};
> +
> bool arch_within_kprobe_blacklist(unsigned long addr)
> {
> if ((addr >= (unsigned long)__kprobes_text_start &&
> @@ -593,5 +602,8 @@ int __kprobes arch_trampoline_kprobe(struct kprobe *p)
>
> int __init arch_init_kprobes(void)
> {
> + register_kernel_break_hook(&kprobes_break_hook);
> + register_kernel_step_hook(&kprobes_step_hook);
> +
> return 0;
> }
> --
> 2.11.0
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 08/10] arm64: debug: Remove redundant user_mode(regs) checks from debug handlers
2019-03-01 13:27 [PATCH 00/10] Rework debug exception handling code Will Deacon
` (6 preceding siblings ...)
2019-03-01 13:28 ` [PATCH 07/10] arm64: kprobes: Avoid calling kprobes debug handlers explicitly Will Deacon
@ 2019-03-01 13:28 ` Will Deacon
2019-03-01 14:13 ` Mark Rutland
2019-03-01 13:28 ` [PATCH 09/10] arm64: probes: Move magic BRK values into brk-imm.h Will Deacon
` (2 subsequent siblings)
10 siblings, 1 reply; 28+ messages in thread
From: Will Deacon @ 2019-03-01 13:28 UTC (permalink / raw)
To: linux-arm-kernel; +Cc: mark.rutland, catalin.marinas, Will Deacon
Now that the debug hook dispatching code takes the triggering exception
level into account, there's no need for the hooks themselves to poke
around with user_mode(regs).
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
arch/arm64/kernel/kgdb.c | 8 +-------
arch/arm64/kernel/probes/kprobes.c | 6 ------
arch/arm64/kernel/probes/uprobes.c | 12 ++++--------
arch/arm64/kernel/traps.c | 6 ------
4 files changed, 5 insertions(+), 27 deletions(-)
diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
index 4c01f299aeb2..30853d5b7859 100644
--- a/arch/arm64/kernel/kgdb.c
+++ b/arch/arm64/kernel/kgdb.c
@@ -244,9 +244,6 @@ int kgdb_arch_handle_exception(int exception_vector, int signo,
static int kgdb_brk_fn(struct pt_regs *regs, unsigned int esr)
{
- if (user_mode(regs))
- return DBG_HOOK_ERROR;
-
kgdb_handle_exception(1, SIGTRAP, 0, regs);
return DBG_HOOK_HANDLED;
}
@@ -254,9 +251,6 @@ NOKPROBE_SYMBOL(kgdb_brk_fn)
static int kgdb_compiled_brk_fn(struct pt_regs *regs, unsigned int esr)
{
- if (user_mode(regs))
- return DBG_HOOK_ERROR;
-
compiled_break = 1;
kgdb_handle_exception(1, SIGTRAP, 0, regs);
@@ -266,7 +260,7 @@ NOKPROBE_SYMBOL(kgdb_compiled_brk_fn);
static int kgdb_step_brk_fn(struct pt_regs *regs, unsigned int esr)
{
- if (user_mode(regs) || !kgdb_single_step)
+ if (!kgdb_single_step)
return DBG_HOOK_ERROR;
kgdb_handle_exception(1, SIGTRAP, 0, regs);
diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
index 3066ffd70cf5..30502a3c8cf0 100644
--- a/arch/arm64/kernel/probes/kprobes.c
+++ b/arch/arm64/kernel/probes/kprobes.c
@@ -450,9 +450,6 @@ kprobe_single_step_handler(struct pt_regs *regs, unsigned int esr)
struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
int retval;
- if (user_mode(regs))
- return DBG_HOOK_ERROR;
-
/* return error if this is not our step */
retval = kprobe_ss_hit(kcb, instruction_pointer(regs));
@@ -473,9 +470,6 @@ static struct step_hook kprobes_step_hook = {
static int __kprobes
kprobe_breakpoint_handler(struct pt_regs *regs, unsigned int esr)
{
- if (user_mode(regs))
- return DBG_HOOK_ERROR;
-
kprobe_handler(regs);
return DBG_HOOK_HANDLED;
}
diff --git a/arch/arm64/kernel/probes/uprobes.c b/arch/arm64/kernel/probes/uprobes.c
index 7d6ea88796a6..f37ab9567676 100644
--- a/arch/arm64/kernel/probes/uprobes.c
+++ b/arch/arm64/kernel/probes/uprobes.c
@@ -171,7 +171,7 @@ int arch_uprobe_exception_notify(struct notifier_block *self,
static int uprobe_breakpoint_handler(struct pt_regs *regs,
unsigned int esr)
{
- if (user_mode(regs) && uprobe_pre_sstep_notifier(regs))
+ if (uprobe_pre_sstep_notifier(regs))
return DBG_HOOK_HANDLED;
return DBG_HOOK_ERROR;
@@ -182,13 +182,9 @@ static int uprobe_single_step_handler(struct pt_regs *regs,
{
struct uprobe_task *utask = current->utask;
- if (user_mode(regs)) {
- WARN_ON(utask &&
- (instruction_pointer(regs) != utask->xol_vaddr + 4));
-
- if (uprobe_post_sstep_notifier(regs))
- return DBG_HOOK_HANDLED;
- }
+ WARN_ON(utask && (instruction_pointer(regs) != utask->xol_vaddr + 4));
+ if (uprobe_post_sstep_notifier(regs))
+ return DBG_HOOK_HANDLED;
return DBG_HOOK_ERROR;
}
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 4be52bdcede6..3bcd56dd94fa 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -943,9 +943,6 @@ int is_valid_bugaddr(unsigned long addr)
static int bug_handler(struct pt_regs *regs, unsigned int esr)
{
- if (user_mode(regs))
- return DBG_HOOK_ERROR;
-
switch (report_bug(regs->pc, regs)) {
case BUG_TRAP_TYPE_BUG:
die("Oops - BUG", regs, 0);
@@ -984,9 +981,6 @@ static int kasan_handler(struct pt_regs *regs, unsigned int esr)
u64 addr = regs->regs[0];
u64 pc = regs->pc;
- if (user_mode(regs))
- return DBG_HOOK_ERROR;
-
kasan_report(addr, size, write, pc);
/*
--
2.11.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 08/10] arm64: debug: Remove redundant user_mode(regs) checks from debug handlers
2019-03-01 13:28 ` [PATCH 08/10] arm64: debug: Remove redundant user_mode(regs) checks from debug handlers Will Deacon
@ 2019-03-01 14:13 ` Mark Rutland
0 siblings, 0 replies; 28+ messages in thread
From: Mark Rutland @ 2019-03-01 14:13 UTC (permalink / raw)
To: Will Deacon; +Cc: catalin.marinas, linux-arm-kernel
On Fri, Mar 01, 2019 at 01:28:07PM +0000, Will Deacon wrote:
> Now that the debug hook dispatching code takes the triggering exception
> level into account, there's no need for the hooks themselves to poke
> around with user_mode(regs).
>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Mark.
> ---
> arch/arm64/kernel/kgdb.c | 8 +-------
> arch/arm64/kernel/probes/kprobes.c | 6 ------
> arch/arm64/kernel/probes/uprobes.c | 12 ++++--------
> arch/arm64/kernel/traps.c | 6 ------
> 4 files changed, 5 insertions(+), 27 deletions(-)
>
> diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
> index 4c01f299aeb2..30853d5b7859 100644
> --- a/arch/arm64/kernel/kgdb.c
> +++ b/arch/arm64/kernel/kgdb.c
> @@ -244,9 +244,6 @@ int kgdb_arch_handle_exception(int exception_vector, int signo,
>
> static int kgdb_brk_fn(struct pt_regs *regs, unsigned int esr)
> {
> - if (user_mode(regs))
> - return DBG_HOOK_ERROR;
> -
> kgdb_handle_exception(1, SIGTRAP, 0, regs);
> return DBG_HOOK_HANDLED;
> }
> @@ -254,9 +251,6 @@ NOKPROBE_SYMBOL(kgdb_brk_fn)
>
> static int kgdb_compiled_brk_fn(struct pt_regs *regs, unsigned int esr)
> {
> - if (user_mode(regs))
> - return DBG_HOOK_ERROR;
> -
> compiled_break = 1;
> kgdb_handle_exception(1, SIGTRAP, 0, regs);
>
> @@ -266,7 +260,7 @@ NOKPROBE_SYMBOL(kgdb_compiled_brk_fn);
>
> static int kgdb_step_brk_fn(struct pt_regs *regs, unsigned int esr)
> {
> - if (user_mode(regs) || !kgdb_single_step)
> + if (!kgdb_single_step)
> return DBG_HOOK_ERROR;
>
> kgdb_handle_exception(1, SIGTRAP, 0, regs);
> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> index 3066ffd70cf5..30502a3c8cf0 100644
> --- a/arch/arm64/kernel/probes/kprobes.c
> +++ b/arch/arm64/kernel/probes/kprobes.c
> @@ -450,9 +450,6 @@ kprobe_single_step_handler(struct pt_regs *regs, unsigned int esr)
> struct kprobe_ctlblk *kcb = get_kprobe_ctlblk();
> int retval;
>
> - if (user_mode(regs))
> - return DBG_HOOK_ERROR;
> -
> /* return error if this is not our step */
> retval = kprobe_ss_hit(kcb, instruction_pointer(regs));
>
> @@ -473,9 +470,6 @@ static struct step_hook kprobes_step_hook = {
> static int __kprobes
> kprobe_breakpoint_handler(struct pt_regs *regs, unsigned int esr)
> {
> - if (user_mode(regs))
> - return DBG_HOOK_ERROR;
> -
> kprobe_handler(regs);
> return DBG_HOOK_HANDLED;
> }
> diff --git a/arch/arm64/kernel/probes/uprobes.c b/arch/arm64/kernel/probes/uprobes.c
> index 7d6ea88796a6..f37ab9567676 100644
> --- a/arch/arm64/kernel/probes/uprobes.c
> +++ b/arch/arm64/kernel/probes/uprobes.c
> @@ -171,7 +171,7 @@ int arch_uprobe_exception_notify(struct notifier_block *self,
> static int uprobe_breakpoint_handler(struct pt_regs *regs,
> unsigned int esr)
> {
> - if (user_mode(regs) && uprobe_pre_sstep_notifier(regs))
> + if (uprobe_pre_sstep_notifier(regs))
> return DBG_HOOK_HANDLED;
>
> return DBG_HOOK_ERROR;
> @@ -182,13 +182,9 @@ static int uprobe_single_step_handler(struct pt_regs *regs,
> {
> struct uprobe_task *utask = current->utask;
>
> - if (user_mode(regs)) {
> - WARN_ON(utask &&
> - (instruction_pointer(regs) != utask->xol_vaddr + 4));
> -
> - if (uprobe_post_sstep_notifier(regs))
> - return DBG_HOOK_HANDLED;
> - }
> + WARN_ON(utask && (instruction_pointer(regs) != utask->xol_vaddr + 4));
> + if (uprobe_post_sstep_notifier(regs))
> + return DBG_HOOK_HANDLED;
>
> return DBG_HOOK_ERROR;
> }
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index 4be52bdcede6..3bcd56dd94fa 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -943,9 +943,6 @@ int is_valid_bugaddr(unsigned long addr)
>
> static int bug_handler(struct pt_regs *regs, unsigned int esr)
> {
> - if (user_mode(regs))
> - return DBG_HOOK_ERROR;
> -
> switch (report_bug(regs->pc, regs)) {
> case BUG_TRAP_TYPE_BUG:
> die("Oops - BUG", regs, 0);
> @@ -984,9 +981,6 @@ static int kasan_handler(struct pt_regs *regs, unsigned int esr)
> u64 addr = regs->regs[0];
> u64 pc = regs->pc;
>
> - if (user_mode(regs))
> - return DBG_HOOK_ERROR;
> -
> kasan_report(addr, size, write, pc);
>
> /*
> --
> 2.11.0
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 09/10] arm64: probes: Move magic BRK values into brk-imm.h
2019-03-01 13:27 [PATCH 00/10] Rework debug exception handling code Will Deacon
` (7 preceding siblings ...)
2019-03-01 13:28 ` [PATCH 08/10] arm64: debug: Remove redundant user_mode(regs) checks from debug handlers Will Deacon
@ 2019-03-01 13:28 ` Will Deacon
2019-03-01 14:16 ` Mark Rutland
2019-03-01 13:28 ` [PATCH 10/10] arm64: debug: Clean up brk_handler() Will Deacon
2019-03-01 16:24 ` [PATCH 00/10] Rework debug exception handling code Catalin Marinas
10 siblings, 1 reply; 28+ messages in thread
From: Will Deacon @ 2019-03-01 13:28 UTC (permalink / raw)
To: linux-arm-kernel; +Cc: mark.rutland, catalin.marinas, Will Deacon
kprobes and uprobes reserve some BRK immediates for installing their
probes. Define these along with the other reservations in brk-imm.h
and rename the ESR definitions to be consistent with the others that we
already have.
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
arch/arm64/include/asm/brk-imm.h | 4 ++++
arch/arm64/include/asm/debug-monitors.h | 7 ++-----
arch/arm64/include/asm/esr.h | 4 +---
arch/arm64/kernel/debug-monitors.c | 2 +-
arch/arm64/kernel/probes/kprobes.c | 2 +-
arch/arm64/kernel/probes/uprobes.c | 2 +-
arch/arm64/kernel/traps.c | 2 +-
7 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/arch/arm64/include/asm/brk-imm.h b/arch/arm64/include/asm/brk-imm.h
index 2945fe6cd863..645ea26cca81 100644
--- a/arch/arm64/include/asm/brk-imm.h
+++ b/arch/arm64/include/asm/brk-imm.h
@@ -11,6 +11,8 @@
/*
* #imm16 values used for BRK instruction generation
+ * 0x004: for installing kprobes
+ * 0x005: for installing uprobes
* Allowed values for kgdb are 0x400 - 0x7ff
* 0x100: for triggering a fault on purpose (reserved)
* 0x400: for dynamic BRK instruction
@@ -18,6 +20,8 @@
* 0x800: kernel-mode BUG() and WARN() traps
* 0x9xx: tag-based KASAN trap (allowed values 0x900 - 0x9ff)
*/
+#define KPROBES_BRK_IMM 0x004
+#define UPROBES_BRK_IMM 0x005
#define FAULT_BRK_IMM 0x100
#define KGDB_DYN_DBG_BRK_IMM 0x400
#define KGDB_COMPILED_DBG_BRK_IMM 0x401
diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
index 2b136f0f6a35..5a731757a3c4 100644
--- a/arch/arm64/include/asm/debug-monitors.h
+++ b/arch/arm64/include/asm/debug-monitors.h
@@ -65,12 +65,9 @@
#define CACHE_FLUSH_IS_SAFE 1
/* kprobes BRK opcodes with ESR encoding */
-#define BRK64_ESR_MASK 0xFFFF
-#define BRK64_ESR_KPROBES 0x0004
-#define BRK64_OPCODE_KPROBES (AARCH64_BREAK_MON | (BRK64_ESR_KPROBES << 5))
+#define BRK64_OPCODE_KPROBES (AARCH64_BREAK_MON | (KPROBES_BRK_IMM << 5))
/* uprobes BRK opcodes with ESR encoding */
-#define BRK64_ESR_UPROBES 0x0005
-#define BRK64_OPCODE_UPROBES (AARCH64_BREAK_MON | (BRK64_ESR_UPROBES << 5))
+#define BRK64_OPCODE_UPROBES (AARCH64_BREAK_MON | (UPROBES_BRK_IMM << 5))
/* AArch32 */
#define DBG_ESR_EVT_BKPT 0x4
diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
index 52233f00d53d..3541720189c9 100644
--- a/arch/arm64/include/asm/esr.h
+++ b/arch/arm64/include/asm/esr.h
@@ -156,9 +156,7 @@
ESR_ELx_WFx_ISS_WFI)
/* BRK instruction trap from AArch64 state */
-#define ESR_ELx_VAL_BRK64(imm) \
- ((ESR_ELx_EC_BRK64 << ESR_ELx_EC_SHIFT) | ESR_ELx_IL | \
- ((imm) & 0xffff))
+#define ESR_ELx_BRK64_ISS_COMMENT_MASK 0xffff
/* ISS field definitions for System instruction traps */
#define ESR_ELx_SYS64_ISS_RES0_SHIFT 22
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index d9616c34a270..55d46ed6ccc2 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -317,7 +317,7 @@ static int call_break_hook(struct pt_regs *regs, unsigned int esr)
rcu_read_lock();
list_for_each_entry_rcu(hook, list, node)
- if ((esr & BRK64_ESR_MASK) == hook->imm)
+ if ((esr & ESR_ELx_BRK64_ISS_COMMENT_MASK) == hook->imm)
fn = hook->fn;
rcu_read_unlock();
diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
index 30502a3c8cf0..42a30f23b85f 100644
--- a/arch/arm64/kernel/probes/kprobes.c
+++ b/arch/arm64/kernel/probes/kprobes.c
@@ -475,7 +475,7 @@ kprobe_breakpoint_handler(struct pt_regs *regs, unsigned int esr)
}
static struct break_hook kprobes_break_hook = {
- .imm = BRK64_ESR_KPROBES,
+ .imm = KPROBES_BRK_IMM,
.fn = kprobe_breakpoint_handler,
};
diff --git a/arch/arm64/kernel/probes/uprobes.c b/arch/arm64/kernel/probes/uprobes.c
index f37ab9567676..605945eac1f8 100644
--- a/arch/arm64/kernel/probes/uprobes.c
+++ b/arch/arm64/kernel/probes/uprobes.c
@@ -191,7 +191,7 @@ static int uprobe_single_step_handler(struct pt_regs *regs,
/* uprobe breakpoint handler hook */
static struct break_hook uprobes_break_hook = {
- .imm = BRK64_ESR_UPROBES,
+ .imm = UPROBES_BRK_IMM,
.fn = uprobe_breakpoint_handler,
};
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 3bcd56dd94fa..387c5e18cc46 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -1019,7 +1019,7 @@ int __init early_brk64(unsigned long addr, unsigned int esr,
struct pt_regs *regs)
{
#ifdef CONFIG_KASAN_SW_TAGS
- if ((esr & BRK64_ESR_MASK) == KASAN_BRK_IMM)
+ if ((esr & ESR_ELx_BRK64_ISS_COMMENT_MASK) == KASAN_BRK_IMM)
return kasan_handler(regs, esr) != DBG_HOOK_HANDLED;
#endif
return bug_handler(regs, esr) != DBG_HOOK_HANDLED;
--
2.11.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 09/10] arm64: probes: Move magic BRK values into brk-imm.h
2019-03-01 13:28 ` [PATCH 09/10] arm64: probes: Move magic BRK values into brk-imm.h Will Deacon
@ 2019-03-01 14:16 ` Mark Rutland
0 siblings, 0 replies; 28+ messages in thread
From: Mark Rutland @ 2019-03-01 14:16 UTC (permalink / raw)
To: Will Deacon; +Cc: catalin.marinas, linux-arm-kernel
On Fri, Mar 01, 2019 at 01:28:08PM +0000, Will Deacon wrote:
> kprobes and uprobes reserve some BRK immediates for installing their
> probes. Define these along with the other reservations in brk-imm.h
> and rename the ESR definitions to be consistent with the others that we
> already have.
>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
Modulo the latent bug with KASAN immediates I mentioned in a prior
patch:
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Mark.
> ---
> arch/arm64/include/asm/brk-imm.h | 4 ++++
> arch/arm64/include/asm/debug-monitors.h | 7 ++-----
> arch/arm64/include/asm/esr.h | 4 +---
> arch/arm64/kernel/debug-monitors.c | 2 +-
> arch/arm64/kernel/probes/kprobes.c | 2 +-
> arch/arm64/kernel/probes/uprobes.c | 2 +-
> arch/arm64/kernel/traps.c | 2 +-
> 7 files changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/arch/arm64/include/asm/brk-imm.h b/arch/arm64/include/asm/brk-imm.h
> index 2945fe6cd863..645ea26cca81 100644
> --- a/arch/arm64/include/asm/brk-imm.h
> +++ b/arch/arm64/include/asm/brk-imm.h
> @@ -11,6 +11,8 @@
>
> /*
> * #imm16 values used for BRK instruction generation
> + * 0x004: for installing kprobes
> + * 0x005: for installing uprobes
> * Allowed values for kgdb are 0x400 - 0x7ff
> * 0x100: for triggering a fault on purpose (reserved)
> * 0x400: for dynamic BRK instruction
> @@ -18,6 +20,8 @@
> * 0x800: kernel-mode BUG() and WARN() traps
> * 0x9xx: tag-based KASAN trap (allowed values 0x900 - 0x9ff)
> */
> +#define KPROBES_BRK_IMM 0x004
> +#define UPROBES_BRK_IMM 0x005
> #define FAULT_BRK_IMM 0x100
> #define KGDB_DYN_DBG_BRK_IMM 0x400
> #define KGDB_COMPILED_DBG_BRK_IMM 0x401
> diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
> index 2b136f0f6a35..5a731757a3c4 100644
> --- a/arch/arm64/include/asm/debug-monitors.h
> +++ b/arch/arm64/include/asm/debug-monitors.h
> @@ -65,12 +65,9 @@
> #define CACHE_FLUSH_IS_SAFE 1
>
> /* kprobes BRK opcodes with ESR encoding */
> -#define BRK64_ESR_MASK 0xFFFF
> -#define BRK64_ESR_KPROBES 0x0004
> -#define BRK64_OPCODE_KPROBES (AARCH64_BREAK_MON | (BRK64_ESR_KPROBES << 5))
> +#define BRK64_OPCODE_KPROBES (AARCH64_BREAK_MON | (KPROBES_BRK_IMM << 5))
> /* uprobes BRK opcodes with ESR encoding */
> -#define BRK64_ESR_UPROBES 0x0005
> -#define BRK64_OPCODE_UPROBES (AARCH64_BREAK_MON | (BRK64_ESR_UPROBES << 5))
> +#define BRK64_OPCODE_UPROBES (AARCH64_BREAK_MON | (UPROBES_BRK_IMM << 5))
>
> /* AArch32 */
> #define DBG_ESR_EVT_BKPT 0x4
> diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
> index 52233f00d53d..3541720189c9 100644
> --- a/arch/arm64/include/asm/esr.h
> +++ b/arch/arm64/include/asm/esr.h
> @@ -156,9 +156,7 @@
> ESR_ELx_WFx_ISS_WFI)
>
> /* BRK instruction trap from AArch64 state */
> -#define ESR_ELx_VAL_BRK64(imm) \
> - ((ESR_ELx_EC_BRK64 << ESR_ELx_EC_SHIFT) | ESR_ELx_IL | \
> - ((imm) & 0xffff))
> +#define ESR_ELx_BRK64_ISS_COMMENT_MASK 0xffff
>
> /* ISS field definitions for System instruction traps */
> #define ESR_ELx_SYS64_ISS_RES0_SHIFT 22
> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> index d9616c34a270..55d46ed6ccc2 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -317,7 +317,7 @@ static int call_break_hook(struct pt_regs *regs, unsigned int esr)
>
> rcu_read_lock();
> list_for_each_entry_rcu(hook, list, node)
> - if ((esr & BRK64_ESR_MASK) == hook->imm)
> + if ((esr & ESR_ELx_BRK64_ISS_COMMENT_MASK) == hook->imm)
> fn = hook->fn;
> rcu_read_unlock();
>
> diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
> index 30502a3c8cf0..42a30f23b85f 100644
> --- a/arch/arm64/kernel/probes/kprobes.c
> +++ b/arch/arm64/kernel/probes/kprobes.c
> @@ -475,7 +475,7 @@ kprobe_breakpoint_handler(struct pt_regs *regs, unsigned int esr)
> }
>
> static struct break_hook kprobes_break_hook = {
> - .imm = BRK64_ESR_KPROBES,
> + .imm = KPROBES_BRK_IMM,
> .fn = kprobe_breakpoint_handler,
> };
>
> diff --git a/arch/arm64/kernel/probes/uprobes.c b/arch/arm64/kernel/probes/uprobes.c
> index f37ab9567676..605945eac1f8 100644
> --- a/arch/arm64/kernel/probes/uprobes.c
> +++ b/arch/arm64/kernel/probes/uprobes.c
> @@ -191,7 +191,7 @@ static int uprobe_single_step_handler(struct pt_regs *regs,
>
> /* uprobe breakpoint handler hook */
> static struct break_hook uprobes_break_hook = {
> - .imm = BRK64_ESR_UPROBES,
> + .imm = UPROBES_BRK_IMM,
> .fn = uprobe_breakpoint_handler,
> };
>
> diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> index 3bcd56dd94fa..387c5e18cc46 100644
> --- a/arch/arm64/kernel/traps.c
> +++ b/arch/arm64/kernel/traps.c
> @@ -1019,7 +1019,7 @@ int __init early_brk64(unsigned long addr, unsigned int esr,
> struct pt_regs *regs)
> {
> #ifdef CONFIG_KASAN_SW_TAGS
> - if ((esr & BRK64_ESR_MASK) == KASAN_BRK_IMM)
> + if ((esr & ESR_ELx_BRK64_ISS_COMMENT_MASK) == KASAN_BRK_IMM)
> return kasan_handler(regs, esr) != DBG_HOOK_HANDLED;
> #endif
> return bug_handler(regs, esr) != DBG_HOOK_HANDLED;
> --
> 2.11.0
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 10/10] arm64: debug: Clean up brk_handler()
2019-03-01 13:27 [PATCH 00/10] Rework debug exception handling code Will Deacon
` (8 preceding siblings ...)
2019-03-01 13:28 ` [PATCH 09/10] arm64: probes: Move magic BRK values into brk-imm.h Will Deacon
@ 2019-03-01 13:28 ` Will Deacon
2019-03-01 14:17 ` Mark Rutland
2019-03-01 16:24 ` [PATCH 00/10] Rework debug exception handling code Catalin Marinas
10 siblings, 1 reply; 28+ messages in thread
From: Will Deacon @ 2019-03-01 13:28 UTC (permalink / raw)
To: linux-arm-kernel; +Cc: mark.rutland, catalin.marinas, Will Deacon
brk_handler() now looks pretty strange and can be refactored to drop its
funny 'handler_found' local variable altogether.
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
arch/arm64/kernel/debug-monitors.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index 55d46ed6ccc2..480d2fa0ac78 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -328,14 +328,12 @@ NOKPROBE_SYMBOL(call_break_hook);
static int brk_handler(unsigned long unused, unsigned int esr,
struct pt_regs *regs)
{
- bool handler_found = false;
-
- if (!handler_found && call_break_hook(regs, esr) == DBG_HOOK_HANDLED)
- handler_found = true;
+ if (call_break_hook(regs, esr) == DBG_HOOK_HANDLED)
+ return 0;
- if (!handler_found && user_mode(regs)) {
+ if (user_mode(regs)) {
send_user_sigtrap(TRAP_BRKPT);
- } else if (!handler_found) {
+ } else {
pr_warn("Unexpected kernel BRK exception at EL1\n");
return -EFAULT;
}
--
2.11.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 28+ messages in thread* Re: [PATCH 10/10] arm64: debug: Clean up brk_handler()
2019-03-01 13:28 ` [PATCH 10/10] arm64: debug: Clean up brk_handler() Will Deacon
@ 2019-03-01 14:17 ` Mark Rutland
0 siblings, 0 replies; 28+ messages in thread
From: Mark Rutland @ 2019-03-01 14:17 UTC (permalink / raw)
To: Will Deacon; +Cc: catalin.marinas, linux-arm-kernel
On Fri, Mar 01, 2019 at 01:28:09PM +0000, Will Deacon wrote:
> brk_handler() now looks pretty strange and can be refactored to drop its
> funny 'handler_found' local variable altogether.
>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
Mark.
> ---
> arch/arm64/kernel/debug-monitors.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> index 55d46ed6ccc2..480d2fa0ac78 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -328,14 +328,12 @@ NOKPROBE_SYMBOL(call_break_hook);
> static int brk_handler(unsigned long unused, unsigned int esr,
> struct pt_regs *regs)
> {
> - bool handler_found = false;
> -
> - if (!handler_found && call_break_hook(regs, esr) == DBG_HOOK_HANDLED)
> - handler_found = true;
> + if (call_break_hook(regs, esr) == DBG_HOOK_HANDLED)
> + return 0;
>
> - if (!handler_found && user_mode(regs)) {
> + if (user_mode(regs)) {
> send_user_sigtrap(TRAP_BRKPT);
> - } else if (!handler_found) {
> + } else {
> pr_warn("Unexpected kernel BRK exception at EL1\n");
> return -EFAULT;
> }
> --
> 2.11.0
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 00/10] Rework debug exception handling code
2019-03-01 13:27 [PATCH 00/10] Rework debug exception handling code Will Deacon
` (9 preceding siblings ...)
2019-03-01 13:28 ` [PATCH 10/10] arm64: debug: Clean up brk_handler() Will Deacon
@ 2019-03-01 16:24 ` Catalin Marinas
10 siblings, 0 replies; 28+ messages in thread
From: Catalin Marinas @ 2019-03-01 16:24 UTC (permalink / raw)
To: Will Deacon; +Cc: mark.rutland, linux-arm-kernel
On Fri, Mar 01, 2019 at 01:27:59PM +0000, Will Deacon wrote:
> Will Deacon (10):
> arm64: debug: Don't propagate UNKNOWN FAR into si_code for debug
> signals
> arm64: debug: Ensure debug handlers check triggering exception level
I queued the first two patches in this series for 5.1. Thanks.
--
Catalin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 28+ messages in thread