Check MS_RDONLY for O_TRUNC before doing file truncation.

PiperOrigin-RevId: 777692904

Author: avagin

pkg/sentry/fsimpl/gofer/filesystem.go

@@ -1079,8 +1079,13 @@ func (d *dentry) open(ctx context.Context, rp *vfs.ResolvingPath, opts *vfs.Open
 		defer d.fs.renameMu.RUnlock()
 	}
 
+	mnt := rp.Mount()
 	trunc := opts.Flags&linux.O_TRUNC != 0 && d.fileType() == linux.S_IFREG
 	if trunc {
+		if err := mnt.CheckBeginWrite(); err != nil {
+			return nil, err
+		}
+		defer mnt.EndWrite()
 		// Lock metadataMu *while* we open a regular file with O_TRUNC because
 		// open(2) will change the file size on server.
 		d.metadataMu.Lock()
@@ -1089,7 +1094,6 @@ func (d *dentry) open(ctx context.Context, rp *vfs.ResolvingPath, opts *vfs.Open
 
 	var vfd *vfs.FileDescription
 	var err error
-	mnt := rp.Mount()
 	switch d.fileType() {
 	case linux.S_IFREG:
 		if !d.fs.opts.regularFilesUseSpecialFileFD {

pkg/sentry/fsimpl/kernfs/filesystem.go

@@ -512,6 +512,8 @@ func (fs *Filesystem) MknodAt(ctx context.Context, rp *vfs.ResolvingPath, opts v
 // OpenAt implements vfs.FilesystemImpl.OpenAt.
 func (fs *Filesystem) OpenAt(ctx context.Context, rp *vfs.ResolvingPath, opts vfs.OpenOptions) (*vfs.FileDescription, error) {
 	ats := vfs.AccessTypesForOpenFlags(&opts)
+	trunc := opts.Flags&linux.O_TRUNC != 0
+	mnt := rp.Mount()
 
 	// Do not create new file.
 	if opts.Flags&linux.O_CREAT == 0 {
@@ -526,6 +528,12 @@ func (fs *Filesystem) OpenAt(ctx context.Context, rp *vfs.ResolvingPath, opts vf
 			fs.mu.RUnlock()
 			return nil, err
 		}
+		if trunc && d.isRegular() {
+			if err := mnt.CheckBeginWrite(); err != nil {
+				return nil, err
+			}
+			defer mnt.EndWrite()
+		}
 		// Open may block so we need to unlock fs.mu. IncRef d to prevent
 		// its destruction while fs.mu is unlocked.
 		d.IncRef()
@@ -561,6 +569,12 @@ func (fs *Filesystem) OpenAt(ctx context.Context, rp *vfs.ResolvingPath, opts vf
 		if err := start.inode.CheckPermissions(ctx, rp.Credentials(), ats); err != nil {
 			return nil, err
 		}
+		if trunc && start.isRegular() {
+			if err := mnt.CheckBeginWrite(); err != nil {
+				return nil, err
+			}
+			defer mnt.EndWrite()
+		}
 		// Open may block so we need to unlock fs.mu. IncRef d to prevent
 		// its destruction while fs.mu is unlocked.
 		start.IncRef()
@@ -612,10 +626,10 @@ afterTrailingSymlink:
 		if err := parent.inode.CheckPermissions(ctx, rp.Credentials(), vfs.MayWrite); err != nil {
 			return nil, err
 		}
-		if err := rp.Mount().CheckBeginWrite(); err != nil {
+		if err := mnt.CheckBeginWrite(); err != nil {
 			return nil, err
 		}
-		defer rp.Mount().EndWrite()
+		defer mnt.EndWrite()
 		// Create and open the child.
 		childI, err := parent.inode.NewFile(ctx, pc, opts)
 		if err != nil {
@@ -646,6 +660,12 @@ afterTrailingSymlink:
 	if err := child.inode.CheckPermissions(ctx, rp.Credentials(), ats); err != nil {
 		return nil, err
 	}
+	if trunc && child.isRegular() {
+		if err := mnt.CheckBeginWrite(); err != nil {
+			return nil, err
+		}
+		defer mnt.EndWrite()
+	}
 	if child.isDir() {
 		// Can't open directories with O_CREAT.
 		if opts.Flags&linux.O_CREAT != 0 {

pkg/sentry/fsimpl/kernfs/kernfs.go

@@ -202,6 +202,9 @@ const (
 
 	// Dentry points to a symlink inode.
 	dflagsIsSymlink
+
+	// Dentry points to a regular file inode.
+	dflagsIsRegular
 )
 
 // Dentry implements vfs.DentryImpl.
@@ -519,12 +522,13 @@ func (d *Dentry) Init(fs *Filesystem, inode Inode) {
 	d.fs = fs
 	d.inode = inode
 	d.refs.Store(1)
-	ftype := inode.Mode().FileType()
-	if ftype == linux.ModeDirectory {
+	switch inode.Mode().FileType() {
+	case linux.ModeDirectory:
 		d.flags = atomicbitops.FromUint32(d.flags.RacyLoad() | dflagsIsDir)
-	}
-	if ftype == linux.ModeSymlink {
+	case linux.ModeSymlink:
 		d.flags = atomicbitops.FromUint32(d.flags.RacyLoad() | dflagsIsSymlink)
+	case linux.ModeRegular:
+		d.flags = atomicbitops.FromUint32(d.flags.RacyLoad() | dflagsIsRegular)
 	}
 	refs.Register(d)
 	inode.RegisterDentry(d)
@@ -553,6 +557,11 @@ func (d *Dentry) isSymlink() bool {
 	return d.flags.Load()&dflagsIsSymlink != 0
 }
 
+// isRegular checks whether the dentry points to a regular file inode.
+func (d *Dentry) isRegular() bool {
+	return d.flags.Load()&dflagsIsRegular != 0
+}
+
 // InotifyWithParent implements vfs.DentryImpl.InotifyWithParent.
 func (d *Dentry) InotifyWithParent(ctx context.Context, events, cookie uint32, et vfs.EventType) {
 	if d.isDir() {

pkg/sentry/fsimpl/tmpfs/filesystem.go

@@ -459,11 +459,19 @@ func (d *dentry) open(ctx context.Context, rp *vfs.ResolvingPath, opts *vfs.Open
 	case *regularFile:
 		var fd regularFileFD
 		fd.LockFD.Init(&d.inode.locks)
-		if err := fd.vfsfd.Init(&fd, opts.Flags, rp.Mount(), &d.vfsd, &vfs.FileDescriptionOptions{AllowDirectIO: true}); err != nil {
+		mnt := rp.Mount()
+		if err := fd.vfsfd.Init(&fd, opts.Flags, mnt, &d.vfsd, &vfs.FileDescriptionOptions{AllowDirectIO: true}); err != nil {
 			return nil, err
 		}
 		if !afterCreate && opts.Flags&linux.O_TRUNC != 0 {
-			if _, err := impl.truncate(0); err != nil {
+			if err := mnt.CheckBeginWrite(); err != nil {
+				fd.vfsfd.DecRef(ctx)
+				return nil, err
+			}
+			_, err := impl.truncate(0)
+			mnt.EndWrite()
+			if err != nil {
+				fd.vfsfd.DecRef(ctx)
 				return nil, err
 			}
 		}

test/syscalls/linux/mount.cc

@@ -18,6 +18,7 @@
 #include <linux/magic.h>
 #include <sched.h>
 #include <stdio.h>
+#include <string.h>
 #include <sys/eventfd.h>
 #include <sys/mman.h>
 #include <sys/mount.h>
@@ -446,6 +447,37 @@ TEST(MountTest, MountReadonly) {
               SyscallFailsWithErrno(EROFS));
 }
 
+TEST(MountTest, BindMountReadonly) {
+  SKIP_IF(!ASSERT_NO_ERRNO_AND_VALUE(HaveCapability(CAP_SYS_ADMIN)));
+
+  auto const dir = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir());
+  auto const bindDir = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir());
+  auto const mnt = ASSERT_NO_ERRNO_AND_VALUE(
+      Mount(dir.path(), bindDir.path(), "", MS_BIND, "", 0));
+
+  std::string const filename = JoinPath(bindDir.path(), "foo");
+  FileDescriptor fd =
+      ASSERT_NO_ERRNO_AND_VALUE(Open(filename, O_RDWR | O_CREAT, 0644));
+  char msg[] = "hello world";
+  EXPECT_THAT(pwrite64(fd.get(), msg, strlen(msg), 0),
+              SyscallSucceedsWithValue(strlen(msg)));
+  fd.reset();
+  EXPECT_THAT(mount(dir.path().c_str(), bindDir.path().c_str(), NULL,
+                    MS_BIND | MS_RDONLY | MS_REMOUNT, NULL),
+              SyscallSucceeds());
+
+  EXPECT_THAT(access(bindDir.path().c_str(), W_OK),
+              SyscallFailsWithErrno(EROFS));
+
+  EXPECT_THAT(open(filename.c_str(), O_RDWR), SyscallFailsWithErrno(EROFS));
+  EXPECT_THAT(open(filename.c_str(), O_RDWR | O_TRUNC),
+              SyscallFailsWithErrno(EROFS));
+  EXPECT_THAT(open(filename.c_str(), O_RDONLY | O_TRUNC),
+              SyscallFailsWithErrno(EROFS));
+  const struct stat s = ASSERT_NO_ERRNO_AND_VALUE(Stat(filename));
+  EXPECT_EQ(s.st_size, strlen(msg));
+}
+
 PosixErrorOr<absl::Time> ATime(absl::string_view file) {
   struct stat s = {};
   if (stat(std::string(file).c_str(), &s) == -1) {

test/syscalls/linux/open.cc

@@ -436,6 +436,7 @@ TEST_F(OpenTest, CanTruncateReadOnly) {
   struct stat stat;
   EXPECT_THAT(fstat(fd1.get(), &stat), SyscallSucceeds());
   EXPECT_EQ(stat.st_size, 0);
+  EXPECT_THAT(write(fd1.get(), "x", 1), SyscallFailsWithErrno(EBADF));
 }
 
 // If we don't have read permission on the file, opening with