[v1] powerpc/kprobes: don't save r13 register in kprobe context

[RESEND PATCH] powerpc/kprobes: don't save r13 register in kprobe context

Posted by pangliyuan 11 months ago

When CONFIG_STACKPROTECTOR_STRONG is enabled and FTRACE is disabled on
powerpc64, repeatedly triggering the kprobe process may cause stack check
failures and panic.

Case:
There is a kprobe(do nothing in handler) attached to the "shmem_get_inode",
and a process A is creating file on tmpfs.

                           CPU0
A |r13 = paca_ptrs[0], paca_ptrs[0]->canary=A->stack_canary
  |touch a file on tmpfs
  |shmem_mknod():
  |    load A's canary through r13 and stored it in A's stack
  |    shmem_get_inode():
  |        enter kprobe first
  |        optinsn_slot():
  |            stored r13 (paca_ptrs[0]) in stack

  ......

  ==> schedule,  B run on CPU0, A run on CPU1

			   CPU0
B |r13 = paca_ptrs[0], paca_ptrs[0]->canary=B->stack_canary
  |do something...
                           CPU1
A |            r13 = paca_ptrs[1], paca_ptrs[1]->canary=A->stack_canary
  |            about to leave 'optinsn_slot', restore r13 from A's stack
  |            r13 = paca_ptrs[0], paca_ptrs[0]->canary=B->stack_canary
  |            leave optinsn_slot, back to shmem_get_inode
  |        leave shmem_get_inode, back to shmem_mknod
  |    do canary check in shmem_mknod, but canary stored in A's stack (A's
       canary) doesn't match the canary loaded through r13 (B's canary),
       so panic

When A(on CPU0) entring optinsn_slot, it stored r13(paca_ptrs[0]) in stack,
then A is scheduled to CPU1 and restore r13 from A's stack when leaving
'optinsn_slot'. Now A is running on CPU1 but r13 point to CPU0's
paca_ptrs[0], at this time paca_ptrs[0]->__current points to another
process B, which cause A use B's canary to do stack check and panic.

This can be simply fixed by not saving and restoring the r13 register,
because on powerpc64, r13 is a special register that reserved to point
to the current process, no need to restore the outdated r13 if scheduled
had happened.

Fixes: 51c9c0843993 ("powerpc/kprobes: Implement Optprobes")
Signed-off-by: pangliyuan <pangliyuan1@huawei.com>
---
 arch/powerpc/kernel/optprobes_head.S | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/kernel/optprobes_head.S b/arch/powerpc/kernel/optprobes_head.S
index 35932f45fb4e..bf0d77e62ba8 100644
--- a/arch/powerpc/kernel/optprobes_head.S
+++ b/arch/powerpc/kernel/optprobes_head.S
@@ -10,12 +10,12 @@
 #include <asm/asm-offsets.h>
 
 #ifdef CONFIG_PPC64
-#define SAVE_30GPRS(base) SAVE_GPRS(2, 31, base)
-#define REST_30GPRS(base) REST_GPRS(2, 31, base)
+#define SAVE_NEEDED_GPRS(base) SAVE_GPRS(2, 12, base); SAVE_GPRS(14, 31, base)
+#define REST_NEEDED_GPRS(base) REST_GPRS(2, 12, base); REST_GPRS(14, 31, base)
 #define TEMPLATE_FOR_IMM_LOAD_INSNS	nop; nop; nop; nop; nop
 #else
-#define SAVE_30GPRS(base) stmw	r2, GPR2(base)
-#define REST_30GPRS(base) lmw	r2, GPR2(base)
+#define SAVE_NEEDED_GPRS(base) stmw	r2, GPR2(base)
+#define REST_NEEDED_GPRS(base) lmw	r2, GPR2(base)
 #define TEMPLATE_FOR_IMM_LOAD_INSNS	nop; nop; nop
 #endif
 
@@ -45,7 +45,7 @@ optprobe_template_entry:
 	/* Save the previous SP into stack */
 	addi	r0,r1,INT_FRAME_SIZE
 	PPC_STL	r0,GPR1(r1)
-	SAVE_30GPRS(r1)
+	SAVE_NEEDED_GPRS(r1)
 	/* Save SPRS */
 	mfmsr	r5
 	PPC_STL	r5,_MSR(r1)
@@ -123,7 +123,7 @@ optprobe_template_call_emulate:
 	PPC_LL	r5,_CCR(r1)
 	mtcr	r5
 	REST_GPR(0,r1)
-	REST_30GPRS(r1)
+	REST_NEEDED_GPRS(r1)
 	/* Restore the previous SP */
 	addi	r1,r1,INT_FRAME_SIZE
 
-- 
2.37.7

Re: [RESEND PATCH] powerpc/kprobes: don't save r13 register in kprobe context

Posted by Christophe Leroy 11 months ago


