[PATCH v2 0/9] kprobes: Fix and improve blacklist symbols

[PATCH v2 0/9] kprobes: Fix and improve blacklist symbols

[Masami Hiramatsu]

Hi,

Here is the v2 series of kprobes blacklist bugfix and improvements mainly
on x86 (since I started testing on qemu-x86).

>From v1, I just removed stable-ml from Cc (but tagged [1/9]) and added
Steve's Ack.

This has been started from discussion about KPROBE_ENENTS_ON_NOTRACE
configuration. I tried to find notrace functions which can cause kernel
crash with kprobes using following script.

====
#!/bin/sh

i=0;
cat notrace_functions | while read f ; do
  if echo p:event$i $f >> /sys/kernel/debug/tracing/kprobe_events; then
     echo "Probing on $f"
     echo 1 > /sys/kernel/debug/tracing/events/kprobes/event$i/enable 
  fi
  i=$((i+1))
done
====

And I found several functions which must be blacklisted.
 - optprobe template code, which is just a template code and
   never be executed. Moreover, since it can be copied and
   reused, if we probe it, it modifies the template code and
   can cause a crash. ([1/9][2/9])
 - functions which is called before kprobe_int3_handler()
   handles kprobes. This can cause a breakpoint recursion. ([3/9])
 - IRQ entry text, which should not be probed since register/pagetable
   status has not been stable at that point. ([4/9])
 - Suffixed symbols, like .constprop, .part etc. Those suffixed
   symbols never be blacklisted even if the non-suffixed version
   has been blacklisted. ([5/9])
 - hardirq tracer also works before int3 handling. ([6/9])
 - preempt_check debug function also is involved in int3 handling.
   ([7/9])
 - RCU debug routine is also called before kprobe_int3_handler().
   ([8/9])
 - Some lockdep functions are also involved in int3 handling.
   ([9/9])

Of course there still may be some functions which can be called
by configuration change, I'll continue to test it.

Thank you,

---

Masami Hiramatsu (9):
      x86/kprobes: Prohibit probing on optprobe template code
      x86/kprobes: Move trampoline code into RODATA
      x86/kprobes: Prohibit probing on functions before kprobe_int3_handler()
      x86/kprobes: Prohibit probing on IRQ handlers directly
      kprobes: Search non-suffixed symbol in blacklist
      kprobes: Prohibit probing on hardirq tracers
      kprobes: Prohibit probing on preempt_check debug functions
      kprobes: Prohibit probing on RCU debug routine
      kprobes: Prohibit probing on lockdep functions


 arch/x86/kernel/alternative.c   |    3 ++-
 arch/x86/kernel/ftrace.c        |    3 ++-
 arch/x86/kernel/kprobes/core.c  |    7 +++++++
 arch/x86/kernel/kprobes/opt.c   |    4 ++--
 arch/x86/kernel/traps.c         |    1 +
 kernel/kprobes.c                |   21 ++++++++++++++++++++-
 kernel/locking/lockdep.c        |    7 ++++++-
 kernel/rcu/tree.c               |    2 ++
 kernel/rcu/update.c             |    2 ++
 kernel/trace/trace_irqsoff.c    |    9 +++++++--
 kernel/trace/trace_preemptirq.c |    5 +++++
 lib/smp_processor_id.c          |    7 +++++--
 12 files changed, 61 insertions(+), 10 deletions(-)

-- 
Masami Hiramatsu (Linaro) <mhiramat@kernel.org>

[PATCH v2 1/9] x86/kprobes: Prohibit probing on optprobe template code

[Masami Hiramatsu]

Prohibit probing on optprobe template code, since it is not
a code but a template instruction sequence. If we modify
this template, copied template must be broken.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Fixes: 9326638cbee2 ("kprobes, x86: Use NOKPROBE_SYMBOL() instead of __kprobes annotation")
Cc: stable@vger.kernel.org
---
 arch/x86/kernel/kprobes/opt.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c
