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>] [day] [month] [year] [list]
Message-ID: <02b42c0fd075402b8ee98f821fd50bb1@huawei.com>
Date: Sat, 18 Jan 2025 07:57:51 +0000
From: pangliyuan <pangliyuan1@...wei.com>
To: Christophe Leroy <christophe.leroy@...roup.eu>, "mpe@...erman.id.au"
	<mpe@...erman.id.au>, "npiggin@...il.com" <npiggin@...il.com>,
	"naveen@...nel.org" <naveen@...nel.org>, "maddy@...ux.ibm.com"
	<maddy@...ux.ibm.com>, "anil.s.keshavamurthy@...el.com"
	<anil.s.keshavamurthy@...el.com>, "davem@...emloft.net"
	<davem@...emloft.net>, "mhiramat@...nel.org" <mhiramat@...nel.org>,
	"rostedt@...dmis.org" <rostedt@...dmis.org>
CC: "wangfangpeng (A)" <wangfangpeng1@...wei.com>,
	"linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RESEND PATCH] powerpc/kprobes: don't save r13 register in kprobe
 context

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@...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.

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@...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.

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?