[PATCH -tip v2 0/7] kprobes: Fix %p in kprobes

[PATCH -tip v2 0/7] kprobes: Fix %p in kprobes

[Masami Hiramatsu]

Hi,

This 2nd 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().

V1 series is here:
 https://lkml.org/lkml/2018/1/25/1

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.

Changes in this version;
 - [1/7] is newly added.
 - patches for MN10300(deleted) and s390(merged) are removed.

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                    |   46 ++++++++++++++++++++++-------------
 5 files changed, 40 insertions(+), 33 deletions(-)

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

[PATCH -tip v2 0/7] kprobes: Fix %p in kprobes

[Masami Hiramatsu]

Hi,

This 2nd 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().

V1 series is here:
 https://lkml.org/lkml/2018/1/25/1

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.

Changes in this version;
 - [1/7] is newly added.
 - patches for MN10300(deleted) and s390(merged) are removed.

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                    |   46 ++++++++++++++++++++++-------------
 5 files changed, 40 insertions(+), 33 deletions(-)

[PATCH -tip v2 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 102160ff5c66..314cec85a8b2 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 -tip v2 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>
---
 kernel/kprobes.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 314cec85a8b2..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%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 -tip v2 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 -tip v2 4/7] 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>
---
 Changes in v2:
  - Rebased on linux-next
---
 kernel/kprobes.c |   18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 94af7c99cf81..58f9e313cf08 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,10 +2171,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);
@@ -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 -tip v2 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 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 0715f827607c..b00778eaca47 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 -tip v2 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 -tip v2 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 -tip v2 0/7] kprobes: Fix %p in kprobes

[Masami Hiramatsu]

On Thu, 26 Apr 2018 17:08:00 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> Hi,
> 
> This 2nd 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().

Oops, I missed to porting this on tip tree, but since this is
including some other archs, it can go directly to each trees
or linus tree.

Anyway, those should be cleanly applicable for the latest
linus tree too.

Thanks,

> 
> V1 series is here:
>  https://lkml.org/lkml/2018/1/25/1
> 
> 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.
> 
> Changes in this version;
>  - [1/7] is newly added.
>  - patches for MN10300(deleted) and s390(merged) are removed.
> 
> 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                    |   46 ++++++++++++++++++++++-------------
>  5 files changed, 40 insertions(+), 33 deletions(-)
> 
> --
> Masami Hiramatsu (Linaro) <mhiramat@kernel.org>


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH -tip v2 0/7] kprobes: Fix %p in kprobes

[Greg KH]

On Thu, Apr 26, 2018 at 05:08:00PM +0900, Masami Hiramatsu wrote:
> Hi,
> 
> This 2nd 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().
> 
> V1 series is here:
>  https://lkml.org/lkml/2018/1/25/1
> 
> 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.
> 
> Changes in this version;
>  - [1/7] is newly added.
>  - patches for MN10300(deleted) and s390(merged) are removed.
> 
> 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                    |   46 ++++++++++++++++++++++-------------
>  5 files changed, 40 insertions(+), 33 deletions(-)
> 
> --
> Masami Hiramatsu (Linaro) <mhiramat@kernel.org>

<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>

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

[Steven Rostedt]

On Thu, 26 Apr 2018 17:10:03 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> 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.

I'm not sure "right before BUG()" is legitimate for using %px. Why not
still use %pS?

-- Steve

Re: [PATCH -tip v2 0/7] kprobes: Fix %p in kprobes

[Steven Rostedt]

On Thu, 26 Apr 2018 10:40:22 +0200
Greg KH <gregkh@linuxfoundation.org> wrote:

> <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>

Is it because he didn't add the Cc: stable@vger.kernel.org tag?

-- Steve

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

[Masami Hiramatsu]

On Thu, 26 Apr 2018 21:13:51 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 26 Apr 2018 17:10:03 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > 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.
> 
> I'm not sure "right before BUG()" is legitimate for using %px. Why not
> still use %pS?

Since BUG() will dump all registers and stacks which includes
raw address information and reboot the system (means reset the
kaslr base offset), I thought it may not add additional damage.

Anyway, for this patch we can use %pS. For the next [5/7] patch,
I'm not sure %pS is good for dumping a stack address...

Thank you,

> 
> -- Steve
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

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

[Steven Rostedt]

On Fri, 27 Apr 2018 11:39:51 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> On Thu, 26 Apr 2018 21:13:51 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Thu, 26 Apr 2018 17:10:03 +0900
> > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >   
> > > 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.  
> > 
> > I'm not sure "right before BUG()" is legitimate for using %px. Why not
> > still use %pS?  
> 
> Since BUG() will dump all registers and stacks which includes
> raw address information and reboot the system (means reset the
> kaslr base offset), I thought it may not add additional damage.

BUG() has been changed to print out %pS and not the address while doing
the stack dump. It only prints the address if it doesn't find a
function name. For example:

CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.11.0-rc3-test+ #48
Hardware name: MSI MS-7823/CSM-H87M-G43 (MS-7823), BIOS V1.6 02/22/2014
Call Trace:
 dump_stack+0x68/0x92
 lockdep_rcu_suspicious+0xf7/0x100
 rcu_read_lock+0x5f/0x61
 __is_insn_slot_addr+0x23/0x5c
 __kernel_text_address+0x4a/0x80
 show_trace_log_lvl+0x157/0x296
 ? module_assert_mutex_or_preempt+0x4f/0x51
 show_stack+0x3d/0x3f
 dump_stack+0x68/0x92
 __warn+0xc2/0xdd
 ? 0xffffffffa0026077
 warn_slowpath_null+0x1d/0x1f
 module_assert_mutex_or_preempt+0x4f/0x51
 __module_address+0x2a/0xac
 ? 0xffffffffa0026077
 __module_text_address+0x12/0x59
 ? 0xffffffffa0026077
 is_module_text_address+0xe/0x16
 __kernel_text_address+0x2b/0x80
 ? 0xffffffffa0026077
 unwind_get_return_address+0x50/0x5c


