kprobes: Fix and improve blacklist symbols

PATCH Superseded Masami Hiramatsu 4 participants 18 messages Jan 12, 2019 – Feb 12, 2019 1 message not in the archive

Subsystems (9)

Stable branch x86 Library code Tracing +5 more subsystems

lore

Share & tools

lore

Masami Hiramatsu

author maintainer

2019-01-12 02:26 UTC #0 lore raw

v2 cover kprobes: Fix and improve blacklist symbols

Hi,

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

Quoted 1 line

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(-)

Signature · 1 line

Masami Hiramatsu (Linaro) <[email protected]>

Masami Hiramatsu

author maintainer

2019-01-12 02:26 UTC #1 lore raw

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

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 <[email protected]>

Fixes 9326638cbee2 ("kprobes, x86: Use NOKPROBE_SYMBOL() instead of __kprobes annotation")

Cc [email protected]

---
 arch/x86/kernel/kprobes/opt.c |    5 +++++
 1 file changed, 5 insertions(+)

arch/x86/kernel/kprobes/opt.c +5 -0

index 6adf6e6c2933..544bd41a514c 100644
@@ -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)

A message that is not in the archive (sent off-list or not yet received): [email protected]

Masami Hiramatsu

author maintainer

2019-01-22 09:41 UTC #3 lore raw

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

Quoted 9 lines · On Wed, 16 Jan 2019 13:35:58 +0000 Sasha Levin <[email protected]> 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.

Quoted 8 lines

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 &)

Quoted 13 lines

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)

Quoted 18 lines

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")

Quoted 1 line

How should we proceed with this patch?

I'll send backport patches for those versions.

Thank you,

Quoted 4 lines

--
Thanks,
Sasha

Signature · 1 line

Masami Hiramatsu <[email protected]>

Masami Hiramatsu

author maintainer

2019-01-12 02:27 UTC #4 lore raw

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

In reply to 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 <[email protected]>

---
 arch/x86/kernel/kprobes/opt.c |    9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

arch/x86/kernel/kprobes/opt.c +2 -7

index 544bd41a514c..f14262952015 100644
@@ -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)

Masami Hiramatsu

author maintainer

2019-01-12 02:27 UTC #5 lore raw

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

In reply to 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 <[email protected]>

---
 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(-)

arch/x86/kernel/alternative.c +2 -1

index ebeac487a20c..e8b628b1b279 100644
@@ -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

arch/x86/kernel/ftrace.c +2 -1

index 7ee8067cbf45..22a548919228 100644
@@ -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)
 {

arch/x86/kernel/traps.c +1 -0

index 9b7c4ca8f0a7..e289ce1332ab 100644
@@ -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)
 {

kernel/rcu/tree.c +2 -0

index 9180158756d2..74db52a0a466 100644
@@ -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

Masami Hiramatsu

author maintainer

2019-01-12 02:27 UTC #6 lore raw

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

In reply to 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 <[email protected]>

---
 arch/x86/kernel/kprobes/core.c |    7 +++++++
 1 file changed, 7 insertions(+)

arch/x86/kernel/kprobes/core.c +7 -0

index f4b954ff5b89..fed46ddb1eef 100644
@@ -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);
 }

Masami Hiramatsu

author maintainer

2019-01-12 02:28 UTC #7 lore raw

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

In reply to 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 <[email protected]>

---
 kernel/kprobes.c |   21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

kernel/kprobes.c +20 -1

index e8c76164f541..faa519f07aad 100644
@@ -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;
 }

Steven Rostedt

maintainer Reviewed-by on v2 5/9

2019-01-14 16:17 UTC #8 lore raw

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

Quoted 10 lines · On Sat, 12 Jan 2019 11:28:02 +0900 Masami Hiramatsu <[email protected]> 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.

Quoted 6 lines

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 <[email protected]>

Reviewed-by Steven Rostedt (VMware) <[email protected]>

-- Steve

Masami Hiramatsu

author maintainer

2019-01-12 02:28 UTC #9 lore raw

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

In reply to 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 <[email protected]>

Acked-by Steven Rostedt (VMware) <[email protected]>

---
 kernel/trace/trace_irqsoff.c    |    9 +++++++--
 kernel/trace/trace_preemptirq.c |    5 +++++
 2 files changed, 12 insertions(+), 2 deletions(-)

kernel/trace/trace_irqsoff.c +7 -2

