From 08920d098b3048167aafb7245a3476556367c528 Mon Sep 17 00:00:00 2001
From: Konstantin Bogomolov <bogomolov@google.com>
Date: Wed, 22 Mar 2023 11:31:56 -0700
Subject: [PATCH] Fix systrap TLS handling on ARM.

With context decoupling off, TLS was not initialized properly because upon
creation of a sysmsg thread the sighandler overwrote TLS with 0. The fix for
this is to write the correct TLS only _after_ the sysmsg thread is initialized.

With context decoupling on, retrieveArchSpecificState was not being used, which
means that TLS was not saved to the sentry at all. So in this case, sysmsg
threads would initially have the correct TLS value, but as soon as it changed
during runtime it would become incorrect.

Reported-by: syzbot+1cbe57d0e13ba2aa1898@syzkaller.appspotmail.com
PiperOrigin-RevId: 518626789
---
 pkg/sentry/platform/systrap/subprocess.go      | 18 +++++++++---------
 .../platform/systrap/subprocess_arm64.go       |  4 +++-
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/pkg/sentry/platform/systrap/subprocess.go b/pkg/sentry/platform/systrap/subprocess.go
index a2901f951..ed2f71e86 100644
--- a/pkg/sentry/platform/systrap/subprocess.go
+++ b/pkg/sentry/platform/systrap/subprocess.go
@@ -694,19 +694,19 @@ func (t *thread) NotifyInterrupt() {
 // The second return value is true if a syscall instruction can be replaced on
 // a function call.
 func (s *subprocess) switchToApp(c *context, ac *arch.Context64) (isSyscall bool, shouldPatchSyscall bool, err error) {
-	// Reset necessary registers.
-	regs := &ac.StateData().Regs
-	s.resetSysemuRegs(regs)
-	ctx := c.sharedContext
-	ctx.shared.Regs = regs.PtraceRegs
-	restoreArchSpecificState(ctx.shared, ac)
-
 	// Get sysmsg thread bound to the context; no-op if contextDecoupling is on.
+	regs := &ac.StateData().Regs
 	sysThread, err := s.getSysmsgThread(regs, c, ac)
 	if err != nil {
 		return false, false, err
 	}
 
+	// Reset necessary registers.
+	s.resetSysemuRegs(regs)
+	ctx := c.sharedContext
+	ctx.shared.Regs = regs.PtraceRegs
+	restoreArchSpecificState(ctx.shared, ac)
+
 	// Check for interrupts, and ensure that future interrupts signal the context.
 	if !c.interrupt.Enable(c.sharedContext) {
 		// Pending interrupt; simulate.
@@ -757,11 +757,11 @@ func (s *subprocess) switchToApp(c *context, ac *arch.Context64) (isSyscall bool
 				return false, false, err
 			}
 		}
-
-		retrieveArchSpecificState(ctx.shared, ac)
 	}
 
+	// Copy register state locally.
 	regs.PtraceRegs = ctx.shared.Regs
+	retrieveArchSpecificState(ctx.shared, ac)
 	// We have a signal. We verify however, that the signal was
 	// either delivered from the kernel or from this process. We
 	// don't respect other signals.
diff --git a/pkg/sentry/platform/systrap/subprocess_arm64.go b/pkg/sentry/platform/systrap/subprocess_arm64.go
index 8d0d1ff9e..21db37029 100644
--- a/pkg/sentry/platform/systrap/subprocess_arm64.go
+++ b/pkg/sentry/platform/systrap/subprocess_arm64.go
@@ -193,7 +193,9 @@ func restoreArchSpecificState(ctx *sysmsg.ThreadContext, ac *arch.Context64) {
 
 func setArchSpecificRegs(sysThread *sysmsgThread, regs *arch.Registers) {
 	if contextDecouplingExp {
-		// Set the start function and initial stack.
+		// Set the start function and initial stack. On ARM __export_start does not
+		// actually get used because we send a signal to the thread upon startup
+		// right away (see archSpecificSysmsgThreadInit below).
 		regs.PtraceRegs.Pc = uint64(stubSysmsgStart + uintptr(sysmsg.Sighandler_blob_offset____export_start))
 		regs.PtraceRegs.Sp = uint64(sysmsg.StackAddrToSyshandlerStack(sysThread.sysmsgPerThreadMemAddr()))
 	}