ARM/ARM64: don't enter kgdb when userspace executes a kgdb break instruction.

The kgdb breakpoint hooks (kgdb_brk_fn and kgdb_compiled_brk_fn) should only be entered when a kgdb break instruction is executed from the kernel. Otherwise, if kgdb is enabled, a userspace program can cause the kernel to drop into the debugger by executing either KGDB_BREAKINST or KGDB_COMPILED_BREAK on ARM, or brk #KGDB_{DYN,COMPILED}_DGB_BRK_IMM on ARM64.

Signed-off-by Omar Sandoval <[email protected]>

---
The following program reproduces the fixed problem on ARM:
.globl _start
_start:

	udf	#65006	@ KGDB_BREAKINST

And on ARM64:
.globl _start
_start:

        brk     #0x400	@ KGDB_DYN_DGB_BRK_IMM

 arch/arm/kernel/kgdb.c                  | 4 ++++
 arch/arm64/include/asm/debug-monitors.h | 4 +++-
 arch/arm64/kernel/debug-monitors.c      | 3 ++-
 arch/arm64/kernel/kgdb.c                | 4 ++++
 4 files changed, 13 insertions(+), 2 deletions(-)

arch/arm/kernel/kgdb.c +4 -0

index 778c2f7..a74b53c 100644
@@ -160,12 +160,16 @@ static int kgdb_compiled_brk_fn(struct pt_regs *regs, unsigned int instr)
 static struct undef_hook kgdb_brkpt_hook = {
 	.instr_mask		= 0xffffffff,
 	.instr_val		= KGDB_BREAKINST,
+	.cpsr_mask		= MODE_MASK,
+	.cpsr_val		= SVC_MODE,
 	.fn			= kgdb_brk_fn
 };
 
 static struct undef_hook kgdb_compiled_brkpt_hook = {
 	.instr_mask		= 0xffffffff,
 	.instr_val		= KGDB_COMPILED_BREAK,
+	.cpsr_mask		= MODE_MASK,
+	.cpsr_val		= SVC_MODE,
 	.fn			= kgdb_compiled_brk_fn
 };
 

arch/arm64/include/asm/debug-monitors.h +3 -1

index 6e9b5b3..e1d27ce 100644
@@ -105,8 +105,10 @@ void unregister_step_hook(struct step_hook *hook);
 
 struct break_hook {
 	struct list_head node;
-	u32 esr_val;
 	u32 esr_mask;
+	u32 esr_val;
+	u64 pstate_mask;
+	u64 pstate_val;
 	int (*fn)(struct pt_regs *regs, unsigned int esr);
 };
 

arch/arm64/kernel/debug-monitors.c +2 -1

index a7fb874..594fac4 100644
@@ -303,7 +303,8 @@ static int call_break_hook(struct pt_regs *regs, unsigned int esr)
 
 	read_lock(&break_hook_lock);
 	list_for_each_entry(hook, &break_hook, node)
-		if ((esr & hook->esr_mask) == hook->esr_val)
+		if ((esr & hook->esr_mask) == hook->esr_val &&
+		    (regs->pstate & hook->pstate_mask) == hook->pstate_val)
 			fn = hook->fn;
 	read_unlock(&break_hook_lock);
 

arch/arm64/kernel/kgdb.c +4 -0

index 75c9cf1..80eb035 100644
@@ -236,12 +236,16 @@ static int kgdb_step_brk_fn(struct pt_regs *regs, unsigned int esr)
 static struct break_hook kgdb_brkpt_hook = {
 	.esr_mask	= 0xffffffff,
 	.esr_val	= DBG_ESR_VAL_BRK(KGDB_DYN_DGB_BRK_IMM),
+	.pstate_mask	= PSR_MODE_MASK,
+	.pstate_val	= PSR_MODE_EL1h,
 	.fn		= kgdb_brk_fn
 };
 
 static struct break_hook kgdb_compiled_brkpt_hook = {
 	.esr_mask	= 0xffffffff,
 	.esr_val	= DBG_ESR_VAL_BRK(KDBG_COMPILED_DBG_BRK_IMM),
+	.pstate_mask	= PSR_MODE_MASK,
+	.pstate_val	= PSR_MODE_EL1h,
 	.fn		= kgdb_compiled_brk_fn
 };
 

