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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <b97011b1-9e9f-4de4-af42-41cd6bf7c53b@csgroup.eu>
Date: Thu, 16 Jan 2025 10:31:41 +0100
From: Christophe Leroy <christophe.leroy@...roup.eu>
To: pangliyuan <pangliyuan1@...wei.com>, mpe@...erman.id.au,
 npiggin@...il.com, naveen@...nel.org, maddy@...ux.ibm.com,
 anil.s.keshavamurthy@...el.com, davem@...emloft.net, mhiramat@...nel.org,
 rostedt@...dmis.org
Cc: wangfangpeng1@...wei.com, linuxppc-dev@...ts.ozlabs.org,
 linux-kernel@...r.kernel.org
Subject: Re: [RESEND PATCH] powerpc/kprobes: don't save r13 register in kprobe
 context



Le 16/01/2025 à 09:45, pangliyuan a écrit :
> [Vous ne recevez pas souvent de courriers de pangliyuan1@...wei.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@...wei.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
>