Hi,
This 4th 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 fixes for arch ports too.
Changes in this version; [1/7] Fix "list" file's mode too. [2/7] Do not use local variables and fix comment. [4/7] Use WARN_ONCE() for single bug. [5/7] Just remove %p.
Thank you,
---
Masami Hiramatsu (7): kprobes: Make list and 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 | 13 +++------ kernel/kprobes.c | 52 +++++++++++++++++++++-------------- 5 files changed, 42 insertions(+), 38 deletions(-)
-- Masami Hiramatsu (Linaro) mhiramat@kernel.org
Since the blacklist and list files on debugfs indicates a sensitive address information to reader, it should be restricted to the root user.
Suggested-by: Thomas Richter tmricht@linux.ibm.com Suggested-by: Ingo Molnar mingo@kernel.org Signed-off-by: Masami Hiramatsu mhiramat@kernel.org Cc: stable@vger.kernel.org --- Changes in v4: - Fix "list" file's mode too. --- kernel/kprobes.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c index ea619021d901..5eb42c82497c 100644 --- a/kernel/kprobes.c +++ b/kernel/kprobes.c @@ -2611,7 +2611,7 @@ static int __init debugfs_kprobe_init(void) if (!dir) return -ENOMEM;
- file = debugfs_create_file("list", 0444, dir, NULL, + file = debugfs_create_file("list", 0400, dir, NULL, &debugfs_kprobes_operations); if (!file) goto error; @@ -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;
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 Cc: stable@vger.kernel.org --- Changes in v4: - Do not use local variables and fix comment. 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 5eb42c82497c..6ec1b5e024bf 100644 --- a/kernel/kprobes.c +++ b/kernel/kprobes.c @@ -2428,8 +2428,16 @@ 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);
- seq_printf(m, "0x%px-0x%px\t%ps\n", (void *)ent->start_addr, - (void *)ent->end_addr, (void *)ent->start_addr); + /* + * If /proc/kallsyms is not showing kernel address, we won't + * show them here either. + */ + if (!kallsyms_show_value()) + seq_printf(m, "0x%px-0x%px\t%ps\n", NULL, NULL, + (void *)ent->start_addr); + else + seq_printf(m, "0x%px-0x%px\t%ps\n", (void *)ent->start_addr, + (void *)ent->end_addr, (void *)ent->start_addr); return 0; }
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 Cc: stable@vger.kernel.org --- kernel/kprobes.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c index 6ec1b5e024bf..548903deaee5 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;
Replace %p with %pS or just remove it if unneeded. And use WARN_ONCE() if it is a single bug.
Signed-off-by: Masami Hiramatsu mhiramat@kernel.org Cc: stable@vger.kernel.org --- Changes in v4: - Use WARN_ONCE() for single bug. Changes in v3: - Do not use %px in any case. Changes in v2: - Rebased on linux-next --- kernel/kprobes.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c index 548903deaee5..bb70658acb97 100644 --- a/kernel/kprobes.c +++ b/kernel/kprobes.c @@ -710,9 +710,7 @@ static void reuse_unused_kprobe(struct kprobe *ap) * there is still a relative jump) and disabled. */ 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); + WARN_ON_ONCE(list_empty(&op->list)); /* Enable the probe again */ ap->flags &= ~KPROBE_FLAG_DISABLED; /* Optimize it again (remove from op->list) */ @@ -985,7 +983,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 +1024,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_ONCE(ret < 0, "Failed to disarm kprobe-ftrace at %pS (%d)\n", + p->addr, ret); return ret; } #else /* !CONFIG_KPROBES_ON_FTRACE */ @@ -2169,11 +2169,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 +2197,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)
Remove all %p uses in error messages in kprobes/x86.
Signed-off-by: Masami Hiramatsu mhiramat@kernel.org Cc: stable@vger.kernel.org --- Changes in v4: - Just remove %p. Changes in v3: - Do not use %px. --- arch/x86/kernel/kprobes/core.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)
diff --git a/arch/x86/kernel/kprobes/core.c b/arch/x86/kernel/kprobes/core.c index 0715f827607c..2c7ec4ae660c 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,10 @@ 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", - stack_addr(regs), saved_sp); - printk(KERN_ERR "Saved registers for jprobe %p\n", jp); + pr_err("current %%sp does not match saved %%sp\n"); + 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(); }
Fix %p uses in error messages by removing it and using general dumper.
Signed-off-by: Masami Hiramatsu mhiramat@kernel.org Cc: stable@vger.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");
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 Cc: stable@vger.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);
linux-stable-mirror@lists.linaro.org
-
Masami Hiramatsu