Re: [PATCH v3 2/3] kgdb: Add NOKPROBE labels on the trap handler functions
Hi, On Sun, Sep 27, 2020 at 2:16 PM Daniel Thompson 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 > --- > 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 -Doug
[PATCH v3 2/3] kgdb: Add NOKPROBE labels on the trap handler functions
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
---
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
Re: [PATCH v3 2/3] kgdb: Add NOKPROBE labels on the trap handler functions
On Mon, Sep 14, 2020 at 05:14:22PM -0700, Doug Anderson wrote: > Hi, > > On Mon, Sep 14, 2020 at 6:02 AM Daniel Thompson > 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. > > Other functions that seem to be missing from a quick skim: > * kgdb_io_ready() > * kgdb_roundup_cpus() > * kgdb_call_nmi_hook() I've grabbed those (and a one or two extras after paying better attention to the entry logic). > I'm not confident in my ability to spot every code path, though, so > I'm not sure at what point we stop looking. I only spent a few > minutes and, if important, I could dig more. Did you have any chance > to see if there was any way to have a magic linker script just add > this to everything under "kernel/debug" or something like that where > we just use a heavier hammer to whack a whole bunch? I think one could play games with linker sections but it would involve adding extra infrastructure for the kprobe blocklist. I'm not convinced that is worth the effort whilst there are acknowledged (and bigger) gaps elsewhere. > In general any extra annotation here is better than no annotation, I > suppose. ...so if you just want to commit what you have (maybe with > the above 3 extra functions) then I suppose it'd be fine. This wasn't quite confident enough for me to convert into an Acked-by: but I plan to pull v4 into -next very shortly after posting it (since everything else is agreed). Daniel.
Re: [PATCH v3 2/3] kgdb: Add NOKPROBE labels on the trap handler functions
Hi, On Mon, Sep 14, 2020 at 6:02 AM Daniel Thompson 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. Other functions that seem to be missing from a quick skim: * kgdb_io_ready() * kgdb_roundup_cpus() * kgdb_call_nmi_hook() I'm not confident in my ability to spot every code path, though, so I'm not sure at what point we stop looking. I only spent a few minutes and, if important, I could dig more. Did you have any chance to see if there was any way to have a magic linker script just add this to everything under "kernel/debug" or something like that where we just use a heavier hammer to whack a whole bunch? In general any extra annotation here is better than no annotation, I suppose. ...so if you just want to commit what you have (maybe with the above 3 extra functions) then I suppose it'd be fine. -Doug -Doug
[PATCH v3 2/3] kgdb: Add NOKPROBE labels on the trap handler functions
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
---
kernel/debug/debug_core.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
index b1277728a835..9618c1e2faf6 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)
{
@@ -302,6 +304,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 +332,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 +396,7 @@ int dbg_deactivate_sw_breakpoints(void)
}
return ret;
}
+NOKPROBE_SYMBOL(dbg_deactivate_sw_breakpoints);
int dbg_remove_sw_break(unsigned long addr)
{
@@ -560,6 +565,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 +573,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 +805,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 +850,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 +885,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 +911,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
