[PATCH] kprobes: x86_64: blacklist non-attachable interrupt functions

[PATCH] kprobes: x86_64: blacklist non-attachable interrupt functions

[Andrea Righi]

These interrupt functions are already non-attachable by kprobes.
Blacklist them explicitly so that they can show up in
/sys/kernel/debug/kprobes/blacklist and tools like BCC can use this
additional information.

Signed-off-by: Andrea Righi <righi.andrea@gmail.com>
---
 arch/x86/entry/entry_64.S | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index ce25d84023c0..1f0efdb7b629 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -566,6 +566,7 @@ ENTRY(interrupt_entry)
 
 	ret
 END(interrupt_entry)
+_ASM_NOKPROBE(interrupt_entry)
 
 
 /* Interrupt entry/exit. */
@@ -766,6 +767,7 @@ native_irq_return_ldt:
 	jmp	native_irq_return_iret
 #endif
 END(common_interrupt)
+_ASM_NOKPROBE(common_interrupt)
 
 /*
  * APIC interrupts.
@@ -780,6 +782,7 @@ ENTRY(\sym)
 	call	\do_sym	/* rdi points to pt_regs */
 	jmp	ret_from_intr
 END(\sym)
+_ASM_NOKPROBE(\sym)
 .endm
 
 /* Make sure APIC interrupt handlers end up in the irqentry section: */
@@ -960,6 +963,7 @@ ENTRY(\sym)
 
 	jmp	error_exit
 	.endif
+_ASM_NOKPROBE(\sym)
 END(\sym)
 .endm
 
-- 
2.17.1

[tip:perf/urgent] kprobes/x86: Blacklist non-attachable interrupt functions

[tip-bot for Andrea Righi]

Commit-ID:  a50480cb6d61d5c5fc13308479407b628b6bc1c5
Gitweb:     https://git.kernel.org/tip/a50480cb6d61d5c5fc13308479407b628b6bc1c5
Author:     Andrea Righi <righi.andrea@gmail.com>
AuthorDate: Thu, 6 Dec 2018 10:56:48 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 6 Dec 2018 16:52:03 +0100

kprobes/x86: Blacklist non-attachable interrupt functions

These interrupt functions are already non-attachable by kprobes.
Blacklist them explicitly so that they can show up in
/sys/kernel/debug/kprobes/blacklist and tools like BCC can use this
additional information.

Signed-off-by: Andrea Righi <righi.andrea@gmail.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: David S. Miller <davem@davemloft.net>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Yonghong Song <yhs@fb.com>
Link: http://lkml.kernel.org/r/20181206095648.GA8249@Dell
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/entry/entry_64.S | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index ce25d84023c0..1f0efdb7b629 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -566,6 +566,7 @@ ENTRY(interrupt_entry)
 
 	ret
 END(interrupt_entry)
+_ASM_NOKPROBE(interrupt_entry)
 
 
 /* Interrupt entry/exit. */
@@ -766,6 +767,7 @@ native_irq_return_ldt:
 	jmp	native_irq_return_iret
 #endif
 END(common_interrupt)
+_ASM_NOKPROBE(common_interrupt)
 
 /*
  * APIC interrupts.
@@ -780,6 +782,7 @@ ENTRY(\sym)
 	call	\do_sym	/* rdi points to pt_regs */
 	jmp	ret_from_intr
 END(\sym)
+_ASM_NOKPROBE(\sym)
 .endm
 
 /* Make sure APIC interrupt handlers end up in the irqentry section: */
@@ -960,6 +963,7 @@ ENTRY(\sym)
 
 	jmp	error_exit
 	.endif
+_ASM_NOKPROBE(\sym)
 END(\sym)
 .endm
 

Re: [PATCH] kprobes: x86_64: blacklist non-attachable interrupt functions

[Masami Hiramatsu]

On Thu, 6 Dec 2018 10:56:48 +0100
Andrea Righi <righi.andrea@gmail.com> wrote:

> These interrupt functions are already non-attachable by kprobes.
> Blacklist them explicitly so that they can show up in
> /sys/kernel/debug/kprobes/blacklist and tools like BCC can use this
> additional information.
> 
> Signed-off-by: Andrea Righi <righi.andrea@gmail.com>

For the short term, this is OK. But this is not the best way to cover
other functions which are also prohibited by arch_within_kprobe_blacklist().
I'll fix this issue later.

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

Thank you,

