diff mbox series

[02/10] arm64: debug: Ensure debug handlers check triggering exception level

Message ID 20190301132809.24653-3-will.deacon@arm.com (mailing list archive)
State New, archived
Headers show
Series Rework debug exception handling code | expand

Commit Message

Will Deacon March 1, 2019, 1:28 p.m. UTC 
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(-)

Comments

Mark Rutland March 1, 2019, 1:46 p.m. UTC | #1 
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
>
Sasha Levin March 5, 2019, 1:35 p.m. UTC | #2 
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
diff mbox series

Patch

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;
 }