[RFC PATCH 0/4] kgdb: Honour the kprobe blacklist when setting breakpoints

[RFC PATCH 0/4] kgdb: Honour the kprobe blacklist when setting breakpoints

[Daniel Thompson]

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 does intrinsically
provide 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 ;-) ) but the current no safety rail approach is not easy to
defend, especially given kprobes provides us with plenty of machinery to
mark parts of the kernel where breakpointing is discouraged.

This patchset introduces some safety rails by using the existing
kprobes infrastructure. It does not cover all locations where
breakpoints can cause trouble but it will definitely block off several
avenues, including the architecture specific parts that are handled by
arch_within_kprobe_blacklist().

This patch is an RFC because:

1. My workstation is still chugging through the compile testing.

2. Patch 4 needs more runtime testing.

3. The code to extract the kprobe blacklist code (patch 4 again) needs
   more review especially for its impact on arch specific code.

To be clear I do plan to do the detailed review of the kprobe blacklist
stuff but would like to check the direction of travel first since the
change is already surprisingly big and maybe there's a better way to
organise things.


Daniel.


Daniel Thompson (4):
  kgdb: Honour the kprobe blacklist when setting breakpoints
  kgdb: Use the kprobe blacklist to limit single stepping
  kgdb: Add NOKPROBE labels on the trap handler functions
  kprobes: Allow the kprobes blacklist to be compiled independently

 arch/Kconfig                            |   6 +-
 arch/arm/probes/kprobes/Makefile        |   1 +
 arch/arm/probes/kprobes/blacklist.c     |  37 ++++
 arch/arm/probes/kprobes/core.c          |  10 -
 arch/powerpc/kernel/Makefile            |   1 +
 arch/powerpc/kernel/kprobes-blacklist.c |  34 ++++
 arch/powerpc/kernel/kprobes.c           |   8 -
 include/asm-generic/kprobes.h           |   2 +-
 include/asm-generic/vmlinux.lds.h       |   2 +-
 include/linux/kgdb.h                    |   1 +
 include/linux/kprobes.h                 |  29 ++-
 kernel/Makefile                         |   1 +
 kernel/debug/debug_core.c               |  31 +++
 kernel/debug/gdbstub.c                  |  10 +-
 kernel/debug/kdb/kdb_bp.c               |  17 +-
 kernel/debug/kdb/kdb_main.c             |  10 +-
 kernel/kprobes.c                        | 204 +------------------
 kernel/kprobes_blacklist.c              | 260 ++++++++++++++++++++++++
 lib/Kconfig.kgdb                        |   1 +
 19 files changed, 427 insertions(+), 238 deletions(-)
 create mode 100644 arch/arm/probes/kprobes/blacklist.c
 create mode 100644 arch/powerpc/kernel/kprobes-blacklist.c
 create mode 100644 kernel/kprobes_blacklist.c

--
2.25.4

[RFC PATCH 1/4] kgdb: Honour the kprobe blacklist when setting breakpoints

[Daniel Thompson]

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.

Modify the default implementation of kgdb_validate_break_address() so
that we honour the kprobe blacklist (if there is one). The resulting
blacklist will include code that kgdb could, in fact, debug but I think
we can assume that anyone with sufficient knowledge to meaningfully
debug that code would trivially be able to find and remove the safety
rail if they need to.

Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
---
 kernel/debug/debug_core.c | 11 +++++++++++
 kernel/debug/kdb/kdb_bp.c |  9 +++++++++
 2 files changed, 20 insertions(+)

diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
index ef94e906f05a..81f56d616e04 100644
--- a/kernel/debug/debug_core.c
+++ b/kernel/debug/debug_core.c
@@ -56,6 +56,7 @@
 #include <linux/vmacache.h>
 #include <linux/rcupdate.h>
 #include <linux/irq.h>
+#include <linux/kprobes.h>
 
 #include <asm/cacheflush.h>
 #include <asm/byteorder.h>
@@ -188,6 +189,16 @@ int __weak kgdb_validate_break_address(unsigned long addr)
 {
 	struct kgdb_bkpt tmp;
 	int err;
+
+	/*
+	 * Disallow breakpoints that are marked as unsuitable for kprobing.
+	 * This check is a little over-zealous because it does include
+	 * code that kgdb is entirely capable of debugging but in exchange
+	 * we can avoid recursive trapping (and all the problems that brings).
+	 */
+	if (within_kprobe_blacklist(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
 	 */
-- 
2.25.4

[RFC PATCH 2/4] kgdb: Use the kprobe blacklist to limit single stepping

[Daniel Thompson]

If we are running in a part of the kernel that dislikes breakpoint
debugging then it is very unlikely to be safe to single step. Add
some safety rails to prevent stepping through anything on the kprobe
blacklist.

As part of this kdb_ss() will no longer set the DOING_SS flags when it
requests a step. This is safe because this flag is already redundant,
returning KDB_CMD_SS is all that is needed to request a step (and this
saves us from having to unset the flag if the safety check fails).

Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
---
 include/linux/kgdb.h        |  1 +
 kernel/debug/debug_core.c   | 13 +++++++++++++
 kernel/debug/gdbstub.c      | 10 +++++++++-
 kernel/debug/kdb/kdb_bp.c   |  8 ++------
 kernel/debug/kdb/kdb_main.c | 10 ++++++++--
 5 files changed, 33 insertions(+), 9 deletions(-)

diff --git a/include/linux/kgdb.h b/include/linux/kgdb.h
index c62d76478adc..93b612d81714 100644
--- a/include/linux/kgdb.h
+++ b/include/linux/kgdb.h
@@ -213,6 +213,7 @@ extern void kgdb_arch_set_pc(struct pt_regs *regs, unsigned long pc);
 
 /* Optional functions. */
 extern int kgdb_validate_break_address(unsigned long addr);
+extern int kgdb_validate_single_step_address(unsigned long addr);
 extern int kgdb_arch_set_breakpoint(struct kgdb_bkpt *bpt);
 extern int kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt);
 
diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
index 81f56d616e04..4a2df4509fe1 100644
--- a/kernel/debug/debug_core.c
+++ b/kernel/debug/debug_core.c
@@ -215,6 +215,18 @@ int __weak kgdb_validate_break_address(unsigned long addr)
 	return err;
 }
 
+int __weak kgdb_validate_single_step_address(unsigned long addr)
+{
+	/*
+	 * Disallow stepping when we are executing code that is marked
+	 * as unsuitable for kprobing.
+	 */
+	if (within_kprobe_blacklist(addr))
+		return -EINVAL;
+
+	return 0;
+}
+
 unsigned long __weak kgdb_arch_pc(int exception, struct pt_regs *regs)
 {
 	return instruction_pointer(regs);
@@ -1192,6 +1204,7 @@ noinline void kgdb_breakpoint(void)
 	atomic_dec(&kgdb_setting_breakpoint);
 }
 EXPORT_SYMBOL_GPL(kgdb_breakpoint);
+NOKPROBE_SYMBOL(kgdb_breakpoint);
 
 static int __init opt_kgdb_wait(char *str)
 {
diff --git a/kernel/debug/gdbstub.c b/kernel/debug/gdbstub.c
index 4b280fc7dd67..beb73a61a16d 100644
--- a/kernel/debug/gdbstub.c
+++ b/kernel/debug/gdbstub.c
@@ -1041,8 +1041,16 @@ int gdb_serial_stub(struct kgdb_state *ks)
 			if (tmp == 0)
 				break;
 			/* Fall through - on tmp < 0 */
-		case 'c': /* Continue packet */
 		case 's': /* Single step packet */
+			error = kgdb_validate_single_step_address(
+					kgdb_arch_pc(ks->ex_vector,
+						     ks->linux_regs));
+			if (error != 0) {
+				error_packet(remcom_out_buffer, error);
+				break;
+			}
+			fallthrough;
+		case 'c': /* Continue packet */
 			if (kgdb_contthread && kgdb_contthread != current) {
 				/* Can't switch threads in kgdb */
 				error_packet(remcom_out_buffer, -EINVAL);
diff --git a/kernel/debug/kdb/kdb_bp.c b/kernel/debug/kdb/kdb_bp.c
index ec4940146612..4853c413f579 100644
--- a/kernel/debug/kdb/kdb_bp.c
+++ b/kernel/debug/kdb/kdb_bp.c
@@ -507,18 +507,14 @@ static int kdb_bc(int argc, const char **argv)
  *	None.
  * Remarks:
  *
- *	Set the arch specific option to trigger a debug trap after the next
- *	instruction.
+ *	KDB_CMD_SS is a command that our caller acts on to effect the step.
  */
 
 static int kdb_ss(int argc, const char **argv)
 {
 	if (argc != 0)
 		return KDB_ARGCOUNT;
-	/*
-	 * Set trace flag and go.
-	 */
-	KDB_STATE_SET(DOING_SS);
+
 	return KDB_CMD_SS;
 }
 
diff --git a/kernel/debug/kdb/kdb_main.c b/kernel/debug/kdb/kdb_main.c
index ec190569f690..4b277c066f48 100644
--- a/kernel/debug/kdb/kdb_main.c
+++ b/kernel/debug/kdb/kdb_main.c
@@ -1189,7 +1189,7 @@ static int kdb_local(kdb_reason_t reason, int error, struct pt_regs *regs,
 		     kdb_dbtrap_t db_result)
 {
 	char *cmdbuf;
-	int diag;
+	int diag, res;
 	struct task_struct *kdb_current =
 		kdb_curr_task(raw_smp_processor_id());
 
@@ -1346,10 +1346,16 @@ static int kdb_local(kdb_reason_t reason, int error, struct pt_regs *regs,
 		}
 		if (diag == KDB_CMD_GO
 		 || diag == KDB_CMD_CPU
-		 || diag == KDB_CMD_SS
 		 || diag == KDB_CMD_KGDB)
 			break;
 
+		if (diag == KDB_CMD_SS) {
+			res = kgdb_validate_single_step_address(instruction_pointer(regs));
+			if (res == 0)
+				break;
+			diag = res;
+		}
+
 		if (diag)
 			kdb_cmderror(diag);
 	}
-- 
2.25.4

[RFC PATCH 3/4] kgdb: Add NOKPROBE labels on the trap handler functions

[Daniel Thompson]

Currently kgdb honours the kprobe blacklist but doesn't place its own
trap handling code on the list. Add macros 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
blacklist risks reducing the capabilities of kprobe and this is 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 | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
index 4a2df4509fe1..21d1d91da4bb 100644
--- a/kernel/debug/debug_core.c
+++ b/kernel/debug/debug_core.c
@@ -184,6 +184,7 @@ int __weak kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt)
 	return probe_kernel_write((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)
 {
@@ -321,6 +322,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:
@@ -411,6 +413,7 @@ int dbg_deactivate_sw_breakpoints(void)
 	}
 	return ret;
 }
+NOKPROBE_SYMBOL(dbg_deactivate_sw_breakpoints);
 
 int dbg_remove_sw_break(unsigned long addr)
 {
@@ -567,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)
 {
@@ -801,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
@@ -845,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.
@@ -879,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)
@@ -904,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)
@@ -1204,7 +1212,6 @@ noinline void kgdb_breakpoint(void)
 	atomic_dec(&kgdb_setting_breakpoint);
 }
 EXPORT_SYMBOL_GPL(kgdb_breakpoint);