> ---
>  arch/x86/entry/entry_64.S | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
> index ce25d84023c0..1f0efdb7b629 100644
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -566,6 +566,7 @@ ENTRY(interrupt_entry)
>  
>  	ret
>  END(interrupt_entry)
> +_ASM_NOKPROBE(interrupt_entry)
>  
>  
>  /* Interrupt entry/exit. */
> @@ -766,6 +767,7 @@ native_irq_return_ldt:
>  	jmp	native_irq_return_iret
>  #endif
>  END(common_interrupt)
> +_ASM_NOKPROBE(common_interrupt)
>  
>  /*
>   * APIC interrupts.
> @@ -780,6 +782,7 @@ ENTRY(\sym)
>  	call	\do_sym	/* rdi points to pt_regs */
>  	jmp	ret_from_intr
>  END(\sym)
> +_ASM_NOKPROBE(\sym)
>  .endm
>  
>  /* Make sure APIC interrupt handlers end up in the irqentry section: */
> @@ -960,6 +963,7 @@ ENTRY(\sym)
>  
>  	jmp	error_exit
>  	.endif
> +_ASM_NOKPROBE(\sym)
>  END(\sym)
>  .endm
>  
> -- 
> 2.17.1


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] kprobes: x86_64: blacklist non-attachable interrupt functions

[Masami Hiramatsu]

Hi Andrea and Ingo,

Here is the patch what I meant. I just ran it on qemu-x86, and seemed working.
After introducing this patch, I will start adding arch_populate_kprobe_blacklist()
to some arches.

Thank you,

[RFC] kprobes: x86/kprobes: Blacklist symbols in arch-defined prohibited area

From: Masami Hiramatsu <mhiramat@kernel.org>

Blacklist symbols in arch-defined probe-prohibited areas.
With this change, user can see all symbols which are prohibited
to probe in debugfs.

All archtectures which have custom prohibit areas should define
its own arch_populate_kprobe_blacklist() function, but unless that,
all symbols marked __kprobes are blacklisted.

Reported-by: Andrea Righi <righi.andrea@gmail.com>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/x86/kernel/kprobes/core.c |   27 ++++++++++++++++++
 include/linux/kprobes.h        |    2 +
 kernel/kprobes.c               |   60 +++++++++++++++++++++++++++++++---------
 3 files changed, 75 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index c33b06f..52771e3 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -1034,6 +1034,33 @@ bool arch_within_kprobe_blacklist(unsigned long addr)
 		 addr < (unsigned long)__entry_text_end);
 }
 
