* [PATCH] ARM/ARM64: don't enter kgdb when userspace executes a kgdb break instruction.
@ 2014-07-30 7:12 Omar Sandoval
2014-07-30 7:20 ` Omar Sandoval
2014-07-30 11:24 ` Will Deacon
0 siblings, 2 replies; 6+ messages in thread
From: Omar Sandoval @ 2014-07-30 7:12 UTC (permalink / raw)
To: linux-arm-kernel
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(-)
diff --git a/arch/arm/kernel/kgdb.c b/arch/arm/kernel/kgdb.c
index 778c2f7..a74b53c 100644
--- a/arch/arm/kernel/kgdb.c
+++ b/arch/arm/kernel/kgdb.c
@@ -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
};
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;
int (*fn)(struct pt_regs *regs, unsigned int esr);
};
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index a7fb874..594fac4 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -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);
diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
index 75c9cf1..80eb035 100644
--- a/arch/arm64/kernel/kgdb.c
+++ b/arch/arm64/kernel/kgdb.c
@@ -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
};
--
2.0.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] ARM/ARM64: don't enter kgdb when userspace executes a kgdb break instruction.
2014-07-30 7:12 [PATCH] ARM/ARM64: don't enter kgdb when userspace executes a kgdb break instruction Omar Sandoval
@ 2014-07-30 7:20 ` Omar Sandoval
2014-07-30 11:24 ` Will Deacon
1 sibling, 0 replies; 6+ messages in thread
From: Omar Sandoval @ 2014-07-30 7:20 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Jul 30, 2014 at 12:12:45AM -0700, 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(-)
>
> diff --git a/arch/arm/kernel/kgdb.c b/arch/arm/kernel/kgdb.c
> index 778c2f7..a74b53c 100644
> --- a/arch/arm/kernel/kgdb.c
> +++ b/arch/arm/kernel/kgdb.c
> @@ -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
> };
>
> 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;
> int (*fn)(struct pt_regs *regs, unsigned int esr);
> };
>
> diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
> index a7fb874..594fac4 100644
> --- a/arch/arm64/kernel/debug-monitors.c
> +++ b/arch/arm64/kernel/debug-monitors.c
> @@ -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);
>
> diff --git a/arch/arm64/kernel/kgdb.c b/arch/arm64/kernel/kgdb.c
> index 75c9cf1..80eb035 100644
> --- a/arch/arm64/kernel/kgdb.c
> +++ b/arch/arm64/kernel/kgdb.c
> @@ -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
> };
>
> --
> 2.0.3
>
--
Hi, all,
Resubmitting this from a couple of weeks ago, also fixing the same problem in
ARM64. Could I get some feedback or an ack on this? Otherwise, please apply it.
Thanks!
Omar
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] ARM/ARM64: don't enter kgdb when userspace executes a kgdb break instruction.
2014-07-30 7:12 [PATCH] ARM/ARM64: don't enter kgdb when userspace executes a kgdb break instruction Omar Sandoval
2014-07-30 7:20 ` Omar Sandoval
@ 2014-07-30 11:24 ` Will Deacon
2014-07-31 5:33 ` Omar Sandoval
1 sibling, 1 reply; 6+ messages in thread
From: Will Deacon @ 2014-07-30 11:24 UTC (permalink / raw)
To: linux-arm-kernel
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
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;
}
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] ARM/ARM64: don't enter kgdb when userspace executes a kgdb break instruction.
2014-07-30 11:24 ` Will Deacon
@ 2014-07-31 5:33 ` Omar Sandoval
2014-07-31 10:46 ` Will Deacon
0 siblings, 1 reply; 6+ messages in thread
From: Omar Sandoval @ 2014-07-31 5:33 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] ARM/ARM64: don't enter kgdb when userspace executes a kgdb break instruction.
2014-07-31 5:33 ` Omar Sandoval
@ 2014-07-31 10:46 ` Will Deacon
2014-08-01 3:07 ` Omar Sandoval
0 siblings, 1 reply; 6+ messages in thread
From: Will Deacon @ 2014-07-31 10:46 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] ARM/ARM64: don't enter kgdb when userspace executes a kgdb break instruction.
2014-07-31 10:46 ` Will Deacon
@ 2014-08-01 3:07 ` Omar Sandoval
0 siblings, 0 replies; 6+ messages in thread
From: Omar Sandoval @ 2014-08-01 3:07 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-08-01 3:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-07-30 7:12 [PATCH] ARM/ARM64: don't enter kgdb when userspace executes a kgdb break instruction Omar Sandoval
2014-07-30 7:20 ` Omar Sandoval
2014-07-30 11:24 ` Will Deacon
2014-07-31 5:33 ` Omar Sandoval
2014-07-31 10:46 ` Will Deacon
2014-08-01 3:07 ` Omar Sandoval
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).