From 1407fdf120abb4cdce53acdcb0d5854f8f773348 Mon Sep 17 00:00:00 2001 From: Lucas Manning Date: Wed, 18 Oct 2023 12:14:49 -0700 Subject: [PATCH] Fix concurrent pivot_root bug. In the old version, we got the VDs for the old root and new root twice: once for updating the FSContext roots, and once for disconnecting/connecting to new mount points. Since there's no locking around getting VDs, this meant that there were some cases where concurrent pivot_root calls could use different values for the new root and for the updated context root. Reported-by: syzbot+83c62c020d2e1c259e6f@syzkaller.appspotmail.com PiperOrigin-RevId: 574552407 --- pkg/sentry/syscalls/linux/sys_file.go | 15 ++---- pkg/sentry/vfs/mount.go | 76 ++++++++++++++------------- 2 files changed, 44 insertions(+), 47 deletions(-) diff --git a/pkg/sentry/syscalls/linux/sys_file.go b/pkg/sentry/syscalls/linux/sys_file.go index e0f1a3ec2..328f89196 100644 --- a/pkg/sentry/syscalls/linux/sys_file.go +++ b/pkg/sentry/syscalls/linux/sys_file.go @@ -422,20 +422,13 @@ func PivotRoot(t *kernel.Task, sysno uintptr, args arch.SyscallArguments) (uintp } defer putOldTpop.Release(t) - oldRootVd := t.FSContext().RootDirectory() - defer oldRootVd.DecRef(t) - newRootVd, err := t.Kernel().VFS().GetDentryAt(t, t.Credentials(), &newRootTpop.pop, &vfs.GetDentryOptions{ - CheckSearchable: true, - }) + newRoot, oldRoot, err := t.Kernel().VFS().PivotRoot(t, t.Credentials(), &newRootTpop.pop, &putOldTpop.pop) if err != nil { return 0, nil, err } - defer newRootVd.DecRef(t) - - if err := t.Kernel().VFS().PivotRoot(t, t.Credentials(), &newRootTpop.pop, &putOldTpop.pop); err != nil { - return 0, nil, err - } - t.Kernel().ReplaceFSContextRoots(t, oldRootVd, newRootVd) + defer newRoot.DecRef(t) + defer oldRoot.DecRef(t) + t.Kernel().ReplaceFSContextRoots(t, oldRoot, newRoot) return 0, nil, nil } diff --git a/pkg/sentry/vfs/mount.go b/pkg/sentry/vfs/mount.go index a59c4cae7..275423dd2 100644 --- a/pkg/sentry/vfs/mount.go +++ b/pkg/sentry/vfs/mount.go @@ -1002,73 +1002,74 @@ retryFirst: // PivotRoot makes location pointed to by newRootPop the root of the current // namespace, and moves the current root to the location pointed to by -// putOldPop. -func (vfs *VirtualFilesystem) PivotRoot(ctx context.Context, creds *auth.Credentials, newRootPop *PathOperation, putOldPop *PathOperation) error { - newRootVd, err := vfs.GetDentryAt(ctx, creds, newRootPop, &GetDentryOptions{CheckSearchable: true}) +// putOldPop. If the operation is successful, it returns virtual dentries for +// the new root and the old root with an extra reference taken. +func (vfs *VirtualFilesystem) PivotRoot(ctx context.Context, creds *auth.Credentials, newRootPop *PathOperation, putOldPop *PathOperation) (newRoot, oldRoot VirtualDentry, err error) { + newRoot, err = vfs.GetDentryAt(ctx, creds, newRootPop, &GetDentryOptions{CheckSearchable: true}) if err != nil { - return err + return } - defer newRootVd.DecRef(ctx) - putOldVd, err := vfs.GetDentryAt(ctx, creds, putOldPop, &GetDentryOptions{CheckSearchable: true}) + defer newRoot.DecRef(ctx) + putOld, err := vfs.GetDentryAt(ctx, creds, putOldPop, &GetDentryOptions{CheckSearchable: true}) if err != nil { - return err + return } - defer putOldVd.DecRef(ctx) - rootVd := RootFromContext(ctx) - defer rootVd.DecRef(ctx) + defer putOld.DecRef(ctx) + oldRoot = RootFromContext(ctx) + defer oldRoot.DecRef(ctx) retry: epoch := vfs.mounts.seq.BeginRead() // Neither new_root nor put_old can be on the same mount as the current // root mount. - if newRootVd.mount == rootVd.mount || putOldVd.mount == rootVd.mount { - return linuxerr.EBUSY + if newRoot.mount == oldRoot.mount || putOld.mount == oldRoot.mount { + return newRoot, oldRoot, linuxerr.EBUSY } // new_root must be a mountpoint. - if newRootVd.mount.root != newRootVd.dentry { - return linuxerr.EINVAL + if newRoot.mount.root != newRoot.dentry { + return newRoot, oldRoot, linuxerr.EINVAL } // put_old must be at or underneath new_root. - path, err := vfs.PathnameReachable(ctx, newRootVd, putOldVd) + path, err := vfs.PathnameReachable(ctx, newRoot, putOld) if err != nil || len(path) == 0 { - return linuxerr.EINVAL + return newRoot, oldRoot, linuxerr.EINVAL } // The current root directory must be a mountpoint // (in the case it has been chrooted). - if rootVd.mount.root != rootVd.dentry { - return linuxerr.EINVAL + if oldRoot.mount.root != oldRoot.dentry { + return newRoot, oldRoot, linuxerr.EINVAL } // The current root and the new root cannot be on the rootfs mount. - if rootVd.mount.parent() == nil || newRootVd.mount.parent() == nil { - return linuxerr.EINVAL + if oldRoot.mount.parent() == nil || newRoot.mount.parent() == nil { + return newRoot, oldRoot, linuxerr.EINVAL } // The current root and the new root must be in the context's mount namespace. ns := MountNamespaceFromContext(ctx) defer ns.DecRef(ctx) vfs.lockMounts() - if rootVd.mount.ns != ns || newRootVd.mount.ns != ns { + if oldRoot.mount.ns != ns || newRoot.mount.ns != ns { vfs.unlockMounts(ctx) - return linuxerr.EINVAL + return newRoot, oldRoot, linuxerr.EINVAL } // Either the mount point at new_root, or the parent mount of that mount // point, has propagation type MS_SHARED. - if newRootParent := newRootVd.mount.parent(); newRootVd.mount.isShared || newRootParent.isShared { + if newRootParent := newRoot.mount.parent(); newRoot.mount.isShared || newRootParent.isShared { vfs.unlockMounts(ctx) - return linuxerr.EINVAL + return newRoot, oldRoot, linuxerr.EINVAL } // put_old is a mount point and has the propagation type MS_SHARED. - if putOldVd.mount.root == putOldVd.dentry && putOldVd.mount.isShared { + if putOld.mount.root == putOld.dentry && putOld.mount.isShared { vfs.unlockMounts(ctx) - return linuxerr.EINVAL + return newRoot, oldRoot, linuxerr.EINVAL } - putOldVd.IncRef() - putOldMp, err := vfs.lockMountpoint(putOldVd) + putOld.IncRef() + putOldMp, err := vfs.lockMountpoint(putOld) if err != nil { vfs.delayDecRef(putOldMp) vfs.unlockMounts(ctx) - return err + return newRoot, oldRoot, err } if !vfs.mounts.seq.BeginWriteOk(epoch) { @@ -1078,21 +1079,24 @@ retry: goto retry } defer vfs.unlockMounts(ctx) - mp := vfs.disconnectLocked(newRootVd.mount) + mp := vfs.disconnectLocked(newRoot.mount) vfs.delayDecRef(mp) - rootMp := vfs.disconnectLocked(rootVd.mount) + rootMp := vfs.disconnectLocked(oldRoot.mount) - vfs.connectLocked(rootVd.mount, putOldMp, ns) + vfs.connectLocked(oldRoot.mount, putOldMp, ns) putOldMp.dentry.mu.Unlock() rootMp.dentry.mu.Lock() - vfs.connectLocked(newRootVd.mount, rootMp, ns) + vfs.connectLocked(newRoot.mount, rootMp, ns) rootMp.dentry.mu.Unlock() vfs.mounts.seq.EndWrite() - vfs.delayDecRef(newRootVd.mount) - vfs.delayDecRef(rootVd.mount) - return nil + vfs.delayDecRef(newRoot.mount) + vfs.delayDecRef(oldRoot.mount) + + newRoot.IncRef() + oldRoot.IncRef() + return } // SetMountReadOnly sets the mount as ReadOnly.