Lock and check pathNodes during walk in MultiGetAttr.

Earlier we were only locking the start directory's opMu while performing the
entire walk in MultiGetAttr. This means that a compromised client could
potentially delete and replace certain path components inside the start
directory while the walk is going on. Depending on the Server's File
implementation, this could lead to symlink based attacks.

Furthermore, calling WalkGetAttr requires the server to provide read
concurrency guarantee as documented while the DefaultMultiGetAttr
implementation was not providing.

PiperOrigin-RevId: 423218838

pkg/p9/client_file.go

@@ -15,6 +15,7 @@
 package p9
 
 import (
+	"errors"
 	"fmt"
 	"io"
 	"sync/atomic"
@@ -126,15 +127,58 @@ func (c *clientFile) MultiGetAttr(names []string) ([]FullStat, error) {
 		return nil, unix.EBADF
 	}
 
-	if !versionSupportsTmultiGetAttr(c.client.version) {
-		return DefaultMultiGetAttr(c, names)
+	if versionSupportsTmultiGetAttr(c.client.version) {
+		rmultigetattr := Rmultigetattr{}
+		if err := c.client.sendRecv(&Tmultigetattr{FID: c.fid, Names: names}, &rmultigetattr); err != nil {
+			return nil, err
+		}
+		return rmultigetattr.Stats, nil
 	}
 
-	rmultigetattr := Rmultigetattr{}
-	if err := c.client.sendRecv(&Tmultigetattr{FID: c.fid, Names: names}, &rmultigetattr); err != nil {
-		return nil, err
+	stats := make([]FullStat, 0, len(names))
+	var start File = c
+	parent := start
+	closeParent := func() {
+		if parent != start {
+			_ = parent.Close()
+		}
 	}
-	return rmultigetattr.Stats, nil
+	defer closeParent()
+	mask := AttrMaskAll()
+	for i, name := range names {
+		if len(name) == 0 && i == 0 {
+			qid, valid, attr, err := parent.GetAttr(mask)
+			if err != nil {
+				return nil, err
+			}
+			stats = append(stats, FullStat{
+				QID:   qid,
+				Valid: valid,
+				Attr:  attr,
+			})
+			continue
+		}
+		qids, child, valid, attr, err := parent.WalkGetAttr([]string{name})
+		if err != nil {
+			if errors.Is(err, unix.ENOENT) {
+				return stats, nil
+			}
+			return nil, err
+		}
+		closeParent()
+		parent = child
+		stats = append(stats, FullStat{
+			QID:   qids[0],
+			Valid: valid,
+			Attr:  attr,
+		})
+		if attr.Mode.FileType() != ModeDirectory {
+			// Doesn't need to continue if entry is not a dir. Including symlinks
+			// that cannot be followed.
+			break
+		}
+	}
+	return stats, nil
 }
 
 // StatFS implements File.StatFS.

pkg/p9/file.go

@@ -15,8 +15,6 @@
 package p9
 
 import (
-	"errors"
-
 	"golang.org/x/sys/unix"
 	"gvisor.dev/gvisor/pkg/fd"
 )
@@ -30,6 +28,10 @@ type AttacherOptions struct {
 	// AllocateOnDeleted is set to true if it's safe to call File.Allocate for
 	// deleted files.
 	AllocateOnDeleted bool
+
+	// MultiGetAttrSupported is set to true if it's safe to call
+	// File.MultiGetAttr with read concurrency guarantee only on start directory.
+	MultiGetAttrSupported bool
 }
 
 // NoServerOptions partially implements Attacher with empty AttacherOptions.
@@ -360,50 +362,3 @@ func (*DisallowServerCalls) Renamed(File, string) {
 func (*DisallowServerCalls) ServerOptions() AttacherOptions {
 	panic("ServerOptions should not be called on the client")
 }
-
-// DefaultMultiGetAttr implements File.MultiGetAttr() on top of File.
-func DefaultMultiGetAttr(start File, names []string) ([]FullStat, error) {
-	stats := make([]FullStat, 0, len(names))
-	parent := start
-	closeParent := func() {
-		if parent != start {
-			_ = parent.Close()
-		}
-	}
-	defer closeParent()
-	mask := AttrMaskAll()
-	for i, name := range names {
-		if len(name) == 0 && i == 0 {
-			qid, valid, attr, err := parent.GetAttr(mask)
-			if err != nil {
-				return nil, err
-			}
-			stats = append(stats, FullStat{
-				QID:   qid,
-				Valid: valid,
-				Attr:  attr,
-			})
-			continue
-		}
-		qids, child, valid, attr, err := parent.WalkGetAttr([]string{name})
-		if err != nil {
-			if errors.Is(err, unix.ENOENT) {
-				return stats, nil
-			}
-			return nil, err
-		}
-		closeParent()
-		parent = child
-		stats = append(stats, FullStat{
-			QID:   qids[0],
-			Valid: valid,
-			Attr:  attr,
-		})
-		if attr.Mode.FileType() != ModeDirectory {
-			// Doesn't need to continue if entry is not a dir. Including symlinks
-			// that cannot be followed.
-			break
-		}
-	}
-	return stats, nil
-}

pkg/p9/handlers.go

@@ -15,6 +15,7 @@
 package p9
 
 import (
+	errors2 "errors"
 	"fmt"
 	"io"
 	"os"
@@ -1537,12 +1538,79 @@ func (t *Tmultigetattr) handle(cs *connState) message {
 	}
 	defer ref.DecRef()
 
-	var stats []FullStat
-	if err := ref.safelyRead(func() (err error) {
-		stats, err = ref.file.MultiGetAttr(t.Names)
-		return err
-	}); err != nil {
-		return newErr(err)
+	if cs.server.options.MultiGetAttrSupported {
+		var stats []FullStat
+		if err := ref.safelyRead(func() (err error) {
+			stats, err = ref.file.MultiGetAttr(t.Names)
+			return err
+		}); err != nil {
+			return newErr(err)
+		}
+		return &Rmultigetattr{Stats: stats}
 	}
+
+	stats := make([]FullStat, 0, len(t.Names))
+	mask := AttrMaskAll()
+	start := ref.file
+	startNode := ref.pathNode
+	parent := start
+	parentNode := startNode
+	closeParent := func() {
+		if parent != start {
+			_ = parent.Close()
+		}
+	}
+	defer closeParent()
+
+	cs.server.renameMu.RLock()
+	defer cs.server.renameMu.RUnlock()
+
+	for i, name := range t.Names {
+		if len(name) == 0 && i == 0 {
+			startNode.opMu.RLock()
+			qid, valid, attr, err := start.GetAttr(mask)
+			startNode.opMu.RUnlock()
+			if err != nil {
+				return newErr(err)
+			}
+			stats = append(stats, FullStat{
+				QID:   qid,
+				Valid: valid,
+				Attr:  attr,
+			})
+			continue
+		}
+
+		parentNode.opMu.RLock()
+		if atomic.LoadUint32(&parentNode.deleted) != 0 {
+			parentNode.opMu.RUnlock()
+			break
+		}
+		qids, child, valid, attr, err := parent.WalkGetAttr([]string{name})
+		if err != nil {
+			parentNode.opMu.RUnlock()
+			if errors2.Is(err, unix.ENOENT) {
+				break
+			}
+			return newErr(err)
+		}
+		stats = append(stats, FullStat{
+			QID:   qids[0],
+			Valid: valid,
+			Attr:  attr,
+		})
+		// Update with next generation.
+		closeParent()
+		parent = child
+		childNode := parentNode.pathNodeFor(name)
+		parentNode.opMu.RUnlock()
+		parentNode = childNode
+		if attr.Mode.FileType() != ModeDirectory {
+			// Doesn't need to continue if entry is not a dir. Including symlinks
+			// that cannot be followed.
+			break
+		}
+	}
+
 	return &Rmultigetattr{Stats: stats}
 }

runsc/fsgofer/fsgofer.go

@@ -150,8 +150,9 @@ func (a *attachPoint) Attach() (p9.File, error) {
 // a file that was created in the same path as the delete file.
 func (a *attachPoint) ServerOptions() p9.AttacherOptions {
 	return p9.AttacherOptions{
-		SetAttrOnDeleted:  true,
-		AllocateOnDeleted: true,
+		SetAttrOnDeleted:      true,
+		AllocateOnDeleted:     true,
+		MultiGetAttrSupported: true,
 	}
 }
 
@@ -869,7 +870,7 @@ func (*localFile) Rename(p9.File, string) error {
 	panic("rename called directly")
 }
 
-// RenameAt implements p9.File.RenameAt.
+// RenameAt implements p9.File.
 func (l *localFile) RenameAt(oldName string, directory p9.File, newName string) error {
 	if err := l.checkROMount(); err != nil {
 		return err
@@ -1133,7 +1134,7 @@ func (l *localFile) Flush() error {
 	return nil
 }
 
-// Bind implements p9.File.Bind.
+// Bind implements p9.File.
 func (l *localFile) Bind(sockType uint32, sockName string, uid p9.UID, gid p9.GID) (p9.File, p9.QID, p9.AttrMask, p9.Attr, error) {
 	if !l.attachPoint.conf.HostUDS {
 		// Bind on host UDS is not allowed. As per mknod(2), which is invoked as
@@ -1252,7 +1253,7 @@ func (l *localFile) Connect(flags p9.ConnectFlags) (*fd.FD, error) {
 	return fd.New(f), nil
 }
 
-// Close implements p9.File.Close.
+// Close implements p9.File.
 func (l *localFile) Close() error {
 	l.mode = invalidMode
 	err := l.file.Close()
@@ -1313,6 +1314,7 @@ func (l *localFile) checkROMount() error {
 	return nil
 }
 
+// MultiGetAttr implements p9.File.
 func (l *localFile) MultiGetAttr(names []string) ([]p9.FullStat, error) {
 	stats := make([]p9.FullStat, 0, len(names))
 
@@ -1329,6 +1331,12 @@ func (l *localFile) MultiGetAttr(names []string) ([]p9.FullStat, error) {
 		names = names[1:]
 	}
 
+	// Note that while performing the walk below, we do not have read
+	// concurrency guarantee for any descendants. So files can be created/deleted
+	// while the walk is being performed. However, this should be fine from a
+	// security perspective as we are using host FDs to walk and checking that
+	// each opened path component is a directory. We also set O_NOFOLLOW to
+	// ensure no symlinks are accidentally followed during walk.
 	parent := l.file.FD()
 	closeParent := func() {
 		if parent != l.file.FD() {
@@ -1377,7 +1385,7 @@ type socketLocalFile struct {
 	sock int
 }
 
-// Close implements p9.File.Close.
+// Close implements p9.File.
 func (l *socketLocalFile) Close() error {
 	err := l.localFile.Close()
 	err2 := unix.Close(l.sock)