support kretprobe-blacklist

This patch introduces architecture dependent kretprobe blacklists to prohibit users from inserting return probes on the function in which kprobes can be inserted but kretprobes can not.

Signed-off-by Masami Hiramatsu <[email protected]>

---
<Problem Description>
When a kretprobe is inserted in the entry of the "__switch_to()", it causes kernel panic on i386 with recent kernel.

<Problem Analysis>
In include/asm-i386/current.h, "current" is defined as an entry of percpu variables.:

DECLARE_PER_CPU(struct task_struct *, current_task); static __always_inline struct task_struct *get_current(void)

 {
         return x86_read_percpu(current_task);

}

#define current get_current()

This mean the "current" macro is separated from its stack register. Kretprobe expects that "current" value when a function is called is not changed, or both of "current" value and stack register are changed in the target function.
But __switch_to() in arch/i386/kernel/process.c changes only the value of "current", but doesn't changes the stack register(this was already switched to new stack before calling __switch_to()):

struct task_struct fastcall * __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
{
...

        x86_write_percpu(current_task, next_p);

        return prev_p;

}

Kretprobe modifies the return address stored in the stack, and it saves the original return address in the kretprobe_instance with the value of "current".
When kretprobe is hit at the return of __switch_to(), it searches kretprobe_instance related to the probe point from a hash list by using the hash-value of the "current" instead of stack address. However, since the value of "current" is already changed, it can't find proper instance, and invokes BUG() macro.

<Solution>
As a result of discussion with other kprobe developers, I'd like to introduce arch-dependent blacklist.
To introduce "__kretprobes" mark (like "__kprobes") is another way. But I thought it is not efficient way, because the kretprobe can be inserted only in the entry of functions and there is no need to check against a whole function.

This patch also removes "__kprobes" mark from "__switch_to" on x86_64 and registers "__switch_to" to the blacklist on x86-64, because that mark is to prohibit user from inserting only kretprobe.

Index: 2.6-mm/include/asm-i386/kprobes.h
===================================================================

kernel/kprobes.c +2 -0