index 98ea6d28df15..829709bfec3d 100644
@@ -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)
 {

kernel/trace/trace_preemptirq.c +5 -0

index 71f553cceb3c..4d8e99fdbbbe 100644
@@ -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

Masami Hiramatsu

author maintainer

2019-01-12 02:29 UTC #10 lore raw

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

In reply to 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 <[email protected]>

---
 lib/smp_processor_id.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

lib/smp_processor_id.c +5 -2

index 85925aaa4fff..157d9e31f6c2 100644
@@ -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);

Masami Hiramatsu

author maintainer

2019-01-12 02:29 UTC #11 lore raw

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

In reply to 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 <[email protected]>

---
 kernel/rcu/update.c |    2 ++
 1 file changed, 2 insertions(+)

kernel/rcu/update.c +2 -0

index 1971869c4072..f4ca36d92138 100644
@@ -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?

Masami Hiramatsu

author maintainer

2019-01-12 02:30 UTC #12 lore raw

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

In reply to 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 <[email protected]>

---
 kernel/locking/lockdep.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

kernel/locking/lockdep.c +6 -1

index 95932333a48b..bc35a54ae3d4 100644
@@ -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)
 {

Andrea Righi

2019-01-12 13:33 UTC #13 lore raw

In reply to Masami Hiramatsu

On Sat, Jan 12, 2019 at 11:25:40AM +0900, Masami Hiramatsu wrote:
...

Quoted 20 lines

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])

Quoted 2 lines

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 <[email protected]>
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 <[email protected]>

---
 lib/bsearch.c | 2 ++
 1 file changed, 2 insertions(+)

lib/bsearch.c +2 -0

index 18b445b010c3..82512fe7b33c 100644
@@ -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);

Signature · 1 line

2.17.1

Masami Hiramatsu

author maintainer Acked-by

2019-01-13 14:23 UTC #14 lore raw

Quoted 22 lines · On Sat, 12 Jan 2019 14:33:24 +0100 Andrea Righi <[email protected]> wrote:

On Sat, Jan 12, 2019 at 11:25:40AM +0900, Masami Hiramatsu wrote:
...
Quoted 20 lines
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])

Quoted 7 lines

Quoted 2 lines
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 :)

Quoted 12 lines

Thanks,
From: Andrea Righi <[email protected]>
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()

Quoted 7 lines

     ->ftrace_location()
       ->ftrace_location_range()
         ->bsearch() -> int3
Prohibit probing on bsearch().
Signed-off-by Andrea Righi <[email protected]>

This looks good to me.

Acked-by Masami Hiramatsu <[email protected]>

Thank you,

Quoted 24 lines

---
 lib/bsearch.c | 2 ++
 1 file changed, 2 insertions(+)

lib/bsearch.c +2 -0

index 18b445b010c3..82512fe7b33c 100644
@@ -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);

Signature · 1 line

2.17.1

Signature · 1 line

Masami Hiramatsu <[email protected]>

Steven Rostedt

maintainer

2019-01-14 16:18 UTC #15 lore raw

In reply to Masami Hiramatsu

Quoted 7 lines · On Sat, 12 Jan 2019 11:25:40 +0900 Masami Hiramatsu <[email protected]> wrote:

Hi,
Here is the v2 series of kprobes blacklist bugfix and improvements mainly on x86 (since I started testing on qemu-x86).
Quoted 1 line
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

author maintainer

2019-02-01 13:32 UTC #16 lore raw

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,

Quoted 19 lines · On Mon, 14 Jan 2019 11:18:10 -0500 Steven Rostedt <[email protected]> wrote:

Quoted 7 lines · On Sat, 12 Jan 2019 11:25:40 +0900 Masami Hiramatsu <[email protected]> wrote:
Hi,
Here is the v2 series of kprobes blacklist bugfix and improvements mainly on x86 (since I started testing on qemu-x86).
Quoted 1 line
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

Signature · 1 line

Masami Hiramatsu <[email protected]>

Ingo Molnar

2019-02-11 13:51 UTC #17 lore raw

Quoted 4 lines · 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?

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

Thanks!

Ingo

Masami Hiramatsu

author maintainer

2019-02-12 16:49 UTC #18 lore raw

Quoted 10 lines · On Mon, 11 Feb 2019 14:50:56 +0100 Ingo Molnar <[email protected]> wrote:

Quoted 4 lines · 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?
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!

Signature · 1 line

Masami Hiramatsu <[email protected]>