> 
> Anyway, for this patch we can use %pS. For the next [5/7] patch,
> I'm not sure %pS is good for dumping a stack address...

Although it now is.

-- Steve

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

[Masami Hiramatsu]

On Thu, 26 Apr 2018 22:48:24 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 27 Apr 2018 11:39:51 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> > On Thu, 26 Apr 2018 21:13:51 -0400
> > Steven Rostedt <rostedt@goodmis.org> wrote:
> > 
> > > On Thu, 26 Apr 2018 17:10:03 +0900
> > > Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > >   
> > > > 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.  
> > > 
> > > I'm not sure "right before BUG()" is legitimate for using %px. Why not
> > > still use %pS?  
> > 
> > Since BUG() will dump all registers and stacks which includes
> > raw address information and reboot the system (means reset the
> > kaslr base offset), I thought it may not add additional damage.
> 
> BUG() has been changed to print out %pS and not the address while doing
> the stack dump. It only prints the address if it doesn't find a
> function name. For example:
> 
> CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.11.0-rc3-test+ #48
> Hardware name: MSI MS-7823/CSM-H87M-G43 (MS-7823), BIOS V1.6 02/22/2014
> Call Trace:
>  dump_stack+0x68/0x92
>  lockdep_rcu_suspicious+0xf7/0x100
>  rcu_read_lock+0x5f/0x61
>  __is_insn_slot_addr+0x23/0x5c
>  __kernel_text_address+0x4a/0x80
>  show_trace_log_lvl+0x157/0x296
>  ? module_assert_mutex_or_preempt+0x4f/0x51
>  show_stack+0x3d/0x3f
>  dump_stack+0x68/0x92
>  __warn+0xc2/0xdd
>  ? 0xffffffffa0026077
>  warn_slowpath_null+0x1d/0x1f
>  module_assert_mutex_or_preempt+0x4f/0x51
>  __module_address+0x2a/0xac
>  ? 0xffffffffa0026077
>  __module_text_address+0x12/0x59
>  ? 0xffffffffa0026077
>  is_module_text_address+0xe/0x16
>  __kernel_text_address+0x2b/0x80
>  ? 0xffffffffa0026077
>  unwind_get_return_address+0x50/0x5c
> 

Ah...
OK, I'll fix that.

> 
> > 
> > Anyway, for this patch we can use %pS. For the next [5/7] patch,
> > I'm not sure %pS is good for dumping a stack address...
> 
> Although it now is.

Got it.

Thank you!


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH -tip v2 0/7] kprobes: Fix %p in kprobes

[Masami Hiramatsu]

On Thu, 26 Apr 2018 21:14:34 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 26 Apr 2018 10:40:22 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > <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>
> 
> Is it because he didn't add the Cc: stable@vger.kernel.org tag?

I think this series actually conflict with Thomas's patch which is already
merged.

I'll update the series on the latest linus tree and resend it.

Thanks,

> 
> -- Steve


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH -tip v2 0/7] kprobes: Fix %p in kprobes

[Masami Hiramatsu]

On Thu, 26 Apr 2018 21:14:34 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 26 Apr 2018 10:40:22 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > <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>
> 
> Is it because he didn't add the Cc: stable@vger.kernel.org tag?

I think this series actually conflict with Thomas's patch which is already
merged.

I'll update the series on the latest linus tree and resend it.

Thanks,

> 
> -- Steve


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH -tip v2 0/7] kprobes: Fix %p in kprobes

[Greg KH]

On Thu, Apr 26, 2018 at 09:14:34PM -0400, Steven Rostedt wrote:
> On Thu, 26 Apr 2018 10:40:22 +0200
> Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> > <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>
> 
> Is it because he didn't add the Cc: stable@vger.kernel.org tag?

Yes.

Re: [PATCH -tip v2 0/7] kprobes: Fix %p in kprobes

[Masami Hiramatsu]

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

> On Thu, Apr 26, 2018 at 09:14:34PM -0400, Steven Rostedt wrote:
> > On Thu, 26 Apr 2018 10:40:22 +0200
> > Greg KH <gregkh@linuxfoundation.org> wrote:
> > 
> > > <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>
> > 
> > Is it because he didn't add the Cc: stable@vger.kernel.org tag?
> 
> Yes.

Ah, I misunderstood that. OK, adding that CC in the body.

Thanks,



-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH -tip v2 0/7] kprobes: Fix %p in kprobes

[Masami Hiramatsu]

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

> On Thu, Apr 26, 2018 at 09:14:34PM -0400, Steven Rostedt wrote:
> > On Thu, 26 Apr 2018 10:40:22 +0200
> > Greg KH <gregkh@linuxfoundation.org> wrote:
> > 
> > > <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>
> > 
> > Is it because he didn't add the Cc: stable@vger.kernel.org tag?
> 
> Yes.

Ah, I misunderstood that. OK, adding that CC in the body.

Thanks,



-- 
Masami Hiramatsu <mhiramat@kernel.org>