-NOKPROBE_SYMBOL(kgdb_breakpoint);
 
 static int __init opt_kgdb_wait(char *str)
 {
-- 
2.25.4

[RFC PATCH 4/4] kprobes: Allow the kprobes blacklist to be compiled independently

[Daniel Thompson]

IMPORTANT:

  As menitoned in the covering letter, this series in an RFC and this
  patch, in particular, is acknowledged as needing more work. In
  particular I haven't trimmed uneccessary #includes after splitting
  out the code and may also have missed some places where an an
  architecture overrides one of the weak symbols used for blacklist
  checking.

  If I don't get objections to the general approach taken to splitting
  out this code then  I will do the final clean up and detailed review.

The kprobes blacklist is useful for other tools that set breakpoints
such as kgdb or kdb. Currently the blacklist is only available on
platforms where CONFIG_KPROBES is set (when kprobes is not set then
the blacklist essentially covers the entire kernel).

Separate out the blacklist handling logic from the rest of kprobes to
allow it to be compiled independently.

Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
---
 arch/Kconfig                            |   6 +-
 arch/arm/probes/kprobes/Makefile        |   1 +
 arch/arm/probes/kprobes/blacklist.c     |  37 ++++
 arch/arm/probes/kprobes/core.c          |  10 -
 arch/powerpc/kernel/Makefile            |   1 +
 arch/powerpc/kernel/kprobes-blacklist.c |  34 ++++
 arch/powerpc/kernel/kprobes.c           |   8 -
 include/asm-generic/kprobes.h           |   2 +-
 include/asm-generic/vmlinux.lds.h       |   2 +-
 include/linux/kprobes.h                 |  29 ++-
 kernel/Makefile                         |   1 +
 kernel/kprobes.c                        | 204 +------------------
 kernel/kprobes_blacklist.c              | 260 ++++++++++++++++++++++++
 lib/Kconfig.kgdb                        |   1 +
 14 files changed, 367 insertions(+), 229 deletions(-)
 create mode 100644 arch/arm/probes/kprobes/blacklist.c
 create mode 100644 arch/powerpc/kernel/kprobes-blacklist.c
 create mode 100644 kernel/kprobes_blacklist.c

diff --git a/arch/Kconfig b/arch/Kconfig
index 786a85d4ad40..dd1d709bd195 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -63,7 +63,7 @@ config KPROBES
 	bool "Kprobes"
 	depends on MODULES
 	depends on HAVE_KPROBES
-	select KALLSYMS
+	select KPROBES_BLACKLIST
 	help
 	  Kprobes allows you to trap at almost any kernel address and
 	  execute a callback function.  register_kprobe() establishes
@@ -71,6 +71,10 @@ config KPROBES
 	  for kernel debugging, non-intrusive instrumentation and testing.
 	  If in doubt, say "N".
 
+config KPROBES_BLACKLIST
+	bool
+	select KALLSYMS
+
 config JUMP_LABEL
 	bool "Optimize very unlikely/likely branches"
 	depends on HAVE_ARCH_JUMP_LABEL
diff --git a/arch/arm/probes/kprobes/Makefile b/arch/arm/probes/kprobes/Makefile
index 14db56f49f0a..8b7ede9fb335 100644
--- a/arch/arm/probes/kprobes/Makefile
+++ b/arch/arm/probes/kprobes/Makefile
@@ -1,5 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_KPROBES)		+= core.o actions-common.o checkers-common.o
+obj-$(CONFIG_KPROBES_BLACKLIST)	+= blacklist.o
 obj-$(CONFIG_ARM_KPROBES_TEST)	+= test-kprobes.o
 test-kprobes-objs		:= test-core.o
 
diff --git a/arch/arm/probes/kprobes/blacklist.c b/arch/arm/probes/kprobes/blacklist.c
new file mode 100644
index 000000000000..c3b37f0b59d0
--- /dev/null
+++ b/arch/arm/probes/kprobes/blacklist.c
@@ -0,0 +1,37 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * arch/arm/kernel/kprobe/blacklist.c
+ *
+ * Kprobes on ARM
+ *
+ * Abhishek Sagar <sagar.abhishek@gmail.com>
+ * Copyright (C) 2006, 2007 Motorola Inc.
+ *
+ * Nicolas Pitre <nico@marvell.com>
+ * Copyright (C) 2007 Marvell Ltd.
+ */
+
+#include <linux/kernel.h>
+#include <linux/kprobes.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/stop_machine.h>
+#include <linux/sched/debug.h>
+#include <linux/stringify.h>
+#include <asm/traps.h>
+#include <asm/opcodes.h>
+#include <asm/cacheflush.h>
+#include <linux/percpu.h>
+#include <linux/bug.h>
+#include <asm/patch.h>
+#include <asm/sections.h>
+
+bool arch_within_kprobe_blacklist(unsigned long addr)
+{
+	void *a = (void *)addr;
+
+	return __in_irqentry_text(addr) ||
+	       in_entry_text(addr) ||
+	       in_idmap_text(addr) ||
+	       memory_contains(__kprobes_text_start, __kprobes_text_end, a, 1);
+}
diff --git a/arch/arm/probes/kprobes/core.c b/arch/arm/probes/kprobes/core.c
index 90b5bc723c83..dce82761f83a 100644
--- a/arch/arm/probes/kprobes/core.c
+++ b/arch/arm/probes/kprobes/core.c
@@ -545,13 +545,3 @@ int __init arch_init_kprobes()
 #endif
 	return 0;
 }
