[v2,3/3] kgdb: Add NOKPROBE labels on the trap handler functions

Message ID 20200716151943.2167652-4-daniel.thompson@linaro.org
State New
Headers show
Series
  • kgdb: Honour the kprobe blacklist when setting breakpoints
Related show

Commit Message

Daniel Thompson July 16, 2020, 3:19 p.m.
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

Comments

Doug Anderson July 17, 2020, 10:39 p.m. | #1
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.
Daniel Thompson July 20, 2020, 8:13 a.m. | #2
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.
Doug Anderson July 21, 2020, 9:22 p.m. | #3
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
Daniel Thompson Sept. 4, 2020, 3:40 p.m. | #4
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.

Patch

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)