| Message ID | 20200716151943.2167652-4-daniel.thompson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | kgdb: Honour the kprobe blacklist when setting breakpoints | expand |
Hi, On Thu, Jul 16, 2020 at 8:20 AM Daniel Thompson <daniel.thompson@linaro.org> 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. > > These changes do not make it impossible to provoke recursive trapping > since they do not cover all the calls that can be made on kgdb's entry > 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 <daniel.thompson@linaro.org> > --- > kernel/debug/debug_core.c | 8 ++++++++ > 1 file changed, 8 insertions(+) I could just be missing something, but... I understand not adding "NOKPROBE_SYMBOL" to generic kernel functions that kgdb happens to call, but I'm not quite sure I understand why all of the kdb / kgdb code itself shouldn't be in the blocklist. I certainly don't object to the functions you added to the blocklist, I guess I'm must trying to understand why it's a bad idea to add more or how you came up with the list of functions that you did.
On Fri, Jul 17, 2020 at 03:39:58PM -0700, Doug Anderson wrote: > Hi, > > On Thu, Jul 16, 2020 at 8:20 AM Daniel Thompson > <daniel.thompson@linaro.org> 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. > > > > These changes do not make it impossible to provoke recursive trapping > > since they do not cover all the calls that can be made on kgdb's entry > > 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 <daniel.thompson@linaro.org> > > --- > > kernel/debug/debug_core.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > I could just be missing something, but... > > I understand not adding "NOKPROBE_SYMBOL" to generic kernel functions > that kgdb happens to call, but I'm not quite sure I understand why all > of the kdb / kgdb code itself shouldn't be in the blocklist. I > certainly don't object to the functions you added to the blocklist, I > guess I'm must trying to understand why it's a bad idea to add more or > how you came up with the list of functions that you did. 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. Therefore I only marked up functions that run between the trap firing and the breakpoints being removed (and also between the breakpoints being reinstated and trap exit). Daniel.
Hi, On Mon, Jul 20, 2020 at 1:13 AM Daniel Thompson <daniel.thompson@linaro.org> wrote: > > On Fri, Jul 17, 2020 at 03:39:58PM -0700, Doug Anderson wrote: > > Hi, > > > > On Thu, Jul 16, 2020 at 8:20 AM Daniel Thompson > > <daniel.thompson@linaro.org> 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. > > > > > > These changes do not make it impossible to provoke recursive trapping > > > since they do not cover all the calls that can be made on kgdb's entry > > > 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 <daniel.thompson@linaro.org> > > > --- > > > kernel/debug/debug_core.c | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > I could just be missing something, but... > > > > I understand not adding "NOKPROBE_SYMBOL" to generic kernel functions > > that kgdb happens to call, but I'm not quite sure I understand why all > > of the kdb / kgdb code itself shouldn't be in the blocklist. I > > certainly don't object to the functions you added to the blocklist, I > > guess I'm must trying to understand why it's a bad idea to add more or > > how you came up with the list of functions that you did. > > 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. Therefore I only marked up functions that run between the trap > firing and the breakpoints being removed (and also between the > breakpoints being reinstated and trap exit). Ah, OK! Could that be added to the commit message? Also, shouldn't you mark kgdb_arch_set_breakpoint()? What about dbg_activate_sw_breakpoints()? I haven't gone and extensively searched, but those two jump out to me as ones that were missed. I suppose that means that if someone tried to set a breakpoint on a kgdb function that wasn't one of the ones that you listed then the system would happily report that the breakpoint has been set (no error given) but that the breakpoint would just have no effect? It wouldn't crash (which is good), it just wouldn't detect that the breakpoint was useless. However, if these were in the NOKPROBE_SYMBOL then you'd get a nice error message. Is there no way we could use a linker script to just mark everything using a linker script or somesuch? -Doug
On Tue, Jul 21, 2020 at 02:22:08PM -0700, Doug Anderson wrote: > Hi, > > On Mon, Jul 20, 2020 at 1:13 AM Daniel Thompson > <daniel.thompson@linaro.org> wrote: > > > > On Fri, Jul 17, 2020 at 03:39:58PM -0700, Doug Anderson wrote: > > > Hi, > > > > > > On Thu, Jul 16, 2020 at 8:20 AM Daniel Thompson > > > <daniel.thompson@linaro.org> 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. > > > > > > > > These changes do not make it impossible to provoke recursive trapping > > > > since they do not cover all the calls that can be made on kgdb's entry > > > > 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 <daniel.thompson@linaro.org> > > > > --- > > > > kernel/debug/debug_core.c | 8 ++++++++ > > > > 1 file changed, 8 insertions(+) > > > > > > I could just be missing something, but... > > > > > > I understand not adding "NOKPROBE_SYMBOL" to generic kernel functions > > > that kgdb happens to call, but I'm not quite sure I understand why all > > > of the kdb / kgdb code itself shouldn't be in the blocklist. I > > > certainly don't object to the functions you added to the blocklist, I > > > guess I'm must trying to understand why it's a bad idea to add more or > > > how you came up with the list of functions that you did. > > > > 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. Therefore I only marked up functions that run between the trap > > firing and the breakpoints being removed (and also between the > > breakpoints being reinstated and trap exit). > > Ah, OK! Could that be added to the commit message? Will do. > Also, shouldn't you mark kgdb_arch_set_breakpoint()? What about > dbg_activate_sw_breakpoints()? I haven't gone and extensively > searched, but those two jump out to me as ones that were missed. Agree. I think I over-focusses on the entry path. I will review the exit path more closely. > I suppose that means that if someone tried to set a breakpoint on a > kgdb function that wasn't one of the ones that you listed then the > system would happily report that the breakpoint has been set (no error > given) but that the breakpoint would just have no effect? It wouldn't > crash (which is good), it just wouldn't detect that the breakpoint was > useless. Assuming the kgdb function is used exclusively from the trap handler then this is correct. > However, if these were in the NOKPROBE_SYMBOL then you'd get > a nice error message. Is there no way we could use a linker script to > just mark everything using a linker script or somesuch? You'd still get odd effects with library functions that are used inside and outside the debugger (which can be breakpointed but don't trigger inside kgdb). Arguably the effect is clearer to users if they can see kdb/kgdb functions behaving the same way as library functions. It's odd but it won't promote false expectations from users. Daniel.
diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c index 4b59bcc90c5d..b056afb1beec 100644 --- a/kernel/debug/debug_core.c +++ b/kernel/debug/debug_core.c @@ -183,6 +183,7 @@ 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) { @@ -315,6 +316,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: @@ -405,6 +407,7 @@ int dbg_deactivate_sw_breakpoints(void) } return ret; } +NOKPROBE_SYMBOL(dbg_deactivate_sw_breakpoints); int dbg_remove_sw_break(unsigned long addr) { @@ -573,6 +576,7 @@ static int kgdb_reenter_check(struct kgdb_state *ks) return 1; } +NOKPROBE_SYMBOL(kgdb_reenter_check); static void dbg_touch_watchdogs(void) { @@ -811,6 +815,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 @@ -855,6 +860,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. @@ -889,6 +895,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) @@ -914,6 +921,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)
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. These changes do not make it impossible to provoke recursive trapping since they do not cover all the calls that can be made on kgdb's entry 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 <daniel.thompson@linaro.org> --- kernel/debug/debug_core.c | 8 ++++++++ 1 file changed, 8 insertions(+) -- 2.25.4