-
-bool arch_within_kprobe_blacklist(unsigned long addr)
-{
-	void *a = (void *)addr;
-
-	return __in_irqentry_text(addr) ||
-	       in_entry_text(addr) ||
-	       in_idmap_text(addr) ||
-	       memory_contains(__kprobes_text_start, __kprobes_text_end, a, 1);
-}
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index 1c4385852d3d..fd912afbb6f1 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -110,6 +110,7 @@ obj-$(CONFIG_KGDB)		+= kgdb.o
 obj-$(CONFIG_BOOTX_TEXT)	+= btext.o
 obj-$(CONFIG_SMP)		+= smp.o
 obj-$(CONFIG_KPROBES)		+= kprobes.o
+obj-$(CONFIG_KPROBES_BLACKLIST)	+= kprobes-blacklist.o
 obj-$(CONFIG_OPTPROBES)		+= optprobes.o optprobes_head.o
 obj-$(CONFIG_KPROBES_ON_FTRACE)	+= kprobes-ftrace.o
 obj-$(CONFIG_UPROBES)		+= uprobes.o
diff --git a/arch/powerpc/kernel/kprobes-blacklist.c b/arch/powerpc/kernel/kprobes-blacklist.c
new file mode 100644
index 000000000000..4410a80e313a
--- /dev/null
+++ b/arch/powerpc/kernel/kprobes-blacklist.c
@@ -0,0 +1,34 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ *  Kernel Probes (KProbes)
+ *
+ * Copyright (C) IBM Corporation, 2002, 2004
+ *
+ * 2002-Oct	Created by Vamsi Krishna S <vamsi_krishna@in.ibm.com> Kernel
+ *		Probes initial implementation ( includes contributions from
+ *		Rusty Russell).
+ * 2004-July	Suparna Bhattacharya <suparna@in.ibm.com> added jumper probes
+ *		interface to access function arguments.
+ * 2004-Nov	Ananth N Mavinakayanahalli <ananth@in.ibm.com> kprobes port
+ *		for PPC64
+ */
+
+#include <linux/kprobes.h>
+#include <linux/ptrace.h>
+#include <linux/preempt.h>
+#include <linux/extable.h>
+#include <linux/kdebug.h>
+#include <linux/slab.h>
+#include <asm/code-patching.h>
+#include <asm/cacheflush.h>
+#include <asm/sstep.h>
+#include <asm/sections.h>
+#include <linux/uaccess.h>
+
+bool arch_within_kprobe_blacklist(unsigned long addr)
+{
+	return  (addr >= (unsigned long)__kprobes_text_start &&
+		 addr < (unsigned long)__kprobes_text_end) ||
+		(addr >= (unsigned long)_stext &&
+		 addr < (unsigned long)__head_end);
+}
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 81efb605113e..9e87a7fe3207 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -30,14 +30,6 @@ DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
 
 struct kretprobe_blackpoint kretprobe_blacklist[] = {{NULL, NULL}};
 
-bool arch_within_kprobe_blacklist(unsigned long addr)
-{
-	return  (addr >= (unsigned long)__kprobes_text_start &&
-		 addr < (unsigned long)__kprobes_text_end) ||
-		(addr >= (unsigned long)_stext &&
-		 addr < (unsigned long)__head_end);
-}
-
 kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
 {
 	kprobe_opcode_t *addr = NULL;
diff --git a/include/asm-generic/kprobes.h b/include/asm-generic/kprobes.h
index 4a982089c95c..40800c6ff52f 100644
--- a/include/asm-generic/kprobes.h
+++ b/include/asm-generic/kprobes.h
@@ -3,7 +3,7 @@
 #define _ASM_GENERIC_KPROBES_H
 
 #if defined(__KERNEL__) && !defined(__ASSEMBLY__)
-#ifdef CONFIG_KPROBES
+#ifdef CONFIG_KPROBES_BLACKLIST
 /*
  * Blacklist ganerating macro. Specify functions which is not probed
  * by using this macro.
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index 71e387a5fe90..bd8bcadba148 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -176,7 +176,7 @@
 #define BRANCH_PROFILE()
 #endif
 
-#ifdef CONFIG_KPROBES
+#ifdef CONFIG_KPROBES_BLACKLIST
 #define KPROBE_BLACKLIST()	. = ALIGN(8);				      \
 				__start_kprobe_blacklist = .;		      \
 				KEEP(*(_kprobe_blacklist))		      \
diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index 04bdaf01112c..ef6521e75761 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -234,10 +234,6 @@ extern int arch_populate_kprobe_blacklist(void);
 extern bool arch_kprobe_on_func_entry(unsigned long offset);
 extern bool kprobe_on_func_entry(kprobe_opcode_t *addr, const char *sym, unsigned long offset);
 
-extern bool within_kprobe_blacklist(unsigned long addr);
-extern int kprobe_add_ksym_blacklist(unsigned long entry);
-extern int kprobe_add_area_blacklist(unsigned long start, unsigned long end);
-
 struct kprobe_insn_cache {
 	struct mutex mutex;
 	void *(*alloc)(void);	/* allocate insn page */
@@ -350,12 +346,10 @@ static inline struct kprobe_ctlblk *get_kprobe_ctlblk(void)
 	return this_cpu_ptr(&kprobe_ctlblk);
 }
 
-kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset);
 int register_kprobe(struct kprobe *p);
 void unregister_kprobe(struct kprobe *p);
 int register_kprobes(struct kprobe **kps, int num);
 void unregister_kprobes(struct kprobe **kps, int num);
-unsigned long arch_deref_entry_point(void *);
 
 int register_kretprobe(struct kretprobe *rp);
 void unregister_kretprobe(struct kretprobe *rp);
@@ -373,6 +367,9 @@ void dump_kprobe(struct kprobe *kp);
 void *alloc_insn_page(void);
 void free_insn_page(void *page);
 
+int init_kprobes(void);
+void debugfs_kprobe_init(struct dentry *dir);
+
 #else /* !CONFIG_KPROBES: */
 
 static inline int kprobes_built_in(void)
@@ -431,11 +428,29 @@ static inline int enable_kprobe(struct kprobe *kp)
 	return -ENOSYS;
 }
 
+static inline int init_kprobes(void)
+{
+	return 0;
+}
+
+static inline void debugfs_kprobe_init(struct dentry *dir)
+{
+}
+#endif /* CONFIG_KPROBES */
+
+#ifdef CONFIG_KPROBES_BLACKLIST
+extern bool within_kprobe_blacklist(unsigned long addr);
+extern int kprobe_add_ksym_blacklist(unsigned long entry);
+extern int kprobe_add_area_blacklist(unsigned long start, unsigned long end);
+kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset);
+unsigned long arch_deref_entry_point(void *);
+#else /* CONFIG_KPROBES_BLACKLIST */
 static inline bool within_kprobe_blacklist(unsigned long addr)
 {
 	return true;
 }