+int __init arch_populate_kprobe_blacklist(void)
+{
+	unsigned long entry;
+	int ret = 0;
+
+	for (entry = (unsigned long)__kprobes_text_start;
+	     entry < (unsigned long)__kprobes_text_end;
+	     entry += ret) {
+		ret = kprobe_add_ksym_blacklist(entry);
+		if (ret < 0)
+			return ret;
+		if (ret == 0)	/* In case of alias symbol */
+			ret = 1;
+	}
+
+	for (entry = (unsigned long)__entry_text_start;
+	     entry < (unsigned long)__entry_text_end;
+	     entry += ret) {
+		ret = kprobe_add_ksym_blacklist(entry);
+		if (ret < 0)
+			return ret;
+		if (ret == 0)	/* In case of alias symbol */
+			ret = 1;
+	}
+	return 0;
+}
+
 int __init arch_init_kprobes(void)
 {
 	return 0;
diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
index e909413..a68cbbe 100644
--- a/include/linux/kprobes.h
+++ b/include/linux/kprobes.h
@@ -242,10 +242,12 @@ extern int arch_init_kprobes(void);
 extern void show_registers(struct pt_regs *regs);
 extern void kprobes_inc_nmissed_count(struct kprobe *p);
 extern bool arch_within_kprobe_blacklist(unsigned long addr);
+extern int arch_populate_kprobe_blacklist(void);
 extern bool arch_kprobe_on_func_entry(unsigned long offset);
 extern bool kprobe_on_func_entry(kprobe_opcode_t *addr, const char *sym, unsigned long offset);
 
 extern bool within_kprobe_blacklist(unsigned long addr);
+extern int kprobe_add_ksym_blacklist(unsigned long entry);
 
 struct kprobe_insn_cache {
 	struct mutex mutex;
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 90e98e2..8b2eb62 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -2093,6 +2093,44 @@ void dump_kprobe(struct kprobe *kp)
 }
 NOKPROBE_SYMBOL(dump_kprobe);
 
+int kprobe_add_ksym_blacklist(unsigned long entry)
+{
+	struct kprobe_blacklist_entry *ent;
+	unsigned long offset = 0, size = 0;
+
+	if (!kernel_text_address(entry) ||
+	    !kallsyms_lookup_size_offset(entry, &size, &offset))
+		return -EINVAL;
+
+	ent = kmalloc(sizeof(*ent), GFP_KERNEL);
+	if (!ent)
+		return -ENOMEM;
+	ent->start_addr = entry - offset;
+	ent->end_addr = entry - offset + size;
+	INIT_LIST_HEAD(&ent->list);
+	list_add_tail(&ent->list, &kprobe_blacklist);
+
+	return (int)size;
+}
+
+/* Add functions in arch defined probe-prohibited area */
+int __weak arch_populate_kprobe_blacklist(void)
+{
+	unsigned long entry;
+	int ret = 0;
+
+	for (entry = (unsigned long)__kprobes_text_start;
+	     entry < (unsigned long)__kprobes_text_end;
+	     entry += ret) {
+		ret = kprobe_add_ksym_blacklist(entry);
+		if (ret < 0)
+			return ret;
+		if (ret == 0)	/* In case of alias symbol */
+			ret = 1;
+	}
+	return 0;
+}
+
 /*
  * Lookup and populate the kprobe_blacklist.
  *
@@ -2104,26 +2142,20 @@ NOKPROBE_SYMBOL(dump_kprobe);
 static int __init populate_kprobe_blacklist(unsigned long *start,
 					     unsigned long *end)
 {
+	unsigned long entry;
 	unsigned long *iter;
-	struct kprobe_blacklist_entry *ent;
-	unsigned long entry, offset = 0, size = 0;
+	int ret;
 
 	for (iter = start; iter < end; iter++) {
 		entry = arch_deref_entry_point((void *)*iter);
-
-		if (!kernel_text_address(entry) ||
-		    !kallsyms_lookup_size_offset(entry, &size, &offset))
+		ret = kprobe_add_ksym_blacklist(entry);
+		if (ret == -EINVAL)
 			continue;
-
-		ent = kmalloc(sizeof(*ent), GFP_KERNEL);
-		if (!ent)
-			return -ENOMEM;
-		ent->start_addr = entry;
-		ent->end_addr = entry + size;
-		INIT_LIST_HEAD(&ent->list);
-		list_add_tail(&ent->list, &kprobe_blacklist);
+		if (ret < 0)
+			return ret;
 	}
-	return 0;
+
+	return arch_populate_kprobe_blacklist();
 }
 
 /* Module notifier call back, checking kprobes on the module */

Re: [PATCH] kprobes: x86_64: blacklist non-attachable interrupt functions

[Andrea Righi]

On Sat, Dec 08, 2018 at 01:01:20AM +0900, Masami Hiramatsu wrote:
> Hi Andrea and Ingo,
> 
> Here is the patch what I meant. I just ran it on qemu-x86, and seemed working.
> After introducing this patch, I will start adding arch_populate_kprobe_blacklist()
> to some arches.
> 
> Thank you,
> 
> [RFC] kprobes: x86/kprobes: Blacklist symbols in arch-defined prohibited area
> 
> From: Masami Hiramatsu <mhiramat@kernel.org>
> 
> Blacklist symbols in arch-defined probe-prohibited areas.
> With this change, user can see all symbols which are prohibited
> to probe in debugfs.
> 
> All archtectures which have custom prohibit areas should define
> its own arch_populate_kprobe_blacklist() function, but unless that,
> all symbols marked __kprobes are blacklisted.
> 
> Reported-by: Andrea Righi <righi.andrea@gmail.com>
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---

[snip]

> +int kprobe_add_ksym_blacklist(unsigned long entry)
> +{
> +	struct kprobe_blacklist_entry *ent;
> +	unsigned long offset = 0, size = 0;
> +
> +	if (!kernel_text_address(entry) ||
> +	    !kallsyms_lookup_size_offset(entry, &size, &offset))
> +		return -EINVAL;
> +
> +	ent = kmalloc(sizeof(*ent), GFP_KERNEL);
> +	if (!ent)
> +		return -ENOMEM;
> +	ent->start_addr = entry - offset;
> +	ent->end_addr = entry - offset + size;

Do we need to take offset into account? The code before wasn't using it.

> +	INIT_LIST_HEAD(&ent->list);
> +	list_add_tail(&ent->list, &kprobe_blacklist);
> +
> +	return (int)size;
> +}
> +
> +/* Add functions in arch defined probe-prohibited area */
> +int __weak arch_populate_kprobe_blacklist(void)
> +{
> +	unsigned long entry;
> +	int ret = 0;
> +
> +	for (entry = (unsigned long)__kprobes_text_start;
> +	     entry < (unsigned long)__kprobes_text_end;
> +	     entry += ret) {
> +		ret = kprobe_add_ksym_blacklist(entry);
> +		if (ret < 0)
> +			return ret;
> +		if (ret == 0)	/* In case of alias symbol */
> +			ret = 1;
> +	}
> +	return 0;
> +}
> +
>  /*
>   * Lookup and populate the kprobe_blacklist.
>   *
> @@ -2104,26 +2142,20 @@ NOKPROBE_SYMBOL(dump_kprobe);
>  static int __init populate_kprobe_blacklist(unsigned long *start,
>  					     unsigned long *end)
>  {
> +	unsigned long entry;
>  	unsigned long *iter;
> -	struct kprobe_blacklist_entry *ent;
> -	unsigned long entry, offset = 0, size = 0;
> +	int ret;
>  
>  	for (iter = start; iter < end; iter++) {
>  		entry = arch_deref_entry_point((void *)*iter);
> -
> -		if (!kernel_text_address(entry) ||
> -		    !kallsyms_lookup_size_offset(entry, &size, &offset))
> +		ret = kprobe_add_ksym_blacklist(entry);
> +		if (ret == -EINVAL)
>  			continue;
> -
> -		ent = kmalloc(sizeof(*ent), GFP_KERNEL);
> -		if (!ent)
> -			return -ENOMEM;
> -		ent->start_addr = entry;
> -		ent->end_addr = entry + size;
> -		INIT_LIST_HEAD(&ent->list);
> -		list_add_tail(&ent->list, &kprobe_blacklist);
> +		if (ret < 0)
> +			return ret;
>  	}
> -	return 0;
> +
> +	return arch_populate_kprobe_blacklist();
>  }
>  
>  /* Module notifier call back, checking kprobes on the module */

Re: [PATCH] kprobes: x86_64: blacklist non-attachable interrupt functions

[Masami Hiramatsu]

On Fri, 7 Dec 2018 18:00:26 +0100
Andrea Righi <righi.andrea@gmail.com> wrote:

> On Sat, Dec 08, 2018 at 01:01:20AM +0900, Masami Hiramatsu wrote:
> > Hi Andrea and Ingo,
> > 
> > Here is the patch what I meant. I just ran it on qemu-x86, and seemed working.
> > After introducing this patch, I will start adding arch_populate_kprobe_blacklist()
> > to some arches.
> > 
> > Thank you,
> > 
> > [RFC] kprobes: x86/kprobes: Blacklist symbols in arch-defined prohibited area
> > 
> > From: Masami Hiramatsu <mhiramat@kernel.org>
> > 
> > Blacklist symbols in arch-defined probe-prohibited areas.
> > With this change, user can see all symbols which are prohibited
> > to probe in debugfs.
> > 
> > All archtectures which have custom prohibit areas should define
> > its own arch_populate_kprobe_blacklist() function, but unless that,
> > all symbols marked __kprobes are blacklisted.
> > 
> > Reported-by: Andrea Righi <righi.andrea@gmail.com>
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > ---
> 
> [snip]
> 
> > +int kprobe_add_ksym_blacklist(unsigned long entry)
> > +{
> > +	struct kprobe_blacklist_entry *ent;
> > +	unsigned long offset = 0, size = 0;
> > +
> > +	if (!kernel_text_address(entry) ||
> > +	    !kallsyms_lookup_size_offset(entry, &size, &offset))
> > +		return -EINVAL;
> > +
> > +	ent = kmalloc(sizeof(*ent), GFP_KERNEL);
> > +	if (!ent)
> > +		return -ENOMEM;
> > +	ent->start_addr = entry - offset;
> > +	ent->end_addr = entry - offset + size;
> 
> Do we need to take offset into account? The code before wasn't using it.

Yes, if we hit an alias symbol (zero-size), we forcibly increment address
and retry it. In that case, offset will be 1.

> 
> > +	INIT_LIST_HEAD(&ent->list);
> > +	list_add_tail(&ent->list, &kprobe_blacklist);
> > +
> > +	return (int)size;
> > +}
> > +
> > +/* Add functions in arch defined probe-prohibited area */
> > +int __weak arch_populate_kprobe_blacklist(void)
> > +{
> > +	unsigned long entry;
> > +	int ret = 0;
> > +
> > +	for (entry = (unsigned long)__kprobes_text_start;
> > +	     entry < (unsigned long)__kprobes_text_end;
> > +	     entry += ret) {
> > +		ret = kprobe_add_ksym_blacklist(entry);
> > +		if (ret < 0)
> > +			return ret;
> > +		if (ret == 0)	/* In case of alias symbol */
> > +			ret = 1;

Here, we incremented.

Thank you,

> > +	}
> > +	return 0;
> > +}
> > +
> >  /*
> >   * Lookup and populate the kprobe_blacklist.
> >   *
> > @@ -2104,26 +2142,20 @@ NOKPROBE_SYMBOL(dump_kprobe);
> >  static int __init populate_kprobe_blacklist(unsigned long *start,
> >  					     unsigned long *end)
> >  {
> > +	unsigned long entry;
> >  	unsigned long *iter;
> > -	struct kprobe_blacklist_entry *ent;
> > -	unsigned long entry, offset = 0, size = 0;
> > +	int ret;
> >  
> >  	for (iter = start; iter < end; iter++) {
> >  		entry = arch_deref_entry_point((void *)*iter);
> > -
> > -		if (!kernel_text_address(entry) ||
> > -		    !kallsyms_lookup_size_offset(entry, &size, &offset))
> > +		ret = kprobe_add_ksym_blacklist(entry);
> > +		if (ret == -EINVAL)
> >  			continue;
> > -
> > -		ent = kmalloc(sizeof(*ent), GFP_KERNEL);
> > -		if (!ent)
> > -			return -ENOMEM;
> > -		ent->start_addr = entry;
> > -		ent->end_addr = entry + size;
> > -		INIT_LIST_HEAD(&ent->list);
> > -		list_add_tail(&ent->list, &kprobe_blacklist);
> > +		if (ret < 0)
> > +			return ret;
> >  	}
> > -	return 0;
> > +
> > +	return arch_populate_kprobe_blacklist();
> >  }
> >  
> >  /* Module notifier call back, checking kprobes on the module */


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] kprobes: x86_64: blacklist non-attachable interrupt functions