@@ -716,6 +716,18 @@ int __kprobes register_kretprobe(struct
 	int ret = 0;
 	struct kretprobe_instance *inst;
 	int i;
+#ifdef ARCH_SUPPORTS_KRETPROBE_BLACKLIST
+	void *addr = rp->kp.addr;

include/asm-i386/kprobes.h typedef u8 kprobe_opcode_t; · line 47

Quoted 9 lines

Index: 2.6-mm/include/asm-i386/kprobes.h
===================================================================

include/asm-i386/kprobes.h +1 -0

@@ -44,6 +44,7 @@ typedef u8 kprobe_opcode_t;
 
 #define ARCH_SUPPORTS_KRETPROBES
 #define flush_insn_slot(p)	do { } while (0)
+#define ARCH_SUPPORTS_KRETPROBE_BLACKLIST

Can we avoid adding this please?

It should at least have been a CONFIG_foo thing, defined in arch/*/Kconfig.

But that still requires nasty ifdefs in the C code. It would be very small overhead just to require that all architectures implement arch_kretprobe_blacklist[] (which can be renamed to kretprobe_blacklist[]).
Architectures which don't need a blacklist can just have { { 0, 0 } }.

If the few bytes of overhead on non-x86 really offends then one could do something like this:

in powerpc header file:

#define kretprobe_blacklist_size 0

in x86 header file:

extern const int kretprobe_blacklist_size;

in x86 C file:

const int kretprobe_blacklist_size = ARRAY_SIZE(kretprobe_blacklist);

and then this code:

Quoted 8 lines

kernel/kprobes.c +2 -0

@@ -716,6 +716,18 @@ int __kprobes register_kretprobe(struct
 	int ret = 0;
 	struct kretprobe_instance *inst;
 	int i;
+#ifdef ARCH_SUPPORTS_KRETPROBE_BLACKLIST
+	void *addr = rp->kp.addr;

kernel/kprobes.c int __kprobes register_kretprobe(struct · line 730

Quoted 10 lines

kernel/kprobes.c +10 -0

+
+	if (addr == NULL)
+		kprobe_lookup_name(rp->kp.symbol_name, addr);
+	addr += rp->kp.offset;
+
+	for (i = 0; arch_kretprobe_blacklist[i].name != NULL; i++) {
+		if (arch_kretprobe_blacklist[i].addr == addr)
+			return -EINVAL;
+	}
+#endif

can be put inside

	if (kretprobe_blacklist_size) {
		...
	}

so the compiler will remove it all for (say) powerpc.

There are lots of ways of doing it but code like this:

kernel/kprobes.c static int __init init_kprobes(void) · line 818

Quoted 10 lines

kernel/kprobes.c +10 -0

+#ifdef ARCH_SUPPORTS_KRETPROBE_BLACKLIST
+	/* lookup the function address from its name */
+	for (i = 0; arch_kretprobe_blacklist[i].name != NULL; i++) {
+		kprobe_lookup_name(arch_kretprobe_blacklist[i].name,
+				   arch_kretprobe_blacklist[i].addr);
+		if (!arch_kretprobe_blacklist[i].addr)
+			printk("kretprobe: Unknown blacklisted function: %s\n",
+			       arch_kretprobe_blacklist[i].name);
+	}
+#endif

really isn't the sort of thing we like to see spreading through core kernel code.

Have a think about it please, see what we can come up with?

Hi Andrew,

I updated my patch and removed #ifdefs from it.
Thank you,

Masami Hiramatsu
Hitachi Computer Products (America), Inc.

This patch introduces architecture dependent kretprobe blacklists to prohibit users from inserting return
probes on the function in which kprobes can be inserted
but kretprobes can not.
This patch also removes "__kprobes" mark from "__switch_to" on x86_64 and registers "__switch_to" to the blacklist on x86-64, because that mark is to prohibit user from inserting only kretprobe.

Signed-off-by Masami Hiramatsu <[email protected]>

---
 arch/avr32/kernel/kprobes.c   |    2 ++
 arch/i386/kernel/kprobes.c    |    7 +++++++
 arch/ia64/kernel/kprobes.c    |    2 ++
 arch/powerpc/kernel/kprobes.c |    2 ++
 arch/s390/kernel/kprobes.c    |    2 ++
 arch/sparc64/kernel/kprobes.c |    2 ++
 arch/x86_64/kernel/kprobes.c  |    7 +++++++
 arch/x86_64/kernel/process.c  |    2 +-
 include/asm-avr32/kprobes.h   |    2 ++
 include/asm-i386/kprobes.h    |    2 ++
 include/asm-ia64/kprobes.h    |    1 +
 include/asm-powerpc/kprobes.h |    1 +
 include/asm-s390/kprobes.h    |    1 +
 include/asm-sparc64/kprobes.h |    2 ++
 include/asm-x86_64/kprobes.h  |    1 +
 include/linux/kprobes.h       |    6 ++++++
 kernel/kprobes.c              |   23 +++++++++++++++++++++++
 17 files changed, 64 insertions(+), 1 deletion(-)

Index: 2.6.23-rc2-mm2/arch/i386/kernel/kprobes.c
===================================================================

arch/i386/kernel/kprobes.c +7 -0

@@ -42,6 +42,13 @@ void jprobe_return_end(void);
 DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
 DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
 
+struct kretprobe_blackpoint kretprobe_blacklist[] = {
+	{"__switch_to", }, /* This function switches only current task, but
+			     doesn't switch kernel stack.*/
+	{NULL, NULL}	/* Terminator */
+};
+const int kretprobe_blacklist_size = ARRAY_SIZE(kretprobe_blacklist);
+
 /* insert a jmp code */
 static __always_inline void set_jmp_op(void *from, void *to)
 {

Index: 2.6.23-rc2-mm2/kernel/kprobes.c
===================================================================

kernel/kprobes.c +23 -0

@@ -716,6 +716,18 @@ int __kprobes register_kretprobe(struct
 	int ret = 0;
 	struct kretprobe_instance *inst;
 	int i;
+	void *addr = rp->kp.addr;
+
+	if (kretprobe_blacklist_size) {
+		if (addr == NULL)
+			kprobe_lookup_name(rp->kp.symbol_name, addr);
+		addr += rp->kp.offset;
+
+		for (i = 0; kretprobe_blacklist[i].name != NULL; i++) {
+			if (kretprobe_blacklist[i].addr == addr)
+				return -EINVAL;
+		}
+	}
 
 	rp->kp.pre_handler = pre_handler_kretprobe;
 	rp->kp.post_handler = NULL;

@@ -794,6 +806,17 @@ static int __init init_kprobes(void)
 		INIT_HLIST_HEAD(&kretprobe_inst_table[i]);
 	}
 
+	if (kretprobe_blacklist_size) {
+		/* lookup the function address from its name */
+		for (i = 0; kretprobe_blacklist[i].name != NULL; i++) {
+			kprobe_lookup_name(kretprobe_blacklist[i].name,
+					   kretprobe_blacklist[i].addr);
+			if (!kretprobe_blacklist[i].addr)
+				printk("kretprobe: lookup failed: %s\n",
+				       kretprobe_blacklist[i].name);
+		}
+	}
+
 	/* By default, kprobes are enabled */
 	kprobe_enabled = true;
 

Index: 2.6.23-rc2-mm2/arch/x86_64/kernel/kprobes.c
===================================================================

arch/x86_64/kernel/kprobes.c +7 -0

@@ -49,6 +49,13 @@ static void __kprobes arch_copy_kprobe(s
 DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
 DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
 
+struct kretprobe_blackpoint kretprobe_blacklist[] = {
+	{"__switch_to", }, /* This function switches only current task, but
+			      doesn't switch kernel stack.*/
+	{NULL, NULL}	/* Terminator */
+};
+const int kretprobe_blacklist_size = ARRAY_SIZE(kretprobe_blacklist);
+
 /*
  * returns non-zero if opcode modifies the interrupt flag.
  */

Index: 2.6.23-rc2-mm2/include/linux/kprobes.h
===================================================================

include/linux/kprobes.h +6 -0

@@ -166,6 +166,12 @@ struct kretprobe_instance {
 	struct task_struct *task;
 };
 
+struct kretprobe_blackpoint {
+	const char *name;
+	void *addr;
+};
+extern struct kretprobe_blackpoint kretprobe_blacklist[];
+
 static inline void kretprobe_assert(struct kretprobe_instance *ri,
 	unsigned long orig_ret_address, unsigned long trampoline_address)
 {

Index: 2.6.23-rc2-mm2/arch/x86_64/kernel/process.c
===================================================================

arch/x86_64/kernel/process.c +1 -1

@@ -584,7 +584,7 @@ static inline void __switch_to_xtra(stru
  *
  * Kprobes not supported here. Set the probe on schedule instead.
  */
-__kprobes struct task_struct *
+struct task_struct *
 __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
 {
 	struct thread_struct *prev = &prev_p->thread,

Index: 2.6.23-rc2-mm2/include/asm-ia64/kprobes.h
===================================================================

include/asm-ia64/kprobes.h +1 -0

@@ -83,6 +83,7 @@ struct kprobe_ctlblk {
 };
 
 #define ARCH_SUPPORTS_KRETPROBES
+#define kretprobe_blacklist_size 0
 
 #define SLOT0_OPCODE_SHIFT	(37)
 #define SLOT1_p1_OPCODE_SHIFT	(37 - (64-46))

Index: 2.6.23-rc2-mm2/include/asm-powerpc/kprobes.h
===================================================================

include/asm-powerpc/kprobes.h +1 -0

@@ -82,6 +82,7 @@ typedef unsigned int kprobe_opcode_t;
 
 #define ARCH_SUPPORTS_KRETPROBES
 #define flush_insn_slot(p)	do { } while (0)
+#define kretprobe_blacklist_size 0
 
 void kretprobe_trampoline(void);
 extern void arch_remove_kprobe(struct kprobe *p);

Index: 2.6.23-rc2-mm2/include/asm-s390/kprobes.h
===================================================================

include/asm-s390/kprobes.h +1 -0

@@ -47,6 +47,7 @@ typedef u16 kprobe_opcode_t;
 	: (((unsigned long)current_thread_info()) + THREAD_SIZE - (ADDR)))
 
 #define ARCH_SUPPORTS_KRETPROBES
+#define kretprobe_blacklist_size 0
 
 #define KPROBE_SWAP_INST	0x10
 

Index: 2.6.23-rc2-mm2/include/asm-avr32/kprobes.h
===================================================================

include/asm-avr32/kprobes.h +2 -0

@@ -17,6 +17,8 @@ typedef u16	kprobe_opcode_t;
 #define BREAKPOINT_INSTRUCTION	0xd673	/* breakpoint */
 #define MAX_INSN_SIZE		2
 
+#define kretprobe_blacklist_size 0
+
 #define arch_remove_kprobe(p)	do { } while (0)
 
 /* Architecture specific copy of original instruction */

Index: 2.6.23-rc2-mm2/include/asm-sparc64/kprobes.h
===================================================================

include/asm-sparc64/kprobes.h +2 -0

@@ -10,6 +10,8 @@ typedef u32 kprobe_opcode_t;
 #define BREAKPOINT_INSTRUCTION_2 0x91d02071 /* ta 0x71 */
 #define MAX_INSN_SIZE 2
 
+#define kretprobe_blacklist_size 0
+
 #define arch_remove_kprobe(p)	do {} while (0)
 
 #define flush_insn_slot(p)		\

Index: 2.6.23-rc2-mm2/arch/avr32/kernel/kprobes.c
===================================================================

arch/avr32/kernel/kprobes.c +2 -0

@@ -22,6 +22,8 @@ DEFINE_PER_CPU(struct kprobe *, current_
 static unsigned long kprobe_status;
 static struct pt_regs jprobe_saved_regs;
 
+struct kretprobe_blackpoint kretprobe_blacklist[] = {{NULL, NULL}};
+
 int __kprobes arch_prepare_kprobe(struct kprobe *p)
 {
 	int ret = 0;

Index: 2.6.23-rc2-mm2/arch/ia64/kernel/kprobes.c
===================================================================

arch/ia64/kernel/kprobes.c +2 -0

@@ -40,6 +40,8 @@ extern void jprobe_inst_return(void);
 DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
 DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
 
+struct kretprobe_blackpoint kretprobe_blacklist[] = {{NULL, NULL}};
+
 enum instruction_type {A, I, M, F, B, L, X, u};
 static enum instruction_type bundle_encoding[32][3] = {
   { M, I, I },				/* 00 */

Index: 2.6.23-rc2-mm2/arch/powerpc/kernel/kprobes.c
===================================================================

arch/powerpc/kernel/kprobes.c +2 -0

@@ -38,6 +38,8 @@
 DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
 DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
 
+struct kretprobe_blackpoint kretprobe_blacklist[] = {{NULL, NULL}};
+
 int __kprobes arch_prepare_kprobe(struct kprobe *p)
 {
 	int ret = 0;

Index: 2.6.23-rc2-mm2/arch/s390/kernel/kprobes.c
===================================================================

arch/s390/kernel/kprobes.c +2 -0

@@ -33,6 +33,8 @@
 DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
 DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
 
+struct kretprobe_blackpoint kretprobe_blacklist[] = {{NULL, NULL}};
+
 int __kprobes arch_prepare_kprobe(struct kprobe *p)
 {
 	/* Make sure the probe isn't going on a difficult instruction */

Index: 2.6.23-rc2-mm2/arch/sparc64/kernel/kprobes.c
===================================================================

arch/sparc64/kernel/kprobes.c +2 -0

@@ -42,6 +42,8 @@
 DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
 DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
 
+struct kretprobe_blackpoint kretprobe_blacklist[] = {{NULL, NULL}};
+
 int __kprobes arch_prepare_kprobe(struct kprobe *p)
 {
 	p->ainsn.insn[0] = *p->addr;

Index: 2.6.23-rc2-mm2/include/asm-i386/kprobes.h
===================================================================

include/asm-i386/kprobes.h +2 -0

@@ -45,6 +45,8 @@ typedef u8 kprobe_opcode_t;
 #define ARCH_SUPPORTS_KRETPROBES
 #define flush_insn_slot(p)	do { } while (0)
 
+extern const int kretprobe_blacklist_size;
+
 void arch_remove_kprobe(struct kprobe *p);
 void kretprobe_trampoline(void);
 

Index: 2.6.23-rc2-mm2/include/asm-x86_64/kprobes.h
===================================================================

include/asm-x86_64/kprobes.h +1 -0

@@ -42,6 +42,7 @@ typedef u8 kprobe_opcode_t;
 	: (((unsigned long)current_thread_info()) + THREAD_SIZE - (ADDR)))
 
 #define ARCH_SUPPORTS_KRETPROBES
+extern const int kretprobe_blacklist_size;
 
 void kretprobe_trampoline(void);
 extern void arch_remove_kprobe(struct kprobe *p);

This patch introduces architecture dependent kretprobe blacklists to prohibit users from inserting return probes on the function in which kprobes can be inserted but kretprobes can not.

Quoted 4 lines · Masami Hiramatsu

This patch introduces architecture dependent kretprobe blacklists to prohibit users from inserting return probes on the function in which kprobes can be inserted but kretprobes can not.

I don't like this at all. If people want to shoot themselves into their own foot using kernel modules they have an unlimited number of ways to do this, and even more with kprobes, so there's no point in making it a little harder for some cases.