-#endif /* CONFIG_KPROBES */
+#endif /* CONFIG_KPROBES_BLACKLIST */
+
 static inline int disable_kretprobe(struct kretprobe *rp)
 {
 	return disable_kprobe(&rp->kp);
diff --git a/kernel/Makefile b/kernel/Makefile
index 4cb4130ced32..7ce7948575df 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -82,6 +82,7 @@ obj-$(CONFIG_AUDITSYSCALL) += auditsc.o audit_watch.o audit_fsnotify.o audit_tre
 obj-$(CONFIG_GCOV_KERNEL) += gcov/
 obj-$(CONFIG_KCOV) += kcov.o
 obj-$(CONFIG_KPROBES) += kprobes.o
+obj-$(CONFIG_KPROBES_BLACKLIST) += kprobes_blacklist.o
 obj-$(CONFIG_FAIL_FUNCTION) += fail_function.o
 obj-$(CONFIG_KGDB) += debug/
 obj-$(CONFIG_DETECT_HUNG_TASK) += hung_task.o
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 2625c241ac00..b592316a5d50 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -59,20 +59,11 @@ static struct {
 	raw_spinlock_t lock ____cacheline_aligned_in_smp;
 } kretprobe_table_locks[KPROBE_TABLE_SIZE];
 
-kprobe_opcode_t * __weak kprobe_lookup_name(const char *name,
-					unsigned int __unused)
-{
-	return ((kprobe_opcode_t *)(kallsyms_lookup_name(name)));
-}
-
 static raw_spinlock_t *kretprobe_table_lock_ptr(unsigned long hash)
 {
 	return &(kretprobe_table_locks[hash].lock);
 }
 
-/* Blacklist -- list of struct kprobe_blacklist_entry */
-static LIST_HEAD(kprobe_blacklist);
-
 #ifdef __ARCH_WANT_KPROBES_INSN_SLOT
 /*
  * kprobe->ainsn.insn points to the copy of the instruction to be
@@ -1419,50 +1410,6 @@ static int register_aggr_kprobe(struct kprobe *orig_p, struct kprobe *p)
 	return ret;
 }
 
-bool __weak arch_within_kprobe_blacklist(unsigned long addr)
-{
-	/* The __kprobes marked functions and entry code must not be probed */
-	return addr >= (unsigned long)__kprobes_text_start &&
-	       addr < (unsigned long)__kprobes_text_end;
-}
-
-static bool __within_kprobe_blacklist(unsigned long addr)
-{
-	struct kprobe_blacklist_entry *ent;
-
-	if (arch_within_kprobe_blacklist(addr))
-		return true;
-	/*
-	 * If there exists a kprobe_blacklist, verify and
-	 * fail any probe registration in the prohibited area
-	 */
-	list_for_each_entry(ent, &kprobe_blacklist, list) {
-		if (addr >= ent->start_addr && addr < ent->end_addr)
-			return true;
-	}
-	return false;
-}
-
-bool within_kprobe_blacklist(unsigned long addr)
-{
-	char symname[KSYM_NAME_LEN], *p;
-
-	if (__within_kprobe_blacklist(addr))
-		return true;
-
-	/* Check if the address is on a suffixed-symbol */
-	if (!lookup_symbol_name(addr, symname)) {
-		p = strchr(symname, '.');
-		if (!p)
-			return false;
-		*p = '\0';
-		addr = (unsigned long)kprobe_lookup_name(symname, 0);
-		if (addr)
-			return __within_kprobe_blacklist(addr);
-	}
-	return false;
-}
-
 /*
  * If we have a symbol_name argument, look it up and add the offset field
  * to it. This way, we can specify a relative address to a symbol.
@@ -1845,11 +1792,6 @@ static struct notifier_block kprobe_exceptions_nb = {
 	.priority = 0x7fffffff /* we need to be notified first */
 };
 
-unsigned long __weak arch_deref_entry_point(void *entry)
-{
-	return (unsigned long)entry;
-}
-
 #ifdef CONFIG_KRETPROBES
 /*
  * This kprobe pre_handler is registered with every kretprobe. When probe
@@ -2143,78 +2085,6 @@ void dump_kprobe(struct kprobe *kp)
 }
 NOKPROBE_SYMBOL(dump_kprobe);
 
-int kprobe_add_ksym_blacklist(unsigned long entry)
-{
-	struct kprobe_blacklist_entry *ent;
-	unsigned long offset = 0, size = 0;
-
-	if (!kernel_text_address(entry) ||
-	    !kallsyms_lookup_size_offset(entry, &size, &offset))
-		return -EINVAL;
-
-	ent = kmalloc(sizeof(*ent), GFP_KERNEL);
-	if (!ent)
-		return -ENOMEM;
-	ent->start_addr = entry;
-	ent->end_addr = entry + size;
-	INIT_LIST_HEAD(&ent->list);
-	list_add_tail(&ent->list, &kprobe_blacklist);
-
-	return (int)size;
-}
-
-/* Add all symbols in given area into kprobe blacklist */
-int kprobe_add_area_blacklist(unsigned long start, unsigned long end)
-{
-	unsigned long entry;
-	int ret = 0;
-
-	for (entry = start; entry < end; entry += ret) {
-		ret = kprobe_add_ksym_blacklist(entry);
-		if (ret < 0)
-			return ret;
-		if (ret == 0)	/* In case of alias symbol */
-			ret = 1;
-	}
-	return 0;
-}
-
-int __init __weak arch_populate_kprobe_blacklist(void)
-{
-	return 0;
-}
-
-/*
- * Lookup and populate the kprobe_blacklist.
- *
- * Unlike the kretprobe blacklist, we'll need to determine
- * the range of addresses that belong to the said functions,
- * since a kprobe need not necessarily be at the beginning
- * of a function.
- */
-static int __init populate_kprobe_blacklist(unsigned long *start,
-					     unsigned long *end)
-{
-	unsigned long entry;
-	unsigned long *iter;
-	int ret;
-
-	for (iter = start; iter < end; iter++) {
-		entry = arch_deref_entry_point((void *)*iter);
-		ret = kprobe_add_ksym_blacklist(entry);
-		if (ret == -EINVAL)
-			continue;
-		if (ret < 0)
-			return ret;
-	}
-
-	/* Symbols in __kprobes_text are blacklisted */
-	ret = kprobe_add_area_blacklist((unsigned long)__kprobes_text_start,
-					(unsigned long)__kprobes_text_end);
-
-	return ret ? : arch_populate_kprobe_blacklist();
-}
-
 /* Module notifier call back, checking kprobes on the module */
 static int kprobes_module_callback(struct notifier_block *nb,
 				   unsigned long val, void *data)
@@ -2264,11 +2134,9 @@ static struct notifier_block kprobe_module_nb = {
 	.priority = 0
 };
 
-/* Markers of _kprobe_blacklist section */
-extern unsigned long __start_kprobe_blacklist[];
-extern unsigned long __stop_kprobe_blacklist[];
 
-static int __init init_kprobes(void)
+
+int __init init_kprobes(void)
 {
 	int i, err = 0;
 
@@ -2280,13 +2148,6 @@ static int __init init_kprobes(void)
 		raw_spin_lock_init(&(kretprobe_table_locks[i].lock));
 	}
 