[Andrea Righi]

On Sat, Dec 08, 2018 at 12:42:10PM +0900, Masami Hiramatsu wrote:
> On Fri, 7 Dec 2018 18:00:26 +0100
> Andrea Righi <righi.andrea@gmail.com> wrote:
> 
> > On Sat, Dec 08, 2018 at 01:01:20AM +0900, Masami Hiramatsu wrote:
> > > Hi Andrea and Ingo,
> > > 
> > > Here is the patch what I meant. I just ran it on qemu-x86, and seemed working.
> > > After introducing this patch, I will start adding arch_populate_kprobe_blacklist()
> > > to some arches.
> > > 
> > > Thank you,
> > > 
> > > [RFC] kprobes: x86/kprobes: Blacklist symbols in arch-defined prohibited area
> > > 
> > > From: Masami Hiramatsu <mhiramat@kernel.org>
> > > 
> > > Blacklist symbols in arch-defined probe-prohibited areas.
> > > With this change, user can see all symbols which are prohibited
> > > to probe in debugfs.
> > > 
> > > All archtectures which have custom prohibit areas should define
> > > its own arch_populate_kprobe_blacklist() function, but unless that,
> > > all symbols marked __kprobes are blacklisted.
> > > 
> > > Reported-by: Andrea Righi <righi.andrea@gmail.com>
> > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > > ---
> > 
> > [snip]
> > 
> > > +int kprobe_add_ksym_blacklist(unsigned long entry)
> > > +{
> > > +	struct kprobe_blacklist_entry *ent;
> > > +	unsigned long offset = 0, size = 0;
> > > +
> > > +	if (!kernel_text_address(entry) ||
> > > +	    !kallsyms_lookup_size_offset(entry, &size, &offset))
> > > +		return -EINVAL;
> > > +
> > > +	ent = kmalloc(sizeof(*ent), GFP_KERNEL);
> > > +	if (!ent)
> > > +		return -ENOMEM;
> > > +	ent->start_addr = entry - offset;
> > > +	ent->end_addr = entry - offset + size;
> > 
> > Do we need to take offset into account? The code before wasn't using it.
> 
> Yes, if we hit an alias symbol (zero-size), we forcibly increment address
> and retry it. In that case, offset will be 1.
> 
> > 
> > > +	INIT_LIST_HEAD(&ent->list);
> > > +	list_add_tail(&ent->list, &kprobe_blacklist);
> > > +
> > > +	return (int)size;
> > > +}
> > > +
> > > +/* Add functions in arch defined probe-prohibited area */
> > > +int __weak arch_populate_kprobe_blacklist(void)
> > > +{
> > > +	unsigned long entry;
> > > +	int ret = 0;
> > > +
> > > +	for (entry = (unsigned long)__kprobes_text_start;
> > > +	     entry < (unsigned long)__kprobes_text_end;
> > > +	     entry += ret) {
> > > +		ret = kprobe_add_ksym_blacklist(entry);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +		if (ret == 0)	/* In case of alias symbol */
> > > +			ret = 1;
> 
> Here, we incremented.
> 
> Thank you,

Makes sense, thanks for the clarification.

-Andrea

Re: [PATCH] kprobes: x86_64: blacklist non-attachable interrupt functions

[Andrea Righi]

On Sat, Dec 08, 2018 at 01:01:20AM +0900, Masami Hiramatsu wrote:
> Hi Andrea and Ingo,
> 
> Here is the patch what I meant. I just ran it on qemu-x86, and seemed working.
> After introducing this patch, I will start adding arch_populate_kprobe_blacklist()
> to some arches.
> 
> Thank you,
> 
> [RFC] kprobes: x86/kprobes: Blacklist symbols in arch-defined prohibited area
> 
> From: Masami Hiramatsu <mhiramat@kernel.org>
> 
> Blacklist symbols in arch-defined probe-prohibited areas.
> With this change, user can see all symbols which are prohibited
> to probe in debugfs.
> 
> All archtectures which have custom prohibit areas should define
> its own arch_populate_kprobe_blacklist() function, but unless that,
> all symbols marked __kprobes are blacklisted.

What about iterating all symbols and use arch_within_kprobe_blacklist()
to check if we need to blacklist them or not.

In this way we don't have to introduce an
arch_populate_kprobe_blacklist() for each architecture.

Something like the following maybe.

Thanks.

[RFC] kprobes: blacklist all symbols in arch-defined prohibited area

From: Andrea Righi <righi.andrea@gmail.com>

Blacklist symbols in arch-defined probe-prohibited areas.
With this change, user can see all symbols which are prohibited
to probe in debugfs.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Andrea Righi <righi.andrea@gmail.com>
---
 kernel/kprobes.c | 55 +++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 41 insertions(+), 14 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 90e98e233647..e67598dd7468 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -2093,6 +2093,35 @@ void dump_kprobe(struct kprobe *kp)
 }
 NOKPROBE_SYMBOL(dump_kprobe);
 
+static int kprobe_blacklist_add(unsigned long entry)
+{
+	struct kprobe_blacklist_entry *ent;
+	unsigned long offset = 0, size = 0;
+
+	if (!kernel_text_address(entry) ||
+	    !kallsyms_lookup_size_offset(entry, &size, &offset))
+		return -EINVAL;
+
+	ent = kmalloc(sizeof(*ent), GFP_KERNEL);
+	if (!ent)
+		return -ENOMEM;
+	ent->start_addr = entry;
+	ent->end_addr = entry + size;
+	INIT_LIST_HEAD(&ent->list);
+	list_add_tail(&ent->list, &kprobe_blacklist);
+
+	return 0;
+}
+
+static int arch_populate_kprobe_blacklist(void *data, const char *name,
+					  struct module *mod,
+					  unsigned long entry)
+{
+	if (arch_within_kprobe_blacklist(entry))
+		kprobe_blacklist_add(entry);
+	return 0;
+}
+
 /*
  * Lookup and populate the kprobe_blacklist.
  *
@@ -2104,24 +2133,22 @@ NOKPROBE_SYMBOL(dump_kprobe);
 static int __init populate_kprobe_blacklist(unsigned long *start,
 					     unsigned long *end)
 {
-	unsigned long *iter;
-	struct kprobe_blacklist_entry *ent;
-	unsigned long entry, offset = 0, size = 0;
+	unsigned long entry, *iter;
+	int ret;
 
+	/* Blacklist all arch_within_kprobe_blacklist() symbols */
+        mutex_lock(&module_mutex);
+	kallsyms_on_each_symbol(arch_populate_kprobe_blacklist, NULL);
+        mutex_unlock(&module_mutex);
+
+	/* Add explicitly blacklisted symbols */
 	for (iter = start; iter < end; iter++) {
 		entry = arch_deref_entry_point((void *)*iter);
-
-		if (!kernel_text_address(entry) ||
-		    !kallsyms_lookup_size_offset(entry, &size, &offset))
+		ret = kprobe_blacklist_add(entry);
+		if (ret == -EINVAL)
 			continue;
-
-		ent = kmalloc(sizeof(*ent), GFP_KERNEL);
-		if (!ent)
-			return -ENOMEM;
-		ent->start_addr = entry;
-		ent->end_addr = entry + size;
-		INIT_LIST_HEAD(&ent->list);
-		list_add_tail(&ent->list, &kprobe_blacklist);
+		if (ret < 0)
+			return ret;
 	}
 	return 0;
 }

