[PATCH v3 0/7] kprobes: Fix %p in kprobes

[PATCH v3 0/7] kprobes: Fix %p in kprobes

[Masami Hiramatsu]

Hi,

This 3rd version of the series which fixes %p uses in kprobes.
Some by replacing with %pS, some by replacing with %px but
masking with kallsyms_show_value().

I've read the thread about %pK and if I understand correctly
we shouldn't print kernel addresses. However, kprobes debugfs
interface can not stop to show the actual probe address because
it should be compared with addresses in kallsyms for debugging.
So, it depends on that kallsyms_show_value() allows to show
address to user, because if it returns true, anyway that user
can dump /proc/kallsyms.
Other error messages are replaced it with %pS or just removed.

This series also including some fixes for arch ports too.

Changes in this version;
 - [2/7]: Updated for the latest linus tree.
 - [4/7][5/7]: Do not use %px.

Thank you,

---

Masami Hiramatsu (7):
      kprobes: Make blacklist root user read only
      kprobes: Show blacklist addresses as same as kallsyms does
      kprobes: Show address of kprobes if kallsyms does
      kprobes: Replace %p with other pointer types
      kprobes/x86: Fix %p uses in error messages
      kprobes/arm: Fix %p uses in error messages
      kprobes/arm64: Fix %p uses in error messages


 arch/arm/probes/kprobes/core.c      |   10 ++++---
 arch/arm/probes/kprobes/test-core.c |    1 -
 arch/arm64/kernel/probes/kprobes.c  |    4 +--
 arch/x86/kernel/kprobes/core.c      |   12 +++------
 kernel/kprobes.c                    |   48 ++++++++++++++++++++++-------------
 5 files changed, 41 insertions(+), 34 deletions(-)

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

[PATCH v3 0/7] kprobes: Fix %p in kprobes

[Masami Hiramatsu]

Hi,

This 3rd version of the series which fixes %p uses in kprobes.
Some by replacing with %pS, some by replacing with %px but
masking with kallsyms_show_value().

I've read the thread about %pK and if I understand correctly
we shouldn't print kernel addresses. However, kprobes debugfs
interface can not stop to show the actual probe address because
it should be compared with addresses in kallsyms for debugging.
So, it depends on that kallsyms_show_value() allows to show
address to user, because if it returns true, anyway that user
can dump /proc/kallsyms.
Other error messages are replaced it with %pS or just removed.

This series also including some fixes for arch ports too.

Changes in this version;
 - [2/7]: Updated for the latest linus tree.
 - [4/7][5/7]: Do not use %px.

Thank you,

---

Masami Hiramatsu (7):
      kprobes: Make blacklist root user read only
      kprobes: Show blacklist addresses as same as kallsyms does
      kprobes: Show address of kprobes if kallsyms does
      kprobes: Replace %p with other pointer types
      kprobes/x86: Fix %p uses in error messages
      kprobes/arm: Fix %p uses in error messages
      kprobes/arm64: Fix %p uses in error messages


 arch/arm/probes/kprobes/core.c      |   10 ++++---
 arch/arm/probes/kprobes/test-core.c |    1 -
 arch/arm64/kernel/probes/kprobes.c  |    4 +--
 arch/x86/kernel/kprobes/core.c      |   12 +++------
 kernel/kprobes.c                    |   48 ++++++++++++++++++++++-------------
 5 files changed, 41 insertions(+), 34 deletions(-)

[PATCH v3 1/7] kprobes: Make blacklist root user read only

[Masami Hiramatsu]

Since the blacklist file indicates a sensitive address
information to reader, it should be restricted to the
root user.

Suggested-by: Thomas Richter <tmricht@linux.ibm.com>
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 kernel/kprobes.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index ea619021d901..51096eece801 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -2621,7 +2621,7 @@ static int __init debugfs_kprobe_init(void)
 	if (!file)
 		goto error;
 