-	err = populate_kprobe_blacklist(__start_kprobe_blacklist,
-					__stop_kprobe_blacklist);
-	if (err) {
-		pr_err("kprobes: failed to populate blacklist: %d\n", err);
-		pr_err("Please take care of using kprobes.\n");
-	}
-
 	if (kretprobe_blacklist_size) {
 		/* lookup the function address from its name */
 		for (i = 0; kretprobe_blacklist[i].name != NULL; i++) {
@@ -2322,7 +2183,6 @@ static int __init init_kprobes(void)
 		init_test_probes();
 	return err;
 }
-subsys_initcall(init_kprobes);
 
 #ifdef CONFIG_DEBUG_FS
 static void report_probe(struct seq_file *pi, struct kprobe *p,
@@ -2417,54 +2277,6 @@ static const struct file_operations debugfs_kprobes_operations = {
 	.release        = seq_release,
 };
 
-/* kprobes/blacklist -- shows which functions can not be probed */
-static void *kprobe_blacklist_seq_start(struct seq_file *m, loff_t *pos)
-{
-	return seq_list_start(&kprobe_blacklist, *pos);
-}
-
-static void *kprobe_blacklist_seq_next(struct seq_file *m, void *v, loff_t *pos)
-{
-	return seq_list_next(v, &kprobe_blacklist, pos);
-}
-
-static int kprobe_blacklist_seq_show(struct seq_file *m, void *v)
-{
-	struct kprobe_blacklist_entry *ent =
-		list_entry(v, struct kprobe_blacklist_entry, list);
-
-	/*
-	 * If /proc/kallsyms is not showing kernel address, we won't
-	 * show them here either.
-	 */
-	if (!kallsyms_show_value())
-		seq_printf(m, "0x%px-0x%px\t%ps\n", NULL, NULL,
-			   (void *)ent->start_addr);
-	else
-		seq_printf(m, "0x%px-0x%px\t%ps\n", (void *)ent->start_addr,
-			   (void *)ent->end_addr, (void *)ent->start_addr);
-	return 0;
-}
-
-static const struct seq_operations kprobe_blacklist_seq_ops = {
-	.start = kprobe_blacklist_seq_start,
-	.next  = kprobe_blacklist_seq_next,
-	.stop  = kprobe_seq_stop,	/* Reuse void function */
-	.show  = kprobe_blacklist_seq_show,
-};
-
-static int kprobe_blacklist_open(struct inode *inode, struct file *filp)
-{
-	return seq_open(filp, &kprobe_blacklist_seq_ops);
-}
-
-static const struct file_operations debugfs_kprobe_blacklist_ops = {
-	.open           = kprobe_blacklist_open,
-	.read           = seq_read,
-	.llseek         = seq_lseek,
-	.release        = seq_release,
-};
-
 static int arm_all_kprobes(void)
 {
 	struct hlist_head *head;
@@ -2615,23 +2427,13 @@ static const struct file_operations fops_kp = {
 	.llseek =	default_llseek,
 };
 
-static int __init debugfs_kprobe_init(void)
+void __init debugfs_kprobe_init(struct dentry *dir)
 {
-	struct dentry *dir;
 	unsigned int value = 1;
 
-	dir = debugfs_create_dir("kprobes", NULL);
-
 	debugfs_create_file("list", 0400, dir, NULL,
 			    &debugfs_kprobes_operations);
 
 	debugfs_create_file("enabled", 0600, dir, &value, &fops_kp);
-
-	debugfs_create_file("blacklist", 0400, dir, NULL,
-			    &debugfs_kprobe_blacklist_ops);
-
-	return 0;
 }
-
-late_initcall(debugfs_kprobe_init);
 #endif /* CONFIG_DEBUG_FS */
diff --git a/kernel/kprobes_blacklist.c b/kernel/kprobes_blacklist.c
new file mode 100644
index 000000000000..ee101dfc8899
--- /dev/null
+++ b/kernel/kprobes_blacklist.c
@@ -0,0 +1,260 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ *  Kernel Probes (KProbes)
+ *  kernel/kprobes_blacklist.c
+ *
+ * Copyright (C) IBM Corporation, 2002, 2004
+ *
+ * 2002-Oct	Created by Vamsi Krishna S <vamsi_krishna@in.ibm.com> Kernel
+ *		Probes initial implementation (includes suggestions from
+ *		Rusty Russell).
+ * 2004-Aug	Updated by Prasanna S Panchamukhi <prasanna@in.ibm.com> with
+ *		hlists and exceptions notifier as suggested by Andi Kleen.
+ * 2004-July	Suparna Bhattacharya <suparna@in.ibm.com> added jumper probes
+ *		interface to access function arguments.
+ * 2004-Sep	Prasanna S Panchamukhi <prasanna@in.ibm.com> Changed Kprobes
+ *		exceptions notifier to be first on the priority list.
+ * 2005-May	Hien Nguyen <hien@us.ibm.com>, Jim Keniston
+ *		<jkenisto@us.ibm.com> and Prasanna S Panchamukhi
+ *		<prasanna@in.ibm.com> added function-return probes.
+ */
+#include <linux/kprobes.h>
+#include <linux/hash.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/stddef.h>
+#include <linux/export.h>
+#include <linux/moduleloader.h>
+#include <linux/kallsyms.h>
+#include <linux/freezer.h>
+#include <linux/seq_file.h>
+#include <linux/debugfs.h>
+#include <linux/sysctl.h>
+#include <linux/kdebug.h>
+#include <linux/memory.h>
+#include <linux/ftrace.h>
+#include <linux/cpu.h>
+#include <linux/jump_label.h>
+
+#include <asm/sections.h>
+#include <asm/cacheflush.h>
+#include <asm/errno.h>
+#include <linux/uaccess.h>
+
+/* Blacklist -- list of struct kprobe_blacklist_entry */
+static LIST_HEAD(kprobe_blacklist);
+
+kprobe_opcode_t * __weak kprobe_lookup_name(const char *name,
+					unsigned int __unused)
+{
+	return ((kprobe_opcode_t *)(kallsyms_lookup_name(name)));
+}
+
+bool __weak arch_within_kprobe_blacklist(unsigned long addr)
+{
+	/* The __kprobes marked functions and entry code must not be probed */
+	return addr >= (unsigned long)__kprobes_text_start &&
+	       addr < (unsigned long)__kprobes_text_end;
+}
+
+unsigned long __weak arch_deref_entry_point(void *entry)
+{
+	return (unsigned long)entry;
+}
+
+static bool __within_kprobe_blacklist(unsigned long addr)
+{
+	struct kprobe_blacklist_entry *ent;
+
+	if (arch_within_kprobe_blacklist(addr))
+		return true;
+	/*
+	 * If there exists a kprobe_blacklist, verify and
+	 * fail any probe registration in the prohibited area
+	 */
+	list_for_each_entry(ent, &kprobe_blacklist, list) {
+		if (addr >= ent->start_addr && addr < ent->end_addr)
+			return true;
+	}
+	return false;
+}
+
+bool within_kprobe_blacklist(unsigned long addr)
+{
+	char symname[KSYM_NAME_LEN], *p;
+
+	if (__within_kprobe_blacklist(addr))
+		return true;
+
+	/* Check if the address is on a suffixed-symbol */
+	if (!lookup_symbol_name(addr, symname)) {
+		p = strchr(symname, '.');
+		if (!p)
+			return false;
+		*p = '\0';
+		addr = (unsigned long)kprobe_lookup_name(symname, 0);
+		if (addr)
+			return __within_kprobe_blacklist(addr);
+	}
+	return false;
+}
+
+int kprobe_add_ksym_blacklist(unsigned long entry)
+{
+	struct kprobe_blacklist_entry *ent;
+	unsigned long offset = 0, size = 0;
+
+	if (!kernel_text_address(entry) ||
+	    !kallsyms_lookup_size_offset(entry, &size, &offset))
+		return -EINVAL;
+
+	ent = kmalloc(sizeof(*ent), GFP_KERNEL);
+	if (!ent)
+		return -ENOMEM;
+	ent->start_addr = entry;
+	ent->end_addr = entry + size;
+	INIT_LIST_HEAD(&ent->list);
+	list_add_tail(&ent->list, &kprobe_blacklist);
+
+	return (int)size;
+}
+
+/* Add all symbols in given area into kprobe blacklist */
+int kprobe_add_area_blacklist(unsigned long start, unsigned long end)
+{
+	unsigned long entry;
+	int ret = 0;
+
+	for (entry = start; entry < end; entry += ret) {
+		ret = kprobe_add_ksym_blacklist(entry);
+		if (ret < 0)
+			return ret;
+		if (ret == 0)	/* In case of alias symbol */
+			ret = 1;
+	}
+	return 0;
+}
+
+int __init __weak arch_populate_kprobe_blacklist(void)
+{
+	return 0;
+}
+
+/*
+ * Lookup and populate the kprobe_blacklist.
+ *
+ * Unlike the kretprobe blacklist, we'll need to determine
+ * the range of addresses that belong to the said functions,
+ * since a kprobe need not necessarily be at the beginning
+ * of a function.
+ */
+static int __init populate_kprobe_blacklist(unsigned long *start,
+					    unsigned long *end)
+{
+	unsigned long entry;
+	unsigned long *iter;
+	int ret;
+
+	for (iter = start; iter < end; iter++) {
+		entry = arch_deref_entry_point((void *)*iter);
+		ret = kprobe_add_ksym_blacklist(entry);
+		if (ret == -EINVAL)
+			continue;
+		if (ret < 0)
+			return ret;
+	}
+
+	/* Symbols in __kprobes_text are blacklisted */
+	ret = kprobe_add_area_blacklist((unsigned long)__kprobes_text_start,
+					(unsigned long)__kprobes_text_end);
+
+	return ret ? : arch_populate_kprobe_blacklist();
+}
+
+/* Markers of _kprobe_blacklist section */
+extern unsigned long __start_kprobe_blacklist[];
+extern unsigned long __stop_kprobe_blacklist[];
+
+static int __init init_kprobes_blacklist(void)
+{
+	int err;
+
+	err = populate_kprobe_blacklist(__start_kprobe_blacklist,
+					__stop_kprobe_blacklist);
+	if (err) {
+		pr_err("kprobes: failed to populate blacklist: %d\n", err);
+		pr_err("Please take care of using kprobes.\n");
+	}
+
+	return init_kprobes();
+}
+subsys_initcall(init_kprobes_blacklist);
+
+#ifdef CONFIG_DEBUG_FS
+/* kprobes/blacklist -- shows which functions can not be probed */
+static void *kprobe_blacklist_seq_start(struct seq_file *m, loff_t *pos)
+{
+	return seq_list_start(&kprobe_blacklist, *pos);
+}
+
+static void *kprobe_blacklist_seq_next(struct seq_file *m, void *v, loff_t *pos)
+{
+	return seq_list_next(v, &kprobe_blacklist, pos);
+}
+
+static void kprobe_blacklist_seq_stop(struct seq_file *f, void *v)
+{
+	/* Nothing to do */
+}
+
+static int kprobe_blacklist_seq_show(struct seq_file *m, void *v)
+{
+	struct kprobe_blacklist_entry *ent =
+		list_entry(v, struct kprobe_blacklist_entry, list);
+
+	/*
+	 * If /proc/kallsyms is not showing kernel address, we won't
+	 * show them here either.
+	 */
+	if (!kallsyms_show_value())
+		seq_printf(m, "0x%px-0x%px\t%ps\n", NULL, NULL,
+			   (void *)ent->start_addr);
+	else
+		seq_printf(m, "0x%px-0x%px\t%ps\n", (void *)ent->start_addr,
+			   (void *)ent->end_addr, (void *)ent->start_addr);
+	return 0;
+}
+
+static const struct seq_operations kprobe_blacklist_seq_ops = {
+	.start = kprobe_blacklist_seq_start,
+	.next  = kprobe_blacklist_seq_next,
+	.stop  = kprobe_blacklist_seq_stop,
+	.show  = kprobe_blacklist_seq_show,
+};
+
+static int kprobe_blacklist_open(struct inode *inode, struct file *filp)
+{
+	return seq_open(filp, &kprobe_blacklist_seq_ops);
+}
+
+static const struct file_operations debugfs_kprobe_blacklist_ops = {
+	.open           = kprobe_blacklist_open,
+	.read           = seq_read,
+	.llseek         = seq_lseek,
+	.release        = seq_release,
+};
+
+static int __init debugfs_kprobe_blacklist_init(void)
+{
+	struct dentry *dir;
+
+	dir = debugfs_create_dir("kprobes", NULL);
+	debugfs_kprobe_init(dir);
+
+	debugfs_create_file("blacklist", 0400, dir, NULL,
+			    &debugfs_kprobe_blacklist_ops);
+
+	return 0;
+}
+late_initcall(debugfs_kprobe_blacklist_init);
+#endif /* CONFIG_DEBUG_FS */
diff --git a/lib/Kconfig.kgdb b/lib/Kconfig.kgdb
index ffa7a76de086..da372d335882 100644
--- a/lib/Kconfig.kgdb
+++ b/lib/Kconfig.kgdb
@@ -7,6 +7,7 @@ menuconfig KGDB
 	bool "KGDB: kernel debugger"
 	depends on HAVE_ARCH_KGDB
 	depends on DEBUG_KERNEL
+	select KPROBES_BLACKLIST
 	help
 	  If you say Y here, it will be possible to remotely debug the
 	  kernel using gdb.  It is recommended but not required, that
-- 
2.25.4

Re: [RFC PATCH 0/4] kgdb: Honour the kprobe blacklist when setting breakpoints

[Peter Zijlstra]

On Fri, Jun 05, 2020 at 02:21:26PM +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 does intrinsically
> provide 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 ;-) ) but the current no safety rail approach is not easy to
> defend, especially given kprobes provides us with plenty of machinery to
> mark parts of the kernel where breakpointing is discouraged.
> 
> This patchset introduces some safety rails by using the existing
> kprobes infrastructure. It does not cover all locations where
> breakpoints can cause trouble but it will definitely block off several
> avenues, including the architecture specific parts that are handled by
> arch_within_kprobe_blacklist().
> 
> This patch is an RFC because:
> 
> 1. My workstation is still chugging through the compile testing.
> 
> 2. Patch 4 needs more runtime testing.
> 
> 3. The code to extract the kprobe blacklist code (patch 4 again) needs
>    more review especially for its impact on arch specific code.
> 
> To be clear I do plan to do the detailed review of the kprobe blacklist
> stuff but would like to check the direction of travel first since the
> change is already surprisingly big and maybe there's a better way to
> organise things.

Thanks for doing these patches, esp 1-3 look very good to me.

I've taken the liberty to bounce the entire set to Masami-San, who is
the kprobes maintainer for comments as well.

Re: [RFC PATCH 0/4] kgdb: Honour the kprobe blacklist when setting breakpoints

[Peter Zijlstra]

On Fri, Jun 05, 2020 at 04:29:53PM +0200, Peter Zijlstra wrote:
> On Fri, Jun 05, 2020 at 02:21:26PM +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 does intrinsically
> > provide 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 ;-) ) but the current no safety rail approach is not easy to
> > defend, especially given kprobes provides us with plenty of machinery to
> > mark parts of the kernel where breakpointing is discouraged.
> > 
> > This patchset introduces some safety rails by using the existing
> > kprobes infrastructure. It does not cover all locations where
> > breakpoints can cause trouble but it will definitely block off several
> > avenues, including the architecture specific parts that are handled by
> > arch_within_kprobe_blacklist().
> > 
> > This patch is an RFC because:
> > 
> > 1. My workstation is still chugging through the compile testing.
> > 
> > 2. Patch 4 needs more runtime testing.
> > 
> > 3. The code to extract the kprobe blacklist code (patch 4 again) needs
> >    more review especially for its impact on arch specific code.
> > 
> > To be clear I do plan to do the detailed review of the kprobe blacklist
> > stuff but would like to check the direction of travel first since the
> > change is already surprisingly big and maybe there's a better way to
> > organise things.
> 
> Thanks for doing these patches, esp 1-3 look very good to me.
> 
> I've taken the liberty to bounce the entire set to Masami-San, who is
> the kprobes maintainer for comments as well.

