kgdb has traditionally adopted a no safety rails approach to breakpoint
placement. If the debugger is commanded to place a breakpoint at an
address then it will do so even if that breakpoint results in kgdb
becoming inoperable.
A stop-the-world debugger with memory peek/poke intrinsically provides
its operator with the means to hose their system in all manner of
exciting ways (not least because stopping-the-world is already a DoS
attack ;-) ). Nevertheless the current no safety rail approach is
difficult to defend, especially given kprobes can provide us with plenty
of machinery to mark the parts of the kernel where breakpointing is
discouraged.
This patchset introduces some safety rails by using the existing kprobes
infrastructure and ensures this will be enabled by default on
architectures that implement kprobes. At present it does not cover
absolutely all locations where breakpoints can cause trouble but it will
block off several avenues, including the architecture specific parts
that are handled by arch_within_kprobe_blacklist().
v4:
* Fixed KConfig dependencies for HONOUR_KPROBE_BLOCKLIST on kernels
where MODULES=n
* Add additional debug_core.c functions to the blocklist (thanks Doug)
* Collected a few tags
v3:
* Dropped the single step blocklist checks. It is not proven that the
code was actually reachable without triggering the catastrophic
failure flag (which inhibits resume already).
* Update patch description for ("kgdb: Add NOKPROBE labels...") and
added symbols that are called during trap exit
* Added a new patch to push the breakpoint activation later in the
flow and ensure the I/O functions are not called with breakpoints
activated.
v2:
* Reworked after initial RFC to make honouring the blocklist require
CONFIG_KPROBES. It is now optional but the blocklist will be enabled
by default for architectures that CONFIG_HAVE_KPROBES
Daniel Thompson (3):
kgdb: Honour the kprobe blocklist when setting breakpoints
kgdb: Add NOKPROBE labels on the trap handler functions
kernel: debug: Centralize dbg_[de]activate_sw_breakpoints
include/linux/kgdb.h | 18 ++++++++++++++++++
kernel/debug/debug_core.c | 22 ++++++++++++++++++++++
kernel/debug/gdbstub.c | 1 -
kernel/debug/kdb/kdb_bp.c | 9 +++++++++
kernel/debug/kdb/kdb_debugger.c | 2 --
lib/Kconfig.kgdb | 15 +++++++++++++++
6 files changed, 64 insertions(+), 3 deletions(-)
--
2.25.4
Currently kgdb has absolutely no safety rails in place to discourage or
prevent a user from placing a breakpoint in dangerous places such as
the debugger's own trap entry/exit and other places where it is not safe
to take synchronous traps.
Introduce a new config symbol KGDB_HONOUR_BLOCKLIST and modify the
default implementation of kgdb_validate_break_address() so that we use
the kprobe blocklist to prohibit instrumentation of critical functions
if the config symbol is set. The config symbol dependencies are set to
ensure that the blocklist will be enabled by default if we enable KGDB
and are compiling for an architecture where we HAVE_KPROBES.
Suggested-by: Peter Zijlstra <[email protected]>
Reviewed-by: Douglas Anderson <[email protected]>
Reviewed-by: Masami Hiramatsu <[email protected]>
Signed-off-by: Daniel Thompson <[email protected]>
---
include/linux/kgdb.h | 18 ++++++++++++++++++
kernel/debug/debug_core.c | 4 ++++
kernel/debug/kdb/kdb_bp.c | 9 +++++++++
lib/Kconfig.kgdb | 15 +++++++++++++++
4 files changed, 46 insertions(+)
diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
index 477b8b7c908f..0d6cf64c8bb1 100644
--- a/include/linux/kgdb.h
+++ b/include/linux/kgdb.h
@@ -16,6 +16,7 @@
#include <linux/linkage.h>
#include <linux/init.h>
#include <linux/atomic.h>
+#include <linux/kprobes.h>
#ifdef CONFIG_HAVE_ARCH_KGDB
#include <asm/kgdb.h>
#endif
@@ -335,6 +336,23 @@ extern int kgdb_nmicallin(int cpu, int trapnr, void *regs, int err_code,
atomic_t *snd_rdy);
extern void gdbstub_exit(int status);
+/*
+ * kgdb and kprobes both use the same (kprobe) blocklist (which makes sense
+ * given they are both typically hooked up to the same trap meaning on most
+ * architectures one cannot be used to debug the other)
+ *
+ * However on architectures where kprobes is not (yet) implemented we permit
+ * breakpoints everywhere rather than blocking everything by default.
+ */
+static inline bool kgdb_within_blocklist(unsigned long addr)
+{
+#ifdef CONFIG_KGDB_HONOUR_BLOCKLIST
+ return within_kprobe_blacklist(addr);
+#else
+ return false;
+#endif
+}
+
extern int kgdb_single_step;
extern atomic_t kgdb_active;
#define in_dbg_master() \
diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
index b16dbc1bf056..b1277728a835 100644
--- a/kernel/debug/debug_core.c
+++ b/kernel/debug/debug_core.c
@@ -188,6 +188,10 @@ int __weak kgdb_validate_break_address(unsigned long addr)
{
struct kgdb_bkpt tmp;
int err;
+
+ if (kgdb_within_blocklist(addr))
+ return -EINVAL;
+
/* Validate setting the breakpoint and then removing it. If the
* remove fails, the kernel needs to emit a bad message because we
* are deep trouble not being able to put things back the way we
diff --git a/kernel/debug/kdb/kdb_bp.c b/kernel/debug/kdb/kdb_bp.c
index d7ebb2c79cb8..ec4940146612 100644
--- a/kernel/debug/kdb/kdb_bp.c
+++ b/kernel/debug/kdb/kdb_bp.c
@@ -306,6 +306,15 @@ static int kdb_bp(int argc, const char **argv)
if (!template.bp_addr)
return KDB_BADINT;
+ /*
+ * This check is redundant (since the breakpoint machinery should
+ * be doing the same check during kdb_bp_install) but gives the
+ * user immediate feedback.
+ */
+ diag = kgdb_validate_break_address(template.bp_addr);
+ if (diag)
+ return diag;
+
/*
* Find an empty bp structure to allocate
*/
diff --git a/lib/Kconfig.kgdb b/lib/Kconfig.kgdb
index 256f2486f9bd..05dae05b6cc9 100644
--- a/lib/Kconfig.kgdb
+++ b/lib/Kconfig.kgdb
@@ -24,6 +24,21 @@ menuconfig KGDB
if KGDB
+config KGDB_HONOUR_BLOCKLIST
+ bool "KGDB: use kprobe blocklist to prohibit unsafe breakpoints"
+ depends on HAVE_KPROBES
+ depends on MODULES
+ select KPROBES
+ default y
+ help
+ If set to Y the debug core will use the kprobe blocklist to
+ identify symbols where it is unsafe to set breakpoints.
+ In particular this disallows instrumentation of functions
+ called during debug trap handling and thus makes it very
+ difficult to inadvertently provoke recursive trap handling.
+
+ If unsure, say Y.
+
config KGDB_SERIAL_CONSOLE
tristate "KGDB: use kgdb over the serial console"
select CONSOLE_POLL
--
2.25.4
During debug trap execution we expect dbg_deactivate_sw_breakpoints()
to be paired with an dbg_activate_sw_breakpoint(). Currently although
the calls are paired correctly they are needlessly smeared across three
different functions. Worse this also results in code to drive polled I/O
being called with breakpoints activated which, in turn, needlessly
increases the set of functions that will recursively trap if breakpointed.
Fix this by moving the activation of breakpoints into the debug core.
Signed-off-by: Daniel Thompson <[email protected]>
Reviewed-by: Douglas Anderson <[email protected]>
---
kernel/debug/debug_core.c | 2 ++
kernel/debug/gdbstub.c | 1 -
kernel/debug/kdb/kdb_debugger.c | 2 --
3 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
index faa1f99ce65a..e4339fd82db0 100644
--- a/kernel/debug/debug_core.c
+++ b/kernel/debug/debug_core.c
@@ -768,6 +768,8 @@ static int kgdb_cpu_enter(struct kgdb_state *ks, struct pt_regs *regs,
}
}
+ dbg_activate_sw_breakpoints();
+
/* Call the I/O driver's post_exception routine */
if (dbg_io_ops->post_exception)
dbg_io_ops->post_exception();
diff --git a/kernel/debug/gdbstub.c b/kernel/debug/gdbstub.c
index cc3c43dfec44..e9a9c3097089 100644
--- a/kernel/debug/gdbstub.c
+++ b/kernel/debug/gdbstub.c
@@ -1061,7 +1061,6 @@ int gdb_serial_stub(struct kgdb_state *ks)
error_packet(remcom_out_buffer, -EINVAL);
break;
}
- dbg_activate_sw_breakpoints();
fallthrough; /* to default processing */
default:
default_handle:
diff --git a/kernel/debug/kdb/kdb_debugger.c b/kernel/debug/kdb/kdb_debugger.c
index 53a0df6e4d92..0220afda3200 100644
--- a/kernel/debug/kdb/kdb_debugger.c
+++ b/kernel/debug/kdb/kdb_debugger.c
@@ -147,7 +147,6 @@ int kdb_stub(struct kgdb_state *ks)
return DBG_PASS_EVENT;
}
kdb_bp_install(ks->linux_regs);
- dbg_activate_sw_breakpoints();
/* Set the exit state to a single step or a continue */
if (KDB_STATE(DOING_SS))
gdbstub_state(ks, "s");
@@ -167,7 +166,6 @@ int kdb_stub(struct kgdb_state *ks)
* differently vs the gdbstub
*/
kgdb_single_step = 0;
- dbg_deactivate_sw_breakpoints();
return DBG_SWITCH_CPU_EVENT;
}
return kgdb_info[ks->cpu].ret_state;
--
2.25.4
Currently kgdb honours the kprobe blocklist but doesn't place its own
trap handling code on the list. Add labels to discourage attempting to
use kgdb to debug itself.
Not every functions that executes from the trap handler needs to be
marked up: relatively early in the trap handler execution (just after
we bring the other CPUs to a halt) all breakpoints are replaced with
the original opcodes. This patch marks up code in the debug_core that
executes between trap entry and the breakpoints being deactivated
and, also, code that executes between breakpoint activation and trap
exit.
To be clear these changes are not sufficient to make recursive trapping
impossible since cover all the library calls made during kgdb's
entry/exit logic. However going much further whilst we are sharing the
kprobe blocklist risks reducing the capabilities of kprobe and this
would be a bad trade off (especially so given kgdb's users are currently
conditioned to avoid recursive traps).
Signed-off-by: Daniel Thompson <[email protected]>
---
kernel/debug/debug_core.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
index b1277728a835..faa1f99ce65a 100644
--- a/kernel/debug/debug_core.c
+++ b/kernel/debug/debug_core.c
@@ -177,12 +177,14 @@ int __weak kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
arch_kgdb_ops.gdb_bpt_instr, BREAK_INSTR_SIZE);
return err;
}
+NOKPROBE_SYMBOL(kgdb_arch_set_breakpoint);
int __weak kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt)
{
return copy_to_kernel_nofault((char *)bpt->bpt_addr,
(char *)bpt->saved_instr, BREAK_INSTR_SIZE);
}
+NOKPROBE_SYMBOL(kgdb_arch_remove_breakpoint);
int __weak kgdb_validate_break_address(unsigned long addr)
{
@@ -212,6 +214,7 @@ unsigned long __weak kgdb_arch_pc(int exception, struct pt_regs *regs)
{
return instruction_pointer(regs);
}
+NOKPROBE_SYMBOL(kgdb_arch_pc);
int __weak kgdb_arch_init(void)
{
@@ -222,6 +225,7 @@ int __weak kgdb_skipexception(int exception, struct pt_regs *regs)
{
return 0;
}
+NOKPROBE_SYMBOL(kgdb_skipexception);
#ifdef CONFIG_SMP
@@ -243,6 +247,7 @@ void __weak kgdb_call_nmi_hook(void *ignored)
*/
kgdb_nmicallback(raw_smp_processor_id(), get_irq_regs());
}
+NOKPROBE_SYMBOL(kgdb_call_nmi_hook);
void __weak kgdb_roundup_cpus(void)
{
@@ -276,6 +281,7 @@ void __weak kgdb_roundup_cpus(void)
kgdb_info[cpu].rounding_up = false;
}
}
+NOKPROBE_SYMBOL(kgdb_roundup_cpus);
#endif
@@ -302,6 +308,7 @@ static void kgdb_flush_swbreak_addr(unsigned long addr)
/* Force flush instruction cache if it was outside the mm */
flush_icache_range(addr, addr + BREAK_INSTR_SIZE);
}
+NOKPROBE_SYMBOL(kgdb_flush_swbreak_addr);
/*
* SW breakpoint management:
@@ -329,6 +336,7 @@ int dbg_activate_sw_breakpoints(void)
}
return ret;
}
+NOKPROBE_SYMBOL(dbg_activate_sw_breakpoints);
int dbg_set_sw_break(unsigned long addr)
{
@@ -392,6 +400,7 @@ int dbg_deactivate_sw_breakpoints(void)
}
return ret;
}
+NOKPROBE_SYMBOL(dbg_deactivate_sw_breakpoints);
int dbg_remove_sw_break(unsigned long addr)
{
@@ -513,6 +522,7 @@ static int kgdb_io_ready(int print_wait)
}
return 1;
}
+NOKPROBE_SYMBOL(kgdb_io_ready);
static int kgdb_reenter_check(struct kgdb_state *ks)
{
@@ -560,6 +570,7 @@ static int kgdb_reenter_check(struct kgdb_state *ks)
return 1;
}
+NOKPROBE_SYMBOL(kgdb_reenter_check);
static void dbg_touch_watchdogs(void)
{
@@ -567,6 +578,7 @@ static void dbg_touch_watchdogs(void)
clocksource_touch_watchdog();
rcu_cpu_stall_reset();
}
+NOKPROBE_SYMBOL(dbg_touch_watchdogs);
static int kgdb_cpu_enter(struct kgdb_state *ks, struct pt_regs *regs,
int exception_state)
@@ -798,6 +810,7 @@ static int kgdb_cpu_enter(struct kgdb_state *ks, struct pt_regs *regs,
return kgdb_info[cpu].ret_state;
}
+NOKPROBE_SYMBOL(kgdb_cpu_enter);
/*
* kgdb_handle_exception() - main entry point from a kernel exception
@@ -842,6 +855,7 @@ kgdb_handle_exception(int evector, int signo, int ecode, struct pt_regs *regs)
arch_kgdb_ops.enable_nmi(1);
return ret;
}
+NOKPROBE_SYMBOL(kgdb_handle_exception);
/*
* GDB places a breakpoint at this function to know dynamically loaded objects.
@@ -876,6 +890,7 @@ int kgdb_nmicallback(int cpu, void *regs)
#endif
return 1;
}
+NOKPROBE_SYMBOL(kgdb_nmicallback);
int kgdb_nmicallin(int cpu, int trapnr, void *regs, int err_code,
atomic_t *send_ready)
@@ -901,6 +916,7 @@ int kgdb_nmicallin(int cpu, int trapnr, void *regs, int err_code,
#endif
return 1;
}
+NOKPROBE_SYMBOL(kgdb_nmicallin);
static void kgdb_console_write(struct console *co, const char *s,
unsigned count)
--
2.25.4
On Sun, Sep 27, 2020 at 10:15:28PM +0100, Daniel Thompson wrote:
> kgdb has traditionally adopted a no safety rails approach to breakpoint
> placement. If the debugger is commanded to place a breakpoint at an
> address then it will do so even if that breakpoint results in kgdb
> becoming inoperable.
>
> A stop-the-world debugger with memory peek/poke intrinsically provides
> its operator with the means to hose their system in all manner of
> exciting ways (not least because stopping-the-world is already a DoS
> attack ;-) ). Nevertheless the current no safety rail approach is
> difficult to defend, especially given kprobes can provide us with plenty
> of machinery to mark the parts of the kernel where breakpointing is
> discouraged.
>
> This patchset introduces some safety rails by using the existing kprobes
> infrastructure and ensures this will be enabled by default on
> architectures that implement kprobes. At present it does not cover
> absolutely all locations where breakpoints can cause trouble but it will
> block off several avenues, including the architecture specific parts
> that are handled by arch_within_kprobe_blacklist().
>
> v4:
> * Fixed KConfig dependencies for HONOUR_KPROBE_BLOCKLIST on kernels
> where MODULES=n
> * Add additional debug_core.c functions to the blocklist (thanks Doug)
> * Collected a few tags
Looks like I neglected to bump the version number in the subject.
For the avoidance of doubt, this comment is correct and the subject
line is broken.
Sorry!
Daniel.
>
> v3:
> * Dropped the single step blocklist checks. It is not proven that the
> code was actually reachable without triggering the catastrophic
> failure flag (which inhibits resume already).
> * Update patch description for ("kgdb: Add NOKPROBE labels...") and
> added symbols that are called during trap exit
> * Added a new patch to push the breakpoint activation later in the
> flow and ensure the I/O functions are not called with breakpoints
> activated.
>
> v2:
> * Reworked after initial RFC to make honouring the blocklist require
> CONFIG_KPROBES. It is now optional but the blocklist will be enabled
> by default for architectures that CONFIG_HAVE_KPROBES
>
> Daniel Thompson (3):
> kgdb: Honour the kprobe blocklist when setting breakpoints
> kgdb: Add NOKPROBE labels on the trap handler functions
> kernel: debug: Centralize dbg_[de]activate_sw_breakpoints
>
> include/linux/kgdb.h | 18 ++++++++++++++++++
> kernel/debug/debug_core.c | 22 ++++++++++++++++++++++
> kernel/debug/gdbstub.c | 1 -
> kernel/debug/kdb/kdb_bp.c | 9 +++++++++
> kernel/debug/kdb/kdb_debugger.c | 2 --
> lib/Kconfig.kgdb | 15 +++++++++++++++
> 6 files changed, 64 insertions(+), 3 deletions(-)
>
> --
> 2.25.4
>
Hi,
On Sun, Sep 27, 2020 at 2:16 PM Daniel Thompson
<[email protected]> wrote:
>
> Currently kgdb honours the kprobe blocklist but doesn't place its own
> trap handling code on the list. Add labels to discourage attempting to
> use kgdb to debug itself.
>
> Not every functions that executes from the trap handler needs to be
> marked up: relatively early in the trap handler execution (just after
> we bring the other CPUs to a halt) all breakpoints are replaced with
> the original opcodes. This patch marks up code in the debug_core that
> executes between trap entry and the breakpoints being deactivated
> and, also, code that executes between breakpoint activation and trap
> exit.
>
> To be clear these changes are not sufficient to make recursive trapping
> impossible since cover all the library calls made during kgdb's
> entry/exit logic. However going much further whilst we are sharing the
> kprobe blocklist risks reducing the capabilities of kprobe and this
> would be a bad trade off (especially so given kgdb's users are currently
> conditioned to avoid recursive traps).
>
> Signed-off-by: Daniel Thompson <[email protected]>
> ---
> kernel/debug/debug_core.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
I didn't go on any more hunts for missing functions since this seems
fine to me. It's mostly just trying to make it a little harder for
someone to shoot themselves in the foot, after all. ;-)
Reviewed-by: Douglas Anderson <[email protected]>
-Doug