lkml.org 
[lkml]   [2020]   [Dec]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
Subject[PATCH] x86/kgdb: Allow removal of early BPs
Date
The problem is that breakpoints that are set early (e.g. via kgdbwait)
cannot be deleted after boot completed (to be precise after mark_rodata_ro
ran).

When setting a breakpoint early there are executable pages that are
writable so the copy_to_kernel_nofault call in kgdb_arch_set_breakpoint
succeeds and the breakpoint is saved as type BP_BREAKPOINT.

Later in the boot write access to these pages is restricted. So when
removing the breakpoint the copy_to_kernel_nofault call in
kgdb_arch_remove_breakpoint is destined to fail and the breakpoint removal
fails. So after copy_to_kernel_nofault failed try to text_poke_kgdb which
can work around nonwriteability.

One thing to consider when doing this is that code can go away during boot
(e.g. .init.text). Previously kgdb_arch_remove_breakpoint handled this case
gracefully by just having copy_to_kernel_nofault fail but if one then calls
text_poke_kgdb the system dies due to the BUG_ON we moved out of
__text_poke. To avoid this __text_poke now returns an error in case of a
nonpresent code page and the error is handled at call site.

Checkpatch complains about two uses of BUG_ON but the new code should not
trigger BUG_ON in cases where the old didn't.

Co-developed-by: Lorena Kretzschmar <qy15sije@cip.cs.fau.de>
Signed-off-by: Lorena Kretzschmar <qy15sije@cip.cs.fau.de>
Signed-off-by: Stefan Saecherl <stefan.saecherl@fau.de>
---
arch/x86/kernel/alternative.c | 16 +++++++----
arch/x86/kernel/kgdb.c | 54 ++++++++++++++++++++++++-----------
2 files changed, 48 insertions(+), 22 deletions(-)

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 2400ad62f330..0f145d837885 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -878,11 +878,9 @@ static void *__text_poke(void *addr, const void *opcode, size_t len)
if (cross_page_boundary)
pages[1] = virt_to_page(addr + PAGE_SIZE);
}
- /*
- * If something went wrong, crash and burn since recovery paths are not
- * implemented.
- */
- BUG_ON(!pages[0] || (cross_page_boundary && !pages[1]));
+
+ if (!pages[0] || (cross_page_boundary && !pages[1]))
+ return ERR_PTR(-EFAULT);

/*
* Map the page without the global bit, as TLB flushing is done with
@@ -976,7 +974,13 @@ void *text_poke(void *addr, const void *opcode, size_t len)
{
lockdep_assert_held(&text_mutex);

- return __text_poke(addr, opcode, len);
+ addr = __text_poke(addr, opcode, len);
+ /*
+ * If something went wrong, crash and burn since recovery paths are not
+ * implemented.
+ */
+ BUG_ON(IS_ERR(addr));
+ return addr;
}

/**
diff --git a/arch/x86/kernel/kgdb.c b/arch/x86/kernel/kgdb.c
index ff7878df96b4..e98c9c43db7c 100644
--- a/arch/x86/kernel/kgdb.c
+++ b/arch/x86/kernel/kgdb.c
@@ -731,6 +731,7 @@ void kgdb_arch_set_pc(struct pt_regs *regs, unsigned long ip)
int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
{
int err;
+ void *addr;

bpt->type = BP_BREAKPOINT;
err = copy_from_kernel_nofault(bpt->saved_instr, (char *)bpt->bpt_addr,
@@ -747,8 +748,14 @@ int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)
*/
if (mutex_is_locked(&text_mutex))
return -EBUSY;
- text_poke_kgdb((void *)bpt->bpt_addr, arch_kgdb_ops.gdb_bpt_instr,
- BREAK_INSTR_SIZE);
+
+ addr = text_poke_kgdb((void *)bpt->bpt_addr, arch_kgdb_ops.gdb_bpt_instr,
+ BREAK_INSTR_SIZE);
+ /* This should never trigger because the above call to copy_from_kernel_nofault
+ * already succeeded.
+ */
+ BUG_ON(IS_ERR(addr));
+
bpt->type = BP_POKE_BREAKPOINT;

return 0;
@@ -756,21 +763,36 @@ int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt)

int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt)
{
- if (bpt->type != BP_POKE_BREAKPOINT)
- goto knl_write;
- /*
- * It is safe to call text_poke_kgdb() because normal kernel execution
- * is stopped on all cores, so long as the text_mutex is not locked.
- */
- if (mutex_is_locked(&text_mutex))
- goto knl_write;
- text_poke_kgdb((void *)bpt->bpt_addr, bpt->saved_instr,
- BREAK_INSTR_SIZE);
- return 0;
+ void *addr;
+ int err;

-knl_write:
- return copy_to_kernel_nofault((char *)bpt->bpt_addr,
- (char *)bpt->saved_instr, BREAK_INSTR_SIZE);
+ if (bpt->type == BP_POKE_BREAKPOINT) {
+ if (mutex_is_locked(&text_mutex)) {
+ err = copy_to_kernel_nofault((char *)bpt->bpt_addr,
+ (char *)bpt->saved_instr,
+ BREAK_INSTR_SIZE);
+ } else {
+ /*
+ * It is safe to call text_poke_kgdb() because normal kernel execution
+ * is stopped on all cores, so long as the text_mutex is not locked.
+ */
+ addr = text_poke_kgdb((void *)bpt->bpt_addr,
+ bpt->saved_instr,
+ BREAK_INSTR_SIZE);
+ err = PTR_ERR_OR_ZERO(addr);
+ }
+ } else {
+ err = copy_to_kernel_nofault((char *)bpt->bpt_addr,
+ (char *)bpt->saved_instr,
+ BREAK_INSTR_SIZE);
+ if (err == -EFAULT && !mutex_is_locked(&text_mutex)) {
+ addr = text_poke_kgdb((void *)bpt->bpt_addr,
+ bpt->saved_instr,
+ BREAK_INSTR_SIZE);
+ err = PTR_ERR_OR_ZERO(addr);
+ }
+ }
+ return err;
}

const struct kgdb_arch arch_kgdb_ops = {
--
2.20.1
\
 
 \ /
  Last update: 2020-12-14 15:29    [W:0.084 / U:1.048 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site