OK, after having had a second look, one thing we can perhaps address
with the last patch, or perhaps on top of that, is extending the
kprobes_blacklist() with data regions.

Because these patches only exclude kgdb from setting breakpoints on
code; data breakpoints do not match what we do with
arch_build_bp_info().

Re: [RFC PATCH 0/4] kgdb: Honour the kprobe blacklist when setting breakpoints

[Daniel Thompson]

On Fri, Jun 05, 2020 at 04:29:53PM +0200, Peter Zijlstra wrote:
> On Fri, Jun 05, 2020 at 02:21:26PM +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 does intrinsically
> > provide 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 ;-) ) but the current no safety rail approach is not easy to
> > defend, especially given kprobes provides us with plenty of machinery to
> > mark parts of the kernel where breakpointing is discouraged.
> > 
> > This patchset introduces some safety rails by using the existing
> > kprobes infrastructure. It does not cover all locations where
> > breakpoints can cause trouble but it will definitely block off several
> > avenues, including the architecture specific parts that are handled by
> > arch_within_kprobe_blacklist().
> > 
> > This patch is an RFC because:
> > 
> > 1. My workstation is still chugging through the compile testing.
> > 
> > 2. Patch 4 needs more runtime testing.
> > 
> > 3. The code to extract the kprobe blacklist code (patch 4 again) needs
> >    more review especially for its impact on arch specific code.
> > 
> > To be clear I do plan to do the detailed review of the kprobe blacklist
> > stuff but would like to check the direction of travel first since the
> > change is already surprisingly big and maybe there's a better way to
> > organise things.
> 
> Thanks for doing these patches, esp 1-3 look very good to me.
> 
> I've taken the liberty to bounce the entire set to Masami-San, who is
> the kprobes maintainer for comments as well.

Not a liberty... leaving out Masami-san was an oversight on my part.
Thanks for connecting!


Daniel.

Re: [RFC PATCH 0/4] kgdb: Honour the kprobe blacklist when setting breakpoints

[Daniel Thompson]

On Fri, Jun 05, 2020 at 04:44:57PM +0200, Peter Zijlstra wrote:
> On Fri, Jun 05, 2020 at 04:29:53PM +0200, Peter Zijlstra wrote:
> > On Fri, Jun 05, 2020 at 02:21:26PM +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 does intrinsically
> > > provide 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 ;-) ) but the current no safety rail approach is not easy to
> > > defend, especially given kprobes provides us with plenty of machinery to
> > > mark parts of the kernel where breakpointing is discouraged.
> > > 
> > > This patchset introduces some safety rails by using the existing
> > > kprobes infrastructure. It does not cover all locations where
> > > breakpoints can cause trouble but it will definitely block off several
> > > avenues, including the architecture specific parts that are handled by
> > > arch_within_kprobe_blacklist().
> > > 
> > > This patch is an RFC because:
> > > 
> > > 1. My workstation is still chugging through the compile testing.
> > > 
> > > 2. Patch 4 needs more runtime testing.
> > > 
> > > 3. The code to extract the kprobe blacklist code (patch 4 again) needs
> > >    more review especially for its impact on arch specific code.
> > > 
> > > To be clear I do plan to do the detailed review of the kprobe blacklist
> > > stuff but would like to check the direction of travel first since the
> > > change is already surprisingly big and maybe there's a better way to
> > > organise things.
> > 
> > Thanks for doing these patches, esp 1-3 look very good to me.
> > 
> > I've taken the liberty to bounce the entire set to Masami-San, who is
> > the kprobes maintainer for comments as well.
> 
> OK, after having had a second look, one thing we can perhaps address
> with the last patch, or perhaps on top of that, is extending the
> kprobes_blacklist() with data regions.
> 
> Because these patches only exclude kgdb from setting breakpoints on
> code; data breakpoints do not match what we do with
> arch_build_bp_info().

