[PATCH] powerpc/tm: Fix userspace r13 corruption

[PATCH] powerpc/tm: Fix userspace r13 corruption

[Michael Neuling]

When we treclaim we store the userspace checkpointed r13 to a scratch
SPR and then later save the scratch SPR to the user thread struct.

Unfortunately, this doesn't work as accessing the user thread struct
can take an SLB fault and the SLB fault handler will write the same
scratch SPRG that now contains the userspace r13.

To fix this, we store r13 to the kernel stack (which can't fault)
before we access the user thread struct.

Found by running P8 guest + powervm + disable_1tb_segments + TM. Seen
as a random userspace segfault with r13 looking like a kernel address.

Signed-off-by: Michael Neuling <mikey@neuling.org>
---
 arch/powerpc/kernel/tm.S | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/tm.S b/arch/powerpc/kernel/tm.S
index 6bffbc5aff..701b0f5b09 100644
--- a/arch/powerpc/kernel/tm.S
+++ b/arch/powerpc/kernel/tm.S
@@ -176,13 +176,20 @@ _GLOBAL(tm_reclaim)
 	std	r1, PACATMSCRATCH(r13)
 	ld	r1, PACAR1(r13)
 
-	/* Store the PPR in r11 and reset to decent value */
 	std	r11, GPR11(r1)			/* Temporary stash */
 
+	/* Store r13 away so we can free up the scratch SPR for the
+	 * SLB fault handler (needed once we start access the
+	 * thread_struct)
+	 */
+	GET_SCRATCH0(r11)
+	std	r11, GPR13(r1)
+
 	/* Reset MSR RI so we can take SLB faults again */
 	li	r11, MSR_RI
 	mtmsrd	r11, 1
 
+	/* Store the PPR in r11 and reset to decent value */
 	mfspr	r11, SPRN_PPR
 	HMT_MEDIUM
 
@@ -211,7 +218,7 @@ _GLOBAL(tm_reclaim)
 	ld	r4, GPR7(r1)			/* user r7 */
 	ld	r5, GPR11(r1)			/* user r11 */
 	ld	r6, GPR12(r1)			/* user r12 */
-	GET_SCRATCH0(8)				/* user r13 */
+	ld	r8, GPR13(r1)			/* user r13 */
 	std	r3, GPR1(r7)
 	std	r4, GPR7(r7)
 	std	r5, GPR11(r7)
-- 
2.17.1

Re: [PATCH] powerpc/tm: Fix userspace r13 corruption

[Breno Leitao]

Hi Mikey,

On 09/24/2018 04:27 AM, Michael Neuling wrote:
> When we treclaim we store the userspace checkpointed r13 to a scratch
> SPR and then later save the scratch SPR to the user thread struct.
> 
> Unfortunately, this doesn't work as accessing the user thread struct
> can take an SLB fault and the SLB fault handler will write the same
> scratch SPRG that now contains the userspace r13.
> 
> To fix this, we store r13 to the kernel stack (which can't fault)
> before we access the user thread struct.
> 
> Found by running P8 guest + powervm + disable_1tb_segments + TM. Seen
> as a random userspace segfault with r13 looking like a kernel address.
> 
> Signed-off-by: Michael Neuling <mikey@neuling.org>

Reviewed-by: Breno Leitao <leitao@debian.org>

I am wondering if the same problem could not happen with r1 as well, since r1
is kept in the paca->tm_scratch after MSR[RI] is enabled, in a code similar
to this:

        std     r1, PACATMSCRATCH(r13)
	..
        li      r11, MSR_RI
	mtmsrd  r11, 1
	....
        ld      r3, PACATMSCRATCH(r13)          /* user r1 */
        std     r3, GPR1(r7)

There might be a corruption if the exception that is raised just after
MSR[RI] is enabled somehow exits through 'fast_exception_return' (being
called by most of the exception handlers as I understand) which will
overwrite paca->tm_scratch with the upcoming MSR (aka SRR1) just before the
SRR1, as:

	ld      r3,_MSR(r1)

	...
	std     r3, PACATMSCRATCH(r13) /* Stash returned-to MSR */


It seems that slb_miss_common and instruction_access_slb does not go through
this path, but handle_page_fault calls
ret_from_except_lite->fast_exception_return, which might cause this
'possible' issue.

Anyway, I would like to investigate if this problem might happen or not for
real, but I do not fully understand what are the exceptions that might be
raised when just MSR[RI] is enabled.

Re: [PATCH] powerpc/tm: Fix userspace r13 corruption

[Michael Neuling]

On Mon, 2018-09-24 at 11:32 -0300, Breno Leitao wrote:
> Hi Mikey,
>=20
> On 09/24/2018 04:27 AM, Michael Neuling wrote:
> > When we treclaim we store the userspace checkpointed r13 to a scratch
> > SPR and then later save the scratch SPR to the user thread struct.
> >=20
> > Unfortunately, this doesn't work as accessing the user thread struct
> > can take an SLB fault and the SLB fault handler will write the same
> > scratch SPRG that now contains the userspace r13.
> >=20
> > To fix this, we store r13 to the kernel stack (which can't fault)
> > before we access the user thread struct.
> >=20
> > Found by running P8 guest + powervm + disable_1tb_segments + TM. Seen
> > as a random userspace segfault with r13 looking like a kernel address.
> >=20
> > Signed-off-by: Michael Neuling <mikey@neuling.org>
>=20
> Reviewed-by: Breno Leitao <leitao@debian.org>
>=20
> I am wondering if the same problem could not happen with r1 as well, sinc=
e r1
> is kept in the paca->tm_scratch after MSR[RI] is enabled, in a code simil=
ar
> to this:
>=20
>         std     r1, PACATMSCRATCH(r13)
> 	..
>         li      r11, MSR_RI
> 	mtmsrd  r11, 1
> 	....
>         ld      r3, PACATMSCRATCH(r13)          /* user r1 */
>         std     r3, GPR1(r7)
>=20
> There might be a corruption if the exception that is raised just after
> MSR[RI] is enabled somehow exits through 'fast_exception_return' (being
> called by most of the exception handlers as I understand) which will
> overwrite paca->tm_scratch with the upcoming MSR (aka SRR1) just before t=
he
> SRR1, as:
>=20
> 	ld      r3,_MSR(r1)
>=20
> 	...
> 	std     r3, PACATMSCRATCH(r13) /* Stash returned-to MSR */
>=20
>=20
> It seems that slb_miss_common and instruction_access_slb does not go thro=
ugh
> this path, but handle_page_fault calls
> ret_from_except_lite->fast_exception_return, which might cause this
> 'possible' issue.

Yeah, good catch! I think we are ok but it's delicate.=20

I'll store it in the kernel stack and avoid PACATMSCRATCH before I turn RI =
on. =20

This look ok?

diff --git a/arch/powerpc/kernel/tm.S b/arch/powerpc/kernel/tm.S
index 701b0f5b09..ff781b2852 100644
--- a/arch/powerpc/kernel/tm.S
+++ b/arch/powerpc/kernel/tm.S
@@ -178,6 +178,12 @@ _GLOBAL(tm_reclaim)
=20
 	std	r11, GPR11(r1)			/* Temporary stash */
=20
+	/* Move r1 to kernel stack in case PACATMSCRATCH is used once
+	 * we turn on RI
+	 */
+	ld	r11, PACATMSCRATCH(r13)
+	std	r11, GPR1(r1)
+
 	/* Store r13 away so we can free up the scratch SPR for the
 	 * SLB fault handler (needed once we start access the
 	 * thread_struct)
@@ -214,7 +220,7 @@ _GLOBAL(tm_reclaim)
 	SAVE_GPR(8, r7)				/* user r8 */
 	SAVE_GPR(9, r7)				/* user r9 */
 	SAVE_GPR(10, r7)			/* user r10 */
-	ld	r3, PACATMSCRATCH(r13)		/* user r1 */
+	ld	r3, GPR1(r1)			/* user r1 */
 	ld	r4, GPR7(r1)			/* user r7 */
 	ld	r5, GPR11(r1)			/* user r11 */
 	ld	r6, GPR12(r1)			/* user r12 */

Re: [PATCH] powerpc/tm: Fix userspace r13 corruption

[Breno Leitao]

Hi Mikey,

On 09/25/2018 02:24 AM, Michael Neuling wrote:
> On Mon, 2018-09-24 at 11:32 -0300, Breno Leitao wrote:
>> Hi Mikey,
>>
>> On 09/24/2018 04:27 AM, Michael Neuling wrote:
>>> When we treclaim we store the userspace checkpointed r13 to a scratch
>>> SPR and then later save the scratch SPR to the user thread struct.
>>>
>>> Unfortunately, this doesn't work as accessing the user thread struct
>>> can take an SLB fault and the SLB fault handler will write the same
>>> scratch SPRG that now contains the userspace r13.
>>>
>>> To fix this, we store r13 to the kernel stack (which can't fault)
>>> before we access the user thread struct.
>>>
>>> Found by running P8 guest + powervm + disable_1tb_segments + TM. Seen
>>> as a random userspace segfault with r13 looking like a kernel address.
>>>
>>> Signed-off-by: Michael Neuling <mikey@neuling.org>
>>
>> Reviewed-by: Breno Leitao <leitao@debian.org>
>>
>> I am wondering if the same problem could not happen with r1 as well, since r1
>> is kept in the paca->tm_scratch after MSR[RI] is enabled, in a code similar
>> to this:
>>
>>         std     r1, PACATMSCRATCH(r13)
>> 	..
>>         li      r11, MSR_RI
>> 	mtmsrd  r11, 1
>> 	....
>>         ld      r3, PACATMSCRATCH(r13)          /* user r1 */
>>         std     r3, GPR1(r7)
>>
>> There might be a corruption if the exception that is raised just after
>> MSR[RI] is enabled somehow exits through 'fast_exception_return' (being
>> called by most of the exception handlers as I understand) which will
>> overwrite paca->tm_scratch with the upcoming MSR (aka SRR1) just before the
>> SRR1, as:
>>
>> 	ld      r3,_MSR(r1)
>>
>> 	...
>> 	std     r3, PACATMSCRATCH(r13) /* Stash returned-to MSR */
>>
>>
>> It seems that slb_miss_common and instruction_access_slb does not go through
>> this path, but handle_page_fault calls
>> ret_from_except_lite->fast_exception_return, which might cause this
>> 'possible' issue.
> 
> Yeah, good catch! I think we are ok but it's delicate. 
> 
> I'll store it in the kernel stack and avoid PACATMSCRATCH before I turn RI on.  
> 
> This look ok?

Yes, That is exactly what I though when I looked at the code. Thanks for
fixing it.

Re: powerpc/tm: Fix userspace r13 corruption

[Michael Ellerman]

On Mon, 2018-09-24 at 07:27:04 UTC, Michael Neuling wrote:
> When we treclaim we store the userspace checkpointed r13 to a scratch
> SPR and then later save the scratch SPR to the user thread struct.
> 
> Unfortunately, this doesn't work as accessing the user thread struct
> can take an SLB fault and the SLB fault handler will write the same
> scratch SPRG that now contains the userspace r13.
> 
> To fix this, we store r13 to the kernel stack (which can't fault)
> before we access the user thread struct.
> 
> Found by running P8 guest + powervm + disable_1tb_segments + TM. Seen
> as a random userspace segfault with r13 looking like a kernel address.
> 
> Signed-off-by: Michael Neuling <mikey@neuling.org>
> Reviewed-by: Breno Leitao <leitao@debian.org>

Applied to powerpc fixes, thanks.

https://git.kernel.org/powerpc/c/cf13435b730a502e814c63c84d93db

cheers