Re: [PATCH] powerpc:Don't print raw EIP/LR hex values in dump_stack() and show_regs()

2020-12-22 Thread Christophe Leroy




Le 22/12/2020 à 18:29, Segher Boessenkool a écrit :

On Tue, Dec 22, 2020 at 09:45:03PM +0800, Xiaoming Ni wrote:

On 2020/12/22 1:12, Segher Boessenkool wrote:

On Mon, Dec 21, 2020 at 04:42:23PM +, David Laight wrote:

From: Segher Boessenkool

Sent: 21 December 2020 16:32

On Mon, Dec 21, 2020 at 04:17:21PM +0100, Christophe Leroy wrote:

Le 21/12/2020 à 04:27, Xiaoming Ni a écrit :

Since the commit 2b0e86cc5de6 ("powerpc/fsl_booke/32: implement KASLR
infrastructure"), the powerpc system is ready to support KASLR.
To reduces the risk of invalidating address randomization, don't print
the
EIP/LR hex values in dump_stack() and show_regs().



I think your change is not enough to hide EIP address, see below a dump
with you patch, you get "Faulting instruction address: 0xc03a0c14"


As far as I can see the patch does nothing to the GPR printout.  Often
GPRs contain code addresses.  As one example, the LR is moved via a GPR
(often GPR0, but not always) for storing on the stack.

So this needs more work.


If the dump_stack() is from an oops you need the real EIP value
on order to stand any chance of making headway.


Or at least the function name + offset, yes.


When the system is healthy, only symbols and offsets are printed,
Output address and symbol + offset when the system is dying
Does this meet both debugging and security requirements?


If you have the vmlinux, sym+off is enough to find what instruction
caused the crash.

It does of course not give all the information you get in a crash dump
with all the registers, so it does hinder debugging a bit.  This is a
tradeoff.

Most debugging will need xmon or similar (or printf-style debugging)
anyway; and otoh the register dump will render KASLR largely
ineffective.


For example:

+static void __show_regs_ip_lr(const char *flag, unsigned long addr)
+{
+ if (system_going_down()) { /* panic oops reboot */
+ pr_cont("%s["REG"] %pS", flag, addr, (void *)addr);
+ } else {
+ pr_cont("%s%pS", flag, (void *)addr);
+ }
+}


*If* you are certain the system goes down immediately, and you are also
certain this information will not help defeat ASLR after a reboot, you
could just print whatever, sure.

Otherwise, you only want to show some very few registers.  Or, make sure
no attackers can ever see these dumps (which is hard, many systems trust
all (local) users with it!)  Which means we first will need some very
different patches, before any of this can be much useful :-(



So IIUC, on one side we enlarge the dumping of registers with commits like 
https://github.com/linuxppc/linux/commit/bf13718bc57ada25016d9fe80323238d0b94506e#diff-8b965e0e62fc1b6ad5e51bf0a539941e929754cdb716041b06b4f4a5f73590f9, 
and on the other side we want to narrow it and hide registers ? I'm lost.


Christophe


Re: [PATCH] powerpc:Don't print raw EIP/LR hex values in dump_stack() and show_regs()

2020-12-22 Thread Segher Boessenkool
On Tue, Dec 22, 2020 at 09:45:03PM +0800, Xiaoming Ni wrote:
> On 2020/12/22 1:12, Segher Boessenkool wrote:
> >On Mon, Dec 21, 2020 at 04:42:23PM +, David Laight wrote:
> >>From: Segher Boessenkool
> >>>Sent: 21 December 2020 16:32
> >>>
> >>>On Mon, Dec 21, 2020 at 04:17:21PM +0100, Christophe Leroy wrote:
> Le 21/12/2020 à 04:27, Xiaoming Ni a écrit :
> >Since the commit 2b0e86cc5de6 ("powerpc/fsl_booke/32: implement KASLR
> >infrastructure"), the powerpc system is ready to support KASLR.
> >To reduces the risk of invalidating address randomization, don't print 
> >the
> >EIP/LR hex values in dump_stack() and show_regs().
> >>>
> I think your change is not enough to hide EIP address, see below a dump
> with you patch, you get "Faulting instruction address: 0xc03a0c14"
> >>>
> >>>As far as I can see the patch does nothing to the GPR printout.  Often
> >>>GPRs contain code addresses.  As one example, the LR is moved via a GPR
> >>>(often GPR0, but not always) for storing on the stack.
> >>>
> >>>So this needs more work.
> >>
> >>If the dump_stack() is from an oops you need the real EIP value
> >>on order to stand any chance of making headway.
> >
> >Or at least the function name + offset, yes.
> >
> When the system is healthy, only symbols and offsets are printed,
> Output address and symbol + offset when the system is dying
> Does this meet both debugging and security requirements?

If you have the vmlinux, sym+off is enough to find what instruction
caused the crash.

It does of course not give all the information you get in a crash dump
with all the registers, so it does hinder debugging a bit.  This is a
tradeoff.

Most debugging will need xmon or similar (or printf-style debugging)
anyway; and otoh the register dump will render KASLR largely
ineffective.

> For example:
> 
> +static void __show_regs_ip_lr(const char *flag, unsigned long addr)
> +{
> + if (system_going_down()) { /* panic oops reboot */
> + pr_cont("%s["REG"] %pS", flag, addr, (void *)addr);
> + } else {
> + pr_cont("%s%pS", flag, (void *)addr);
> + }
> +}

*If* you are certain the system goes down immediately, and you are also
certain this information will not help defeat ASLR after a reboot, you
could just print whatever, sure.

Otherwise, you only want to show some very few registers.  Or, make sure
no attackers can ever see these dumps (which is hard, many systems trust
all (local) users with it!)  Which means we first will need some very
different patches, before any of this can be much useful :-(


Segher


Re: [PATCH] powerpc:Don't print raw EIP/LR hex values in dump_stack() and show_regs()

2020-12-22 Thread Xiaoming Ni

On 2020/12/22 1:12, Segher Boessenkool wrote:

On Mon, Dec 21, 2020 at 04:42:23PM +, David Laight wrote:

From: Segher Boessenkool

Sent: 21 December 2020 16:32

On Mon, Dec 21, 2020 at 04:17:21PM +0100, Christophe Leroy wrote:

Le 21/12/2020 à 04:27, Xiaoming Ni a écrit :

Since the commit 2b0e86cc5de6 ("powerpc/fsl_booke/32: implement KASLR
infrastructure"), the powerpc system is ready to support KASLR.
To reduces the risk of invalidating address randomization, don't print the
EIP/LR hex values in dump_stack() and show_regs().



I think your change is not enough to hide EIP address, see below a dump
with you patch, you get "Faulting instruction address: 0xc03a0c14"


As far as I can see the patch does nothing to the GPR printout.  Often
GPRs contain code addresses.  As one example, the LR is moved via a GPR
(often GPR0, but not always) for storing on the stack.

So this needs more work.


If the dump_stack() is from an oops you need the real EIP value
on order to stand any chance of making headway.


Or at least the function name + offset, yes.


When the system is healthy, only symbols and offsets are printed,
Output address and symbol + offset when the system is dying
Does this meet both debugging and security requirements?
For example:

+static void __show_regs_ip_lr(const char *flag, unsigned long addr)
+{
+ if (system_going_down()) { /* panic oops reboot */
+ pr_cont("%s["REG"] %pS", flag, addr, (void *)addr);
+ } else {
+ pr_cont("%s%pS", flag, (void *)addr);
+ }
+}
+
 static void __show_regs(struct pt_regs *regs)
 {
int i, trap;

-   printk("NIP:  "REG" LR: "REG" CTR: "REG"\n",
-  regs->nip, regs->link, regs->ctr);
+ __show_regs_ip_lr("NIP: ", regs->nip);
+ __show_regs_ip_lr(" LR: ", regs->link);
+ pr_cont(" CTR: "REG"\n", regs->ctr);
printk("REGS: %px TRAP: %04lx   %s  (%s)\n",
   regs, regs->trap, print_tainted(), init_utsname()->release);
printk("MSR:  "REG" ", regs->msr);





Otherwise you might just as well just print 'borked - tough luck'.


Yes.  ASLR is a house of cards.  But that isn't constructive wrt this
patch :-)


Segher
.



Thanks
Xiaoming Ni


Re: [PATCH] powerpc:Don't print raw EIP/LR hex values in dump_stack() and show_regs()

2020-12-21 Thread Segher Boessenkool
On Mon, Dec 21, 2020 at 04:42:23PM +, David Laight wrote:
> From: Segher Boessenkool
> > Sent: 21 December 2020 16:32
> > 
> > On Mon, Dec 21, 2020 at 04:17:21PM +0100, Christophe Leroy wrote:
> > > Le 21/12/2020 à 04:27, Xiaoming Ni a écrit :
> > > >Since the commit 2b0e86cc5de6 ("powerpc/fsl_booke/32: implement KASLR
> > > >infrastructure"), the powerpc system is ready to support KASLR.
> > > >To reduces the risk of invalidating address randomization, don't print 
> > > >the
> > > >EIP/LR hex values in dump_stack() and show_regs().
> > 
> > > I think your change is not enough to hide EIP address, see below a dump
> > > with you patch, you get "Faulting instruction address: 0xc03a0c14"
> > 
> > As far as I can see the patch does nothing to the GPR printout.  Often
> > GPRs contain code addresses.  As one example, the LR is moved via a GPR
> > (often GPR0, but not always) for storing on the stack.
> > 
> > So this needs more work.
> 
> If the dump_stack() is from an oops you need the real EIP value
> on order to stand any chance of making headway.

Or at least the function name + offset, yes.

> Otherwise you might just as well just print 'borked - tough luck'.

Yes.  ASLR is a house of cards.  But that isn't constructive wrt this
patch :-)


Segher


RE: [PATCH] powerpc:Don't print raw EIP/LR hex values in dump_stack() and show_regs()

2020-12-21 Thread David Laight
From: Segher Boessenkool
> Sent: 21 December 2020 16:32
> 
> On Mon, Dec 21, 2020 at 04:17:21PM +0100, Christophe Leroy wrote:
> > Le 21/12/2020 à 04:27, Xiaoming Ni a écrit :
> > >Since the commit 2b0e86cc5de6 ("powerpc/fsl_booke/32: implement KASLR
> > >infrastructure"), the powerpc system is ready to support KASLR.
> > >To reduces the risk of invalidating address randomization, don't print the
> > >EIP/LR hex values in dump_stack() and show_regs().
> 
> > I think your change is not enough to hide EIP address, see below a dump
> > with you patch, you get "Faulting instruction address: 0xc03a0c14"
> 
> As far as I can see the patch does nothing to the GPR printout.  Often
> GPRs contain code addresses.  As one example, the LR is moved via a GPR
> (often GPR0, but not always) for storing on the stack.
> 
> So this needs more work.

If the dump_stack() is from an oops you need the real EIP value
on order to stand any chance of making headway.

Otherwise you might just as well just print 'borked - tough luck'.

David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, 
UK
Registration No: 1397386 (Wales)



Re: [PATCH] powerpc:Don't print raw EIP/LR hex values in dump_stack() and show_regs()

2020-12-21 Thread Segher Boessenkool
On Mon, Dec 21, 2020 at 04:17:21PM +0100, Christophe Leroy wrote:
> Le 21/12/2020 à 04:27, Xiaoming Ni a écrit :
> >Since the commit 2b0e86cc5de6 ("powerpc/fsl_booke/32: implement KASLR
> >infrastructure"), the powerpc system is ready to support KASLR.
> >To reduces the risk of invalidating address randomization, don't print the
> >EIP/LR hex values in dump_stack() and show_regs().

> I think your change is not enough to hide EIP address, see below a dump 
> with you patch, you get "Faulting instruction address: 0xc03a0c14"

As far as I can see the patch does nothing to the GPR printout.  Often
GPRs contain code addresses.  As one example, the LR is moved via a GPR
(often GPR0, but not always) for storing on the stack.

So this needs more work.


Segher


Re: [PATCH] powerpc:Don't print raw EIP/LR hex values in dump_stack() and show_regs()

2020-12-21 Thread Christophe Leroy




Le 21/12/2020 à 04:27, Xiaoming Ni a écrit :

Since the commit 2b0e86cc5de6 ("powerpc/fsl_booke/32: implement KASLR
infrastructure"), the powerpc system is ready to support KASLR.
To reduces the risk of invalidating address randomization, don't print the
EIP/LR hex values in dump_stack() and show_regs().


I think this change is worth providing more details. Explain how the change 
improves debugging.



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


I think your change is not enough to hide EIP address, see below a dump with you patch, you get 
"Faulting instruction address: 0xc03a0c14"


Also, you replace a 8 digits value by a potentially long ling. Should should therefore put NIP and 
LR on different lines, and put CTR somewhere else (next to XER ?)


Now, the NIP and LR before the REGS and after the GPRs are exactly the same. No 
need to duplicate.

root@vgoip:~# busybox echo ACCESS_USERSPACE > /sys/kernel/debug/provoke-crash/DI
RECT
[  198.698395] lkdtm: Performing direct entry ACCESS_USERSPACE
[  198.698742] lkdtm: attempting bad read at 77ce8000
[  198.698844] Kernel attempted to read user page (77ce8000) - exploit attempt? 
(uid: 0)
[  198.706489] BUG: Unable to handle kernel data access on read at 0x77ce8000
[  198.713274] Faulting instruction address: 0xc03a0c14
[  198.718187] Oops: Kernel access of bad area, sig: 11 [#1]
[  198.723516] BE PAGE_SIZE=16K PREEMPT CMPC885
[  198.731272] CPU: 0 PID: 370 Comm: busybox Not tainted 
5.10.0-s3k-dev-13158-g8e389861badd #4386
[  198.739785] NIP: lkdtm_ACCESS_USERSPACE+0xbc/0x110 LR: lkdtm_ACCESS_USERSPACE+0xbc/0x110 CTR: 


[  198.748994] REGS: cad83d30 TRAP: 0300   Not tainted  
(5.10.0-s3k-dev-13158-g8e389861badd)
[  198.757081] MSR:  9032   CR: 2400  XER: 
[  198.763881] DAR: 77ce8000 DSISR: 8800
[  198.763881] GPR00: c03a0c14 cad83df0 c28a8000 0026 c093e518 0001 
0027 0027
[  198.763881] GPR08: c09a26dc   3000 24002228 100d3dd6 
100a2ffc 
[  198.763881] GPR16: 100cd280 100b 107e42ec 107e54b5 100d 100d 
 100a2fdc
[  198.763881] GPR24: ffef  cad83f10 0011 c078298c c293 
c084b980 77ce8000
[  198.802865] NIP lkdtm_ACCESS_USERSPACE+0xbc/0x110
[  198.807514] LR lkdtm_ACCESS_USERSPACE+0xbc/0x110
[  198.812077] Call Trace:
[  198.814485]  [cad83df0] lkdtm_ACCESS_USERSPACE+0xbc/0x110 (unreliable)
[  198.820940]  [cad83e20] direct_entry+0xe0/0x164
[  198.825415]  [cad83e50] full_proxy_write+0x78/0xbc
[  198.830148]  [cad83e80] vfs_write+0x12c/0x478
[  198.834452]  [cad83f00] ksys_write+0x78/0x128
[  198.838754]  [cad83f30] ret_from_syscall+0x0/0x34
[  198.843401] --- interrupt: c01 at 0xfd51d0c
[  198.847532] NIP: 0xfd51d0c LR: 0x10008404 CTR: 0fcff380
[  198.852696] REGS: cad83f40 TRAP: 0c01   Not tainted  
(5.10.0-s3k-dev-13158-g8e389861badd)
[  198.860784] MSR:  d032   CR: 44002424  XER: 
[  198.867928]
[  198.867928] GPR00: 0004 7fde21f0 77cf34d0 0001 10640008 0011 
 0fd5b36c
[  198.867928] GPR08:  00024000  0009 8402
[  198.884193] NIP 0xfd51d0c
[  198.886775] LR 0x10008404
[  198.889357] --- interrupt: c01
[  198.892373] Instruction dump:
[  198.895298] 7d295279 3940 40820078 80010034 83e1002c 7c0803a6 38210030 
4e800020
[  198.903214] 3c60c085 7fe4fb78 3863c874 4bcc5805 <813f> 3c60c085 3d29c0df 
3929c0de
[  198.911316] ---[ end trace ba4047052f99b7bf ]---
[  198.915865]
Segmentation fault






Signed-off-by: Xiaoming Ni 
---
  arch/powerpc/kernel/process.c | 12 ++--
  1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index a66f435dabbf..913cf1ea702e 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1455,8 +1455,8 @@ static void __show_regs(struct pt_regs *regs)
  {
int i, trap;
  
-	printk("NIP:  "REG" LR: "REG" CTR: "REG"\n",

-  regs->nip, regs->link, regs->ctr);
+   printk("NIP: %pS LR: %pS CTR: "REG"\n",
+  (void *)regs->nip, (void *)regs->link, regs->ctr);
printk("REGS: %px TRAP: %04lx   %s  (%s)\n",
   regs, regs->trap, print_tainted(), init_utsname()->release);
printk("MSR:  "REG" ", regs->msr);
@@ -1493,8 +1493,8 @@ static void __show_regs(struct pt_regs *regs)
 * above info out without failing
 */
if (IS_ENABLED(CONFIG_KALLSYMS)) {
-   printk("NIP ["REG"] %pS\n", regs->nip, (void *)regs->nip);
-   printk("LR ["REG"] %pS\n", regs->link, (void *)regs->link);
+   printk("NIP %pS\n", (void *)regs->nip);
+   printk("LR %pS\n", (void *)regs->link);
}
  }
  
@@ -2160,8 +2160,8 @@ void show_stack(struct task_struct *tsk, unsigned 

[PATCH] powerpc:Don't print raw EIP/LR hex values in dump_stack() and show_regs()

2020-12-20 Thread Xiaoming Ni
Since the commit 2b0e86cc5de6 ("powerpc/fsl_booke/32: implement KASLR
infrastructure"), the powerpc system is ready to support KASLR.
To reduces the risk of invalidating address randomization, don't print the
EIP/LR hex values in dump_stack() and show_regs().

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

Signed-off-by: Xiaoming Ni 
---
 arch/powerpc/kernel/process.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index a66f435dabbf..913cf1ea702e 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1455,8 +1455,8 @@ static void __show_regs(struct pt_regs *regs)
 {
int i, trap;
 
-   printk("NIP:  "REG" LR: "REG" CTR: "REG"\n",
-  regs->nip, regs->link, regs->ctr);
+   printk("NIP: %pS LR: %pS CTR: "REG"\n",
+  (void *)regs->nip, (void *)regs->link, regs->ctr);
printk("REGS: %px TRAP: %04lx   %s  (%s)\n",
   regs, regs->trap, print_tainted(), init_utsname()->release);
printk("MSR:  "REG" ", regs->msr);
@@ -1493,8 +1493,8 @@ static void __show_regs(struct pt_regs *regs)
 * above info out without failing
 */
if (IS_ENABLED(CONFIG_KALLSYMS)) {
-   printk("NIP ["REG"] %pS\n", regs->nip, (void *)regs->nip);
-   printk("LR ["REG"] %pS\n", regs->link, (void *)regs->link);
+   printk("NIP %pS\n", (void *)regs->nip);
+   printk("LR %pS\n", (void *)regs->link);
}
 }
 
@@ -2160,8 +2160,8 @@ void show_stack(struct task_struct *tsk, unsigned long 
*stack,
newsp = stack[0];
ip = stack[STACK_FRAME_LR_SAVE];
if (!firstframe || ip != lr) {
-   printk("%s["REG"] ["REG"] %pS",
-   loglvl, sp, ip, (void *)ip);
+   printk("%s ["REG"] %pS",
+   loglvl, sp, (void *)ip);
ret_addr = ftrace_graph_ret_addr(current,
_idx, ip, stack);
if (ret_addr != ip)
-- 
2.27.0