diff mbox

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

Message ID 20140730112414.GJ12239@arm.com
State New
Headers show

Commit Message

Will Deacon July 30, 2014, 11:24 a.m. UTC 
Hello,

On Wed, Jul 30, 2014 at 08:12:45AM +0100, Omar Sandoval wrote:
> 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 <osandov@osandov.com>
> ---
> 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.

> diff --git a/arch/arm64/include/asm/debug-monitors.h b/arch/arm64/include/asm/debug-monitors.h
> index 6e9b5b3..e1d27ce 100644
> --- a/arch/arm64/include/asm/debug-monitors.h
> +++ b/arch/arm64/include/asm/debug-monitors.h
> @@ -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;

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?

Will

--->8

the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Comments

Omar Sandoval July 31, 2014, 5:33 a.m. UTC | #1 
Hi,

On Wed, Jul 30, 2014 at 12:24:14PM +0100, Will Deacon wrote:
> 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.

> 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.

> 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 don't currenty have access to a physical ARM64 board, so I did all the
testing for this with QEMU (loosely following the instructions here:
http://www.bennee.com/~alex/blog/2014/05/09/running-linux-in-qemus-aarch64-system-emulation-mode/).
Again, it was sufficient to build with CONFIG_KGDB=y and
CONFIG_KGDB_SERIAL_CONSOLE=y and boot with kgdboc enabled, i.e., passing
--append "console=ttyAMA0 kgdboc=ttyAMA0,115200" to QEMU. Here's a snippet from
that session.

# ./brk

Entering kdb (current=0xffffffc07df60a80, pid 71) on processor 0 Oops: (null)
due to oops @ 0x4000d8

dCPU: 0 PID: 71 Comm: brk Not tainted 3.16.0-rc7ajb #2
dtask: ffffffc07df60a80 ti: ffffffc07d0c0000 task.ti: ffffffc07d0c0000
PC is at 0x4000d8
LR is at 0x0
pc : [<00000000004000d8>] lr : [<0000000000000000>] pstate: 00000000
sp : 0000007fceefcdd0
x29: 0000000000000000 x28: 0000000000000000 
x27: 0000000000000000 x26: 0000000000000000 
x25: 0000000000000000 x24: 0000000000000000 
x23: 0000000000000000 x22: 0000000000000000 
x21: 0000000000000000 x20: 0000000000000000 
x19: 0000000000000000 x18: 0000000000000000 
x17: 0000000000000000 x16: 0000000000000000 
x15: 0000000000000000 x14: 0000000000000000 
x13: 0000000000000000 x12: 0000000000000000 
x11: 0000000000000000 x10: 0000000000000000 
x9 : 0000000000000000 x8 : 0000000000000000 
x7 : 0000000000000000 x6 : 0000000000000000 
x5 : 0000000000000000 x4 : 0000000000000000 
x3 : 0000000000000000 x2 : 0000000000000000 
x1 : 0000000000000000 x0 : 0000000000000000 

Let me know if you still have trouble reproducing this.

Thanks again!

Omar
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Will Deacon July 31, 2014, 10:46 a.m. UTC | #2 
On Thu, Jul 31, 2014 at 06:33:23AM +0100, Omar Sandoval wrote:
> Hi,

Hi Omar,

> On Wed, Jul 30, 2014 at 12:24:14PM +0100, Will Deacon wrote:
> > 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.
> 
> > 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.

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

> > 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 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Omar Sandoval Aug. 1, 2014, 3:07 a.m. UTC | #3 
Hi, Will,

On Thu, Jul 31, 2014 at 11:46:53AM +0100, Will Deacon wrote:
> I'll merge the arm64 diff I proposed. Could you repost the ARM part please?
I've just reposted it, hopefully we can get that merged in soon as well.

> 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.
Agreed, hopefully the actual footprint of this bug isn't too large, but it's
worth fixing anyways.

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

Thanks again!
Omar
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
diff mbox

Patch

diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index a7fb874b595e..fe5b94078d82 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -315,20 +315,20 @@  static int brk_handler(unsigned long addr, unsigned int esr,
 {
        siginfo_t info;
 
-       if (call_break_hook(regs, esr) == DBG_HOOK_HANDLED)
-               return 0;
+       if (user_mode(regs)) {
+               info = (siginfo_t) {
+                       .si_signo = SIGTRAP,
+                       .si_errno = 0,
+                       .si_code  = TRAP_BRKPT,
+                       .si_addr  = (void __user *)instruction_pointer(regs),
+               };
 
-       if (!user_mode(regs))
+               force_sig_info(SIGTRAP, &info, current);
+       } else if (call_break_hook(regs, esr) != DBG_HOOK_HANDLED) {
+               pr_warning("Unexpected kernel BRK exception at EL1\n");
                return -EFAULT;
+       }
 
-       info = (siginfo_t) {
-               .si_signo = SIGTRAP,
-               .si_errno = 0,
-               .si_code  = TRAP_BRKPT,
-               .si_addr  = (void __user *)instruction_pointer(regs),
-       };
-
-       force_sig_info(SIGTRAP, &info, current);
        return 0;
 }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in