| Message ID | 20250116084539.58847-1-pangliyuan1@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RESEND] powerpc/kprobes: don't save r13 register in kprobe context | expand |
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 >
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
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(-)