From 960b564a68400b0fe1ef2917193f738ca1b05c73 Mon Sep 17 00:00:00 2001 From: Lucas Manning Date: Thu, 17 Aug 2023 15:11:53 -0700 Subject: [PATCH] Fix small mount propagation bug. The MS_PRIVATE flag was not being cleared in addPeer. Reported-by: syzbot+ea0d65b2da47bf96a8d1@syzkaller.appspotmail.com PiperOrigin-RevId: 557949154 --- pkg/sentry/vfs/mount.go | 40 ++++++++++++++--------------------- pkg/sentry/vfs/namespace.go | 2 +- pkg/sentry/vfs/propagation.go | 24 ++++++++++----------- test/syscalls/linux/mount.cc | 17 +++++++++++++++ 4 files changed, 45 insertions(+), 38 deletions(-) diff --git a/pkg/sentry/vfs/mount.go b/pkg/sentry/vfs/mount.go index 7e3ddac74..89479e140 100644 --- a/pkg/sentry/vfs/mount.go +++ b/pkg/sentry/vfs/mount.go @@ -87,8 +87,8 @@ type Mount struct { // Mount. children is protected by VirtualFilesystem.mountMu. children map[*Mount]struct{} - // propFlags are the propagation flags set on this mount. - propFlags uint32 + // isShared indicates this mount has the MS_SHARED propagation type. + isShared bool // sharedEntry represents an entry in a circular list (ring) of mounts in a // shared peer group. @@ -116,14 +116,14 @@ func (sharedMapper) linkerFor(mnt *Mount) *sharedEntry { return &mnt.sharedEntry func newMount(vfs *VirtualFilesystem, fs *Filesystem, root *Dentry, mntns *MountNamespace, opts *MountOptions) *Mount { mnt := &Mount{ - ID: vfs.lastMountID.Add(1), - Flags: opts.Flags, - vfs: vfs, - fs: fs, - root: root, - ns: mntns, - propFlags: linux.MS_PRIVATE, - refs: atomicbitops.FromInt64(1), + ID: vfs.lastMountID.Add(1), + Flags: opts.Flags, + vfs: vfs, + fs: fs, + root: root, + ns: mntns, + isShared: false, + refs: atomicbitops.FromInt64(1), } if opts.ReadOnly { mnt.setReadOnlyLocked(true) @@ -148,20 +148,12 @@ func (mnt *Mount) generateOptionalTags() string { defer mnt.vfs.mountMu.Unlock() // TODO(b/249777195): Support MS_SLAVE and MS_UNBINDABLE propagation types. var optional string - if mnt.shared() { + if mnt.isShared { optional = fmt.Sprintf("shared:%d", mnt.groupID) } return optional } -func (mnt *Mount) shared() bool { - return mnt.propFlags&linux.MS_SHARED != 0 -} - -func (mnt *Mount) private() bool { - return mnt.propFlags&linux.MS_PRIVATE != 0 -} - // NewFilesystem creates a new filesystem object not yet associated with any // mounts. It can be installed into the filesystem tree with ConnectMountAt. // Note that only the filesystem-specific mount options from opts are used by @@ -319,7 +311,7 @@ func (vfs *VirtualFilesystem) cloneMount(mnt *Mount, root *Dentry, mopts *MountO } } clone := vfs.NewDisconnectedMount(mnt.fs, root, opts) - if mnt.shared() { + if mnt.isShared { vfs.addPeer(mnt, clone) } return clone @@ -443,7 +435,7 @@ func (vfs *VirtualFilesystem) UmountAt(ctx context.Context, creds *auth.Credenti umountTree := []*Mount{vd.mount} parent, mountpoint := vd.mount.parent(), vd.mount.point() - if parent != nil && parent.shared() { + if parent != nil && parent.isShared { for peer := parent.sharedEntry.Next(); peer != parent; peer = peer.sharedEntry.Next() { umountMnt := vfs.mounts.Lookup(peer, mountpoint) // From https://www.kernel.org/doc/Documentation/filesystems/sharedsubtree.txt: @@ -537,7 +529,7 @@ func (vfs *VirtualFilesystem) umountRecursiveLocked(mnt *Mount, opts *umountRecu if parent := mnt.parent(); parent != nil && (opts.disconnectHierarchy || !parent.umounted) { vdsToDecRef = append(vdsToDecRef, vfs.disconnectLocked(mnt)) } - if mnt.shared() { + if mnt.isShared { vfs.setPropagation(mnt, linux.MS_PRIVATE) } } @@ -885,12 +877,12 @@ retry: // 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.shared() || newRootParent.shared() { + if newRootParent := newRootVd.mount.parent(); newRootVd.mount.isShared || newRootParent.isShared { vfs.mountMu.Unlock() return linuxerr.EINVAL } // put_old is a mount point and has the propagation type MS_SHARED. - if putOldVd.mount.root == putOldVd.dentry && putOldVd.mount.shared() { + if putOldVd.mount.root == putOldVd.dentry && putOldVd.mount.isShared { vfs.mountMu.Unlock() return linuxerr.EINVAL } diff --git a/pkg/sentry/vfs/namespace.go b/pkg/sentry/vfs/namespace.go index 5bf1ca8cf..d5488143a 100644 --- a/pkg/sentry/vfs/namespace.go +++ b/pkg/sentry/vfs/namespace.go @@ -179,7 +179,7 @@ func (vfs *VirtualFilesystem) CloneMountNamespace( ns.root.root.IncRef() ns.root.fs.IncRef() newns.root = newMount(vfs, ns.root.fs, ns.root.root, newns, &MountOptions{Flags: ns.root.Flags, ReadOnly: ns.root.ReadOnly()}) - if ns.root.shared() { + if ns.root.isShared { vfs.addPeer(ns.root, newns.root) } vfs.updateRootAndCWD(ctx, root, cwd, ns.root, newns.root) diff --git a/pkg/sentry/vfs/propagation.go b/pkg/sentry/vfs/propagation.go index 1c1e7e786..e45fa5718 100644 --- a/pkg/sentry/vfs/propagation.go +++ b/pkg/sentry/vfs/propagation.go @@ -56,26 +56,27 @@ func propTypeToString(pflag uint32) string { func (vfs *VirtualFilesystem) setPropagation(mnt *Mount, pflag uint32) error { switch pflag { case linux.MS_SHARED: - if mnt.private() { + if !mnt.isShared { id, err := vfs.allocateGroupID() if err != nil { return err } mnt.groupID = id sharedRingInit(mnt) + mnt.isShared = true } case linux.MS_PRIVATE: - if mnt.shared() { + if mnt.isShared { if sharedRingEmpty(mnt) { vfs.freeGroupID(mnt.groupID) } sharedRingRemove(mnt) mnt.groupID = 0 + mnt.isShared = false } default: panic(fmt.Sprintf("unsupported propagation type: %s", propTypeToString(pflag))) } - mnt.propFlags = pflag return nil } @@ -84,9 +85,8 @@ func (vfs *VirtualFilesystem) setPropagation(mnt *Mount, pflag uint32) error { // // +checklocks:vfs.mountMu func (vfs *VirtualFilesystem) addPeer(mnt *Mount, new *Mount) { - mnt.propFlags |= linux.MS_SHARED sharedRingAdd(mnt, new) - new.propFlags |= linux.MS_SHARED + new.isShared = true new.groupID = mnt.groupID } @@ -99,10 +99,10 @@ func (vfs *VirtualFilesystem) addPeer(mnt *Mount, new *Mount) { // +checklocksalias:mnt.vfs.mountMu=vfs.mountMu func (vfs *VirtualFilesystem) preparePropagationTree(mnt *Mount, vd VirtualDentry) map[*Mount]VirtualDentry { tree := map[*Mount]VirtualDentry{} - if !vd.mount.shared() { + if !vd.mount.isShared { return tree } - if !mnt.shared() { + if !mnt.isShared { vfs.setPropagation(mnt, linux.MS_SHARED) } for peer := vd.mount.sharedEntry.Next(); peer != vd.mount; peer = peer.sharedEntry.Next() { @@ -184,11 +184,9 @@ func (vfs *VirtualFilesystem) SetMountPropagationAt(ctx context.Context, creds * func (vfs *VirtualFilesystem) SetMountPropagation(mnt *Mount, propFlags uint32) { vfs.mountMu.Lock() defer vfs.mountMu.Unlock() - if propFlags != mnt.propFlags { - if propFlags&(linux.MS_SHARED|linux.MS_PRIVATE) != 0 { - vfs.setPropagation(mnt, propFlags) - } else { - panic(fmt.Sprintf("unsupported propagation type: %s", propTypeToString(propFlags))) - } + if propFlags&(linux.MS_SHARED|linux.MS_PRIVATE) != 0 { + vfs.setPropagation(mnt, propFlags) + } else { + panic(fmt.Sprintf("unsupported propagation type: %s", propTypeToString(propFlags))) } } diff --git a/test/syscalls/linux/mount.cc b/test/syscalls/linux/mount.cc index a5aa3fe55..d989ef675 100644 --- a/test/syscalls/linux/mount.cc +++ b/test/syscalls/linux/mount.cc @@ -49,6 +49,7 @@ #include "test/util/capability_util.h" #include "test/util/file_descriptor.h" #include "test/util/fs_util.h" +#include "test/util/linux_capability_util.h" #include "test/util/mount_util.h" #include "test/util/multiprocess_util.h" #include "test/util/posix_error.h" @@ -1443,6 +1444,22 @@ TEST(MountTest, DeadMountsAreDecRefd) { } } +TEST(MountTest, UmountSharedBind) { + SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(HaveCapability(CAP_SYS_ADMIN))); + std::string home = NewTempAbsPath(); + ASSERT_NO_ERRNO(Mkdir(home)); + ASSERT_THAT(chdir(home.c_str()), SyscallSucceeds()); + constexpr char dirpath[] = "./file"; + + ASSERT_THAT(mkdir(dirpath, 0), SyscallSucceeds()); + ASSERT_THAT(mount(dirpath, dirpath, 0, MS_BIND, 0), SyscallSucceeds()); + ASSERT_THAT(mount(0, dirpath, 0, MS_SHARED, 0), SyscallSucceeds()); + ASSERT_THAT(mount(dirpath, dirpath, 0, MS_BIND, 0), SyscallSucceeds()); + ASSERT_THAT(mount(0, dirpath, 0, MS_SHARED, 0), SyscallSucceeds()); + ASSERT_THAT(umount2(dirpath, MNT_DETACH), SyscallSucceeds()); + ASSERT_THAT(umount2(dirpath, MNT_DETACH), SyscallSucceeds()); +} + TEST(MountTest, MountNamespace) { SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(HaveCapability(CAP_SYS_ADMIN)));