[PATCH] arm:traps: Don't print stack or raw PC/LR values in backtraces

[PATCH] arm:traps: Don't print stack or raw PC/LR values in backtraces

[Xiaoming Ni]

Printing raw pointer values in backtraces has potential security
implications and are of questionable value anyway.

This patch follows x86 and arm64's lead and removes the "Exception stack:"
dump from kernel backtraces:
	commit a25ffd3a6302a6 ("arm64: traps: Don't print stack or raw
	 PC/LR values in backtraces")
	commit 0ee1dd9f5e7eae ("x86/dumpstack: Remove raw stack dump")
	commit bb5e5ce545f203 ("x86/dumpstack: Remove kernel text
	 addresses from stack dump")

Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
---
 arch/arm/kernel/process.c |  3 +--
 arch/arm/kernel/traps.c   | 12 +++++-------
 2 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index 8e6ace03e960..71c9e5597d39 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -121,8 +121,7 @@ void __show_regs(struct pt_regs *regs)
 
 	printk("PC is at %pS\n", (void *)instruction_pointer(regs));
 	printk("LR is at %pS\n", (void *)regs->ARM_lr);
-	printk("pc : [<%08lx>]    lr : [<%08lx>]    psr: %08lx\n",
-	       regs->ARM_pc, regs->ARM_lr, regs->ARM_cpsr);
+	printk("psr: %08lx\n", regs->ARM_cpsr);
 	printk("sp : %08lx  ip : %08lx  fp : %08lx\n",
 	       regs->ARM_sp, regs->ARM_ip, regs->ARM_fp);
 	printk("r10: %08lx  r9 : %08lx  r8 : %08lx\n",
diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
index 17d5a785df28..b0b188e01070 100644
--- a/arch/arm/kernel/traps.c
+++ b/arch/arm/kernel/traps.c
@@ -60,23 +60,18 @@ static int __init user_debug_setup(char *str)
 __setup("user_debug=", user_debug_setup);
 #endif
 
-static void dump_mem(const char *, const char *, unsigned long, unsigned long);
-
 void dump_backtrace_entry(unsigned long where, unsigned long from,
 			  unsigned long frame, const char *loglvl)
 {
 	unsigned long end = frame + 4 + sizeof(struct pt_regs);
 
 #ifdef CONFIG_KALLSYMS
-	printk("%s[<%08lx>] (%ps) from [<%08lx>] (%pS)\n",
-		loglvl, where, (void *)where, from, (void *)from);
+	printk("%s (%ps) from (%pS)\n",
+		loglvl, (void *)where, (void *)from);
 #else
 	printk("%sFunction entered at [<%08lx>] from [<%08lx>]\n",
 		loglvl, where, from);
 #endif
-
-	if (in_entry_text(from) && end <= ALIGN(frame, THREAD_SIZE))
-		dump_mem(loglvl, "Exception stack", frame + 4, end);
 }
 
 void dump_backtrace_stm(u32 *stack, u32 instruction, const char *loglvl)
@@ -125,6 +120,9 @@ static void dump_mem(const char *lvl, const char *str, unsigned long bottom,
 	mm_segment_t fs;
 	int i;
 
+	/* Do not print virtual addresses in non-reset scenarios */
+	if (!panic_on_oops)
+		return;
 	/*
 	 * We need to switch to kernel mode so that we can use __get_user
 	 * to safely read from kernel space.  Note that we now dump the
-- 
2.27.0

Re: [PATCH] arm:traps: Don't print stack or raw PC/LR values in backtraces

[Russell King - ARM Linux admin]

On Fri, Oct 09, 2020 at 03:59:57PM +0800, Xiaoming Ni wrote:
> Printing raw pointer values in backtraces has potential security
> implications and are of questionable value anyway.
> 
> This patch follows x86 and arm64's lead and removes the "Exception stack:"
> dump from kernel backtraces:
> 	commit a25ffd3a6302a6 ("arm64: traps: Don't print stack or raw
> 	 PC/LR values in backtraces")
> 	commit 0ee1dd9f5e7eae ("x86/dumpstack: Remove raw stack dump")
> 	commit bb5e5ce545f203 ("x86/dumpstack: Remove kernel text
> 	 addresses from stack dump")
> 
> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>

I am really not happy about this - it hurts at least my ability to
debug the kernel when people post oopses to the mailing list. If
people wish to make the kernel harder to debug, and are prepared
to be told "your kernel is undebuggable" then this patch is fine.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] arm:traps: Don't print stack or raw PC/LR values in backtraces

[Sebastian Andrzej Siewior]

On 2020-10-09 09:08:50 [+0100], Russell King - ARM Linux admin wrote:
> I am really not happy about this - it hurts at least my ability to
> debug the kernel when people post oopses to the mailing list. If
> people wish to make the kernel harder to debug, and are prepared
> to be told "your kernel is undebuggable" then this patch is fine.

I haven't look at the patch but don't they keep/add the representation:
  PC: symbol+offset/size
  LR: symbol+offset/size

? This is needed at very least as a replacement for the missing address.

Sebastian

Re: [PATCH] arm:traps: Don't print stack or raw PC/LR values in backtraces

[Xiaoming Ni]

On 2020/10/9 16:18, Sebastian Andrzej Siewior wrote:
> On 2020-10-09 09:08:50 [+0100], Russell King - ARM Linux admin wrote:
>> I am really not happy about this - it hurts at least my ability to
>> debug the kernel when people post oopses to the mailing list. If

In the reset scenario, dump_mem is retained:

@@ -125,6 +118,9 @@ static void dump_mem(const char *lvl, const char 
*str, unsigned long bottom,
         mm_segment_t fs;
         int i;

+ /* Do not print virtual addresses in non-reset scenarios */
+ if (!panic_on_oops)
+         return;


>> people wish to make the kernel harder to debug, and are prepared
>> to be told "your kernel is undebuggable" then this patch is fine.
> 
> I haven't look at the patch but don't they keep/add the representation:
>    PC: symbol+offset/size
>    LR: symbol+offset/size
> 
> ? This is needed at very least as a replacement for the missing address.

Yes, only %08lx was deleted, but %ps is still retained.

-	printk("%s[<%08lx>] (%ps) from [<%08lx>] (%pS)\n",
-		loglvl, where, (void *)where, from, (void *)from);
+	printk("%s (%ps) from (%pS)\n",
+		loglvl, (void *)where, (void *)from);

Thanks
Xiaoming Ni

Re: [PATCH] arm:traps: Don't print stack or raw PC/LR values in backtraces

[Josh Poimboeuf]

On Fri, Oct 09, 2020 at 09:08:50AM +0100, Russell King - ARM Linux admin wrote:
> On Fri, Oct 09, 2020 at 03:59:57PM +0800, Xiaoming Ni wrote:
> > Printing raw pointer values in backtraces has potential security
> > implications and are of questionable value anyway.
> > 
> > This patch follows x86 and arm64's lead and removes the "Exception stack:"
> > dump from kernel backtraces:
> > 	commit a25ffd3a6302a6 ("arm64: traps: Don't print stack or raw
> > 	 PC/LR values in backtraces")
> > 	commit 0ee1dd9f5e7eae ("x86/dumpstack: Remove raw stack dump")
> > 	commit bb5e5ce545f203 ("x86/dumpstack: Remove kernel text
> > 	 addresses from stack dump")
> > 
> > Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
> 
> I am really not happy about this - it hurts at least my ability to
> debug the kernel when people post oopses to the mailing list. If
> people wish to make the kernel harder to debug, and are prepared
> to be told "your kernel is undebuggable" then this patch is fine.

At least on x86 we've had this for four years now, without any apparent
harm to debugability.  scripts/faddr2line helps.

-- 
Josh

Re: [PATCH] arm:traps: Don't print stack or raw PC/LR values in backtraces

[Russell King - ARM Linux admin]

On Fri, Oct 09, 2020 at 10:18:20AM +0200, Sebastian Andrzej Siewior wrote:
> On 2020-10-09 09:08:50 [+0100], Russell King - ARM Linux admin wrote:
> > I am really not happy about this - it hurts at least my ability to
> > debug the kernel when people post oopses to the mailing list. If
> > people wish to make the kernel harder to debug, and are prepared
> > to be told "your kernel is undebuggable" then this patch is fine.
> 
> I haven't look at the patch but don't they keep/add the representation:
>   PC: symbol+offset/size
>   LR: symbol+offset/size
> 
> ? This is needed at very least as a replacement for the missing address.

I don't have a problem getting rid of the hex numbers in [< >]
although then I will need to convert the symbol back to an address
using the vmlinux to then calculate its address to then find the
appropriate place in the objdump output - because objdump does
_not_ use the symbol+offset annotation.  Yes, I really do look up
the numeric addresses in the objdump output to then read the
disassembly.

$ objdump -d vmlinux | less

and then search for the address is the fastest and most convenient
way for me rather than having to deal with some random script.

Maybe I'm just antequated about how I do my debugging, but this
seems to me to be the most efficient and fastest way.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH] arm:traps: Don't print stack or raw PC/LR values in backtraces

[Xiaoming Ni]

On 2020/10/12 5:32, Russell King - ARM Linux admin wrote:
> On Fri, Oct 09, 2020 at 10:18:20AM +0200, Sebastian Andrzej Siewior wrote:
>> On 2020-10-09 09:08:50 [+0100], Russell King - ARM Linux admin wrote:
>>> I am really not happy about this - it hurts at least my ability to
>>> debug the kernel when people post oopses to the mailing list. If
>>> people wish to make the kernel harder to debug, and are prepared
>>> to be told "your kernel is undebuggable" then this patch is fine.
>>
>> I haven't look at the patch but don't they keep/add the representation:
>>    PC: symbol+offset/size
>>    LR: symbol+offset/size
>>
>> ? This is needed at very least as a replacement for the missing address.
> 
> I don't have a problem getting rid of the hex numbers in [< >]
> although then I will need to convert the symbol back to an address
> using the vmlinux to then calculate its address to then find the
> appropriate place in the objdump output - because objdump does
> _not_ use the symbol+offset annotation.  Yes, I really do look up
> the numeric addresses in the objdump output to then read the
> disassembly.
> 
> $ objdump -d vmlinux | less
> 
> and then search for the address is the fastest and most convenient
> way for me rather than having to deal with some random script.
> 
> Maybe I'm just antequated about how I do my debugging, but this
> seems to me to be the most efficient and fastest way.
> 
The loading address of the kernel module is not fixed, so symbol+offset 
is more useful than a hexadecimal address when the call stack contains 
kernel module symbols.
Delete the PC/LR address and retain the sysbol+offset. The kernel can 
still be debugged.

Thanks
Xiaoming Ni

Re: [PATCH] arm:traps: Don't print stack or raw PC/LR values in backtraces

[Sebastian Andrzej Siewior]

On 2020-10-11 22:32:38 [+0100], Russell King - ARM Linux admin wrote:
> I don't have a problem getting rid of the hex numbers in [< >]
> although then I will need to convert the symbol back to an address
> using the vmlinux to then calculate its address to then find the
> appropriate place in the objdump output - because objdump does
> _not_ use the symbol+offset annotation.  Yes, I really do look up
> the numeric addresses in the objdump output to then read the
> disassembly.
> 
> $ objdump -d vmlinux | less
> 
> and then search for the address is the fastest and most convenient
> way for me rather than having to deal with some random script.
> 
> Maybe I'm just antequated about how I do my debugging, but this
> seems to me to be the most efficient and fastest way.

besides what Josh mentioned, there is also 
         scripts/decode_stacktrace.sh path-vmlinux
 
where you can copy/paste your complete stack trace / dmesg and it will
decode it line by line. So if you invoke
        scripts/decode_stacktrace.sh vmlinux.o

and paste this:

|[    7.568155] 001: PC is at do_work_pending+0x190/0x5c4
|[    7.568641] 001: LR is at slow_work_pending+0xc/0x20
|[    7.569232] 001: Backtrace:
|[    7.569367] 001: [<c020c2d0>] (do_work_pending) from [<c02000cc>] (slow_work_pending+0xc/0x20)

you get this in return:
|[    7.568155] 001: PC is at do_work_pending (arch/arm/kernel/signal.c:616 arch/arm/kernel/signal.c:670)
|[    7.568641] 001: LR is at slow_work_pending (arch/arm/kernel/entry-common.S:112)
|[    7.569232] 001: Backtrace:
|[    7.569367] 001: (do_work_pending) from slow_work_pending (arch/arm/kernel/entry-common.S:112)

Sebastian

Re: [PATCH] arm:traps: Don't print stack or raw PC/LR values in backtraces

[Russell King - ARM Linux admin]

On Mon, Oct 12, 2020 at 12:03:18PM +0200, Sebastian Andrzej Siewior wrote:
> On 2020-10-11 22:32:38 [+0100], Russell King - ARM Linux admin wrote:
> > I don't have a problem getting rid of the hex numbers in [< >]
> > although then I will need to convert the symbol back to an address
> > using the vmlinux to then calculate its address to then find the
> > appropriate place in the objdump output - because objdump does
> > _not_ use the symbol+offset annotation.  Yes, I really do look up
> > the numeric addresses in the objdump output to then read the
> > disassembly.
> > 
> > $ objdump -d vmlinux | less
> > 
> > and then search for the address is the fastest and most convenient
> > way for me rather than having to deal with some random script.
> > 
> > Maybe I'm just antequated about how I do my debugging, but this
> > seems to me to be the most efficient and fastest way.
> 
> besides what Josh mentioned, there is also 
>          scripts/decode_stacktrace.sh path-vmlinux
>  
> where you can copy/paste your complete stack trace / dmesg and it will
> decode it line by line. So if you invoke
>         scripts/decode_stacktrace.sh vmlinux.o
> 
> and paste this:
> 
> |[    7.568155] 001: PC is at do_work_pending+0x190/0x5c4
> |[    7.568641] 001: LR is at slow_work_pending+0xc/0x20
> |[    7.569232] 001: Backtrace:
> |[    7.569367] 001: [<c020c2d0>] (do_work_pending) from [<c02000cc>] (slow_work_pending+0xc/0x20)
> 
> you get this in return:
> |[    7.568155] 001: PC is at do_work_pending (arch/arm/kernel/signal.c:616 arch/arm/kernel/signal.c:670)
> |[    7.568641] 001: LR is at slow_work_pending (arch/arm/kernel/entry-common.S:112)
> |[    7.569232] 001: Backtrace:
> |[    7.569367] 001: (do_work_pending) from slow_work_pending (arch/arm/kernel/entry-common.S:112)

That's assuming:

1) you have built the kernel with debug information enabled
   (I never do, it uses way too much disk space)
2) you want to look at the C code rather than the assembly.

I think you've assumed that I debug oopses by looking primerily at C
code. I don't. I understand the assembly and then look at the C code.

I've stated in the past in detail how I debug the kernel when someone
has posted a hard-to-debug oops, going through the validation of the
dumped state, sometimes to find the bug in a function several parents
up from the one where the oops actually occurred.

However, as I've said, I have little problem removing the hex values
inside [< >] as I can work around that. Removing the other information
is what I'm objecting to.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!