* [RESEND PATCH] powerpc/kprobes: don't save r13 register in kprobe context
2024-12-09 9:10 [PATCH] " pangliyuan
@ 2025-01-16 8:45 ` pangliyuan
2025-01-16 9:31 ` Christophe Leroy
0 siblings, 1 reply; 3+ messages in thread
From: pangliyuan @ 2025-01-16 8:45 UTC (permalink / raw)
To: pangliyuan1, mpe, npiggin, christophe.leroy, naveen, maddy,
anil.s.keshavamurthy, davem, mhiramat, rostedt
Cc: wangfangpeng1, linuxppc-dev, linux-kernel
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [RESEND PATCH] powerpc/kprobes: don't save r13 register in kprobe context
2025-01-16 8:45 ` [RESEND PATCH] " pangliyuan
@ 2025-01-16 9:31 ` Christophe Leroy
0 siblings, 0 replies; 3+ messages in thread
From: Christophe Leroy @ 2025-01-16 9:31 UTC (permalink / raw)
To: pangliyuan, mpe, npiggin, naveen, maddy, anil.s.keshavamurthy,
davem, mhiramat, rostedt
Cc: wangfangpeng1, linuxppc-dev, linux-kernel
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [RESEND PATCH] powerpc/kprobes: don't save r13 register in kprobe context
@ 2025-01-18 7:57 pangliyuan
0 siblings, 0 replies; 3+ messages in thread
From: pangliyuan @ 2025-01-18 7:57 UTC (permalink / raw)
To: Christophe Leroy, mpe@ellerman.id.au, npiggin@gmail.com,
naveen@kernel.org, maddy@linux.ibm.com,
anil.s.keshavamurthy@intel.com, davem@davemloft.net,
mhiramat@kernel.org, rostedt@goodmis.org
Cc: wangfangpeng (A), linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org
On Sat, Jan 18, 2025 at 15:40:01PM +0800, pangliyuan wrote:
> 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.
You are right, r13 points to the paca structure attached to a given CPU, There
are issues with my description here, I will fix them in my next patch.
>
>By the way, don't we have the same problem on powerpc32 with register r2 ?
On ppc32, r2 really points to current process. Assume there is a process PA,
no matter which CPU the PA is running on, r2 on that CPU will point to PA,
therefore, saving and restoring r2 still make r2 point to the same PA.
On ppc64, each cpu has it's own paca structure pointed by r13. If you save
the r13 while runing on cpu A, and then switch to cpu B and restore the r13,
it will cause r13 on cpu B point to the paca attached to cpu A. So if you
get current process on cpu B, you will actually get the process running on
cpu A.
>>
>> 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.
Yes, the name of this macro is indeed not very accurate. Do you have any better
suggestions? How about using SAVE_29GRPS/REST_29GRPS for ppc64 while keeping
SAVE_30GRPS/REST_30GRPS for ppc32?
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-01-18 7:58 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-01-18 7:57 [RESEND PATCH] powerpc/kprobes: don't save r13 register in kprobe context pangliyuan
-- strict thread matches above, loose matches on Subject: below --
2024-12-09 9:10 [PATCH] " pangliyuan
2025-01-16 8:45 ` [RESEND PATCH] " pangliyuan
2025-01-16 9:31 ` Christophe Leroy
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).