Signature · 1 line

2.0.3

The kgdb breakpoint hooks (kgdb_brk_fn and kgdb_compiled_brk_fn) should only be entered when a kgdb break instruction is executed from the kernel. Otherwise, if kgdb is enabled, a userspace program can cause the kernel to drop into the debugger by executing either KGDB_BREAKINST or KGDB_COMPILED_BREAK on ARM, or brk #KGDB_{DYN,COMPILED}_DGB_BRK_IMM on ARM64.

Signed-off-by Omar Sandoval <[email protected]>

---
The following program reproduces the fixed problem on ARM:
.globl _start
_start:

	udf	#65006	@ KGDB_BREAKINST

And on ARM64:
.globl _start
_start:

        brk     #0x400	@ KGDB_DYN_DGB_BRK_IMM

 arch/arm/kernel/kgdb.c                  | 4 ++++
 arch/arm64/include/asm/debug-monitors.h | 4 +++-
 arch/arm64/kernel/debug-monitors.c      | 3 ++-
 arch/arm64/kernel/kgdb.c                | 4 ++++
 4 files changed, 13 insertions(+), 2 deletions(-)

Whilst this sounds like a worrying problem, I've failed to reproduce it on arm64. Executing a brk instruction with either KGDB_DYN_DGB_BRK_IMM or KDBG_COMPILED_DBG_BRK_IMM immediates from userspace results in a SIGTRAP being delivered, assumedly because kgdb_handle_exception simply returns when kgdb isn't active.

From what I can tell, the break hooks are registered so long as kgdb is enabled

The following (totally untested) diff is simpler for arm64, but again, I'm not sure we even have a problem here.

On which systems have you managed to reproduce this and how?

Hi,

Quoted 5 lines

Whilst this sounds like a worrying problem, I've failed to reproduce it on arm64. Executing a brk instruction with either KGDB_DYN_DGB_BRK_IMM or KDBG_COMPILED_DBG_BRK_IMM immediates from userspace results in a SIGTRAP being delivered, assumedly because kgdb_handle_exception simply returns when kgdb isn't active.

From what I can tell, the break hooks are registered so long as kgdb is enabled at all - i.e., the kernel was compiled with CONFIG_KGDB=y and, for example, CONFIG_KGDB_SERIAL_CONSOLE=y and kgdboc was passed on the kernel command line. kgdb_handle_exception doesn't seem to check whether the debugger is active.

Quoted 2 lines

The following (totally untested) diff is simpler for arm64, but again, I'm not sure we even have a problem here.

This diff also fixes the problem. I don't have a strong preference for either approach, so I can revise the patch with this approach instead if you'd prefer that.

Quoted 1 line

On which systems have you managed to reproduce this and how?

I first reproduced this on a Raspberry Pi. The recommended distro, Raspbian, distributes a kernel compiled with CONFIG_KGDB=y, CONFIG_KGDB_KDB=y, and CONFIG_KDB_KEYBOARD=y, so it was sufficient to have a keyboard plugged in. However, I also reproduced it by booting with kgdboc on the command line, as CONFIG_KGDB_SERIAL_CONSOLE was also enabled. Additionally, I reproduced it and then verified that my patch fixed it on self-compiled kernels.

I'll merge the arm64 diff I proposed. Could you repost the ARM part please?

I think enabling and activating kgdb by default is a pretty crazy thing to do, but I agree that we shouldn't allow userspace to trap into it either.

Once you repost the ARM patches, we can look at getting them merged via rmk.

Cheers,

Will