index 6adf6e6c2933..544bd41a514c 100644
--- a/arch/x86/kernel/kprobes/opt.c
+++ b/arch/x86/kernel/kprobes/opt.c
@@ -141,6 +141,11 @@ asm (
 
 void optprobe_template_func(void);
 STACK_FRAME_NON_STANDARD(optprobe_template_func);
+NOKPROBE_SYMBOL(optprobe_template_func);
+NOKPROBE_SYMBOL(optprobe_template_entry);
+NOKPROBE_SYMBOL(optprobe_template_val);
+NOKPROBE_SYMBOL(optprobe_template_call);
+NOKPROBE_SYMBOL(optprobe_template_end);
 
 #define TMPL_MOVE_IDX \
 	((long)optprobe_template_val - (long)optprobe_template_entry)

[PATCH v2 2/9] x86/kprobes: Move trampoline code into RODATA

[Masami Hiramatsu]

Move optprobe trampoline code into RODATA since it is
not executed, but copied and modified to be used on
a trampoline buffer.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/x86/kernel/kprobes/opt.c |    9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/kprobes/opt.c b/arch/x86/kernel/kprobes/opt.c
index 544bd41a514c..f14262952015 100644
--- a/arch/x86/kernel/kprobes/opt.c
+++ b/arch/x86/kernel/kprobes/opt.c
@@ -97,6 +97,7 @@ static void synthesize_set_arg1(kprobe_opcode_t *addr, unsigned long val)
 }
 
 asm (
+			".pushsection .rodata\n"
 			"optprobe_template_func:\n"
 			".global optprobe_template_entry\n"
 			"optprobe_template_entry:\n"
@@ -136,16 +137,10 @@ asm (
 #endif
 			".global optprobe_template_end\n"
 			"optprobe_template_end:\n"
-			".type optprobe_template_func, @function\n"
-			".size optprobe_template_func, .-optprobe_template_func\n");
+			".popsection\n");
 
 void optprobe_template_func(void);
 STACK_FRAME_NON_STANDARD(optprobe_template_func);
-NOKPROBE_SYMBOL(optprobe_template_func);
-NOKPROBE_SYMBOL(optprobe_template_entry);
-NOKPROBE_SYMBOL(optprobe_template_val);
-NOKPROBE_SYMBOL(optprobe_template_call);
-NOKPROBE_SYMBOL(optprobe_template_end);
 
 #define TMPL_MOVE_IDX \
 	((long)optprobe_template_val - (long)optprobe_template_entry)

[PATCH v2 3/9] x86/kprobes: Prohibit probing on functions before kprobe_int3_handler()

[Masami Hiramatsu]

Prohibit probing on the functions called before kprobe_int3_handler()
in do_int3(). More specifically, ftrace_int3_handler(),
poke_int3_handler(), and ist_enter(). And since rcu_nmi_enter() is
called by ist_enter(), it also should be marked as NOKPROBE_SYMBOL.

Since those are handled before kprobe_int3_handler(), probing those
functions can cause a breakpoint recursion and crash the kernel.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/x86/kernel/alternative.c |    3 ++-
 arch/x86/kernel/ftrace.c      |    3 ++-
 arch/x86/kernel/traps.c       |    1 +
 kernel/rcu/tree.c             |    2 ++
 4 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index ebeac487a20c..e8b628b1b279 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -11,6 +11,7 @@
 #include <linux/stop_machine.h>
 #include <linux/slab.h>
 #include <linux/kdebug.h>
+#include <linux/kprobes.h>
 #include <asm/text-patching.h>
 #include <asm/alternative.h>
 #include <asm/sections.h>
@@ -764,8 +765,8 @@ int poke_int3_handler(struct pt_regs *regs)
 	regs->ip = (unsigned long) bp_int3_handler;
 
 	return 1;
-
 }
+NOKPROBE_SYMBOL(poke_int3_handler);
 
 /**
  * text_poke_bp() -- update instructions on live kernel on SMP
diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 7ee8067cbf45..22a548919228 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -269,7 +269,7 @@ int ftrace_update_ftrace_func(ftrace_func_t func)
 	return ret;
 }
 
-static int is_ftrace_caller(unsigned long ip)
+static nokprobe_inline int is_ftrace_caller(unsigned long ip)
 {
 	if (ip == ftrace_update_func)
 		return 1;
@@ -299,6 +299,7 @@ int ftrace_int3_handler(struct pt_regs *regs)
 
 	return 1;
 }
+NOKPROBE_SYMBOL(ftrace_int3_handler);
 
 static int ftrace_write(unsigned long ip, const char *val, int size)
 {
diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
index 9b7c4ca8f0a7..e289ce1332ab 100644
--- a/arch/x86/kernel/traps.c
+++ b/arch/x86/kernel/traps.c
@@ -111,6 +111,7 @@ void ist_enter(struct pt_regs *regs)
 	/* This code is a bit fragile.  Test it. */
 	RCU_LOCKDEP_WARN(!rcu_is_watching(), "ist_enter didn't work");
 }