Re: [PATCH] kprobes: x86_64: blacklist non-attachable interrupt functions

[Masami Hiramatsu]

On Fri, 7 Dec 2018 18:58:05 +0100
Andrea Righi <righi.andrea@gmail.com> wrote:

> On Sat, Dec 08, 2018 at 01:01:20AM +0900, Masami Hiramatsu wrote:
> > Hi Andrea and Ingo,
> > 
> > Here is the patch what I meant. I just ran it on qemu-x86, and seemed working.
> > After introducing this patch, I will start adding arch_populate_kprobe_blacklist()
> > to some arches.
> > 
> > Thank you,
> > 
> > [RFC] kprobes: x86/kprobes: Blacklist symbols in arch-defined prohibited area
> > 
> > From: Masami Hiramatsu <mhiramat@kernel.org>
> > 
> > Blacklist symbols in arch-defined probe-prohibited areas.
> > With this change, user can see all symbols which are prohibited
> > to probe in debugfs.
> > 
> > All archtectures which have custom prohibit areas should define
> > its own arch_populate_kprobe_blacklist() function, but unless that,
> > all symbols marked __kprobes are blacklisted.
> 
> What about iterating all symbols and use arch_within_kprobe_blacklist()
> to check if we need to blacklist them or not.

Sorry, I don't want to iterate all ksyms since it may take a long time
(especially embedded small devices.)

