[PATCH] powerpc/tm: Fix userspace r13 corruption

[Michael Neuling]

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 at neuling.org>
> 
> Reviewed-by: Breno Leitao <leitao at 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?

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)
 
 	std	r11, GPR11(r1)			/* Temporary stash */
 
+	/* 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 */