Right, my patches will only affect the code paths where kgdb sets
software breakpoints.

In principle h/w breakpoints, whether they are code or data, should be
able to rely on hw_breakpoint_arch_parse() and any errors from the hw
breakpoint API will propagate into the kgdb core and do the right
thing.

In practice it looks like kgdb/x86/hw_breakpoint have plumbed together
in a manner that circumvents the parse (and therefore#
arch_build_bp_info() never runs). Rather the h/w/ break problem using
the kprobe blacklist I think we could just fix these code paths.

The following is 100% untested (not even compile) and I copied a line
or two from register_perf_hw_breakpoint() without checking what they
do ;-). Nevertheless I hope it gives a clear idea about what I am
talking about! If this was developed into a "real" patch then I think
dbg_release_bp_slot() should perhaps be replaced with a better API that
doesn't bypass the checks rather than solving everything in kgdb.c .


Daniel.


diff --git a/arch/x86/kernel/kgdb.c b/arch/x86/kernel/kgdb.c
index c44fe7d8d9a4..64ac0ee9b55c 100644
--- a/arch/x86/kernel/kgdb.c
+++ b/arch/x86/kernel/kgdb.c
@@ -223,11 +223,12 @@ static void kgdb_correct_hw_break(void)
 		hw_breakpoint_restore();
 }
 
-static int hw_break_reserve_slot(int breakno)
+static int kgdb_register_hw_break(int breakno)
 {
 	int cpu;
 	int cnt = 0;
 	struct perf_event **pevent;
+        struct arch_hw_breakpoint hw = { };
 
 	if (dbg_is_early)
 		return 0;
@@ -237,6 +238,11 @@ static int hw_break_reserve_slot(int breakno)
 		pevent = per_cpu_ptr(breakinfo[breakno].pev, cpu);
 		if (dbg_reserve_bp_slot(*pevent))
 			goto fail;
+		if (hw_breakpoint_arch_parse(*pevent, &(*pevent)->attr, hw)) {
+			cnt++;
+			goto fail;
+		}
+		(*pevent)->hw.info = hw;
 	}
 
 	return 0;
@@ -361,7 +367,7 @@ kgdb_set_hw_break(unsigned long addr, int len, enum kgdb_bptype bptype)
 		return -1;
 	}
 	breakinfo[i].addr = addr;
-	if (hw_break_reserve_slot(i)) {
+	if (kgdb_register_hw_break(i)) {
 		breakinfo[i].addr = 0;
 		return -1;
 	}

Re: [RFC PATCH 0/4] kgdb: Honour the kprobe blacklist when setting breakpoints

[Masami Hiramatsu]

On Fri, 5 Jun 2020 16:29:53 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Fri, Jun 05, 2020 at 02:21:26PM +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 does intrinsically
> > provide 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 ;-) ) but the current no safety rail approach is not easy to
> > defend, especially given kprobes provides us with plenty of machinery to
> > mark parts of the kernel where breakpointing is discouraged.
> > 
> > This patchset introduces some safety rails by using the existing
> > kprobes infrastructure. It does not cover all locations where
> > breakpoints can cause trouble but it will definitely block off several
> > avenues, including the architecture specific parts that are handled by
> > arch_within_kprobe_blacklist().
> > 
> > This patch is an RFC because:
> > 
> > 1. My workstation is still chugging through the compile testing.
> > 
> > 2. Patch 4 needs more runtime testing.
> > 
> > 3. The code to extract the kprobe blacklist code (patch 4 again) needs
> >    more review especially for its impact on arch specific code.
> > 
> > To be clear I do plan to do the detailed review of the kprobe blacklist
> > stuff but would like to check the direction of travel first since the
> > change is already surprisingly big and maybe there's a better way to
> > organise things.
> 
> Thanks for doing these patches, esp 1-3 look very good to me.
> 
> I've taken the liberty to bounce the entire set to Masami-San, who is
> the kprobes maintainer for comments as well.

Thanks Peter to Cc me.

Reusing kprobe blacklist is good to me as far as it doesn't expand it
only for kgdb. For example, if a function which can cause a recursive
trap issue only when the kgdb puts a breakpoint, it should be covered
by kgdb blacklist, or we should make a "noinstr-list" including
both :)

Thus, Nack for PATCH 4/4, that seems a bit selfish change. If kgdb wants
to use the "kprobes blacklist", we should make CONFIG_KGDB depending on
CONFIG_KPROBES. Or, (as I pointed) we should make independent "noinstr-list"
and use it from both.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [RFC PATCH 3/4] kgdb: Add NOKPROBE labels on the trap handler functions

[Masami Hiramatsu]

On Fri,  5 Jun 2020 14:21:29 +0100
Daniel Thompson <daniel.thompson@linaro.org> wrote:

> Currently kgdb honours the kprobe blacklist but doesn't place its own
> trap handling code on the list. Add macros 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
> blacklist risks reducing the capabilities of kprobe and this is 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 | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/debug/debug_core.c b/kernel/debug/debug_core.c
> index 4a2df4509fe1..21d1d91da4bb 100644
> --- a/kernel/debug/debug_core.c
> +++ b/kernel/debug/debug_core.c
> @@ -184,6 +184,7 @@ int __weak kgdb_arch_remove_breakpoint(struct kgdb_bkpt *bpt)
>  	return probe_kernel_write((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)
>  {
> @@ -321,6 +322,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:
> @@ -411,6 +413,7 @@ int dbg_deactivate_sw_breakpoints(void)
>  	}
>  	return ret;
>  }
> +NOKPROBE_SYMBOL(dbg_deactivate_sw_breakpoints);
>  
>  int dbg_remove_sw_break(unsigned long addr)
>  {
> @@ -567,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)
>  {
> @@ -801,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
> @@ -845,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.
> @@ -879,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)
> @@ -904,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)
> @@ -1204,7 +1212,6 @@ noinline void kgdb_breakpoint(void)
>  	atomic_dec(&kgdb_setting_breakpoint);
>  }
>  EXPORT_SYMBOL_GPL(kgdb_breakpoint);
> -NOKPROBE_SYMBOL(kgdb_breakpoint);

BTW, why did you mark this NOKPROBE in 2/4 and remove it 3/4 again?

Thank you,


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [RFC PATCH 0/4] kgdb: Honour the kprobe blacklist when setting breakpoints

[Daniel Thompson]

On Thu, Jun 11, 2020 at 09:42:09PM +0900, Masami Hiramatsu wrote:
> On Fri, 5 Jun 2020 16:29:53 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Fri, Jun 05, 2020 at 02:21:26PM +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 does intrinsically
> > > provide 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 ;-) ) but the current no safety rail approach is not easy to
> > > defend, especially given kprobes provides us with plenty of machinery to
> > > mark parts of the kernel where breakpointing is discouraged.
> > > 
> > > This patchset introduces some safety rails by using the existing
> > > kprobes infrastructure. It does not cover all locations where
> > > breakpoints can cause trouble but it will definitely block off several
> > > avenues, including the architecture specific parts that are handled by
> > > arch_within_kprobe_blacklist().
> > > 
> > > This patch is an RFC because:
> > > 
> > > 1. My workstation is still chugging through the compile testing.
> > > 
> > > 2. Patch 4 needs more runtime testing.
> > > 
> > > 3. The code to extract the kprobe blacklist code (patch 4 again) needs
> > >    more review especially for its impact on arch specific code.
> > > 
> > > To be clear I do plan to do the detailed review of the kprobe blacklist
> > > stuff but would like to check the direction of travel first since the
> > > change is already surprisingly big and maybe there's a better way to
> > > organise things.
> > 
> > Thanks for doing these patches, esp 1-3 look very good to me.
> > 
> > I've taken the liberty to bounce the entire set to Masami-San, who is
> > the kprobes maintainer for comments as well.
> 
> Thanks Peter to Cc me.
> 
> Reusing kprobe blacklist is good to me as far as it doesn't expand it
> only for kgdb. For example, if a function which can cause a recursive
> trap issue only when the kgdb puts a breakpoint, it should be covered
> by kgdb blacklist, or we should make a "noinstr-list" including
> both :)

Recursion is what focuses the mind but I don't think we'd need
recursion for there to be problems.