> 
> In this way we don't have to introduce an
> arch_populate_kprobe_blacklist() for each architecture.

Hmm, I had a same idea, but there are some arch which prohibit probing
extable entries (e.g. arm64.) For correctness of the blacklist, I think
it should be listed (not entire the function body).
I also rather like to remove arch_within_kprobe_blacklist() instead.

Thank you,

> 
> Something like the following maybe.
> 
> Thanks.
> 
> [RFC] kprobes: blacklist all symbols in arch-defined prohibited area
> 
> From: Andrea Righi <righi.andrea@gmail.com>
> 
> Blacklist symbols in arch-defined probe-prohibited areas.
> With this change, user can see all symbols which are prohibited
> to probe in debugfs.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> Signed-off-by: Andrea Righi <righi.andrea@gmail.com>
> ---
>  kernel/kprobes.c | 55 +++++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 41 insertions(+), 14 deletions(-)
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 90e98e233647..e67598dd7468 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -2093,6 +2093,35 @@ void dump_kprobe(struct kprobe *kp)
>  }
>  NOKPROBE_SYMBOL(dump_kprobe);
>  
> +static int kprobe_blacklist_add(unsigned long entry)
> +{
> +	struct kprobe_blacklist_entry *ent;
> +	unsigned long offset = 0, size = 0;
> +
> +	if (!kernel_text_address(entry) ||
> +	    !kallsyms_lookup_size_offset(entry, &size, &offset))
> +		return -EINVAL;
> +
> +	ent = kmalloc(sizeof(*ent), GFP_KERNEL);
> +	if (!ent)
> +		return -ENOMEM;
> +	ent->start_addr = entry;
> +	ent->end_addr = entry + size;
> +	INIT_LIST_HEAD(&ent->list);
> +	list_add_tail(&ent->list, &kprobe_blacklist);
> +
> +	return 0;
> +}
> +
> +static int arch_populate_kprobe_blacklist(void *data, const char *name,
> +					  struct module *mod,
> +					  unsigned long entry)
> +{
> +	if (arch_within_kprobe_blacklist(entry))
> +		kprobe_blacklist_add(entry);
> +	return 0;
> +}
> +
>  /*
>   * Lookup and populate the kprobe_blacklist.
>   *
> @@ -2104,24 +2133,22 @@ NOKPROBE_SYMBOL(dump_kprobe);
>  static int __init populate_kprobe_blacklist(unsigned long *start,
>  					     unsigned long *end)
>  {
> -	unsigned long *iter;
> -	struct kprobe_blacklist_entry *ent;
> -	unsigned long entry, offset = 0, size = 0;
> +	unsigned long entry, *iter;
> +	int ret;
>  
> +	/* Blacklist all arch_within_kprobe_blacklist() symbols */
> +        mutex_lock(&module_mutex);
> +	kallsyms_on_each_symbol(arch_populate_kprobe_blacklist, NULL);
> +        mutex_unlock(&module_mutex);
> +
> +	/* Add explicitly blacklisted symbols */
>  	for (iter = start; iter < end; iter++) {
>  		entry = arch_deref_entry_point((void *)*iter);
> -
> -		if (!kernel_text_address(entry) ||
> -		    !kallsyms_lookup_size_offset(entry, &size, &offset))
> +		ret = kprobe_blacklist_add(entry);
> +		if (ret == -EINVAL)
>  			continue;
> -
> -		ent = kmalloc(sizeof(*ent), GFP_KERNEL);
> -		if (!ent)
> -			return -ENOMEM;
> -		ent->start_addr = entry;
> -		ent->end_addr = entry + size;
> -		INIT_LIST_HEAD(&ent->list);
> -		list_add_tail(&ent->list, &kprobe_blacklist);
> +		if (ret < 0)
> +			return ret;
>  	}
>  	return 0;
>  }


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH] kprobes: x86_64: blacklist non-attachable interrupt functions

