Fix race between unshare(2) and execve(2).

There are two issues being fixed: 1) checkAndPreventSharingOutsideTG() was
accessing t.fsContext without grabbing t.mu and thus causing a data race with
a concurrent unshare and 2) the concurrent unshare might present an incomplete
view of the shared fscontext and cause execve to incorrectly conclude that it
was shared outside the thread group.

Reported-by: syzbot+e95172bececeffb258a9@syzkaller.appspotmail.com
Reported-by: syzbot+f834131c3fbdeec4b560@syzkaller.appspotmail.com
PiperOrigin-RevId: 800327461

Author: shailend-g

pkg/sentry/kernel/BUILD

@@ -40,6 +40,17 @@ go_template_instance(
     },
 )
 
+go_template_instance(
+    name = "atomicptr_fscontext",
+    out = "atomicptr_fscontext_unsafe.go",
+    package = "kernel",
+    prefix = "fsContext",
+    template = "//pkg/sync/atomicptr:generic_atomicptr",
+    types = {
+        "Value": "fsContext",
+    },
+)
+
 declare_mutex(
     name = "user_counters_mutex",
     out = "user_counters_mutex.go",

pkg/sentry/kernel/fs_context.go

@@ -61,6 +61,20 @@ func NewFSContext(root, cwd vfs.VirtualDentry, umask uint) *FSContext {
 	return &f
 }
 
+// destroy destroys the FSContext.
+//
+// Preconditions: f must have no refcount.
+func (f *FSContext) destroy(ctx context.Context) {
+	// Hold f.mu so that we don't race with RootDirectory() and
+	// WorkingDirectory().
+	f.mu.Lock()
+	defer f.mu.Unlock()
+	f.root.DecRef(ctx)
+	f.root = vfs.VirtualDentry{}
+	f.cwd.DecRef(ctx)
+	f.cwd = vfs.VirtualDentry{}
+}
+
 // DecRef implements RefCounter.DecRef.
 //
 // When f reaches zero references, DecRef will be called on both root and cwd
@@ -71,15 +85,7 @@ func NewFSContext(root, cwd vfs.VirtualDentry, umask uint) *FSContext {
 // proc files or other mechanisms.
 func (f *FSContext) DecRef(ctx context.Context) {
 	f.FSContextRefs.DecRef(func() {
-		// Hold f.mu so that we don't race with RootDirectory() and
-		// WorkingDirectory().
-		f.mu.Lock()
-		defer f.mu.Unlock()
-
-		f.root.DecRef(ctx)
-		f.root = vfs.VirtualDentry{}
-		f.cwd.DecRef(ctx)
-		f.cwd = vfs.VirtualDentry{}
+		f.destroy(ctx)
 	})
 }
 
@@ -202,7 +208,7 @@ func (f *FSContext) checkAndPreventSharingOutsideTG(tg *ThreadGroup) bool {
 
 	tgCount := int64(0)
 	tg.ForEachTask(func(t *Task) bool {
-		if t.fsContext == f {
+		if t.FSContext() == f {
 			tgCount++
 		}
 		return true
@@ -233,3 +239,20 @@ func (f *FSContext) share() bool {
 	f.IncRef()
 	return true
 }
+
+// unshareFromTask removes the FSContext f from the given Task t and replaces it with newF.
+// It returns a bool indicating whether f needs to be destroyed.
+
+// This func operates without compromising a concurrent checkAndPreventSharingOutsideTG(): t's
+// association with f is severed atomically by holding f.mu, allowing the concurrent func to
+// correctly ascribe extra ref counts to tasks outside of t's thread group.
+//
+// Preconditions: The caller must be on the task goroutine and must hold t.mu.
+func (f *FSContext) unshareFromTask(t *Task, newF *FSContext) bool {
+	f.mu.Lock()
+	defer f.mu.Unlock()
+	t.fsContext.Store(newF)
+	destroy := false
+	f.FSContextRefs.DecRef(func() { destroy = true })
+	return destroy
+}

pkg/sentry/kernel/kernel.go

@@ -1993,9 +1993,9 @@ func (k *Kernel) ReplaceFSContextRoots(ctx context.Context, oldRoot vfs.VirtualD
 	k.tasks.mu.RLock()
 	oldRootDecRefs := 0
 	k.tasks.forEachTaskLocked(func(t *Task) {
-		t.mu.Lock()
+		t.mu.Lock() // To prevent t's task goroutine from unsharing its fsContext from under us.
 		defer t.mu.Unlock()
-		if fsc := t.fsContext; fsc != nil {
+		if fsc := t.FSContext(); fsc != nil {
 			fsc.mu.Lock()
 			defer fsc.mu.Unlock()
 			if fsc.root == oldRoot {

pkg/sentry/kernel/kernel_state.go

@@ -67,6 +67,16 @@ func (t *Task) loadSeccomp(_ context.Context, seccompData *taskSeccomp) {
 	t.seccomp.Store(seccompData)
 }
 
+// saveFsContext is invoked by stateify.
+func (t *Task) saveFsContext() *FSContext {
+	return t.fsContext.Load()
+}
+
+// loadFsContext is invoked by stateify.
+func (t *Task) loadFsContext(_ context.Context, fsContext *FSContext) {
+	t.fsContext.Store(fsContext)
+}
+
 // saveAppCPUClockLast is invoked by stateify.
 func (tg *ThreadGroup) saveAppCPUClockLast() *Task {
 	return tg.appCPUClockLast.Load()

pkg/sentry/kernel/task.go

@@ -291,8 +291,9 @@ type Task struct {
 
 	// fsContext is the task's filesystem context.
 	//
-	// fsContext is protected by mu, and is owned by the task goroutine.
-	fsContext *FSContext
+	// fsContext is protected by mu, and is owned by the task goroutine. It can be read without
+	// synchronization using FSContext().
+	fsContext atomic.Pointer[FSContext] `state:".(*FSContext)"`
 
 	// fdTable is the task's file descriptor table.
 	//
@@ -750,7 +751,7 @@ func (t *Task) SyscallRestartBlock() SyscallRestartBlock {
 func (t *Task) IsChrooted() bool {
 	realRoot := t.mountNamespace.Root(t)
 	defer realRoot.DecRef(t)
-	root := t.fsContext.RootDirectory()
+	root := t.FSContext().RootDirectory()
 	defer root.DecRef(t)
 	return root != realRoot
 }
@@ -766,10 +767,11 @@ func (t *Task) TaskImage() *TaskImage {
 // FSContext returns t's FSContext. FSContext does not take an additional
 // reference on the returned FSContext.
 //
-// Precondition: The caller must be running on the task goroutine, or t.mu must
-// be locked.
+// Precondition: No synchronization is needed for just read access. If the caller instead intends to
+// modify the contents of the FSContext, it must either be running on the task goroutine or have
+// t.mu locked.
 func (t *Task) FSContext() *FSContext {
-	return t.fsContext
+	return t.fsContext.Load()
 }
 
 // FDTable returns t's FDTable. FDMTable does not take an additional reference

pkg/sentry/kernel/task_clone.go

@@ -192,9 +192,9 @@ func (t *Task) Clone(args *linux.CloneArgs) (ThreadID, *SyscallControl, error) {
 
 	var fsContext *FSContext
 	if args.Flags&linux.CLONE_FS == 0 || args.Flags&linux.CLONE_NEWNS != 0 {
-		fsContext = t.fsContext.Fork()
+		fsContext = t.FSContext().Fork()
 	} else {
-		fsContext = t.fsContext
+		fsContext = t.FSContext()
 		if !fsContext.share() {
 			// Linux fails clone with EAGAIN if there is a concurrent execve, see kernel/fork.c:copy_fs().
 			return 0, nil, linuxerr.EAGAIN
@@ -518,7 +518,7 @@ func (t *Task) Setns(fd *vfs.FileDescription, flags int32) error {
 			!t.Credentials().HasCapability(linux.CAP_SYS_ADMIN) {
 			return linuxerr.EPERM
 		}
-		oldFSContext := t.fsContext
+		oldFSContext := t.FSContext()
 		// The current task has to be an exclusive owner of its fs context.
 		if oldFSContext.ReadRefs() != 1 {
 			return linuxerr.EINVAL
@@ -535,7 +535,7 @@ func (t *Task) Setns(fd *vfs.FileDescription, flags int32) error {
 		ns.IncRef()
 		t.mu.Lock()
 		t.mountNamespace = ns
-		t.fsContext = fsContext
+		t.fsContext.Store(fsContext)
 		t.mu.Unlock()
 		oldNS.DecRef(t)
 		oldFSContext.DecRef(t)
@@ -671,7 +671,6 @@ func (t *Task) Unshare(flags int32) error {
 	defer cu.Clean()
 	t.mu.Lock()
 	defer t.mu.Unlock()
-	// Can't defer unlock: DecRefs must occur without holding t.mu.
 	if flags&linux.CLONE_NEWUTS != 0 {
 		if !haveCapSysAdmin {
 			return linuxerr.EPERM
@@ -701,16 +700,21 @@ func (t *Task) Unshare(flags int32) error {
 		cu.Add(func() { oldFDTable.DecRef(t) })
 	}
 	if flags&linux.CLONE_FS != 0 || flags&linux.CLONE_NEWNS != 0 {
-		oldFSContext := t.fsContext
-		t.fsContext = oldFSContext.Fork()
-		cu.Add(func() { oldFSContext.DecRef(t) })
+		oldFSContext := t.FSContext()
+		// unshareFromTask() lowers the old fs context's ref count, but its for us to
+		// destroy it if there are no other references.
+		if oldFSContext.unshareFromTask(t, oldFSContext.Fork()) {
+			// destroy() requires t.mu to not be held, hence the deferral.
+			cu.Add(func() { oldFSContext.destroy(t) })
+		}
 	}
 	if flags&linux.CLONE_NEWNS != 0 {
 		if !haveCapSysAdmin {
 			return linuxerr.EPERM
 		}
 		oldMountNS := t.mountNamespace
-		mntns, err := t.k.vfs.CloneMountNamespace(t, creds.UserNamespace, oldMountNS, &t.fsContext.root, &t.fsContext.cwd, t.k)
+		fsContext := t.FSContext()
+		mntns, err := t.k.vfs.CloneMountNamespace(t, creds.UserNamespace, oldMountNS, &fsContext.root, &fsContext.cwd, t.k)
 		if err != nil {
 			return err
 		}

pkg/sentry/kernel/task_context.go

@@ -96,7 +96,7 @@ func (t *Task) contextValue(key any, isTaskGoroutine bool) any {
 			t.mu.Lock()
 			defer t.mu.Unlock()
 		}
-		return t.fsContext.RootDirectory()
+		return t.FSContext().RootDirectory()
 	case vfs.CtxMountNamespace:
 		if !isTaskGoroutine {
 			t.mu.Lock()

pkg/sentry/kernel/task_exec.go

@@ -144,15 +144,16 @@ func (r *runExecveAfterExecveCredsLock) execute(t *Task) taskRunState {
 		return (*runInterrupt)(nil)
 	}
 
-	root := t.FSContext().RootDirectory()
+	fsContext := t.FSContext()
+	root := fsContext.RootDirectory()
 	defer root.DecRef(t)
-	wd := t.FSContext().WorkingDirectory()
+	wd := fsContext.WorkingDirectory()
 	defer wd.DecRef(t)
 
 	// Cannot gain privileges if the ptracer's capabilities prevent it.
 	stopPrivGain := t.shouldStopPrivGainDueToPtracerLocked()
 	// Cannot gain privileges if the FSContext is shared outside of our thread group.
-	if t.fsContext.checkAndPreventSharingOutsideTG(t.tg) {
+	if fsContext.checkAndPreventSharingOutsideTG(t.tg) {
 		stopPrivGain = true
 	}
 
@@ -612,8 +613,8 @@ func (t *Task) shouldStopPrivGainDueToPtracerLocked() bool {
 
 // Releases the mutex that serializes an execve(2) with a PTRACE_ATTACH and allows t.fsContext to
 // be shared again via clone(2). The caller must have called execveCredsMutexStartLock() and
-// fsContext.CheckAndPreventSharingOutsideTG() earlier.
+// fsContext.checkAndPreventSharingOutsideTG() earlier.
 func (t *Task) releaseExecveCredsLocks() {
 	t.execveCredsMutexUnlock()
-	t.fsContext.allowSharing()
+	t.FSContext().allowSharing()
 }

pkg/sentry/kernel/task_exit.go

@@ -298,7 +298,7 @@ func (*runExitMain) execute(t *Task) taskRunState {
 	// Releasing the MM unblocks a blocked CLONE_VFORK parent.
 	t.unstopVforkParent()
 
-	t.fsContext.DecRef(t)
+	t.FSContext().DecRef(t)
 	t.fdTable.DecRef(t)
 
 	// Detach task from all cgroups. This must happen before potentially the

pkg/sentry/kernel/task_start.go

@@ -159,7 +159,6 @@ func (ts *TaskSet) newTask(ctx context.Context, cfg *TaskConfig) (*Task, error)
 		signalMask:      atomicbitops.FromUint64(uint64(cfg.SignalMask)),
 		signalStack:     linux.SignalStack{Flags: linux.SS_DISABLE},
 		image:           *image,
-		fsContext:       cfg.FSContext,
 		fdTable:         cfg.FDTable,
 		k:               cfg.Kernel,
 		ptraceTracees:   make(map[*Task]struct{}),
@@ -183,6 +182,7 @@ func (ts *TaskSet) newTask(ctx context.Context, cfg *TaskConfig) (*Task, error)
 	}
 	t.netns = cfg.NetworkNamespace
 	t.creds.Store(cfg.Credentials)
+	t.fsContext.Store(cfg.FSContext)
 	t.endStopCond.L = &t.tg.signalHandlers.mu
 	// We don't construct t.blockingTimer until Task.run(); see that function
 	// for justification.