Re: [PATCH] ppc64/kprobe: Fix oops when kprobed on 'stdu' instruction

2017-04-10 Thread Anton Blanchard
Hi Ravi,

> If we set a kprobe on a 'stdu' instruction on powerpc64, we see a
> kernel OOPS:

Ouch! We should mark this for stable.

Anton


Re: [PATCH] ppc64/kprobe: Fix oops when kprobed on 'stdu' instruction

2017-04-10 Thread Naveen N. Rao

Excerpts from Ravi Bangoria's message of April 10, 2017 20:50:
If we set a kprobe on a 'stdu' instruction on powerpc64, we see a kernel 
OOPS:


  [ 1275.165932] Bad kernel stack pointer cd93c840 at c0009868
  [ 1275.166378] Oops: Bad kernel stack pointer, sig: 6 [#1]
  ...
  GPR00: c01fcd93cb30 cd93c840 c15c5e00 cd93c840
  ...
  [ 1275.178305] NIP [c0009868] resume_kernel+0x2c/0x58
  [ 1275.178594] LR [c0006208] program_check_common+0x108/0x180

Basically, on 64 bit system, when user probes on 'stdu' instruction,
kernel does not emulate actual store in emulate_step itself because it
may corrupt exception frame. So kernel does actual store operation in
exception return code i.e. resume_kernel().

resume_kernel() loads the saved stack pointer from memory using lwz,
effectively loading a corrupt (32bit) address, causing the kernel crash.

Fix this by loading the 64bit value instead.


Thanks for fixing this!



Fixes: 8e9f69371536 ("powerpc/kprobe: Don't emulate store when kprobe stwu r1")


I think this should really be:
Fixes: be96f63375a1 ("powerpc: Split out instruction analysis part of 
emulate_step()")


...since the original commit just handled stwu on powerpc64 as well. In 
some ways, the 64-bit part of that commit wasn't that useful, but it 
never addressed stdu directly.



Signed-off-by: Ravi Bangoria 
---
History:
  Commit 8e9f69371536 ("powerpc/kprobe: Don't emulate store when kprobe
  stwu r1") fixed exception frame corruption for 32 bit system which uses
  'stwu' instruction for stack frame allocation. This commit also added
  code for 64 bit system but did not enabled it for 'stdu' instruction.
  So 'stdu' instruction on 64 bit machine was emulating actual store in
  emulate_step() itself until...

  Commit be96f63375a1 ("powerpc: Split out instruction analysis part of
  emulate_step()"), enabled it for 'stdu' instruction on 64 bit machine.

  Since then it's broken. So this should also go into stable.


Hmm... so I think kprobe on 'stdu' has always been broken on powerpc64.  
We haven't noticed since most stdu operations were probably landing in 
the red zone so the exception frame never got corrupted. In that sense, 
this fix is needed for BE ever since load/store emulation was added.


For LE, this is only getting exposed now due to your recent patch to 
enable load/store emulation on LE.




 arch/powerpc/kernel/entry_64.S | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 6432d4b..530f6e9 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -689,7 +689,7 @@ resume_kernel:

addir8,r1,INT_FRAME_SIZE/* Get the kprobed function entry */

-   lwz r3,GPR1(r1)
+   ld  r3,GPR1(r1)
subir3,r3,INT_FRAME_SIZE/* dst: Allocate a trampoline exception 
frame */
mr  r4,r1   /* src:  current exception frame */
mr  r1,r3   /* Reroute the trampoline frame to r1 */
@@ -704,7 +704,7 @@ resume_kernel:
bdnz2b

/* Do real store operation to complete stwu */


Can you also update the above comment to refer to 'stdu'?
Apart from that, for this patch:
Reviewed-by: Naveen N. Rao 

- Naveen



-   lwz r5,GPR1(r1)
+   ld  r5,GPR1(r1)
std r8,0(r5)

/* Clear _TIF_EMULATE_STACK_STORE flag */
--
1.9.3






[PATCH] ppc64/kprobe: Fix oops when kprobed on 'stdu' instruction

2017-04-10 Thread Ravi Bangoria
If we set a kprobe on a 'stdu' instruction on powerpc64, we see a kernel 
OOPS:

  [ 1275.165932] Bad kernel stack pointer cd93c840 at c0009868
  [ 1275.166378] Oops: Bad kernel stack pointer, sig: 6 [#1]
  ...
  GPR00: c01fcd93cb30 cd93c840 c15c5e00 cd93c840
  ...
  [ 1275.178305] NIP [c0009868] resume_kernel+0x2c/0x58
  [ 1275.178594] LR [c0006208] program_check_common+0x108/0x180

Basically, on 64 bit system, when user probes on 'stdu' instruction,
kernel does not emulate actual store in emulate_step itself because it
may corrupt exception frame. So kernel does actual store operation in
exception return code i.e. resume_kernel().

resume_kernel() loads the saved stack pointer from memory using lwz,
effectively loading a corrupt (32bit) address, causing the kernel crash.

Fix this by loading the 64bit value instead.

Fixes: 8e9f69371536 ("powerpc/kprobe: Don't emulate store when kprobe stwu r1")
Signed-off-by: Ravi Bangoria 
---
History:
  Commit 8e9f69371536 ("powerpc/kprobe: Don't emulate store when kprobe
  stwu r1") fixed exception frame corruption for 32 bit system which uses
  'stwu' instruction for stack frame allocation. This commit also added
  code for 64 bit system but did not enabled it for 'stdu' instruction.
  So 'stdu' instruction on 64 bit machine was emulating actual store in
  emulate_step() itself until...

  Commit be96f63375a1 ("powerpc: Split out instruction analysis part of
  emulate_step()"), enabled it for 'stdu' instruction on 64 bit machine.

  Since then it's broken. So this should also go into stable.

 arch/powerpc/kernel/entry_64.S | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 6432d4b..530f6e9 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -689,7 +689,7 @@ resume_kernel:
 
addir8,r1,INT_FRAME_SIZE/* Get the kprobed function entry */
 
-   lwz r3,GPR1(r1)
+   ld  r3,GPR1(r1)
subir3,r3,INT_FRAME_SIZE/* dst: Allocate a trampoline exception 
frame */
mr  r4,r1   /* src:  current exception frame */
mr  r1,r3   /* Reroute the trampoline frame to r1 */
@@ -704,7 +704,7 @@ resume_kernel:
bdnz2b
 
/* Do real store operation to complete stwu */
-   lwz r5,GPR1(r1)
+   ld  r5,GPR1(r1)
std r8,0(r5)
 
/* Clear _TIF_EMULATE_STACK_STORE flag */
-- 
1.9.3