+NOKPROBE_SYMBOL(ist_enter);
 
 void ist_exit(struct pt_regs *regs)
 {
diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 9180158756d2..74db52a0a466 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -62,6 +62,7 @@
 #include <linux/suspend.h>
 #include <linux/ftrace.h>
 #include <linux/tick.h>
+#include <linux/kprobes.h>
 
 #include "tree.h"
 #include "rcu.h"
@@ -872,6 +873,7 @@ void rcu_nmi_enter(void)
 {
 	rcu_nmi_enter_common(false);
 }
+NOKPROBE_SYMBOL(rcu_nmi_enter);
 
 /**
  * rcu_irq_enter - inform RCU that current CPU is entering irq away from idle

[PATCH v2 4/9] x86/kprobes: Prohibit probing on IRQ handlers directly

[Masami Hiramatsu]

Prohibit probing on IRQ handlers in irqentry_text because
if it interrupts user mode, at that point we haven't changed
to kernel space yet and which eventually leads a double fault.
E.g.

 # echo p apic_timer_interrupt > kprobe_events
 # echo 1 > events/kprobes/enable
 PANIC: double fault, error_code: 0x0
 CPU: 1 PID: 814 Comm: less Not tainted 4.20.0-rc3+ #30
 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996)
 RIP: 0010:error_entry+0x12/0xf0
 [snip]
 Call Trace:
  <ENTRY_TRAMPOLINE>
  ? native_iret+0x7/0x7
  ? async_page_fault+0x8/0x30
  ? trace_hardirqs_on_thunk+0x1c/0x1c
  ? error_entry+0x7c/0xf0
  ? async_page_fault+0x8/0x30
  ? native_iret+0x7/0x7
  ? int3+0xa/0x20
  ? trace_hardirqs_on_thunk+0x1c/0x1c
  ? error_entry+0x7c/0xf0
  ? int3+0xa/0x20
  ? apic_timer_interrupt+0x1/0x20
  </ENTRY_TRAMPOLINE>
 Kernel panic - not syncing: Machine halted.
 Kernel Offset: disabled
 ---[ end Kernel panic - not syncing: Machine halted. ]---

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/x86/kernel/kprobes/core.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index f4b954ff5b89..fed46ddb1eef 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -1072,6 +1072,13 @@ NOKPROBE_SYMBOL(kprobe_fault_handler);
 
 int __init arch_populate_kprobe_blacklist(void)
 {
+	int ret;
+
+	ret = kprobe_add_area_blacklist((unsigned long)__irqentry_text_start,
+					 (unsigned long)__irqentry_text_end);
+	if (ret)
+		return ret;
+
 	return kprobe_add_area_blacklist((unsigned long)__entry_text_start,
 					 (unsigned long)__entry_text_end);
 }

[PATCH v2 5/9] kprobes: Search non-suffixed symbol in blacklist

[Masami Hiramatsu]

Newer gcc can generate some different instances of a function
with suffixed symbols if the function is optimized and only
has a part of that. (e.g. .constprop, .part etc.)

In this case, it is not enough to check the entry of kprobe
blacklist because it only records non-suffixed symbol address.

To fix this issue, search non-suffixed symbol in blacklist if
given address is within a symbol which has a suffix.

Note that this can cause false positive cases if a kprobe-safe
function is optimized to suffixed instance and has same name
symbol which is blacklisted.
But I would like to chose a fail-safe design for this issue.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 kernel/kprobes.c |   21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index e8c76164f541..faa519f07aad 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1396,7 +1396,7 @@ bool __weak arch_within_kprobe_blacklist(unsigned long addr)
 	       addr < (unsigned long)__kprobes_text_end;
 }
 
-bool within_kprobe_blacklist(unsigned long addr)
+static bool __within_kprobe_blacklist(unsigned long addr)
 {
 	struct kprobe_blacklist_entry *ent;
 
@@ -1410,7 +1410,26 @@ bool within_kprobe_blacklist(unsigned long addr)
 		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;
 }
 

[PATCH v2 6/9] kprobes: Prohibit probing on hardirq tracers

[Masami Hiramatsu]

Since kprobes breakpoint handling involves hardirq tracer,
probing these functions cause breakpoint recursion problem.

Prohibit probing on those functions.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/trace_irqsoff.c    |    9 +++++++--
 kernel/trace/trace_preemptirq.c |    5 +++++
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace_irqsoff.c b/kernel/trace/trace_irqsoff.c
index 98ea6d28df15..829709bfec3d 100644
--- a/kernel/trace/trace_irqsoff.c
+++ b/kernel/trace/trace_irqsoff.c
@@ -14,6 +14,7 @@
 #include <linux/uaccess.h>
 #include <linux/module.h>
 #include <linux/ftrace.h>
+#include <linux/kprobes.h>
 
 #include "trace.h"
 
@@ -368,7 +369,7 @@ check_critical_timing(struct trace_array *tr,
 	__trace_function(tr, CALLER_ADDR0, parent_ip, flags, pc);
 }
 
-static inline void
+static nokprobe_inline void
 start_critical_timing(unsigned long ip, unsigned long parent_ip, int pc)
 {
 	int cpu;
@@ -404,7 +405,7 @@ start_critical_timing(unsigned long ip, unsigned long parent_ip, int pc)
 	atomic_dec(&data->disabled);
 }
 
-static inline void
+static nokprobe_inline void
 stop_critical_timing(unsigned long ip, unsigned long parent_ip, int pc)
 {
 	int cpu;
@@ -446,6 +447,7 @@ void start_critical_timings(void)
 		start_critical_timing(CALLER_ADDR0, CALLER_ADDR1, pc);
 }
 EXPORT_SYMBOL_GPL(start_critical_timings);
+NOKPROBE_SYMBOL(start_critical_timings);
 
 void stop_critical_timings(void)
 {
@@ -455,6 +457,7 @@ void stop_critical_timings(void)
 		stop_critical_timing(CALLER_ADDR0, CALLER_ADDR1, pc);
 }
 EXPORT_SYMBOL_GPL(stop_critical_timings);
+NOKPROBE_SYMBOL(stop_critical_timings);
 
 #ifdef CONFIG_FUNCTION_TRACER
 static bool function_enabled;
@@ -615,6 +618,7 @@ void tracer_hardirqs_on(unsigned long a0, unsigned long a1)
 	if (!preempt_trace(pc) && irq_trace())
 		stop_critical_timing(a0, a1, pc);
 }
+NOKPROBE_SYMBOL(tracer_hardirqs_on);
 
 void tracer_hardirqs_off(unsigned long a0, unsigned long a1)
 {
@@ -623,6 +627,7 @@ void tracer_hardirqs_off(unsigned long a0, unsigned long a1)
 	if (!preempt_trace(pc) && irq_trace())
 		start_critical_timing(a0, a1, pc);
 }
+NOKPROBE_SYMBOL(tracer_hardirqs_off);
 
 static int irqsoff_tracer_init(struct trace_array *tr)
 {
diff --git a/kernel/trace/trace_preemptirq.c b/kernel/trace/trace_preemptirq.c
index 71f553cceb3c..4d8e99fdbbbe 100644
--- a/kernel/trace/trace_preemptirq.c
+++ b/kernel/trace/trace_preemptirq.c
@@ -9,6 +9,7 @@
 #include <linux/uaccess.h>
 #include <linux/module.h>
 #include <linux/ftrace.h>
+#include <linux/kprobes.h>
 #include "trace.h"
 
 #define CREATE_TRACE_POINTS
@@ -30,6 +31,7 @@ void trace_hardirqs_on(void)
 	lockdep_hardirqs_on(CALLER_ADDR0);
 }
 EXPORT_SYMBOL(trace_hardirqs_on);
+NOKPROBE_SYMBOL(trace_hardirqs_on);
 
 void trace_hardirqs_off(void)
 {
@@ -43,6 +45,7 @@ void trace_hardirqs_off(void)
 	lockdep_hardirqs_off(CALLER_ADDR0);
 }
 EXPORT_SYMBOL(trace_hardirqs_off);
+NOKPROBE_SYMBOL(trace_hardirqs_off);
 
 __visible void trace_hardirqs_on_caller(unsigned long caller_addr)
 {
@@ -56,6 +59,7 @@ __visible void trace_hardirqs_on_caller(unsigned long caller_addr)
 	lockdep_hardirqs_on(CALLER_ADDR0);
 }
 EXPORT_SYMBOL(trace_hardirqs_on_caller);
+NOKPROBE_SYMBOL(trace_hardirqs_on_caller);
 
 __visible void trace_hardirqs_off_caller(unsigned long caller_addr)
 {
@@ -69,6 +73,7 @@ __visible void trace_hardirqs_off_caller(unsigned long caller_addr)
 	lockdep_hardirqs_off(CALLER_ADDR0);
 }
 EXPORT_SYMBOL(trace_hardirqs_off_caller);
+NOKPROBE_SYMBOL(trace_hardirqs_off_caller);
 #endif /* CONFIG_TRACE_IRQFLAGS */
 
 #ifdef CONFIG_TRACE_PREEMPT_TOGGLE

[PATCH v2 7/9] kprobes: Prohibit probing on preempt_check debug functions

[Masami Hiramatsu]

Since kprobes depends on preempt disable/enable, probing
on the preempt debug routine can cause recursive breakpoint
problem.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 lib/smp_processor_id.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lib/smp_processor_id.c b/lib/smp_processor_id.c
index 85925aaa4fff..157d9e31f6c2 100644
--- a/lib/smp_processor_id.c
+++ b/lib/smp_processor_id.c
@@ -5,10 +5,11 @@
  * DEBUG_PREEMPT variant of smp_processor_id().
  */
 #include <linux/export.h>
+#include <linux/kprobes.h>
 #include <linux/sched.h>
 
-notrace static unsigned int check_preemption_disabled(const char *what1,
-							const char *what2)
+notrace static nokprobe_inline
+unsigned int check_preemption_disabled(const char *what1, const char *what2)
 {
 	int this_cpu = raw_smp_processor_id();
 
@@ -56,9 +57,11 @@ notrace unsigned int debug_smp_processor_id(void)
 	return check_preemption_disabled("smp_processor_id", "");
 }
 EXPORT_SYMBOL(debug_smp_processor_id);
+NOKPROBE_SYMBOL(debug_smp_processor_id);
 
 notrace void __this_cpu_preempt_check(const char *op)
 {
 	check_preemption_disabled("__this_cpu_", op);
 }
 EXPORT_SYMBOL(__this_cpu_preempt_check);
+NOKPROBE_SYMBOL(__this_cpu_preempt_check);

[PATCH v2 8/9] kprobes: Prohibit probing on RCU debug routine

[Masami Hiramatsu]

Since kprobe itself depends on RCU, probing on RCU debug
routine can cause recursive breakpoint problem.
Prohibit probing on RCU debug routines.

int3
 ->do_int3()
   ->ist_enter()
     ->RCU_LOCKDEP_WARN()
       ->debug_lockdep_rcu_enabled() -> int3

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 kernel/rcu/update.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
index 1971869c4072..f4ca36d92138 100644
--- a/kernel/rcu/update.c
+++ b/kernel/rcu/update.c
@@ -52,6 +52,7 @@
 #include <linux/tick.h>
 #include <linux/rcupdate_wait.h>
 #include <linux/sched/isolation.h>
+#include <linux/kprobes.h>
 
 #define CREATE_TRACE_POINTS
 
@@ -249,6 +250,7 @@ int notrace debug_lockdep_rcu_enabled(void)
 	       current->lockdep_recursion == 0;
 }
 EXPORT_SYMBOL_GPL(debug_lockdep_rcu_enabled);