Le 16/01/2025 à 09:45, pangliyuan a écrit :
> [Vous ne recevez pas souvent de courriers de pangliyuan1@huawei.com. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ]
> 
> When CONFIG_STACKPROTECTOR_STRONG is enabled and FTRACE is disabled on
> powerpc64, repeatedly triggering the kprobe process may cause stack check
> failures and panic.
> 
> Case:
> There is a kprobe(do nothing in handler) attached to the "shmem_get_inode",
> and a process A is creating file on tmpfs.
> 
>                             CPU0
> A |r13 = paca_ptrs[0], paca_ptrs[0]->canary=A->stack_canary
>    |touch a file on tmpfs
>    |shmem_mknod():
>    |    load A's canary through r13 and stored it in A's stack
>    |    shmem_get_inode():
>    |        enter kprobe first
>    |        optinsn_slot():
>    |            stored r13 (paca_ptrs[0]) in stack
> 
>    ......
> 
>    ==> schedule,  B run on CPU0, A run on CPU1
> 
>                             CPU0
> B |r13 = paca_ptrs[0], paca_ptrs[0]->canary=B->stack_canary
>    |do something...
>                             CPU1
> A |            r13 = paca_ptrs[1], paca_ptrs[1]->canary=A->stack_canary
>    |            about to leave 'optinsn_slot', restore r13 from A's stack
>    |            r13 = paca_ptrs[0], paca_ptrs[0]->canary=B->stack_canary
>    |            leave optinsn_slot, back to shmem_get_inode
>    |        leave shmem_get_inode, back to shmem_mknod
>    |    do canary check in shmem_mknod, but canary stored in A's stack (A's
>         canary) doesn't match the canary loaded through r13 (B's canary),
>         so panic
> 
> When A(on CPU0) entring optinsn_slot, it stored r13(paca_ptrs[0]) in stack,
> then A is scheduled to CPU1 and restore r13 from A's stack when leaving
> 'optinsn_slot'. Now A is running on CPU1 but r13 point to CPU0's
> paca_ptrs[0], at this time paca_ptrs[0]->__current points to another
> process B, which cause A use B's canary to do stack check and panic.
> 
> This can be simply fixed by not saving and restoring the r13 register,
> because on powerpc64, r13 is a special register that reserved to point
> to the current process, no need to restore the outdated r13 if scheduled
> had happened.

Does r13 really points to current process ? I thought it was pointing to 
PACA which is a structure attached to a given CPU not a given process.

By the way, don't we have the same problem on powerpc32 with register r2 ?

> 
> Fixes: 51c9c0843993 ("powerpc/kprobes: Implement Optprobes")
> Signed-off-by: pangliyuan <pangliyuan1@huawei.com>
> ---
>   arch/powerpc/kernel/optprobes_head.S | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/optprobes_head.S b/arch/powerpc/kernel/optprobes_head.S
> index 35932f45fb4e..bf0d77e62ba8 100644
> --- a/arch/powerpc/kernel/optprobes_head.S
> +++ b/arch/powerpc/kernel/optprobes_head.S
> @@ -10,12 +10,12 @@
>   #include <asm/asm-offsets.h>
> 
>   #ifdef CONFIG_PPC64
> -#define SAVE_30GPRS(base) SAVE_GPRS(2, 31, base)
> -#define REST_30GPRS(base) REST_GPRS(2, 31, base)
> +#define SAVE_NEEDED_GPRS(base) SAVE_GPRS(2, 12, base); SAVE_GPRS(14, 31, base)
> +#define REST_NEEDED_GPRS(base) REST_GPRS(2, 12, base); REST_GPRS(14, 31, base)

This macro name seems a bit sketchy, as far as I understand r0 and r1 
also need to be saved/restored allthough they are not handled by this macro.


>   #define TEMPLATE_FOR_IMM_LOAD_INSNS    nop; nop; nop; nop; nop
>   #else
> -#define SAVE_30GPRS(base) stmw r2, GPR2(base)
> -#define REST_30GPRS(base) lmw  r2, GPR2(base)
> +#define SAVE_NEEDED_GPRS(base) stmw    r2, GPR2(base)
> +#define REST_NEEDED_GPRS(base) lmw     r2, GPR2(base)
>   #define TEMPLATE_FOR_IMM_LOAD_INSNS    nop; nop; nop
>   #endif
> 
> @@ -45,7 +45,7 @@ optprobe_template_entry:
>          /* Save the previous SP into stack */
>          addi    r0,r1,INT_FRAME_SIZE
>          PPC_STL r0,GPR1(r1)
> -       SAVE_30GPRS(r1)
> +       SAVE_NEEDED_GPRS(r1)
>          /* Save SPRS */
>          mfmsr   r5
>          PPC_STL r5,_MSR(r1)
> @@ -123,7 +123,7 @@ optprobe_template_call_emulate:
>          PPC_LL  r5,_CCR(r1)
>          mtcr    r5
>          REST_GPR(0,r1)
> -       REST_30GPRS(r1)
> +       REST_NEEDED_GPRS(r1)
>          /* Restore the previous SP */
>          addi    r1,r1,INT_FRAME_SIZE
> 
> --
> 2.37.7
>