For example taking a kprobe trap whilst executing the kgdb trap handler
(or vice versa) is already likely to be fragile and is almost certainly
untested on most or all architectures. Further if I understood Peter's
original nudge correctly then, in addition, x86 plans to explicitly
prohibit this anyway.

On other words I think there will only be one blacklist.


> Thus, Nack for PATCH 4/4, that seems a bit selfish change. If kgdb wants
> to use the "kprobes blacklist", we should make CONFIG_KGDB depending on
> CONFIG_KPROBES.

Some of the architectures currently have kgdb support but do not have
kprobes...


> Or, (as I pointed) we should make independent "noinstr-list"
> and use it from both.

This sounds like this wouldn't really be a functional change over
what I have proposed. More like augmenting it with a massive symbol
rename (and maybe a little bit of extra code movement in the headers
to give us linux/noinstr.h).

Taking my cues from things like set_fs() I originally decided to keep
away from such a big rename ;-) .

Personally I'm open to a rename. I could write PATCH 4/4 assuming a
rename will come (e.g. different naming for new files and Kconfig
options) and follow that with an automatically generated
rename patch (or patches).


Daniel.

Re: [RFC PATCH 0/4] kgdb: Honour the kprobe blacklist when setting breakpoints

[Masami Hiramatsu]

On Thu, 11 Jun 2020 15:32:40 +0100
Daniel Thompson <daniel.thompson@linaro.org> wrote:

> On Thu, Jun 11, 2020 at 09:42:09PM +0900, Masami Hiramatsu wrote:
> > On Fri, 5 Jun 2020 16:29:53 +0200
> > Peter Zijlstra <peterz@infradead.org> wrote:
> > 
> > > On Fri, Jun 05, 2020 at 02:21:26PM +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 does intrinsically
> > > > provide 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 ;-) ) but the current no safety rail approach is not easy to
> > > > defend, especially given kprobes provides us with plenty of machinery to
> > > > mark parts of the kernel where breakpointing is discouraged.
> > > > 
> > > > This patchset introduces some safety rails by using the existing
> > > > kprobes infrastructure. It does not cover all locations where
> > > > breakpoints can cause trouble but it will definitely block off several
> > > > avenues, including the architecture specific parts that are handled by
> > > > arch_within_kprobe_blacklist().
> > > > 
> > > > This patch is an RFC because:
> > > > 
> > > > 1. My workstation is still chugging through the compile testing.
> > > > 
> > > > 2. Patch 4 needs more runtime testing.
> > > > 
> > > > 3. The code to extract the kprobe blacklist code (patch 4 again) needs
> > > >    more review especially for its impact on arch specific code.
> > > > 
> > > > To be clear I do plan to do the detailed review of the kprobe blacklist
> > > > stuff but would like to check the direction of travel first since the
> > > > change is already surprisingly big and maybe there's a better way to
> > > > organise things.
> > > 
> > > Thanks for doing these patches, esp 1-3 look very good to me.
> > > 
> > > I've taken the liberty to bounce the entire set to Masami-San, who is
> > > the kprobes maintainer for comments as well.
> > 
> > Thanks Peter to Cc me.
> > 
> > Reusing kprobe blacklist is good to me as far as it doesn't expand it
> > only for kgdb. For example, if a function which can cause a recursive
> > trap issue only when the kgdb puts a breakpoint, it should be covered
> > by kgdb blacklist, or we should make a "noinstr-list" including
> > both :)
> 
> Recursion is what focuses the mind but I don't think we'd need
> recursion for there to be problems.
> 
> For example taking a kprobe trap whilst executing the kgdb trap handler
> (or vice versa) is already likely to be fragile and is almost certainly
> untested on most or all architectures.

Ah, OK. Actually, on x86 that is not a problem (it can handle recursive int3
if software handles it correctly), but other arch may not accept it.
Hmm, then using NOKPROBE_SYMBOL() is reasonable.

> Further if I understood Peter's
> original nudge correctly then, in addition, x86 plans to explicitly
> prohibit this anyway.
> 
> On other words I think there will only be one blacklist.

Agreed.

> > Thus, Nack for PATCH 4/4, that seems a bit selfish change. If kgdb wants
> > to use the "kprobes blacklist", we should make CONFIG_KGDB depending on
> > CONFIG_KPROBES.
> 
> Some of the architectures currently have kgdb support but do not have
> kprobes...

"depends on KPROBES if HAVE_KPROBES" ? :-)

(Anyway, it is a good chance to port kprobe on such arch...)

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [RFC PATCH 0/4] kgdb: Honour the kprobe blacklist when setting breakpoints

[Daniel Thompson]

On Fri, Jun 12, 2020 at 07:13:49PM +0900, Masami Hiramatsu wrote:
> On Thu, 11 Jun 2020 15:32:40 +0100
> Daniel Thompson <daniel.thompson@linaro.org> wrote:
> 
> > On Thu, Jun 11, 2020 at 09:42:09PM +0900, Masami Hiramatsu wrote:
> > > On Fri, 5 Jun 2020 16:29:53 +0200
> > > Peter Zijlstra <peterz@infradead.org> wrote:
> > > 
> > > > On Fri, Jun 05, 2020 at 02:21:26PM +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 does intrinsically
> > > > > provide 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 ;-) ) but the current no safety rail approach is not easy to
> > > > > defend, especially given kprobes provides us with plenty of machinery to
> > > > > mark parts of the kernel where breakpointing is discouraged.
> > > > > 
> > > > > This patchset introduces some safety rails by using the existing
> > > > > kprobes infrastructure. It does not cover all locations where
> > > > > breakpoints can cause trouble but it will definitely block off several
> > > > > avenues, including the architecture specific parts that are handled by
> > > > > arch_within_kprobe_blacklist().
> > > > > 
> > > > > This patch is an RFC because:
> > > > > 
> > > > > 1. My workstation is still chugging through the compile testing.
> > > > > 
> > > > > 2. Patch 4 needs more runtime testing.
> > > > > 
> > > > > 3. The code to extract the kprobe blacklist code (patch 4 again) needs
> > > > >    more review especially for its impact on arch specific code.
> > > > > 
> > > > > To be clear I do plan to do the detailed review of the kprobe blacklist
> > > > > stuff but would like to check the direction of travel first since the
> > > > > change is already surprisingly big and maybe there's a better way to
> > > > > organise things.
> > > > 
> > > > Thanks for doing these patches, esp 1-3 look very good to me.
> > > > 
> > > > I've taken the liberty to bounce the entire set to Masami-San, who is
> > > > the kprobes maintainer for comments as well.
> > > 
> > > Thanks Peter to Cc me.
> > > 
> > > Reusing kprobe blacklist is good to me as far as it doesn't expand it
> > > only for kgdb. For example, if a function which can cause a recursive
> > > trap issue only when the kgdb puts a breakpoint, it should be covered
> > > by kgdb blacklist, or we should make a "noinstr-list" including
> > > both :)
> > 
> > Recursion is what focuses the mind but I don't think we'd need
> > recursion for there to be problems.
> > 
> > For example taking a kprobe trap whilst executing the kgdb trap handler
> > (or vice versa) is already likely to be fragile and is almost certainly
> > untested on most or all architectures.
> 
> Ah, OK. Actually, on x86 that is not a problem (it can handle recursive int3
> if software handles it correctly), but other arch may not accept it.
> Hmm, then using NOKPROBE_SYMBOL() is reasonable.
> 
> > Further if I understood Peter's
> > original nudge correctly then, in addition, x86 plans to explicitly
> > prohibit this anyway.
> > 
> > On other words I think there will only be one blacklist.
> 
> Agreed.
> 
> > > Thus, Nack for PATCH 4/4, that seems a bit selfish change. If kgdb wants
> > > to use the "kprobes blacklist", we should make CONFIG_KGDB depending on
> > > CONFIG_KPROBES.
> > 
> > Some of the architectures currently have kgdb support but do not have
> > kprobes...
> 
> "depends on KPROBES if HAVE_KPROBES" ? :-)

I'm personally be OK with something like:

#ifndef CONFIG_KGDB_ALLOW_UNSAFE_BREAKPOINTS
  if (within_kprobe_blacklist())
    ...
#endif

And then setup Kconfig so that KGDB_ALLOW_UNSAFE_BREAKPOINTS
defaults to n and add a extra check to put a warning in dmesg
that breakpoints are disabled.


> (Anyway, it is a good chance to port kprobe on such arch...)

I like kprobes very much... but not quite enough to want to
implement it on architectures I don't use ;-).


Daniel.