-	file = debugfs_create_file("blacklist", 0444, dir, NULL,
+	file = debugfs_create_file("blacklist", 0400, dir, NULL,
 				&debugfs_kprobe_blacklist_ops);
 	if (!file)
 		goto error;

[PATCH v3 2/7] kprobes: Show blacklist addresses as same as kallsyms does

[Masami Hiramatsu]

Show kprobes blacklist addresses under same condition of
showing kallsyms addresses.

Since there are several name conflict for local symbols,
kprobe blacklist needs to show each addresses so that
user can identify where is on blacklist by comparing
with kallsyms.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 Changes in v3:
  - Updated based on the latest linus tree.
---
 kernel/kprobes.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 51096eece801..e7d7e3e8598a 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -2427,9 +2427,17 @@ static int kprobe_blacklist_seq_show(struct seq_file *m, void *v)
 {
 	struct kprobe_blacklist_entry *ent =
 		list_entry(v, struct kprobe_blacklist_entry, list);
+	void *start = (void *)ent->start_addr, *end = (void *)ent->end_addr;
 
-	seq_printf(m, "0x%px-0x%px\t%ps\n", (void *)ent->start_addr,
-		   (void *)ent->end_addr, (void *)ent->start_addr);
+	/*
+	 * As long as kallsyms shows the address, kprobes blacklist also
+	 * show it, Or, it shows null address and symbol.
+	 */
+	if (!kallsyms_show_value())
+		start = end = NULL;
+
+	seq_printf(m, "0x%px-0x%px\t%ps\n", start, end,
+		   (void *)ent->start_addr);
 	return 0;
 }
 

[PATCH v3 3/7] kprobes: Show address of kprobes if kallsyms does

[Masami Hiramatsu]

Show probed address in debugfs kprobe list file as same
as kallsyms does. This information is used for checking
kprobes are placed in the expected address. So it should
be able to compared with address in kallsyms.

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

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index e7d7e3e8598a..94af7c99cf81 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -2326,6 +2326,7 @@ static void report_probe(struct seq_file *pi, struct kprobe *p,
 		const char *sym, int offset, char *modname, struct kprobe *pp)
 {
 	char *kprobe_type;
+	void *addr = p->addr;
 
 	if (p->pre_handler == pre_handler_kretprobe)
 		kprobe_type = "r";
@@ -2334,13 +2335,16 @@ static void report_probe(struct seq_file *pi, struct kprobe *p,
 	else
 		kprobe_type = "k";
 
+	if (!kallsyms_show_value())
+		addr = NULL;
+
 	if (sym)
-		seq_printf(pi, "%p  %s  %s+0x%x  %s ",
-			p->addr, kprobe_type, sym, offset,
+		seq_printf(pi, "%px  %s  %s+0x%x  %s ",
+			addr, kprobe_type, sym, offset,
 			(modname ? modname : " "));
-	else
-		seq_printf(pi, "%p  %s  %p ",
-			p->addr, kprobe_type, p->addr);
+	else	/* try to use %pS */
+		seq_printf(pi, "%px  %s  %pS ",
+			addr, kprobe_type, p->addr);
 
 	if (!pp)
 		pp = p;

[PATCH v3 4/7] kprobes: Replace %p with other pointer types

[Masami Hiramatsu]

Replace %p with %pS or just remove it if unneeded.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 Changes in v3:
  - Do not use %px in any case.
 Changes in v2:
  - Rebased on linux-next
---
 kernel/kprobes.c |   20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 94af7c99cf81..a72ff4999b11 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -712,7 +712,7 @@ static void reuse_unused_kprobe(struct kprobe *ap)
 	op = container_of(ap, struct optimized_kprobe, kp);
 	if (unlikely(list_empty(&op->list)))
 		printk(KERN_WARNING "Warning: found a stray unused "
-			"aggrprobe@%p\n", ap->addr);
+			"aggrprobe@%pS\n", ap->addr);
 	/* Enable the probe again */
 	ap->flags &= ~KPROBE_FLAG_DISABLED;
 	/* Optimize it again (remove from op->list) */
@@ -985,7 +985,8 @@ static int arm_kprobe_ftrace(struct kprobe *p)
 	ret = ftrace_set_filter_ip(&kprobe_ftrace_ops,
 				   (unsigned long)p->addr, 0, 0);
 	if (ret) {
-		pr_debug("Failed to arm kprobe-ftrace at %p (%d)\n", p->addr, ret);
+		pr_debug("Failed to arm kprobe-ftrace at %pS (%d)\n",
+			 p->addr, ret);
 		return ret;
 	}
 
@@ -1025,7 +1026,8 @@ static int disarm_kprobe_ftrace(struct kprobe *p)
 
 	ret = ftrace_set_filter_ip(&kprobe_ftrace_ops,
 			   (unsigned long)p->addr, 1, 0);
-	WARN(ret < 0, "Failed to disarm kprobe-ftrace at %p (%d)\n", p->addr, ret);
+	WARN(ret < 0, "Failed to disarm kprobe-ftrace at %pS (%d)\n",
+	     p->addr, ret);
 	return ret;
 }
 #else	/* !CONFIG_KPROBES_ON_FTRACE */
@@ -2169,11 +2171,12 @@ int enable_kprobe(struct kprobe *kp)
 }
 EXPORT_SYMBOL_GPL(enable_kprobe);
 
+/* Caller must NOT call this in usual path. This is only for critical case */
 void dump_kprobe(struct kprobe *kp)
 {
-	printk(KERN_WARNING "Dumping kprobe:\n");
-	printk(KERN_WARNING "Name: %s\nAddress: %p\nOffset: %x\n",
-	       kp->symbol_name, kp->addr, kp->offset);
+	pr_err("Dumping kprobe:\n");
+	pr_err("Name: %s\nOffset: %x\nAddress: %pS\n",
+	       kp->symbol_name, kp->offset, kp->addr);
 }
 NOKPROBE_SYMBOL(dump_kprobe);
 
@@ -2196,11 +2199,8 @@ static int __init populate_kprobe_blacklist(unsigned long *start,
 		entry = arch_deref_entry_point((void *)*iter);
 
 		if (!kernel_text_address(entry) ||
-		    !kallsyms_lookup_size_offset(entry, &size, &offset)) {
-			pr_err("Failed to find blacklist at %p\n",
-				(void *)entry);
+		    !kallsyms_lookup_size_offset(entry, &size, &offset))
 			continue;
-		}
 
 		ent = kmalloc(sizeof(*ent), GFP_KERNEL);
 		if (!ent)

[PATCH v3 5/7] kprobes/x86: Fix %p uses in error messages

[Masami Hiramatsu]

Fix %p uses in error messages in kprobes/x86.
- Some %p uses are not needed. Just remove it (or remove message).
- One %p use seems important to show some address
  so replaced with %pS.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 Changes in v3:
  - Do not use %px.
---
 arch/x86/kernel/kprobes/core.c |   12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c
index 0715f827607c..d2c9d5c8e4ce 100644
--- a/arch/x86/kernel/kprobes/core.c
+++ b/arch/x86/kernel/kprobes/core.c
@@ -391,8 +391,6 @@ int __copy_instruction(u8 *dest, u8 *src, u8 *real, struct insn *insn)
 			  - (u8 *) real;
 		if ((s64) (s32) newdisp != newdisp) {
 			pr_err("Kprobes error: new displacement does not fit into s32 (%llx)\n", newdisp);
-			pr_err("\tSrc: %p, Dest: %p, old disp: %x\n",
-				src, real, insn->displacement.value);
 			return 0;
 		}
 		disp = (u8 *) dest + insn_offset_displacement(insn);
@@ -636,8 +634,7 @@ static int reenter_kprobe(struct kprobe *p, struct pt_regs *regs,
 		 * Raise a BUG or we'll continue in an endless reentering loop
 		 * and eventually a stack overflow.
 		 */
-		printk(KERN_WARNING "Unrecoverable kprobe detected at %p.\n",
-		       p->addr);
+		pr_err("Unrecoverable kprobe detected.\n");
 		dump_kprobe(p);
 		BUG();
 	default:
@@ -1146,12 +1143,11 @@ int longjmp_break_handler(struct kprobe *p, struct pt_regs *regs)
 	    (addr < (u8 *) jprobe_return_end)) {
 		if (stack_addr(regs) != saved_sp) {
 			struct pt_regs *saved_regs = &kcb->jprobe_saved_regs;
-			printk(KERN_ERR
-			       "current sp %p does not match saved sp %p\n",
+			pr_err("current sp %pS does not match saved sp %pS\n",
 			       stack_addr(regs), saved_sp);
-			printk(KERN_ERR "Saved registers for jprobe %p\n", jp);
+			pr_err("Saved registers for jprobe\n");
 			show_regs(saved_regs);
-			printk(KERN_ERR "Current registers\n");
+			pr_err("Current registers\n");
 			show_regs(regs);
 			BUG();
 		}

[PATCH v3 6/7] kprobes/arm: Fix %p uses in error messages

[Masami Hiramatsu]

Fix %p uses in error messages by removing it and
using general dumper.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/arm/probes/kprobes/core.c      |   10 +++++-----
 arch/arm/probes/kprobes/test-core.c |    1 -
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/arch/arm/probes/kprobes/core.c b/arch/arm/probes/kprobes/core.c
index e90cc8a08186..e90165a56620 100644
--- a/arch/arm/probes/kprobes/core.c
+++ b/arch/arm/probes/kprobes/core.c
@@ -289,8 +289,8 @@ void __kprobes kprobe_handler(struct pt_regs *regs)
 				break;
 			case KPROBE_REENTER:
 				/* A nested probe was hit in FIQ, it is a BUG */
-				pr_warn("Unrecoverable kprobe detected at %p.\n",
-					p->addr);
+				pr_warn("Unrecoverable kprobe detected.\n");
+				dump_kprobe(p);
 				/* fall through */
 			default:
 				/* impossible cases */
@@ -615,11 +615,11 @@ int __kprobes longjmp_break_handler(struct kprobe *p, struct pt_regs *regs)
 		if (orig_sp != stack_addr) {
 			struct pt_regs *saved_regs =
 				(struct pt_regs *)kcb->jprobe_saved_regs.ARM_sp;
-			printk("current sp %lx does not match saved sp %lx\n",
+			pr_err("current sp %lx does not match saved sp %lx\n",
 			       orig_sp, stack_addr);
-			printk("Saved registers for jprobe %p\n", jp);
+			pr_err("Saved registers for jprobe\n");
 			show_regs(saved_regs);
-			printk("Current registers\n");
+			pr_err("Current registers\n");
 			show_regs(regs);
 			BUG();
 		}
diff --git a/arch/arm/probes/kprobes/test-core.c b/arch/arm/probes/kprobes/test-core.c
index 9ed0129bed3c..b5c892e24244 100644
--- a/arch/arm/probes/kprobes/test-core.c
+++ b/arch/arm/probes/kprobes/test-core.c
@@ -1460,7 +1460,6 @@ static bool check_test_results(void)
 	print_registers(&result_regs);
 
 	if (mem) {
-		pr_err("current_stack=%p\n", current_stack);
 		pr_err("expected_memory:\n");
 		print_memory(expected_memory, mem_size);
 		pr_err("result_memory:\n");

[PATCH v3 7/7] kprobes/arm64: Fix %p uses in error messages

[Masami Hiramatsu]

Fix %p uses in error messages by removing it because
those are redundant or meaningless.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
Acked-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/kernel/probes/kprobes.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/probes/kprobes.c b/arch/arm64/kernel/probes/kprobes.c
index d849d9804011..34f78d07a068 100644
--- a/arch/arm64/kernel/probes/kprobes.c
+++ b/arch/arm64/kernel/probes/kprobes.c
@@ -275,7 +275,7 @@ static int __kprobes reenter_kprobe(struct kprobe *p,
 		break;
 	case KPROBE_HIT_SS:
 	case KPROBE_REENTER:
-		pr_warn("Unrecoverable kprobe detected at %p.\n", p->addr);
+		pr_warn("Unrecoverable kprobe detected.\n");
 		dump_kprobe(p);
 		BUG();
 		break;
@@ -521,7 +521,7 @@ int __kprobes longjmp_break_handler(struct kprobe *p, struct pt_regs *regs)
 		    (struct pt_regs *)kcb->jprobe_saved_regs.sp;
 		pr_err("current sp %lx does not match saved sp %lx\n",
 		       orig_sp, stack_addr);
-		pr_err("Saved registers for jprobe %p\n", jp);
+		pr_err("Saved registers for jprobe\n");
 		__show_regs(saved_regs);
 		pr_err("Current registers\n");
 		__show_regs(regs);

Re: [PATCH v3 1/7] kprobes: Make blacklist root user read only

[Greg KH]

On Fri, Apr 27, 2018 at 03:40:09PM +0900, Masami Hiramatsu wrote:
> Since the blacklist file indicates a sensitive address
> information to reader, it should be restricted to the
> root user.
> 
> Suggested-by: Thomas Richter <tmricht@linux.ibm.com>
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  kernel/kprobes.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>

If you want these to go to the stable tree(s), that is great, but please
mark them properly.  As you did it here for this series, it will not
work.

thanks,

greg k-h

Re: [PATCH v3 4/7] kprobes: Replace %p with other pointer types

[Ingo Molnar]

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

> @@ -712,7 +712,7 @@ static void reuse_unused_kprobe(struct kprobe *ap)
>  	op = container_of(ap, struct optimized_kprobe, kp);
>  	if (unlikely(list_empty(&op->list)))
>  		printk(KERN_WARNING "Warning: found a stray unused "
> -			"aggrprobe@%p\n", ap->addr);
> +			"aggrprobe@%pS\n", ap->addr);

Is this a kernel bug? If yes then please convert this to a WARN_ON_ONCE() that 
doesn't print any kernel addresses.

>  	/* Enable the probe again */
>  	ap->flags &= ~KPROBE_FLAG_DISABLED;
>  	/* Optimize it again (remove from op->list) */
> @@ -985,7 +985,8 @@ static int arm_kprobe_ftrace(struct kprobe *p)
>  	ret = ftrace_set_filter_ip(&kprobe_ftrace_ops,
>  				   (unsigned long)p->addr, 0, 0);
>  	if (ret) {
> -		pr_debug("Failed to arm kprobe-ftrace at %p (%d)\n", p->addr, ret);
> +		pr_debug("Failed to arm kprobe-ftrace at %pS (%d)\n",
> +			 p->addr, ret);
>  		return ret;

Can this happen during normal use? If yes then just remove the warning message.

>  	}
>  
> @@ -1025,7 +1026,8 @@ static int disarm_kprobe_ftrace(struct kprobe *p)
>  
>  	ret = ftrace_set_filter_ip(&kprobe_ftrace_ops,
>  			   (unsigned long)p->addr, 1, 0);
> -	WARN(ret < 0, "Failed to disarm kprobe-ftrace at %p (%d)\n", p->addr, ret);
> +	WARN(ret < 0, "Failed to disarm kprobe-ftrace at %pS (%d)\n",
> +	     p->addr, ret);

As this is a signal of a kernel bug, just convert this to a regular WARN_ONCE() 
that doesn't print any kernel addresses.

> +/* Caller must NOT call this in usual path. This is only for critical case */
>  void dump_kprobe(struct kprobe *kp)
>  {
> -	printk(KERN_WARNING "Dumping kprobe:\n");
> -	printk(KERN_WARNING "Name: %s\nAddress: %p\nOffset: %x\n",
> -	       kp->symbol_name, kp->addr, kp->offset);
> +	pr_err("Dumping kprobe:\n");
> +	pr_err("Name: %s\nOffset: %x\nAddress: %pS\n",
> +	       kp->symbol_name, kp->offset, kp->addr);

No, this function should just go away and be replaced by a WARN() in 
reenter_kprobe().

Thanks,

	Ingo

Re: [PATCH v3 5/7] kprobes/x86: Fix %p uses in error messages

[Ingo Molnar]

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

>  	    (addr < (u8 *) jprobe_return_end)) {
>  		if (stack_addr(regs) != saved_sp) {
>  			struct pt_regs *saved_regs = &kcb->jprobe_saved_regs;
> -			printk(KERN_ERR
> -			       "current sp %p does not match saved sp %p\n",
> +			pr_err("current sp %pS does not match saved sp %pS\n",
>  			       stack_addr(regs), saved_sp);

Why does this use %pS? Stack pointers are typically pointing into general kernel 
RAM, which is totally pointless to display 'symbolically'.

Thanks,

	Ingo

Re: [PATCH v3 1/7] kprobes: Make blacklist root user read only

[Ingo Molnar]

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

> Since the blacklist file indicates a sensitive address
> information to reader, it should be restricted to the
> root user.
> 
> Suggested-by: Thomas Richter <tmricht@linux.ibm.com>
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  kernel/kprobes.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index ea619021d901..51096eece801 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -2621,7 +2621,7 @@ static int __init debugfs_kprobe_init(void)
>  	if (!file)
>  		goto error;
>  
> -	file = debugfs_create_file("blacklist", 0444, dir, NULL,
> +	file = debugfs_create_file("blacklist", 0400, dir, NULL,
>  				&debugfs_kprobe_blacklist_ops);
>  	if (!file)
>  		goto error;

Note that in a typical Linux distro debugfs is already root-only:

  fomalhaut:~> ls -ld /sys/kernel/debug
  drwx------ 28 root root 0 Apr 23 08:55 /sys/kernel/debug

but this change might make sense if debugfs is mounted in some other fashion.

But the patch looks incomplete, 'blacklist' is not the only word-readable file in 
the kprobes hierarchy. The kprobes directory itself, and the 'list' file is 
readable as well:

  [root@fomalhaut ~]# ls -ld /sys/kernel/debug/kprobes
  drwxr-xr-x 2 root root 0 Apr 23 08:55 /sys/kernel/debug/kprobes

  [root@fomalhaut ~]# ls -l /sys/kernel/debug/kprobes/

  -r--r--r-- 1 root root 0 Apr 23 08:55 blacklist
  -rw------- 1 root root 0 Apr 23 08:55 enabled
  -r--r--r-- 1 root root 0 Apr 23 08:55 list

So not just the blacklist should be 400 but 'list' as well, and the main kprobes 
directory as well.

Thanks,

	Ingo

Re: [PATCH v3 2/7] kprobes: Show blacklist addresses as same as kallsyms does

[Ingo Molnar]

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

> +	/*
> +	 * As long as kallsyms shows the address, kprobes blacklist also
> +	 * show it, Or, it shows null address and symbol.
> +	 */

Please _read_ the comments you write!

In which universe does a capitalized 'Or' make sense, even if we ignore the 
various other spelling mistakes?

Also, that sentence is unnecessarily complex, just say this:

> +	/*
> +	 * If /proc/kallsyms is not showing kernel addresses then we won't show 
> +      * them here either:
> +	 */

But I'm unhappy about the messy typing and the messy code flow:

+       void *start = (void *)ent->start_addr, *end = (void *)ent->end_addr;

+       /*
+        * As long as kallsyms shows the address, kprobes blacklist also
+        * show it, Or, it shows null address and symbol.
+        */
+       if (!kallsyms_show_value())
+               start = end = NULL;
+
+       seq_printf(m, "0x%px-0x%px\t%ps\n", start, end,
+                  (void *)ent->start_addr);


All three 'void *' type casts here are due to the bad type choices here:

struct kprobe_blacklist_entry {
        struct list_head list;
        unsigned long start_addr;
        unsigned long end_addr;
};

The natural type of ->start_addr and ->end_addr is 'void *', AFAICS this would 
remove some other type casts from the kprobes code as well, such as from the 
arch_deref_entry_point()...

But the whole code flow introduced by this patch is messy as hell as well.
Why cannot this do the obvious thing:

	if (!kallsyms_show_value())
		seq_printf(m, "0x%px-0x%px\t%ps\n", NULL, NULL, ent->start_addr);
	else
		seq_printf(m, "0x%px-0x%px\t%ps\n", ent->start_addr, ent->end_addr, ent->start_addr);

?

This variant eliminates the unnecessary complication over local variables and 
makes it abundantly clear what gets printed and how.

( Note that the kprobe_blacklist_entry type cleanup should still be done, 
  regardless of code flow choices. )

Thanks,

	Ingo

Re: [PATCH v3 1/7] kprobes: Make blacklist root user read only

[Masami Hiramatsu]

On Fri, 27 Apr 2018 08:56:11 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Fri, Apr 27, 2018 at 03:40:09PM +0900, Masami Hiramatsu wrote:
> > Since the blacklist file indicates a sensitive address
> > information to reader, it should be restricted to the
> > root user.
> > 
> > Suggested-by: Thomas Richter <tmricht@linux.ibm.com>
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > ---
> >  kernel/kprobes.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> <formletter>
> 
> This is not the correct way to submit patches for inclusion in the
> stable kernel tree.  Please read:
>     https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> for how to do this properly.
> 
> </formletter>
> 
> If you want these to go to the stable tree(s), that is great, but please
> mark them properly.  As you did it here for this series, it will not
> work.

Sorry Greg. I'll mark it properly for next version.

Thanks,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v3 5/7] kprobes/x86: Fix %p uses in error messages

[Masami Hiramatsu]

On Fri, 27 Apr 2018 08:58:17 +0200
Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> >  	    (addr < (u8 *) jprobe_return_end)) {
> >  		if (stack_addr(regs) != saved_sp) {
> >  			struct pt_regs *saved_regs = &kcb->jprobe_saved_regs;
> > -			printk(KERN_ERR
> > -			       "current sp %p does not match saved sp %p\n",
> > +			pr_err("current sp %pS does not match saved sp %pS\n",
> >  			       stack_addr(regs), saved_sp);
> 
> Why does this use %pS? Stack pointers are typically pointing into general kernel 
> RAM, which is totally pointless to display 'symbolically'.

That was I pointed in the last thread, but not accepted.
OK, anyway, there are no good reason to show such a dynamic value.
And now jprobes are gone. I just drop %p from this part.

Thanks,


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v3 4/7] kprobes: Replace %p with other pointer types

[Masami Hiramatsu]

On Fri, 27 Apr 2018 08:56:36 +0200
Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > @@ -712,7 +712,7 @@ static void reuse_unused_kprobe(struct kprobe *ap)
> >  	op = container_of(ap, struct optimized_kprobe, kp);
> >  	if (unlikely(list_empty(&op->list)))
> >  		printk(KERN_WARNING "Warning: found a stray unused "
> > -			"aggrprobe@%p\n", ap->addr);
> > +			"aggrprobe@%pS\n", ap->addr);
> 
> Is this a kernel bug? If yes then please convert this to a WARN_ON_ONCE() that 
> doesn't print any kernel addresses.

OK, I will do.

> 
> >  	/* Enable the probe again */
> >  	ap->flags &= ~KPROBE_FLAG_DISABLED;
> >  	/* Optimize it again (remove from op->list) */
> > @@ -985,7 +985,8 @@ static int arm_kprobe_ftrace(struct kprobe *p)
> >  	ret = ftrace_set_filter_ip(&kprobe_ftrace_ops,
> >  				   (unsigned long)p->addr, 0, 0);
> >  	if (ret) {
> > -		pr_debug("Failed to arm kprobe-ftrace at %p (%d)\n", p->addr, ret);
> > +		pr_debug("Failed to arm kprobe-ftrace at %pS (%d)\n",
> > +			 p->addr, ret);
> >  		return ret;
> 
> Can this happen during normal use? If yes then just remove the warning message.

Yes, it can, for example probing on live-patched function.

> 
> >  	}
> >  
> > @@ -1025,7 +1026,8 @@ static int disarm_kprobe_ftrace(struct kprobe *p)
> >  
> >  	ret = ftrace_set_filter_ip(&kprobe_ftrace_ops,
> >  			   (unsigned long)p->addr, 1, 0);
> > -	WARN(ret < 0, "Failed to disarm kprobe-ftrace at %p (%d)\n", p->addr, ret);
> > +	WARN(ret < 0, "Failed to disarm kprobe-ftrace at %pS (%d)\n",
> > +	     p->addr, ret);
> 
> As this is a signal of a kernel bug, just convert this to a regular WARN_ONCE() 
> that doesn't print any kernel addresses.

Would you mean just replace WARN with WARN_ONCE as below?

WARN_ONCE(ret < 0, "Failed to disarm kprobe-ftrace at %pS (%d)\n",
	     p->addr, ret);

Anyway, without printing kprobe->addr, it may be meaningless for this case.

> 
> > +/* Caller must NOT call this in usual path. This is only for critical case */
> >  void dump_kprobe(struct kprobe *kp)
> >  {
> > -	printk(KERN_WARNING "Dumping kprobe:\n");
> > -	printk(KERN_WARNING "Name: %s\nAddress: %p\nOffset: %x\n",
> > -	       kp->symbol_name, kp->addr, kp->offset);
> > +	pr_err("Dumping kprobe:\n");
> > +	pr_err("Name: %s\nOffset: %x\nAddress: %pS\n",
> > +	       kp->symbol_name, kp->offset, kp->addr);
> 
> No, this function should just go away and be replaced by a WARN() in 
> reenter_kprobe().

Would you consider to use pr_err() here? If so, I'll move this
dump as you suggested.

Thank you,


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v3 1/7] kprobes: Make blacklist root user read only

[Masami Hiramatsu]

On Fri, 27 Apr 2018 09:04:12 +0200
Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > Since the blacklist file indicates a sensitive address
> > information to reader, it should be restricted to the
> > root user.
> > 
> > Suggested-by: Thomas Richter <tmricht@linux.ibm.com>
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > ---
> >  kernel/kprobes.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > index ea619021d901..51096eece801 100644
> > --- a/kernel/kprobes.c
> > +++ b/kernel/kprobes.c
> > @@ -2621,7 +2621,7 @@ static int __init debugfs_kprobe_init(void)
> >  	if (!file)
> >  		goto error;
> >  
> > -	file = debugfs_create_file("blacklist", 0444, dir, NULL,
> > +	file = debugfs_create_file("blacklist", 0400, dir, NULL,
> >  				&debugfs_kprobe_blacklist_ops);
> >  	if (!file)
> >  		goto error;
> 
> Note that in a typical Linux distro debugfs is already root-only:
> 
>   fomalhaut:~> ls -ld /sys/kernel/debug
>   drwx------ 28 root root 0 Apr 23 08:55 /sys/kernel/debug
> 
> but this change might make sense if debugfs is mounted in some other fashion.
> 
> But the patch looks incomplete, 'blacklist' is not the only word-readable file in 
> the kprobes hierarchy. The kprobes directory itself, and the 'list' file is 
> readable as well:
> 
>   [root@fomalhaut ~]# ls -ld /sys/kernel/debug/kprobes
>   drwxr-xr-x 2 root root 0 Apr 23 08:55 /sys/kernel/debug/kprobes
> 
>   [root@fomalhaut ~]# ls -l /sys/kernel/debug/kprobes/
> 
>   -r--r--r-- 1 root root 0 Apr 23 08:55 blacklist
>   -rw------- 1 root root 0 Apr 23 08:55 enabled
>   -r--r--r-- 1 root root 0 Apr 23 08:55 list
> 
> So not just the blacklist should be 400 but 'list' as well, and the main kprobes 
> directory as well.

OK, I'll mark it 0400 too.
For the kprobes directory, as Thomas pointed in the original thread,
that is currently debugfs API's limitation.

https://patchwork.kernel.org/patch/10364817/

We only have debugfs_create_dir() but it doesn't have "mode" flag.

struct dentry *debugfs_create_dir(const char *name, struct dentry *parent);

And doesn't care the parent mode bits.

Thanks,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v3 2/7] kprobes: Show blacklist addresses as same as kallsyms does

[Masami Hiramatsu]

On Fri, 27 Apr 2018 09:14:17 +0200
Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > +	/*
> > +	 * As long as kallsyms shows the address, kprobes blacklist also
> > +	 * show it, Or, it shows null address and symbol.
> > +	 */
> 
> Please _read_ the comments you write!
> 
> In which universe does a capitalized 'Or' make sense, even if we ignore the 
> various other spelling mistakes?

It's a typo. I mean "show it. Or, it shows..." anyway,

> 
> Also, that sentence is unnecessarily complex, just say this:
> 
> > +	/*
> > +	 * If /proc/kallsyms is not showing kernel addresses then we won't show 
> > +      * them here either:
> > +	 */

OK, look good to me.

> 
> But I'm unhappy about the messy typing and the messy code flow:
> 
> +       void *start = (void *)ent->start_addr, *end = (void *)ent->end_addr;
> 
> +       /*
> +        * As long as kallsyms shows the address, kprobes blacklist also
> +        * show it, Or, it shows null address and symbol.
> +        */
> +       if (!kallsyms_show_value())
> +               start = end = NULL;
> +
> +       seq_printf(m, "0x%px-0x%px\t%ps\n", start, end,
> +                  (void *)ent->start_addr);
> 
> 
> All three 'void *' type casts here are due to the bad type choices here:
> 
> struct kprobe_blacklist_entry {
>         struct list_head list;
>         unsigned long start_addr;
>         unsigned long end_addr;
> };
> 
> The natural type of ->start_addr and ->end_addr is 'void *', AFAICS this would 
> remove some other type casts from the kprobes code as well, such as from the 
> arch_deref_entry_point()...

Would you really think we should handle all the address with 'void *'?
IOW, are there any policy that we handle the generic address by 'void *'
or 'unsigned long'?
For example, other address checker like kernel_text_address(),
module_text_address(), and ftrace_location() receive 'unsigned long'.
(only jump_label_text_reserved() using 'void *')

> 
> But the whole code flow introduced by this patch is messy as hell as well.
> Why cannot this do the obvious thing:
> 
> 	if (!kallsyms_show_value())
> 		seq_printf(m, "0x%px-0x%px\t%ps\n", NULL, NULL, ent->start_addr);
> 	else
> 		seq_printf(m, "0x%px-0x%px\t%ps\n", ent->start_addr, ent->end_addr, ent->start_addr);
> 
> ?

Both are OK to me. I just didn't want to repeat the printk format string there.

> 
> This variant eliminates the unnecessary complication over local variables and 
> makes it abundantly clear what gets printed and how.

Agreed, it may shorten the patch.

> ( Note that the kprobe_blacklist_entry type cleanup should still be done, 
>   regardless of code flow choices. )
> 
> Thanks,
> 
> 	Ingo

Thank you,


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v3 4/7] kprobes: Replace %p with other pointer types

[Masami Hiramatsu]

On Sat, 28 Apr 2018 00:42:02 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> > > +/* Caller must NOT call this in usual path. This is only for critical case */
> > >  void dump_kprobe(struct kprobe *kp)
> > >  {
> > > -	printk(KERN_WARNING "Dumping kprobe:\n");
> > > -	printk(KERN_WARNING "Name: %s\nAddress: %p\nOffset: %x\n",
> > > -	       kp->symbol_name, kp->addr, kp->offset);
> > > +	pr_err("Dumping kprobe:\n");
> > > +	pr_err("Name: %s\nOffset: %x\nAddress: %pS\n",
> > > +	       kp->symbol_name, kp->offset, kp->addr);
> > 
> > No, this function should just go away and be replaced by a WARN() in 
> > reenter_kprobe().
> 
> Would you consider to use pr_err() here? If so, I'll move this
> dump as you suggested.

So, this is actually called right before BUG(), which means we found
a non-recoverable error while recovering from reentrant kprobes.
Since the BUG() dumps all registers and stack as same as WARN(),
I think we should keep it.
(I would like to dump all kprobes fields like flags, etc. so that
 we can find it is broken or not.)

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>