[Andrea Righi]

On Sat, Dec 08, 2018 at 12:48:59PM +0900, Masami Hiramatsu wrote:
> On Fri, 7 Dec 2018 18:58:05 +0100
> Andrea Righi <righi.andrea@gmail.com> wrote:
> 
> > On Sat, Dec 08, 2018 at 01:01:20AM +0900, Masami Hiramatsu wrote:
> > > Hi Andrea and Ingo,
> > > 
> > > Here is the patch what I meant. I just ran it on qemu-x86, and seemed working.
> > > After introducing this patch, I will start adding arch_populate_kprobe_blacklist()
> > > to some arches.
> > > 
> > > Thank you,
> > > 
> > > [RFC] kprobes: x86/kprobes: Blacklist symbols in arch-defined prohibited area
> > > 
> > > From: Masami Hiramatsu <mhiramat@kernel.org>
> > > 
> > > Blacklist symbols in arch-defined probe-prohibited areas.
> > > With this change, user can see all symbols which are prohibited
> > > to probe in debugfs.
> > > 
> > > All archtectures which have custom prohibit areas should define
> > > its own arch_populate_kprobe_blacklist() function, but unless that,
> > > all symbols marked __kprobes are blacklisted.
> > 
> > What about iterating all symbols and use arch_within_kprobe_blacklist()
> > to check if we need to blacklist them or not.
> 
> Sorry, I don't want to iterate all ksyms since it may take a long time
> (especially embedded small devices.)
> 
> > 
> > In this way we don't have to introduce an
> > arch_populate_kprobe_blacklist() for each architecture.
> 
> Hmm, I had a same idea, but there are some arch which prohibit probing
> extable entries (e.g. arm64.) For correctness of the blacklist, I think
> it should be listed (not entire the function body).
> I also rather like to remove arch_within_kprobe_blacklist() instead.

