Masami Hiramatsu
2013-Nov-08 12:52 UTC
[PATCH -tip RFC 0/2] kprobes: introduce NOKPROBE_SYMBOL() and prohibit probing on .entry.text
Currently the blacklist is maintained by hand in kprobes.c which is separated from the function definition and is hard to catch up the kernel update. To solve this issue, I've tried to implement new NOKPROBE_SYMBOL() macro for making kprobe blacklist at build time. Since the NOKPROBE_SYMBOL() macros can be placed right after the function is defined, it is easy to maintain. At this moment, I applied the macro only for the symbols which is listed in kprobes.c. As we discussed in previous thread, if the gcc accepts to introduce new annotation to store the function address (and size) at somewhere, we can easily move onto that by replacing NOKPROBE_SYMBOL() with nokprobe annotation (and just modifying the populate_kprobe_blacklist() a bit). This series also includes a change which prohibits probing on the address in .entry.text because the code is used for very low-level sensitive interrupt/syscall entries. Probing such code may cause unexpected result (actually most of that area is already in the kprobe blacklist). So I've decide to prohibit probing all of them. Since Ingo wasn't convinced about the idea in the previous discussion, I just make this series as RFC series. I'd like to ask again with actual implementation and plan. Thank you, --- Masami Hiramatsu (2): kprobes: Prohibit probing on .entry.text code kprobes: Introduce NOKPROBE_SYMBOL() macro for blacklist arch/x86/kernel/entry_32.S | 33 ------------ arch/x86/kernel/entry_64.S | 20 -------- arch/x86/kernel/paravirt.c | 4 ++ include/asm-generic/vmlinux.lds.h | 9 +++ include/linux/kprobes.h | 19 +++++++ kernel/kprobes.c | 98 ++++++++++++++++++------------------- kernel/sched/core.c | 1 7 files changed, 80 insertions(+), 104 deletions(-) -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu.pt at hitachi.com
Masami Hiramatsu
2013-Nov-08 12:52 UTC
[PATCH -tip RFC 1/2] kprobes: Prohibit probing on .entry.text code
.entry.text is a code area which is used for interrupt/syscall entries, and there are many sensitive codes. Thus, it is better to prohibit probing on all of such codes instead of a part of that. Since some symbols are already registered on kprobe blacklist, this also removes them from the blacklist. Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt at hitachi.com> --- arch/x86/kernel/entry_32.S | 33 --------------------------------- arch/x86/kernel/entry_64.S | 20 -------------------- kernel/kprobes.c | 10 +++++----- 3 files changed, 5 insertions(+), 58 deletions(-) diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S index fd1bc1b..6d19cfb 100644 --- a/arch/x86/kernel/entry_32.S +++ b/arch/x86/kernel/entry_32.S @@ -315,10 +315,6 @@ ENTRY(ret_from_kernel_thread) ENDPROC(ret_from_kernel_thread) /* - * Interrupt exit functions should be protected against kprobes - */ - .pushsection .kprobes.text, "ax" -/* * Return to user mode is not as complex as all this looks, * but we want the default path for a system call return to * go as quickly as possible which is why some of this is @@ -372,10 +368,6 @@ need_resched: END(resume_kernel) #endif CFI_ENDPROC -/* - * End of kprobes section - */ - .popsection /* SYSENTER_RETURN points to after the "sysenter" instruction in the vsyscall page. See vsyscall-sysentry.S, which defines the symbol. */ @@ -495,10 +487,6 @@ sysexit_audit: PTGS_TO_GS_EX ENDPROC(ia32_sysenter_target) -/* - * syscall stub including irq exit should be protected against kprobes - */ - .pushsection .kprobes.text, "ax" # system call handler stub ENTRY(system_call) RING0_INT_FRAME # can't unwind into user space anyway @@ -691,10 +679,6 @@ syscall_badsys: jmp resume_userspace END(syscall_badsys) CFI_ENDPROC -/* - * End of kprobes section - */ - .popsection .macro FIXUP_ESPFIX_STACK /* @@ -781,10 +765,6 @@ common_interrupt: ENDPROC(common_interrupt) CFI_ENDPROC -/* - * Irq entries should be protected against kprobes - */ - .pushsection .kprobes.text, "ax" #define BUILD_INTERRUPT3(name, nr, fn) \ ENTRY(name) \ RING0_INT_FRAME; \ @@ -961,10 +941,6 @@ ENTRY(spurious_interrupt_bug) jmp error_code CFI_ENDPROC END(spurious_interrupt_bug) -/* - * End of kprobes section - */ - .popsection #ifdef CONFIG_XEN /* Xen doesn't set %esp to be precisely what the normal sysenter @@ -1239,11 +1215,6 @@ return_to_handler: jmp *%ecx #endif -/* - * Some functions should be protected against kprobes - */ - .pushsection .kprobes.text, "ax" - ENTRY(page_fault) RING0_EC_FRAME ASM_CLAC @@ -1443,7 +1414,3 @@ ENTRY(async_page_fault) END(async_page_fault) #endif -/* - * End of kprobes section - */ - .popsection diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S index 603be7c..263c6cf 100644 --- a/arch/x86/kernel/entry_64.S +++ b/arch/x86/kernel/entry_64.S @@ -487,8 +487,6 @@ ENDPROC(native_usergs_sysret64) TRACE_IRQS_OFF .endm -/* save complete stack frame */ - .pushsection .kprobes.text, "ax" ENTRY(save_paranoid) XCPT_FRAME 1 RDI+8 cld @@ -517,7 +515,6 @@ ENTRY(save_paranoid) 1: ret CFI_ENDPROC END(save_paranoid) - .popsection /* * A newly forked process directly context switches into this address. @@ -975,10 +972,6 @@ END(interrupt) call \func .endm -/* - * Interrupt entry/exit should be protected against kprobes - */ - .pushsection .kprobes.text, "ax" /* * The interrupt stubs push (~vector+0x80) onto the stack and * then jump to common_interrupt. @@ -1113,10 +1106,6 @@ ENTRY(retint_kernel) CFI_ENDPROC END(common_interrupt) -/* - * End of kprobes section - */ - .popsection /* * APIC interrupts. @@ -1466,11 +1455,6 @@ apicinterrupt3 HYPERVISOR_CALLBACK_VECTOR \ hyperv_callback_vector hyperv_vector_handler #endif /* CONFIG_HYPERV */ -/* - * Some functions should be protected against kprobes - */ - .pushsection .kprobes.text, "ax" - paranoidzeroentry_ist debug do_debug DEBUG_STACK paranoidzeroentry_ist int3 do_int3 DEBUG_STACK paranoiderrorentry stack_segment do_stack_segment @@ -1887,7 +1871,3 @@ ENTRY(ignore_sysret) CFI_ENDPROC END(ignore_sysret) -/* - * End of kprobes section - */ - .popsection diff --git a/kernel/kprobes.c b/kernel/kprobes.c index a0d367a..ec0dbc7 100644 --- a/kernel/kprobes.c +++ b/kernel/kprobes.c @@ -96,9 +96,6 @@ static raw_spinlock_t *kretprobe_table_lock_ptr(unsigned long hash) static struct kprobe_blackpoint kprobe_blacklist[] = { {"preempt_schedule",}, {"native_get_debugreg",}, - {"irq_entries_start",}, - {"common_interrupt",}, - {"mcount",}, /* mcount can be called from everywhere */ {NULL} /* Terminator */ }; @@ -1328,8 +1325,11 @@ static int __kprobes in_kprobes_functions(unsigned long addr) { struct kprobe_blackpoint *kb; - if (addr >= (unsigned long)__kprobes_text_start && - addr < (unsigned long)__kprobes_text_end) + /* The __kprobes marked functions and entry code must not be probed */ + if ((addr >= (unsigned long)__kprobes_text_start && + addr < (unsigned long)__kprobes_text_end) || + (addr >= (unsigned long)__entry_text_start && + addr < (unsigned long)__entry_text_end)) return -EINVAL; /* * If there exists a kprobe_blacklist, verify and
Masami Hiramatsu
2013-Nov-08 12:52 UTC
[PATCH -tip RFC 2/2] kprobes: Introduce NOKPROBE_SYMBOL() macro for blacklist
Introduce NOKPROBE_SYMBOL() macro which builds a kprobe blacklist in build time. The usage of this macro is similar to the EXPORT_SYMBOL, put the NOKPROBE_SYMBOL(function); just after the function definition. If CONFIG_KPROBES=y, the macro is expanded to the definition of a static data structure of kprobe_blackpoint which is initialized for the function and put the address of the data structure in the "_kprobe_blacklist" section. Since the data structures are not fully initialized by the macro (because there is no "size" information), those are re-initialized at boot time by using kallsyms. Signed-off-by: Masami Hiramatsu <masami.hiramatsu.pt at hitachi.com> --- arch/x86/kernel/paravirt.c | 4 ++ include/asm-generic/vmlinux.lds.h | 9 ++++ include/linux/kprobes.h | 19 ++++++++ kernel/kprobes.c | 88 ++++++++++++++++++------------------- kernel/sched/core.c | 1 5 files changed, 75 insertions(+), 46 deletions(-) diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c index 1b10af8..4c785fd 100644 --- a/arch/x86/kernel/paravirt.c +++ b/arch/x86/kernel/paravirt.c @@ -23,6 +23,7 @@ #include <linux/efi.h> #include <linux/bcd.h> #include <linux/highmem.h> +#include <linux/kprobes.h> #include <asm/bug.h> #include <asm/paravirt.h> @@ -389,6 +390,9 @@ __visible struct pv_cpu_ops pv_cpu_ops = { .end_context_switch = paravirt_nop, }; +/* At this point, native_get_debugreg has real function entry */ +NOKPROBE_SYMBOL(native_get_debugreg); + struct pv_apic_ops pv_apic_ops = { #ifdef CONFIG_X86_LOCAL_APIC .startup_ipi_hook = paravirt_nop, diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h index 83e2c31..294ea96 100644 --- a/include/asm-generic/vmlinux.lds.h +++ b/include/asm-generic/vmlinux.lds.h @@ -109,6 +109,14 @@ #define BRANCH_PROFILE() #endif +#ifdef CONFIG_KPROBES +#define KPROBE_BLACKLIST() VMLINUX_SYMBOL(__start_kprobe_blacklist) = .; \ + *(_kprobe_blacklist) \ + VMLINUX_SYMBOL(__stop_kprobe_blacklist) = .; +#else +#define KPROBE_BLACKLIST() +#endif + #ifdef CONFIG_EVENT_TRACING #define FTRACE_EVENTS() . = ALIGN(8); \ VMLINUX_SYMBOL(__start_ftrace_events) = .; \ @@ -487,6 +495,7 @@ *(.init.rodata) \ FTRACE_EVENTS() \ TRACE_SYSCALLS() \ + KPROBE_BLACKLIST() \ MEM_DISCARD(init.rodata) \ CLK_OF_TABLES() \ CLKSRC_OF_TABLES() \ diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h index 925eaf2..a403038 100644 --- a/include/linux/kprobes.h +++ b/include/linux/kprobes.h @@ -206,6 +206,7 @@ struct kretprobe_blackpoint { }; struct kprobe_blackpoint { + struct list_head list; const char *name; unsigned long start_addr; unsigned long range; @@ -476,4 +477,22 @@ static inline int enable_jprobe(struct jprobe *jp) return enable_kprobe(&jp->kp); } +#ifdef CONFIG_KPROBES +/* + * Blacklist ganerating macro. Specify functions which is not probed + * by using this macro. + */ +#define NOKPROBE_SYMBOL(fname) \ +static struct kprobe_blackpoint __used \ + _kprobe_bp_##fname = { \ + .name = #fname, \ + .start_addr = (unsigned long)fname, \ +}; \ +static struct kprobe_blackpoint __used \ + __attribute__((section("_kprobe_blacklist"))) \ + *_p_kprobe_bp_##fname = &_kprobe_bp_##fname; +#else +#define NOKPROBE_SYMBOL(fname) +#endif + #endif /* _LINUX_KPROBES_H */ diff --git a/kernel/kprobes.c b/kernel/kprobes.c index ec0dbc7..1fab712 100644 --- a/kernel/kprobes.c +++ b/kernel/kprobes.c @@ -86,18 +86,8 @@ static raw_spinlock_t *kretprobe_table_lock_ptr(unsigned long hash) return &(kretprobe_table_locks[hash].lock); } -/* - * Normally, functions that we'd want to prohibit kprobes in, are marked - * __kprobes. But, there are cases where such functions already belong to - * a different section (__sched for preempt_schedule) - * - * For such cases, we now have a blacklist - */ -static struct kprobe_blackpoint kprobe_blacklist[] = { - {"preempt_schedule",}, - {"native_get_debugreg",}, - {NULL} /* Terminator */ -}; +/* Blacklist -- list of struct kprobe_blackpoint */ +static LIST_HEAD(kprobe_blacklist); #ifdef __ARCH_WANT_KPROBES_INSN_SLOT /* @@ -1321,9 +1311,9 @@ out: return ret; } -static int __kprobes in_kprobes_functions(unsigned long addr) +static int __kprobes in_nokprobe_functions(unsigned long addr) { - struct kprobe_blackpoint *kb; + struct kprobe_blackpoint *bp; /* The __kprobes marked functions and entry code must not be probed */ if ((addr >= (unsigned long)__kprobes_text_start && @@ -1335,12 +1325,10 @@ static int __kprobes in_kprobes_functions(unsigned long addr) * If there exists a kprobe_blacklist, verify and * fail any probe registration in the prohibited area */ - for (kb = kprobe_blacklist; kb->name != NULL; kb++) { - if (kb->start_addr) { - if (addr >= kb->start_addr && - addr < (kb->start_addr + kb->range)) - return -EINVAL; - } + list_for_each_entry(bp, &kprobe_blacklist, list) { + if (addr >= bp->start_addr && + addr < (bp->start_addr + bp->range)) + return -EINVAL; } return 0; } @@ -1433,7 +1421,7 @@ static __kprobes int check_kprobe_address_safe(struct kprobe *p, /* Ensure it is not in reserved area nor out of text */ if (!kernel_text_address((unsigned long) p->addr) || - in_kprobes_functions((unsigned long) p->addr) || + in_nokprobe_functions((unsigned long) p->addr) || jump_label_text_reserved(p->addr, p->addr)) { ret = -EINVAL; goto out; @@ -2062,14 +2050,41 @@ static struct notifier_block kprobe_module_nb = { .priority = 0 }; -static int __init init_kprobes(void) +/* + * 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 void populate_kprobe_blacklist(struct kprobe_blackpoint **start, + struct kprobe_blackpoint **end) { - int i, err = 0; + struct kprobe_blackpoint **iter, *bp; unsigned long offset = 0, size = 0; char *modname, namebuf[128]; const char *symbol_name; - void *addr; - struct kprobe_blackpoint *kb; + + for (iter = start; (unsigned long)iter < (unsigned long)end; iter++) { + bp = *iter; + symbol_name = kallsyms_lookup(bp->start_addr, + &size, &offset, &modname, namebuf); + if (!symbol_name) + continue; + + bp->range = size; + INIT_LIST_HEAD(&bp->list); + list_add_tail(&bp->list, &kprobe_blacklist); + } +} + +extern struct kprobe_blackpoint *__start_kprobe_blacklist[]; +extern struct kprobe_blackpoint *__stop_kprobe_blacklist[]; + +static int __init init_kprobes(void) +{ + int i, err = 0; /* FIXME allocate the probe table, currently defined statically */ /* initialize all list heads */ @@ -2079,27 +2094,8 @@ static int __init init_kprobes(void) raw_spin_lock_init(&(kretprobe_table_locks[i].lock)); } - /* - * 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. - */ - for (kb = kprobe_blacklist; kb->name != NULL; kb++) { - kprobe_lookup_name(kb->name, addr); - if (!addr) - continue; - - kb->start_addr = (unsigned long)addr; - symbol_name = kallsyms_lookup(kb->start_addr, - &size, &offset, &modname, namebuf); - if (!symbol_name) - kb->range = 0; - else - kb->range = size; - } + populate_kprobe_blacklist(__start_kprobe_blacklist, + __stop_kprobe_blacklist); if (kretprobe_blacklist_size) { /* lookup the function address from its name */ diff --git a/kernel/sched/core.c b/kernel/sched/core.c index 1deccd7..527fd78 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -2645,6 +2645,7 @@ asmlinkage void __sched notrace preempt_schedule(void) barrier(); } while (need_resched()); } +NOKPROBE_SYMBOL(preempt_schedule); EXPORT_SYMBOL(preempt_schedule); /*
Ingo Molnar
2013-Nov-11 11:16 UTC
[PATCH -tip RFC 0/2] kprobes: introduce NOKPROBE_SYMBOL() and prohibit probing on .entry.text
* Masami Hiramatsu <masami.hiramatsu.pt at hitachi.com> wrote:> Currently the blacklist is maintained by hand in kprobes.c > which is separated from the function definition and is hard > to catch up the kernel update. > To solve this issue, I've tried to implement new > NOKPROBE_SYMBOL() macro for making kprobe blacklist at > build time. Since the NOKPROBE_SYMBOL() macros can be placed > right after the function is defined, it is easy to maintain. > At this moment, I applied the macro only for the symbols > which is listed in kprobes.c. As we discussed in previous > thread, if the gcc accepts to introduce new annotation to > store the function address (and size) at somewhere, we can > easily move onto that by replacing NOKPROBE_SYMBOL() with > nokprobe annotation (and just modifying the > populate_kprobe_blacklist() a bit). > > This series also includes a change which prohibits probing > on the address in .entry.text because the code is used for > very low-level sensitive interrupt/syscall entries. Probing > such code may cause unexpected result (actually most of > that area is already in the kprobe blacklist). > So I've decide to prohibit probing all of them. > > Since Ingo wasn't convinced about the idea in the previous > discussion, I just make this series as RFC series. > I'd like to ask again with actual implementation and plan. > > Thank you, > > --- > > Masami Hiramatsu (2): > kprobes: Prohibit probing on .entry.text code > kprobes: Introduce NOKPROBE_SYMBOL() macro for blacklist > > > arch/x86/kernel/entry_32.S | 33 ------------ > arch/x86/kernel/entry_64.S | 20 -------- > arch/x86/kernel/paravirt.c | 4 ++ > include/asm-generic/vmlinux.lds.h | 9 +++ > include/linux/kprobes.h | 19 +++++++ > kernel/kprobes.c | 98 ++++++++++++++++++------------------- > kernel/sched/core.c | 1 > 7 files changed, 80 insertions(+), 104 deletions(-)Ok, I like it after all. Mind changing over arch/x86/kprobes/* to use this new facility? There's no sense in kprobes internals using two types After that we can convert all the rest, probably as part of this series. There's a good reason now to do it: it's not just about cleanliness, it will also impact generated code less. Thanks, Ingo
Masami Hiramatsu
2013-Nov-11 17:18 UTC
[PATCH -tip RFC 0/2] kprobes: introduce NOKPROBE_SYMBOL() and prohibit probing on .entry.text
(2013/11/11 20:16), Ingo Molnar wrote:> > * Masami Hiramatsu <masami.hiramatsu.pt at hitachi.com> wrote: > >> Currently the blacklist is maintained by hand in kprobes.c >> which is separated from the function definition and is hard >> to catch up the kernel update. >> To solve this issue, I've tried to implement new >> NOKPROBE_SYMBOL() macro for making kprobe blacklist at >> build time. Since the NOKPROBE_SYMBOL() macros can be placed >> right after the function is defined, it is easy to maintain. >> At this moment, I applied the macro only for the symbols >> which is listed in kprobes.c. As we discussed in previous >> thread, if the gcc accepts to introduce new annotation to >> store the function address (and size) at somewhere, we can >> easily move onto that by replacing NOKPROBE_SYMBOL() with >> nokprobe annotation (and just modifying the >> populate_kprobe_blacklist() a bit). >> >> This series also includes a change which prohibits probing >> on the address in .entry.text because the code is used for >> very low-level sensitive interrupt/syscall entries. Probing >> such code may cause unexpected result (actually most of >> that area is already in the kprobe blacklist). >> So I've decide to prohibit probing all of them. >> >> Since Ingo wasn't convinced about the idea in the previous >> discussion, I just make this series as RFC series. >> I'd like to ask again with actual implementation and plan. >> >> Thank you, >> >> --- >> >> Masami Hiramatsu (2): >> kprobes: Prohibit probing on .entry.text code >> kprobes: Introduce NOKPROBE_SYMBOL() macro for blacklist >> >> >> arch/x86/kernel/entry_32.S | 33 ------------ >> arch/x86/kernel/entry_64.S | 20 -------- >> arch/x86/kernel/paravirt.c | 4 ++ >> include/asm-generic/vmlinux.lds.h | 9 +++ >> include/linux/kprobes.h | 19 +++++++ >> kernel/kprobes.c | 98 ++++++++++++++++++------------------- >> kernel/sched/core.c | 1 >> 7 files changed, 80 insertions(+), 104 deletions(-) > > Ok, I like it after all. > > Mind changing over arch/x86/kprobes/* to use this new facility? There's no > sense in kprobes internals using two typesSure, that's why I introduced this :)> After that we can convert all the rest, probably as part of this series.OK, I'll do. :) BTW, converting all the __kprobes involves many archs, which kprobes ported. In that case, which mailing-list would better me to post the series, linux-arch?> > There's a good reason now to do it: it's not just about cleanliness, it > will also impact generated code less.Thank you! -- Masami HIRAMATSU IT Management Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu.pt at hitachi.com
Maybe Matching Threads
- [PATCH -tip RFC 0/2] kprobes: introduce NOKPROBE_SYMBOL() and prohibit probing on .entry.text
- [PATCH -tip RFC 0/2] kprobes: introduce NOKPROBE_SYMBOL() and prohibit probing on .entry.text
- [PATCH -tip RFC 0/2] kprobes: introduce NOKPROBE_SYMBOL() and prohibit probing on .entry.text
- [PATCH -tip v3 00/23] kprobes: introduce NOKPROBE_SYMBOL() and general cleaning of kprobe blacklist
- [PATCH -tip v3 00/23] kprobes: introduce NOKPROBE_SYMBOL() and general cleaning of kprobe blacklist