+NOKPROBE_SYMBOL(debug_lockdep_rcu_enabled);
 
 /**
  * rcu_read_lock_held() - might we be in RCU read-side critical section?

[PATCH v2 9/9] kprobes: Prohibit probing on lockdep functions

[Masami Hiramatsu]

Some lockdep functions can be involved in breakpoint handling
and probing on those functions can cause a breakpoint recursion.
Prohibit probing on those functions by blacklist.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 kernel/locking/lockdep.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 95932333a48b..bc35a54ae3d4 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -50,6 +50,7 @@
 #include <linux/random.h>
 #include <linux/jhash.h>
 #include <linux/nmi.h>
+#include <linux/kprobes.h>
 
 #include <asm/sections.h>
 
@@ -2814,6 +2815,7 @@ void lockdep_hardirqs_on(unsigned long ip)
 	__trace_hardirqs_on_caller(ip);
 	current->lockdep_recursion = 0;
 }
+NOKPROBE_SYMBOL(lockdep_hardirqs_on);
 
 /*
  * Hardirqs were disabled:
@@ -2843,6 +2845,7 @@ void lockdep_hardirqs_off(unsigned long ip)
 	} else
 		debug_atomic_inc(redundant_hardirqs_off);
 }
+NOKPROBE_SYMBOL(lockdep_hardirqs_off);
 
 /*
  * Softirqs will be enabled:
@@ -3650,7 +3653,8 @@ __lock_release(struct lockdep_map *lock, int nested, unsigned long ip)
 	return 0;
 }
 
-static int __lock_is_held(const struct lockdep_map *lock, int read)
+static nokprobe_inline
+int __lock_is_held(const struct lockdep_map *lock, int read)
 {
 	struct task_struct *curr = current;
 	int i;
@@ -3883,6 +3887,7 @@ int lock_is_held_type(const struct lockdep_map *lock, int read)
 	return ret;
 }
 EXPORT_SYMBOL_GPL(lock_is_held_type);
+NOKPROBE_SYMBOL(lock_is_held_type);
 
 struct pin_cookie lock_pin_lock(struct lockdep_map *lock)
 {

Re: [PATCH v2 0/9] kprobes: Fix and improve blacklist symbols

[Andrea Righi]

On Sat, Jan 12, 2019 at 11:25:40AM +0900, Masami Hiramatsu wrote:
...
> And I found several functions which must be blacklisted.
>  - optprobe template code, which is just a template code and
>    never be executed. Moreover, since it can be copied and
>    reused, if we probe it, it modifies the template code and
>    can cause a crash. ([1/9][2/9])
>  - functions which is called before kprobe_int3_handler()
>    handles kprobes. This can cause a breakpoint recursion. ([3/9])
>  - IRQ entry text, which should not be probed since register/pagetable
>    status has not been stable at that point. ([4/9])
>  - Suffixed symbols, like .constprop, .part etc. Those suffixed
>    symbols never be blacklisted even if the non-suffixed version
>    has been blacklisted. ([5/9])
>  - hardirq tracer also works before int3 handling. ([6/9])
>  - preempt_check debug function also is involved in int3 handling.
>    ([7/9])
>  - RCU debug routine is also called before kprobe_int3_handler().
>    ([8/9])
>  - Some lockdep functions are also involved in int3 handling.
>    ([9/9])
> 
> Of course there still may be some functions which can be called
> by configuration change, I'll continue to test it.

Hi Masami,

I think I've found another recursion problem. Could you include also
this one?

Thanks,

From: Andrea Righi <righi.andrea@gmail.com>
Subject: [PATCH] kprobes: prohibit probing on bsearch()

Since kprobe breakpoing handler is using bsearch(), probing on this
routine can cause recursive breakpoint problem.

int3
 ->do_int3()
   ->ftrace_int3_handler()
     ->ftrace_location()
       ->ftrace_location_range()
         ->bsearch() -> int3

Prohibit probing on bsearch().

Signed-off-by: Andrea Righi <righi.andrea@gmail.com>
---
 lib/bsearch.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/bsearch.c b/lib/bsearch.c
index 18b445b010c3..82512fe7b33c 100644
--- a/lib/bsearch.c
+++ b/lib/bsearch.c
@@ -11,6 +11,7 @@
 
 #include <linux/export.h>
 #include <linux/bsearch.h>
+#include <linux/kprobes.h>
 
 /*
  * bsearch - binary search an array of elements
@@ -53,3 +54,4 @@ void *bsearch(const void *key, const void *base, size_t num, size_t size,
 	return NULL;
 }
 EXPORT_SYMBOL(bsearch);
+NOKPROBE_SYMBOL(bsearch);
-- 
2.17.1

Re: [PATCH v2 0/9] kprobes: Fix and improve blacklist symbols

[Masami Hiramatsu]

On Sat, 12 Jan 2019 14:33:24 +0100
Andrea Righi <righi.andrea@gmail.com> wrote:

> On Sat, Jan 12, 2019 at 11:25:40AM +0900, Masami Hiramatsu wrote:
> ...
> > And I found several functions which must be blacklisted.
> >  - optprobe template code, which is just a template code and
> >    never be executed. Moreover, since it can be copied and
> >    reused, if we probe it, it modifies the template code and
> >    can cause a crash. ([1/9][2/9])
> >  - functions which is called before kprobe_int3_handler()
> >    handles kprobes. This can cause a breakpoint recursion. ([3/9])
> >  - IRQ entry text, which should not be probed since register/pagetable
> >    status has not been stable at that point. ([4/9])
> >  - Suffixed symbols, like .constprop, .part etc. Those suffixed
> >    symbols never be blacklisted even if the non-suffixed version
> >    has been blacklisted. ([5/9])
> >  - hardirq tracer also works before int3 handling. ([6/9])
> >  - preempt_check debug function also is involved in int3 handling.
> >    ([7/9])
> >  - RCU debug routine is also called before kprobe_int3_handler().
> >    ([8/9])
> >  - Some lockdep functions are also involved in int3 handling.
> >    ([9/9])
> > 
> > Of course there still may be some functions which can be called
> > by configuration change, I'll continue to test it.
> 
> Hi Masami,
> 
> I think I've found another recursion problem. Could you include also
> this one?

Yeah, if I will make new version, but basically please feel free to
send such blacklist update patch to LKML, me and Ingo :)

> 
> Thanks,
> 
> From: Andrea Righi <righi.andrea@gmail.com>
> Subject: [PATCH] kprobes: prohibit probing on bsearch()
> 
> Since kprobe breakpoing handler is using bsearch(), probing on this
> routine can cause recursive breakpoint problem.
> 
> int3
>  ->do_int3()
>    ->ftrace_int3_handler()
>      ->ftrace_location()
>        ->ftrace_location_range()
>          ->bsearch() -> int3
> 
> Prohibit probing on bsearch().
> 
> Signed-off-by: Andrea Righi <righi.andrea@gmail.com>

This looks good to me.

Acked-by: Masami Hiramatsu <mhiramat@kernel.org>

Thank you,


> ---
>  lib/bsearch.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/bsearch.c b/lib/bsearch.c
> index 18b445b010c3..82512fe7b33c 100644
> --- a/lib/bsearch.c
> +++ b/lib/bsearch.c
> @@ -11,6 +11,7 @@
>  
>  #include <linux/export.h>
>  #include <linux/bsearch.h>
> +#include <linux/kprobes.h>
>  
>  /*
>   * bsearch - binary search an array of elements
> @@ -53,3 +54,4 @@ void *bsearch(const void *key, const void *base, size_t num, size_t size,
>  	return NULL;
>  }
>  EXPORT_SYMBOL(bsearch);
> +NOKPROBE_SYMBOL(bsearch);
> -- 
> 2.17.1
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v2 5/9] kprobes: Search non-suffixed symbol in blacklist

[Steven Rostedt]

On Sat, 12 Jan 2019 11:28:02 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Newer gcc can generate some different instances of a function
> with suffixed symbols if the function is optimized and only
> has a part of that. (e.g. .constprop, .part etc.)
> 
> In this case, it is not enough to check the entry of kprobe
> blacklist because it only records non-suffixed symbol address.
> 
> To fix this issue, search non-suffixed symbol in blacklist if
> given address is within a symbol which has a suffix.
> 
> Note that this can cause false positive cases if a kprobe-safe
> function is optimized to suffixed instance and has same name
> symbol which is blacklisted.
> But I would like to chose a fail-safe design for this issue.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>

Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

Re: [PATCH v2 0/9] kprobes: Fix and improve blacklist symbols

[Steven Rostedt]

On Sat, 12 Jan 2019 11:25:40 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Hi,
> 
> Here is the v2 series of kprobes blacklist bugfix and improvements mainly
> on x86 (since I started testing on qemu-x86).
> 
> >From v1, I just removed stable-ml from Cc (but tagged [1/9]) and added  
> Steve's Ack.

Ingo, I acted the ftrace change and also gave a review by to the added
function in kprobes.c.

Since this changes x86, could you take it in your tree?

Thanks!

-- Steve

Re: [PATCH v2 1/9] x86/kprobes: Prohibit probing on optprobe template code

[Masami Hiramatsu]

On Wed, 16 Jan 2019 13:35:58 +0000
Sasha Levin <sashal@kernel.org> wrote:

> Hi,
> 
> [This is an automated email]
> 
> This commit has been processed because it contains a "Fixes:" tag,
> fixing commit: 9326638cbee2 kprobes, x86: Use NOKPROBE_SYMBOL() instead of __kprobes annotation.
> 
> The bot has tested the following trees: v4.20.2, v4.19.15, v4.14.93, v4.9.150, v4.4.170, v3.18.132.
> 
> v4.20.2: Build OK!
> v4.19.15: Build OK!
> v4.14.93: Build OK!
> v4.9.150: Build failed! Errors:
>     ./include/linux/kprobes.h:494:22: error: initializer element is not constant
>     ./include/linux/kprobes.h:494:22: error: initializer element is not constant
>     ./include/linux/kprobes.h:494:22: error: initializer element is not constant
>     ./include/linux/kprobes.h:494:22: error: initializer element is not constant

Ah, this is related to

commit 54a7d50b9205 ("x86: mark kprobe templates as character arrays, not single characters")

Before this change, I used & optprobe_template_*, so it the patch should be

+NOKPROBE_SYMBOL(optprobe_template_func);
+NOKPROBE_SYMBOL(&optprobe_template_entry);
+NOKPROBE_SYMBOL(&optprobe_template_val);
+NOKPROBE_SYMBOL(&optprobe_template_call);
+NOKPROBE_SYMBOL(&optprobe_template_end);

(since optprobe_template_func is a function symbol, it doesn't need &)

> 
> v4.4.170: Failed to apply! Possible dependencies:
>     21266be9ed54 ("arch: consolidate CONFIG_STRICT_DEVM in lib/Kconfig.debug")
>     44a95dae1d22 ("KVM: x86: Detect and Initialize AVIC support")
>     520040146a0a ("KVM: x86: Use vector-hashing to deliver lowest-priority interrupts")
>     5881f73757cc ("svm: Introduce AMD IOMMU avic_ga_log_notifier")
>     5c919412fe61 ("kvm/x86: Hyper-V synthetic interrupt controller")
>     6308630bd3db ("kvm/x86: split ioapic-handled and EOI exit bitmaps")
>     c0dd671686b2 ("objtool: Mark non-standard object files and directories")
>     c207aee48037 ("objtool, x86: Add several functions and files to the objtool whitelist")
>     c6d308534aef ("UBSAN: run-time undefined behavior sanity checker")
>     d62caabb41f3 ("kvm/x86: per-vcpu apicv deactivation support")
>     f876f440df39 ("crypto: sha256-mb - SHA256 multibuffer job manager and glue code")

OK, commit 935893a17a99 ("objtool, x86: Add several functions and files to the objtool whitelist")
introduced optprobe_template_func, so for v4.4.X and v3.18.X, we don't need the first line.
So the patch should be something like,

                        "optprobe_template_end:\n");

+NOKPROBE_SYMBOL(&optprobe_template_entry);
+NOKPROBE_SYMBOL(&optprobe_template_val);
+NOKPROBE_SYMBOL(&optprobe_template_call);
+NOKPROBE_SYMBOL(&optprobe_template_end);
+
 #define TMPL_MOVE_IDX \
         ((long)&optprobe_template_val - (long)&optprobe_template_entry)


> 
> v3.18.132: Failed to apply! Possible dependencies:
>     0b24becc810d ("kasan: add kernel address sanitizer infrastructure")
>     131484c8da97 ("x86/debug: Remove perpetually broken, unmaintainable dwarf annotations")
>     24933b82c0d9 ("x86/asm/entry: Rename 'init_tss' to 'cpu_tss'")
>     76f5df43cab5 ("x86/asm/entry/64: Always allocate a complete "struct pt_regs" on the kernel stack")
>     8ef46a672a7d ("x86/asm/entry: Add this_cpu_sp0() to read sp0 for the current cpu")
>     905a36a28518 ("x86/asm/entry: Move entry_64.S and entry_32.S to arch/x86/entry/")
>     9d0c914c60f4 ("x86/asm/entry/64/compat: Change the 32-bit sysenter code to use sp0")
>     a232e3d558ee ("x86/asm/entry/32: Update "interrupt off" comments")
>     b87cf63e2a5f ("x86/asm/entry: Add comments about various syscall instructions")
>     c0dd671686b2 ("objtool: Mark non-standard object files and directories")
>     c207aee48037 ("objtool, x86: Add several functions and files to the objtool whitelist")
>     c63f06dd1579 ("kasan: move KASAN_SANITIZE in arch/x86/boot/Makefile")
>     ef7f0d6a6ca8 ("x86_64: add KASan support")
>     f876f440df39 ("crypto: sha256-mb - SHA256 multibuffer job manager and glue code")
> 
> 
> How should we proceed with this patch?

I'll send backport patches for those versions.

Thank you,

> 
> --
> Thanks,
> Sasha


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v2 0/9] kprobes: Fix and improve blacklist symbols

[Masami Hiramatsu]

Hi Ingo,

Can I ask you to pick this series and Andrea's patch?
Or would I better update this series on the latest tip/master?

Thank you,

On Mon, 14 Jan 2019 11:18:10 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Sat, 12 Jan 2019 11:25:40 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > Hi,
> > 
> > Here is the v2 series of kprobes blacklist bugfix and improvements mainly
> > on x86 (since I started testing on qemu-x86).
> > 
> > >From v1, I just removed stable-ml from Cc (but tagged [1/9]) and added  
> > Steve's Ack.
> 
> Ingo, I acted the ftrace change and also gave a review by to the added
> function in kprobes.c.
> 
> Since this changes x86, could you take it in your tree?
> 
> Thanks!
> 
> -- Steve


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v2 0/9] kprobes: Fix and improve blacklist symbols

[Ingo Molnar]

* Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Hi Ingo,
> 
> Can I ask you to pick this series and Andrea's patch?
> Or would I better update this series on the latest tip/master?

Yeah, an updated series with Andrea's patch included, against latest 
-tip, would be nice.

Thanks!

	Ingo

Re: [PATCH v2 0/9] kprobes: Fix and improve blacklist symbols

[Masami Hiramatsu]

On Mon, 11 Feb 2019 14:50:56 +0100
Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > Hi Ingo,
> > 
> > Can I ask you to pick this series and Andrea's patch?
> > Or would I better update this series on the latest tip/master?
> 
> Yeah, an updated series with Andrea's patch included, against latest 
> -tip, would be nice.

Hi, I sent the series on the latest -tip with Andrea's patch.

Thank you!



-- 
Masami Hiramatsu <mhiramat@kernel.org>