OK. Thanks.

-Andrea

Re: [PATCH] kprobes: x86_64: blacklist non-attachable interrupt functions

[Masami Hiramatsu]

On Sat, 8 Dec 2018 08:09:22 +0100
Andrea Righi <righi.andrea@gmail.com> wrote:

> On Sat, Dec 08, 2018 at 12:48:59PM +0900, Masami Hiramatsu wrote:
> > On Fri, 7 Dec 2018 18:58:05 +0100
> > Andrea Righi <righi.andrea@gmail.com> wrote:
> > 
> > > On Sat, Dec 08, 2018 at 01:01:20AM +0900, Masami Hiramatsu wrote:
> > > > Hi Andrea and Ingo,
> > > > 
> > > > Here is the patch what I meant. I just ran it on qemu-x86, and seemed working.
> > > > After introducing this patch, I will start adding arch_populate_kprobe_blacklist()
> > > > to some arches.
> > > > 
> > > > Thank you,
> > > > 
> > > > [RFC] kprobes: x86/kprobes: Blacklist symbols in arch-defined prohibited area
> > > > 
> > > > From: Masami Hiramatsu <mhiramat@kernel.org>
> > > > 
> > > > Blacklist symbols in arch-defined probe-prohibited areas.
> > > > With this change, user can see all symbols which are prohibited
> > > > to probe in debugfs.
> > > > 
> > > > All archtectures which have custom prohibit areas should define
> > > > its own arch_populate_kprobe_blacklist() function, but unless that,
> > > > all symbols marked __kprobes are blacklisted.
> > > 
> > > What about iterating all symbols and use arch_within_kprobe_blacklist()
> > > to check if we need to blacklist them or not.
> > 
> > Sorry, I don't want to iterate all ksyms since it may take a long time
> > (especially embedded small devices.)
> > 
> > > 
> > > In this way we don't have to introduce an
> > > arch_populate_kprobe_blacklist() for each architecture.
> > 
> > Hmm, I had a same idea, but there are some arch which prohibit probing
> > extable entries (e.g. arm64.) For correctness of the blacklist, I think
> > it should be listed (not entire the function body).

Aah, my bad memory. I remember that the extable should not be in blacklist,
since it is a kind of "unsupported instruction to be probed" on some arch.
I'll move that in arch_prepare_kprobe() on arm64.

> > I also rather like to remove arch_within_kprobe_blacklist() instead.

Anyway, this is true. If I could make a complete list, I will remove
this function.

Thank you,

> 
> OK. Thanks.
> 
> -Andrea


-- 
Masami Hiramatsu <mhiramat@kernel.org>