setgid directories for VFS1 tmpfs, overlayfs, and goferfs

PiperOrigin-RevId: 375780659

pkg/sentry/fs/attr.go

@@ -478,6 +478,20 @@ func (f FilePermissions) AnyRead() bool {
 	return f.User.Read || f.Group.Read || f.Other.Read
 }
 
+// HasSetUIDOrGID returns true if either the setuid or setgid bit is set.
+func (f FilePermissions) HasSetUIDOrGID() bool {
+	return f.SetUID || f.SetGID
+}
+
+// DropSetUIDAndMaybeGID turns off setuid, and turns off setgid if f allows
+// group execution.
+func (f *FilePermissions) DropSetUIDAndMaybeGID() {
+	f.SetUID = false
+	if f.Group.Execute {
+		f.SetGID = false
+	}
+}
+
 // FileOwner represents ownership of a file.
 //
 // +stateify savable

pkg/sentry/fs/dev/dev.go

@@ -16,6 +16,7 @@
 package dev
 
 import (
+	"fmt"
 	"math"
 
 	"gvisor.dev/gvisor/pkg/context"
@@ -90,6 +91,11 @@ func newSymlink(ctx context.Context, target string, msrc *fs.MountSource) *fs.In
 
 // New returns the root node of a device filesystem.
 func New(ctx context.Context, msrc *fs.MountSource) *fs.Inode {
+	shm, err := tmpfs.NewDir(ctx, nil, fs.RootOwner, fs.FilePermsFromMode(0777), msrc, nil /* parent */)
+	if err != nil {
+		panic(fmt.Sprintf("tmpfs.NewDir failed: %v", err))
+	}
+
 	contents := map[string]*fs.Inode{
 		"fd":     newSymlink(ctx, "/proc/self/fd", msrc),
 		"stdin":  newSymlink(ctx, "/proc/self/fd/0", msrc),
@@ -108,7 +114,7 @@ func New(ctx context.Context, msrc *fs.MountSource) *fs.Inode {
 		"random":  newMemDevice(ctx, newRandomDevice(ctx, fs.RootOwner, 0444), msrc, randomDevMinor),
 		"urandom": newMemDevice(ctx, newRandomDevice(ctx, fs.RootOwner, 0444), msrc, urandomDevMinor),
 
-		"shm": tmpfs.NewDir(ctx, nil, fs.RootOwner, fs.FilePermsFromMode(0777), msrc),
+		"shm": shm,
 
 		// A devpts is typically mounted at /dev/pts to provide
 		// pseudoterminal support. Place an empty directory there for

pkg/sentry/fs/fsutil/host_mappable.go

@@ -155,12 +155,20 @@ func (h *HostMappable) DecRef(fr memmap.FileRange) {
 //   T2: Appends to file causing it to grow
 //   T2: Writes to mapped pages and COW happens
 //   T1: Continues and wronly invalidates the page mapped in step above.
-func (h *HostMappable) Truncate(ctx context.Context, newSize int64) error {
+func (h *HostMappable) Truncate(ctx context.Context, newSize int64, uattr fs.UnstableAttr) error {
 	h.truncateMu.Lock()
 	defer h.truncateMu.Unlock()
 
 	mask := fs.AttrMask{Size: true}
 	attr := fs.UnstableAttr{Size: newSize}
+
+	// Truncating a file clears privilege bits.
+	if uattr.Perms.HasSetUIDOrGID() {
+		mask.Perms = true
+		attr.Perms = uattr.Perms
+		attr.Perms.DropSetUIDAndMaybeGID()
+	}
+
 	if err := h.backingFile.SetMaskedAttributes(ctx, mask, attr, false); err != nil {
 		return err
 	}
@@ -193,10 +201,17 @@ func (h *HostMappable) Allocate(ctx context.Context, offset int64, length int64)
 }
 
 // Write writes to the file backing this mappable.
-func (h *HostMappable) Write(ctx context.Context, src usermem.IOSequence, offset int64) (int64, error) {
+func (h *HostMappable) Write(ctx context.Context, src usermem.IOSequence, offset int64, uattr fs.UnstableAttr) (int64, error) {
 	h.truncateMu.RLock()
+	defer h.truncateMu.RUnlock()
 	n, err := src.CopyInTo(ctx, &writer{ctx: ctx, hostMappable: h, off: offset})
-	h.truncateMu.RUnlock()
+	if n > 0 && uattr.Perms.HasSetUIDOrGID() {
+		mask := fs.AttrMask{Perms: true}
+		uattr.Perms.DropSetUIDAndMaybeGID()
+		if err := h.backingFile.SetMaskedAttributes(ctx, mask, uattr, false); err != nil {
+			return n, err
+		}
+	}
 	return n, err
 }
 

pkg/sentry/fs/fsutil/inode_cached.go

@@ -310,6 +310,12 @@ func (c *CachingInodeOperations) Truncate(ctx context.Context, inode *fs.Inode,
 	now := ktime.NowFromContext(ctx)
 	masked := fs.AttrMask{Size: true}
 	attr := fs.UnstableAttr{Size: size}
+	if c.attr.Perms.HasSetUIDOrGID() {
+		masked.Perms = true
+		attr.Perms = c.attr.Perms
+		attr.Perms.DropSetUIDAndMaybeGID()
+		c.attr.Perms = attr.Perms
+	}
 	if err := c.backingFile.SetMaskedAttributes(ctx, masked, attr, false); err != nil {
 		c.dataMu.Unlock()
 		return err
@@ -685,13 +691,14 @@ func (rw *inodeReadWriter) ReadToBlocks(dsts safemem.BlockSeq) (uint64, error) {
 	return done, nil
 }
 
-// maybeGrowFile grows the file's size if data has been written past the old
-// size.
+// maybeUpdateAttrs updates the file's attributes after a write. It updates
+// size if data has been written past the old size, and setuid/setgid if any
+// bytes were written.
 //
 // Preconditions:
 // * rw.c.attrMu must be locked.
 // * rw.c.dataMu must be locked.
-func (rw *inodeReadWriter) maybeGrowFile() {
+func (rw *inodeReadWriter) maybeUpdateAttrs(nwritten uint64) {
 	// If the write ends beyond the file's previous size, it causes the
 	// file to grow.
 	if rw.offset > rw.c.attr.Size {
@@ -705,6 +712,12 @@ func (rw *inodeReadWriter) maybeGrowFile() {
 		rw.c.attr.Usage = rw.offset
 		rw.c.dirtyAttr.Usage = true
 	}
+
+	// If bytes were written, ensure setuid and setgid are cleared.
+	if nwritten > 0 && rw.c.attr.Perms.HasSetUIDOrGID() {
+		rw.c.dirtyAttr.Perms = true
+		rw.c.attr.Perms.DropSetUIDAndMaybeGID()
+	}
 }
 
 // WriteFromBlocks implements safemem.Writer.WriteFromBlocks.
@@ -732,7 +745,7 @@ func (rw *inodeReadWriter) WriteFromBlocks(srcs safemem.BlockSeq) (uint64, error
 			segMR := seg.Range().Intersect(mr)
 			ims, err := mf.MapInternal(seg.FileRangeOf(segMR), hostarch.Write)
 			if err != nil {
-				rw.maybeGrowFile()
+				rw.maybeUpdateAttrs(done)
 				rw.c.dataMu.Unlock()
 				return done, err
 			}
@@ -744,7 +757,7 @@ func (rw *inodeReadWriter) WriteFromBlocks(srcs safemem.BlockSeq) (uint64, error
 			srcs = srcs.DropFirst64(n)
 			rw.c.dirty.MarkDirty(segMR)
 			if err != nil {
-				rw.maybeGrowFile()
+				rw.maybeUpdateAttrs(done)
 				rw.c.dataMu.Unlock()
 				return done, err
 			}
@@ -765,7 +778,7 @@ func (rw *inodeReadWriter) WriteFromBlocks(srcs safemem.BlockSeq) (uint64, error
 			srcs = srcs.DropFirst64(n)
 			// Partial writes are fine. But we must stop writing.
 			if n != src.NumBytes() || err != nil {
-				rw.maybeGrowFile()
+				rw.maybeUpdateAttrs(done)
 				rw.c.dataMu.Unlock()
 				return done, err
 			}
@@ -774,7 +787,7 @@ func (rw *inodeReadWriter) WriteFromBlocks(srcs safemem.BlockSeq) (uint64, error
 			seg, gap = gap.NextSegment(), FileRangeGapIterator{}
 		}
 	}
-	rw.maybeGrowFile()
+	rw.maybeUpdateAttrs(done)
 	rw.c.dataMu.Unlock()
 	return done, nil
 }

pkg/sentry/fs/gofer/file.go

@@ -237,10 +237,20 @@ func (f *fileOperations) Write(ctx context.Context, file *fs.File, src usermem.I
 	// and availability of a host-mappable FD.
 	if f.inodeOperations.session().cachePolicy.useCachingInodeOps(file.Dirent.Inode) {
 		n, err = f.inodeOperations.cachingInodeOps.Write(ctx, src, offset)
-	} else if f.inodeOperations.fileState.hostMappable != nil {
-		n, err = f.inodeOperations.fileState.hostMappable.Write(ctx, src, offset)
 	} else {
-		n, err = src.CopyInTo(ctx, f.handles.readWriterAt(ctx, offset))
+		uattr, e := f.UnstableAttr(ctx, file)
+		if e != nil {
+			return 0, e
+		}
+		if f.inodeOperations.fileState.hostMappable != nil {
+			n, err = f.inodeOperations.fileState.hostMappable.Write(ctx, src, offset, uattr)
+		} else {
+			n, err = src.CopyInTo(ctx, f.handles.readWriterAt(ctx, offset))
+			if n > 0 && uattr.Perms.HasSetUIDOrGID() {
+				uattr.Perms.DropSetUIDAndMaybeGID()
+				f.inodeOperations.SetPermissions(ctx, file.Dirent.Inode, uattr.Perms)
+			}
+		}
 	}
 
 	if n == 0 {

pkg/sentry/fs/gofer/inode.go

@@ -600,11 +600,25 @@ func (i *inodeOperations) Truncate(ctx context.Context, inode *fs.Inode, length
 	if i.session().cachePolicy.useCachingInodeOps(inode) {
 		return i.cachingInodeOps.Truncate(ctx, inode, length)
 	}
-	if i.session().cachePolicy == cacheRemoteRevalidating {
-		return i.fileState.hostMappable.Truncate(ctx, length)
+
+	uattr, err := i.fileState.unstableAttr(ctx)
+	if err != nil {
+		return err
 	}
 
-	return i.fileState.file.setAttr(ctx, p9.SetAttrMask{Size: true}, p9.SetAttr{Size: uint64(length)})
+	if i.session().cachePolicy == cacheRemoteRevalidating {
+		return i.fileState.hostMappable.Truncate(ctx, length, uattr)
+	}
+
+	mask := p9.SetAttrMask{Size: true}
+	attr := p9.SetAttr{Size: uint64(length)}
+	if uattr.Perms.HasSetUIDOrGID() {
+		mask.Permissions = true
+		uattr.Perms.DropSetUIDAndMaybeGID()
+		attr.Permissions = p9.FileMode(uattr.Perms.LinuxMode())
+	}
+
+	return i.fileState.file.setAttr(ctx, mask, attr)
 }
 
 // GetXattr implements fs.InodeOperations.GetXattr.

pkg/sentry/fs/gofer/path.go

@@ -130,7 +130,16 @@ func (i *inodeOperations) Create(ctx context.Context, dir *fs.Inode, name string
 		panic(fmt.Sprintf("Create called with unknown or unset open flags: %v", flags))
 	}
 
+	// If the parent directory has setgid enabled, change the new file's owner.
 	owner := fs.FileOwnerFromContext(ctx)
+	parentUattr, err := dir.UnstableAttr(ctx)
+	if err != nil {
+		return nil, err
+	}
+	if parentUattr.Perms.SetGID {
+		owner.GID = parentUattr.Owner.GID
+	}
+
 	hostFile, err := newFile.create(ctx, name, openFlags, p9.FileMode(perm.LinuxMode()), p9.UID(owner.UID), p9.GID(owner.GID))
 	if err != nil {
 		// Could not create the file.
@@ -225,7 +234,18 @@ func (i *inodeOperations) CreateDirectory(ctx context.Context, dir *fs.Inode, s
 		return syserror.ENAMETOOLONG
 	}
 
+	// If the parent directory has setgid enabled, change the new directory's
+	// owner and enable setgid.
 	owner := fs.FileOwnerFromContext(ctx)
+	parentUattr, err := dir.UnstableAttr(ctx)
+	if err != nil {
+		return err
+	}
+	if parentUattr.Perms.SetGID {
+		owner.GID = parentUattr.Owner.GID
+		perm.SetGID = true
+	}
+
 	if _, err := i.fileState.file.mkdir(ctx, s, p9.FileMode(perm.LinuxMode()), p9.UID(owner.UID), p9.GID(owner.GID)); err != nil {
 		return err
 	}

pkg/sentry/fs/tmpfs/fs.go

@@ -151,5 +151,5 @@ func (f *Filesystem) Mount(ctx context.Context, device string, flags fs.MountSou
 	}
 
 	// Construct the tmpfs root.
-	return NewDir(ctx, nil, owner, perms, msrc), nil
+	return NewDir(ctx, nil, owner, perms, msrc, nil /* parent */)
 }

pkg/sentry/fs/tmpfs/inode_file.go

@@ -226,6 +226,12 @@ func (f *fileInodeOperations) Truncate(ctx context.Context, _ *fs.Inode, size in
 		now := ktime.NowFromContext(ctx)
 		f.attr.ModificationTime = now
 		f.attr.StatusChangeTime = now
+
+		// Truncating clears privilege bits.
+		f.attr.Perms.SetUID = false
+		if f.attr.Perms.Group.Execute {
+			f.attr.Perms.SetGID = false
+		}
 	}
 	f.dataMu.Unlock()
 
@@ -363,7 +369,14 @@ func (f *fileInodeOperations) write(ctx context.Context, src usermem.IOSequence,
 	now := ktime.NowFromContext(ctx)
 	f.attr.ModificationTime = now
 	f.attr.StatusChangeTime = now
-	return src.CopyInTo(ctx, &fileReadWriter{f, offset})
+	nwritten, err := src.CopyInTo(ctx, &fileReadWriter{f, offset})
+
+	// Writing clears privilege bits.
+	if nwritten > 0 {
+		f.attr.Perms.DropSetUIDAndMaybeGID()
+	}
+
+	return nwritten, err
 }
 
 type fileReadWriter struct {

pkg/sentry/fs/tmpfs/tmpfs.go

@@ -87,7 +87,20 @@ type Dir struct {
 var _ fs.InodeOperations = (*Dir)(nil)
 
 // NewDir returns a new directory.
-func NewDir(ctx context.Context, contents map[string]*fs.Inode, owner fs.FileOwner, perms fs.FilePermissions, msrc *fs.MountSource) *fs.Inode {
+func NewDir(ctx context.Context, contents map[string]*fs.Inode, owner fs.FileOwner, perms fs.FilePermissions, msrc *fs.MountSource, parent *fs.Inode) (*fs.Inode, error) {
+	// If the parent has setgid enabled, the new directory enables it and changes
+	// its GID.
+	if parent != nil {
+		parentUattr, err := parent.UnstableAttr(ctx)
+		if err != nil {
+			return nil, err
+		}
+		if parentUattr.Perms.SetGID {
+			owner.GID = parentUattr.Owner.GID
+			perms.SetGID = true
+		}
+	}
+
 	d := &Dir{
 		ramfsDir: ramfs.NewDir(ctx, contents, owner, perms),
 		kernel:   kernel.KernelFromContext(ctx),
@@ -101,7 +114,7 @@ func NewDir(ctx context.Context, contents map[string]*fs.Inode, owner fs.FileOwn
 		InodeID:   tmpfsDevice.NextIno(),
 		BlockSize: hostarch.PageSize,
 		Type:      fs.Directory,
-	})
+	}), nil
 }
 
 // afterLoad is invoked by stateify.
@@ -219,11 +232,21 @@ func (d *Dir) SetTimestamps(ctx context.Context, i *fs.Inode, ts fs.TimeSpec) er
 func (d *Dir) newCreateOps() *ramfs.CreateOps {
 	return &ramfs.CreateOps{
 		NewDir: func(ctx context.Context, dir *fs.Inode, perms fs.FilePermissions) (*fs.Inode, error) {
-			return NewDir(ctx, nil, fs.FileOwnerFromContext(ctx), perms, dir.MountSource), nil
+			return NewDir(ctx, nil, fs.FileOwnerFromContext(ctx), perms, dir.MountSource, dir)
 		},
 		NewFile: func(ctx context.Context, dir *fs.Inode, perms fs.FilePermissions) (*fs.Inode, error) {
+			// If the parent has setgid enabled, change the GID of the new file.
+			owner := fs.FileOwnerFromContext(ctx)
+			parentUattr, err := dir.UnstableAttr(ctx)
+			if err != nil {
+				return nil, err
+			}
+			if parentUattr.Perms.SetGID {
+				owner.GID = parentUattr.Owner.GID
+			}
+
 			uattr := fs.WithCurrentTime(ctx, fs.UnstableAttr{
-				Owner: fs.FileOwnerFromContext(ctx),
+				Owner: owner,
 				Perms: perms,
 				// Always start unlinked.
 				Links: 0,

pkg/sentry/fs/user/user_test.go

@@ -104,7 +104,10 @@ func TestGetExecUserHome(t *testing.T) {
 		t.Run(name, func(t *testing.T) {
 			ctx := contexttest.Context(t)
 			msrc := fs.NewPseudoMountSource(ctx)
-			rootInode := tmpfs.NewDir(ctx, nil, fs.RootOwner, fs.FilePermsFromMode(0777), msrc)
+			rootInode, err := tmpfs.NewDir(ctx, nil, fs.RootOwner, fs.FilePermsFromMode(0777), msrc, nil /* parent */)
+			if err != nil {
+				t.Fatalf("tmpfs.NewDir failed: %v", err)
+			}
 
 			mns, err := fs.NewMountNamespace(ctx, rootInode)
 			if err != nil {

pkg/sentry/fsimpl/overlay/regular_file.go

@@ -207,9 +207,10 @@ func (fd *regularFileFD) SetStat(ctx context.Context, opts vfs.SetStatOptions) e
 		return err
 	}
 
-	// Changing owners may clear one or both of the setuid and setgid bits,
-	// so we may have to update opts before setting d.mode.
-	if opts.Stat.Mask&(linux.STATX_UID|linux.STATX_GID) != 0 {
+	// Changing owners or truncating may clear one or both of the setuid and
+	// setgid bits, so we may have to update opts before setting d.mode.
+	inotifyMask := opts.Stat.Mask
+	if opts.Stat.Mask&(linux.STATX_UID|linux.STATX_GID|linux.STATX_SIZE) != 0 {
 		stat, err := wrappedFD.Stat(ctx, vfs.StatOptions{
 			Mask: linux.STATX_MODE,
 		})
@@ -218,10 +219,14 @@ func (fd *regularFileFD) SetStat(ctx context.Context, opts vfs.SetStatOptions) e
 		}
 		opts.Stat.Mode = stat.Mode
 		opts.Stat.Mask |= linux.STATX_MODE
+		// Don't generate inotify IN_ATTRIB for size-only changes (truncations).
+		if opts.Stat.Mask&(linux.STATX_UID|linux.STATX_GID) != 0 {
+			inotifyMask |= linux.STATX_MODE
+		}
 	}
 
 	d.updateAfterSetStatLocked(&opts)
-	if ev := vfs.InotifyEventFromStatMask(opts.Stat.Mask); ev != 0 {
+	if ev := vfs.InotifyEventFromStatMask(inotifyMask); ev != 0 {
 		d.InotifyWithParent(ctx, ev, 0, vfs.InodeEvent)
 	}
 	return nil

pkg/sentry/syscalls/linux/sys_file.go

@@ -1673,9 +1673,11 @@ func chown(t *kernel.Task, d *fs.Dirent, uid auth.UID, gid auth.GID) error {
 	if err != nil {
 		return err
 	}
+
 	c := t.Credentials()
 	hasCap := d.Inode.CheckCapability(t, linux.CAP_CHOWN)
 	isOwner := uattr.Owner.UID == c.EffectiveKUID
+	var clearPrivilege bool
 	if uid.Ok() {
 		kuid := c.UserNamespace.MapToKUID(uid)
 		// Valid UID must be supplied if UID is to be changed.
@@ -1693,6 +1695,11 @@ func chown(t *kernel.Task, d *fs.Dirent, uid auth.UID, gid auth.GID) error {
 			return syserror.EPERM
 		}
 
+		// The setuid and setgid bits are cleared during a chown.
+		if uattr.Owner.UID != kuid {
+			clearPrivilege = true
+		}
+
 		owner.UID = kuid
 	}
 	if gid.Ok() {
@@ -1711,6 +1718,11 @@ func chown(t *kernel.Task, d *fs.Dirent, uid auth.UID, gid auth.GID) error {
 			return syserror.EPERM
 		}
 
+		// The setuid and setgid bits are cleared during a chown.
+		if uattr.Owner.GID != kgid {
+			clearPrivilege = true
+		}
+
 		owner.GID = kgid
 	}
 
@@ -1721,10 +1733,14 @@ func chown(t *kernel.Task, d *fs.Dirent, uid auth.UID, gid auth.GID) error {
 	if err := d.Inode.SetOwner(t, d, owner); err != nil {
 		return err
 	}
+	// Clear privilege bits if needed and they are set.
+	if clearPrivilege && uattr.Perms.HasSetUIDOrGID() && !fs.IsDir(d.Inode.StableAttr) {
+		uattr.Perms.DropSetUIDAndMaybeGID()
+		if !d.Inode.SetPermissions(t, d, uattr.Perms) {
+			return syserror.EPERM
+		}
+	}
 
-	// When the owner or group are changed by an unprivileged user,
-	// chown(2) also clears the set-user-ID and set-group-ID bits, but
-	// we do not support them.
 	return nil
 }
 

test/syscalls/linux/setgid.cc

@@ -115,8 +115,6 @@ class SetgidDirTest : public ::testing::Test {
   void SetUp() override {
     original_gid_ = getegid();
 
-    SKIP_IF(IsRunningWithVFS1());
-
     // If we can't find two usable groups, we're in an unsupporting environment.
     // Skip the test.
     PosixErrorOr<std::pair<gid_t, gid_t>> groups = Groups();
@@ -294,8 +292,8 @@ TEST_F(SetgidDirTest, ChownFileClears) {
   EXPECT_EQ(stats.st_mode & (S_ISUID | S_ISGID), 0);
 }
 
-// Chowning a file with setgid enabled, but not the group exec bit, does not
-// clear the setgid bit. Such files are mandatory locked.
+// Chowning a file with setgid enabled, but not the group exec bit, clears the
+// setuid bit and not the setgid bit. Such files are mandatory locked.
 TEST_F(SetgidDirTest, ChownNoExecFileDoesNotClear) {
   // Set group to G1, create a directory, and enable setgid.
   auto g1owned = JoinPath(temp_dir_.path(), "g1owned/");
@@ -345,7 +343,6 @@ struct FileModeTestcase {
 class FileModeTest : public ::testing::TestWithParam<FileModeTestcase> {};
 
 TEST_P(FileModeTest, WriteToFile) {
-  SKIP_IF(IsRunningWithVFS1());
   PosixErrorOr<std::pair<gid_t, gid_t>> groups = Groups();
   SKIP_IF(!groups.ok());
 
@@ -372,7 +369,6 @@ TEST_P(FileModeTest, WriteToFile) {
 }
 
 TEST_P(FileModeTest, TruncateFile) {
-  SKIP_IF(IsRunningWithVFS1());
   PosixErrorOr<std::pair<gid_t, gid_t>> groups = Groups();
   SKIP_IF(!groups.ok());