[PATCH 0/8] kprobes: Fix %p in kprobes

[PATCH 0/8] kprobes: Fix %p in kprobes

[Masami Hiramatsu]

Hi,

This series 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, and one critical
function uses %px which is called right before BUG().

Also, I tried to fix this issue on each arch port. I searched
it by

 # find arch/* | grep -e 'kprobe.*c' | xargs grep -w %p

And fixed all %p uses in those files.

Thank you,

---

Masami Hiramatsu (8):
      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
      kprobes/MN10300: Fix %p uses in error messages
      kprobes/s390: 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/mn10300/kernel/kprobes.c       |    6 +++--
 arch/s390/kernel/kprobes.c          |    2 +-
 arch/x86/kernel/kprobes/core.c      |   12 +++-------
 kernel/kprobes.c                    |   42 ++++++++++++++++++++++-------------
 7 files changed, 41 insertions(+), 36 deletions(-)

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

[PATCH 1/8] 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>
---
 kernel/kprobes.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index da2ccf142358..cb15991ba676 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -2382,9 +2382,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%p-0x%p\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 2/8] 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 cb15991ba676..a5f13e379ae1 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -2281,6 +2281,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";
@@ -2289,13 +2290,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 3/8] kprobes: Replace %p with other pointer types

[Masami Hiramatsu]

Replace %p with appropriate pointer types (or just remove it)
 - Use %pS if possible
 - Use %px only for the function right before BUG().
 - Remove unneeded error message.

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

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index a5f13e379ae1..02887975d445 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) */
@@ -984,7 +984,7 @@ static void arm_kprobe_ftrace(struct kprobe *p)
 
 	ret = ftrace_set_filter_ip(&kprobe_ftrace_ops,
 				   (unsigned long)p->addr, 0, 0);
-	WARN(ret < 0, "Failed to arm kprobe-ftrace at %p (%d)\n", p->addr, ret);
+	WARN(ret < 0, "Failed to arm kprobe-ftrace at %pS (%d)\n", p->addr, ret);
 	kprobe_ftrace_enabled++;
 	if (kprobe_ftrace_enabled == 1) {
 		ret = register_ftrace_function(&kprobe_ftrace_ops);
@@ -1004,7 +1004,7 @@ static void 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);
 }
 #else	/* !CONFIG_KPROBES_ON_FTRACE */
 #define prepare_kprobe(p)	arch_prepare_kprobe(p)
@@ -2124,10 +2124,11 @@ 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",
+	pr_err("Dumping kprobe:\n");
+	pr_err("Name: %s\nAddress: %px\nOffset: %x\n",
 	       kp->symbol_name, kp->addr, kp->offset);
 }
 NOKPROBE_SYMBOL(dump_kprobe);
@@ -2151,11 +2152,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 4/8] 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 is right before the BUG() so replaced with %px.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 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 bd36f3c33cd0..aea956aedad7 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 %px does not match saved sp %px\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 5/8] 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 52d1cd14fda4..8f37d505194f 100644
--- a/arch/arm/probes/kprobes/core.c
+++ b/arch/arm/probes/kprobes/core.c
@@ -291,8 +291,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 */
@@ -617,11 +617,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 6/8] 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>
---
 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);

[PATCH 7/8] kprobes/MN10300: Fix %p uses in error messages

[Masami Hiramatsu]

Replace %p with %px because it is right before BUG().

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/mn10300/kernel/kprobes.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/mn10300/kernel/kprobes.c b/arch/mn10300/kernel/kprobes.c
index 0311a7fcea16..e539fac00321 100644
--- a/arch/mn10300/kernel/kprobes.c
+++ b/arch/mn10300/kernel/kprobes.c
@@ -632,9 +632,9 @@ int __kprobes longjmp_break_handler(struct kprobe *p, struct pt_regs *regs)
 
 	if (addr == (u8 *) jprobe_return_bp_addr) {
 		if (jprobe_saved_regs_location != regs) {
-			printk(KERN_ERR"JPROBE:"
-			       " Current regs (%p) does not match saved regs"
-			       " (%p).\n",
+			pr_err("JPROBE:"
+			       " Current regs (%px) does not match saved regs"
+			       " (%px).\n",
 			       regs, jprobe_saved_regs_location);
 			BUG();
 		}

[PATCH 8/8] kprobes/s390: Fix %p uses in error messages

[Masami Hiramatsu]

Remove %p because the kprobe will be dumped in
dump_kprobe().

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 arch/s390/kernel/kprobes.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/s390/kernel/kprobes.c b/arch/s390/kernel/kprobes.c
index af3722c28fd9..df30e5b9a572 100644
--- a/arch/s390/kernel/kprobes.c
+++ b/arch/s390/kernel/kprobes.c
@@ -282,7 +282,7 @@ static void kprobe_reenter_check(struct kprobe_ctlblk *kcb, struct kprobe *p)
 		 * is a BUG. The code path resides in the .kprobes.text
 		 * section and is executed with interrupts disabled.
 		 */
-		printk(KERN_EMERG "Invalid kprobe detected at %p.\n", p->addr);
+		pr_err("Invalid kprobe detected.\n");
 		dump_kprobe(p);
 		BUG();
 	}

Re: [PATCH 6/8] kprobes/arm64: Fix %p uses in error messages

[Will Deacon]

On Thu, Jan 25, 2018 at 02:29:32PM +0900, Masami Hiramatsu wrote:
> Fix %p uses in error messages by removing it because
> those are redundant or meaningless.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  arch/arm64/kernel/probes/kprobes.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Acked-by: Will Deacon <will.deacon@arm.com>

I guess Catalin can just pick this one up via arm64.

Will

> 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 6/8] kprobes/arm64: Fix %p uses in error messages

[Masami Hiramatsu]

On Thu, 25 Jan 2018 16:42:31 +0000
Will Deacon <will.deacon@arm.com> wrote:

> On Thu, Jan 25, 2018 at 02:29:32PM +0900, Masami Hiramatsu wrote:
> > Fix %p uses in error messages by removing it because
> > those are redundant or meaningless.
> > 
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > ---
> >  arch/arm64/kernel/probes/kprobes.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> Acked-by: Will Deacon <will.deacon@arm.com>
> 
> I guess Catalin can just pick this one up via arm64.

OK, thanks!

> 
> Will
> 
> > 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);
> > 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 8/8] kprobes/s390: Fix %p uses in error messages

[Heiko Carstens]

On Thu, Jan 25, 2018 at 02:30:32PM +0900, Masami Hiramatsu wrote:
> Remove %p because the kprobe will be dumped in
> dump_kprobe().
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  arch/s390/kernel/kprobes.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/s390/kernel/kprobes.c b/arch/s390/kernel/kprobes.c
> index af3722c28fd9..df30e5b9a572 100644
> --- a/arch/s390/kernel/kprobes.c
> +++ b/arch/s390/kernel/kprobes.c
> @@ -282,7 +282,7 @@ static void kprobe_reenter_check(struct kprobe_ctlblk *kcb, struct kprobe *p)
>  		 * is a BUG. The code path resides in the .kprobes.text
>  		 * section and is executed with interrupts disabled.
>  		 */
> -		printk(KERN_EMERG "Invalid kprobe detected at %p.\n", p->addr);
> +		pr_err("Invalid kprobe detected.\n");

Given that this change makes sense anyway, I just applied it to the s390
tree. :)

Thanks!