Fix semantics of Mount RPC in lisafs.

Earlier lisafs let the client choose where the connection will be mounted.
Because the client can be compromised, we can not trust the Mount RPC to
dictate the mount path. Instead, decide the mount path on startup on the server
as per the sandbox configuration.

lisafs made the following two assumptions where were incorrect:
- runsc/fsgofer always chroot()s the gofer process. This is currently always
  the case but in the future this might not be true.
- Non root mountpoints will not always correspond to the same directory inside
  the root mount. For instance, if application sets bind mount
  `-v host/dir:app/dir`, then it is not necessary that the app/dir endpoint is
  placed at path "app/dir" inside the root endpoint. This is currently the case
  for runsc/fsgofer, but it might not be in the future.

To support attach paths, make the client do a normal Walk RPC to the attach
point. The Mount RPC now mounts the connection to an endpoint that was
predetermined during startup according to sandbox configuration.

PiperOrigin-RevId: 424948561

pkg/lisafs/client.go

@@ -72,7 +72,7 @@ type Client struct {
 // the server and creates channels for fast IPC. NewClient takes ownership over
 // the passed socket. On success, it returns the initialized client along with
 // the root Inode.
-func NewClient(sock *unet.Socket, mountPath string) (*Client, *Inode, error) {
+func NewClient(sock *unet.Socket) (*Client, *Inode, error) {
 	maxChans := maxChannels()
 	c := &Client{
 		sockComm:          newSockComm(sock),
@@ -97,11 +97,8 @@ func NewClient(sock *unet.Socket, mountPath string) (*Client, *Inode, error) {
 	// Mount RPC below.
 	c.supported = make([]bool, Mount+1)
 	c.supported[Mount] = true
-	mountMsg := MountReq{
-		MountPath: SizedString(mountPath),
-	}
 	var mountResp MountResp
-	if err := c.SndRcvMessage(Mount, uint32(mountMsg.SizeBytes()), mountMsg.MarshalBytes, mountResp.CheckedUnmarshal, nil); err != nil {
+	if err := c.SndRcvMessage(Mount, 0, NoopMarshal, mountResp.CheckedUnmarshal, nil); err != nil {
 		return nil, nil, err
 	}
 

pkg/lisafs/connection.go

@@ -15,6 +15,8 @@
 package lisafs
 
 import (
+	"path"
+	"path/filepath"
 	"runtime/debug"
 
 	"golang.org/x/sys/unix"
@@ -44,13 +46,15 @@ type Connection struct {
 	// associated with it for its entire lifetime.
 	server *Server
 
+	// mountPath is the path to a file inside the server that is served to this
+	// connection as its root FD. IOW, this connection is mounted at this path.
+	// mountPath is trusted because it is configured by the server (trusted) as
+	// per the user's sandbox configuration. mountPath is immutable.
+	mountPath string
+
 	// maxMessageSize is the cached value of server.impl.MaxMessageSize().
 	maxMessageSize uint32
 
-	// mounted is a one way flag indicating whether this connection has been
-	// mounted correctly and the server is initialized properly.
-	mounted bool
-
 	// readonly indicates if this connection is readonly. All write operations
 	// will fail with EROFS.
 	readonly bool
@@ -79,13 +83,20 @@ type Connection struct {
 	nextFDID FDID
 }
 
-// CreateConnection initializes a new connection - creating a server if
-// required. The connection must be started separately.
-func (s *Server) CreateConnection(sock *unet.Socket, readonly bool) (*Connection, error) {
+// CreateConnection initializes a new connection which will be mounted at
+// mountPath. The connection must be started separately.
+func (s *Server) CreateConnection(sock *unet.Socket, mountPath string, readonly bool) (*Connection, error) {
+	mountPath = path.Clean(mountPath)
+	if !filepath.IsAbs(mountPath) {
+		log.Warningf("mountPath %q is not absolute", mountPath)
+		return nil, unix.EINVAL
+	}
+
 	c := &Connection{
 		sockComm:       newSockComm(sock),
 		server:         s,
 		maxMessageSize: s.impl.MaxMessageSize(),
+		mountPath:      mountPath,
 		readonly:       readonly,
 		channels:       make([]*channel, 0, maxChannels()),
 		fds:            make(map[FDID]genericFD),
@@ -183,11 +194,6 @@ func (c *Connection) handleMsg(comm Communicator, m MID, payloadLen uint32) (ret
 		}
 	}()
 
-	if !c.mounted && m != Mount {
-		log.Warningf("connection must first be mounted")
-		return c.respondError(comm, unix.EINVAL)
-	}
-
 	// Check if the message is supported for forward compatibility.
 	if int(m) >= len(c.server.handlers) || c.server.handlers[m] == nil {
 		log.Warningf("received request which is not supported by the server, MID = %d", m)

pkg/lisafs/connection_test.go

@@ -57,9 +57,10 @@ func (fd *testControlFD) FD() *lisafs.ControlFD {
 func (fd *testControlFD) Close() {}
 
 // Mount implements lisafs.Mount.
-func (s *testServer) Mount(c *lisafs.Connection) (*lisafs.ControlFD, linux.Statx, error) {
+func (s *testServer) Mount(c *lisafs.Connection, mountNode *lisafs.Node) (*lisafs.ControlFD, linux.Statx, error) {
 	dummyRoot := &testControlFD{}
-	dummyRoot.Init(c, s.Root(), linux.ModeDirectory, dummyRoot)
+	mountNode.IncRef() // Ref is transferred to ControlFD.
+	dummyRoot.Init(c, mountNode, linux.ModeDirectory, dummyRoot)
 	return dummyRoot.FD(), linux.Statx{Mode: linux.S_IFDIR}, nil
 }
 
@@ -85,16 +86,16 @@ func runServerClient(t testing.TB, clientFn func(c *lisafs.Client)) {
 	}
 
 	ts := &testServer{}
-	ts.Server.Init(ts, lisafs.ServerOpts{})
-	ts.Server.SetHandlers(handlers[:])
-	conn, err := ts.CreateConnection(serverSocket, false /* readonly */)
+	ts.Init(ts, lisafs.ServerOpts{})
+	ts.SetHandlers(handlers[:])
+	conn, err := ts.CreateConnection(serverSocket, "/" /* mountPath */, false /* readonly */)
 	if err != nil {
 		t.Fatalf("starting connection failed: %v", err)
 		return
 	}
 	ts.StartConnection(conn)
 
-	c, _, err := lisafs.NewClient(clientSocket, "/")
+	c, _, err := lisafs.NewClient(clientSocket)
 	if err != nil {
 		t.Fatalf("client creation failed: %v", err)
 	}

pkg/lisafs/handlers.go

@@ -17,8 +17,6 @@ package lisafs
 import (
 	"fmt"
 	"math"
-	"path"
-	"path/filepath"
 	"strings"
 
 	"golang.org/x/sys/unix"
@@ -90,53 +88,60 @@ func ErrorHandler(c *Connection, comm Communicator, payloadLen uint32) (uint32,
 // that Mount is the first message on the connection. Only after the connection
 // has been successfully mounted can other channels be created.
 func MountHandler(c *Connection, comm Communicator, payloadLen uint32) (uint32, error) {
-	var req MountReq
-	if _, ok := req.CheckedUnmarshal(comm.PayloadBuf(payloadLen)); !ok {
-		return 0, unix.EIO
-	}
-
-	mountPath := path.Clean(string(req.MountPath))
-	if !filepath.IsAbs(mountPath) {
-		log.Warningf("mountPath %q is not absolute", mountPath)
-		return 0, unix.EINVAL
-	}
-
-	if c.mounted {
-		log.Warningf("connection has already been mounted at %q", mountPath)
-		return 0, unix.EBUSY
-	}
-
 	var (
 		mountPointFD   *ControlFD
 		mountPointStat linux.Statx
+		mountNode      = c.server.root
 	)
 	if err := c.server.withRenameReadLock(func() (err error) {
-		mountPointFD, mountPointStat, err = c.ServerImpl().Mount(c)
-		if err != nil {
-			return err
-		}
+		// Maintain extra ref on mountNode to ensure existence during walk.
+		mountNode.IncRef()
+		defer func() {
+			// Drop extra ref on mountNode. Wrap the defer call with a func so that
+			// mountNode is evaluated on execution, not on defer itself.
+			mountNode.DecRef(nil)
+		}()
 
-		for pit := fspath.Parse(mountPath).Begin; pit.Ok(); pit = pit.Next() {
-			mountPointFD.node.opMu.RLock()
-			nextFD, nextStat, err := mountPointFD.impl.Walk(pit.String())
-			mountPointFD.node.opMu.RUnlock()
-			if err != nil {
+		// Walk to the mountpoint.
+		pit := fspath.Parse(c.mountPath).Begin
+		for pit.Ok() {
+			curName := pit.String()
+			if err := checkSafeName(curName); err != nil {
 				return err
 			}
-
-			c.removeControlFDLocked(mountPointFD.id)
-			mountPointFD = nextFD
-			mountPointStat = nextStat
+			mountNode.opMu.RLock()
+			if mountNode.isDeleted() {
+				mountNode.opMu.RUnlock()
+				return unix.ENOENT
+			}
+			mountNode.childrenMu.Lock()
+			next := mountNode.LookupChildLocked(curName)
+			if next == nil {
+				next = &Node{}
+				next.InitLocked(curName, mountNode)
+			} else {
+				next.IncRef()
+			}
+			mountNode.childrenMu.Unlock()
+			mountNode.opMu.RUnlock()
+			// next has an extra ref as needed. Drop extra ref on mountNode.
+			mountNode.DecRef(nil)
+			pit = pit.Next()
+			mountNode = next
 		}
+
+		// Provide Mount with read concurrency guarantee.
+		mountNode.opMu.RLock()
+		defer mountNode.opMu.RUnlock()
+		if mountNode.isDeleted() {
+			return unix.ENOENT
+		}
+		mountPointFD, mountPointStat, err = c.ServerImpl().Mount(c, mountNode)
 		return err
 	}); err != nil {
-		if mountPointFD != nil {
-			c.removeFD(mountPointFD.id)
-		}
 		return 0, err
 	}
 
-	c.mounted = true
 	resp := MountResp{
 		Root: Inode{
 			ControlFD: mountPointFD.id,

pkg/lisafs/message.go

@@ -268,26 +268,6 @@ type Inode struct {
 	Stat      linux.Statx
 }
 
-// MountReq represents a Mount request.
-type MountReq struct {
-	MountPath SizedString
-}
-
-// SizeBytes implements marshal.Marshallable.SizeBytes.
-func (m *MountReq) SizeBytes() int {
-	return m.MountPath.SizeBytes()
-}
-
-// MarshalBytes implements marshal.Marshallable.MarshalBytes.
-func (m *MountReq) MarshalBytes(dst []byte) []byte {
-	return m.MountPath.MarshalBytes(dst)
-}
-
-// CheckedUnmarshal implements marshal.CheckedMarshallable.CheckedUnmarshal.
-func (m *MountReq) CheckedUnmarshal(src []byte) ([]byte, bool) {
-	return m.MountPath.CheckedUnmarshal(src)
-}
-
 // MountResp represents a Mount response.
 type MountResp struct {
 	Root Inode

pkg/lisafs/server.go

@@ -103,20 +103,15 @@ func (s *Server) Wait() {
 	s.connWg.Wait()
 }
 
-// Root returns the server's root node.
-func (s *Server) Root() *Node {
-	return s.root
-}
-
 // ServerImpl contains the implementation details for a Server.
 // Implementations of ServerImpl should contain their associated Server by
 // value as their first field.
 type ServerImpl interface {
 	// Mount is called when a Mount RPC is made. It mounts the connection on
-	// filesystem root.
+	// mountNode.
 	//
-	// Mount has rename read concurrency guarantee.
-	Mount(c *Connection) (*ControlFD, linux.Statx, error)
+	// Mount has a read concurrency guarantee on mountNode.
+	Mount(c *Connection, mountNode *Node) (*ControlFD, linux.Statx, error)
 
 	// SupportedMessages returns a list of messages that the server
 	// implementation supports.

pkg/lisafs/testsuite/testsuite.go

@@ -91,14 +91,14 @@ func runServerClient(t *testing.T, tester Tester, testFn testFunc) {
 	}
 
 	server := tester.NewServer(t)
-	conn, err := server.CreateConnection(serverSocket, false /* readonly */)
+	conn, err := server.CreateConnection(serverSocket, mountPath, false /* readonly */)
 	if err != nil {
 		t.Fatalf("starting connection failed: %v", err)
 		return
 	}
 	server.StartConnection(conn)
 
-	c, root, err := lisafs.NewClient(clientSocket, mountPath)
+	c, root, err := lisafs.NewClient(clientSocket)
 	if err != nil {
 		t.Fatalf("client creation failed: %v", err)
 	}

pkg/sentry/fsimpl/gofer/gofer.go

@@ -39,6 +39,7 @@ package gofer
 
 import (
 	"fmt"
+	"path"
 	"strconv"
 	"strings"
 	"sync/atomic"
@@ -350,7 +351,11 @@ func (fstype FilesystemType) GetFilesystem(ctx context.Context, vfsObj *vfs.Virt
 	fsopts.aname = "/"
 	if aname, ok := mopts[moptAname]; ok {
 		delete(mopts, moptAname)
-		fsopts.aname = aname
+		if !path.IsAbs(aname) {
+			ctx.Warningf("gofer.FilesystemType.GetFilesystem: aname is not absolute: %s=%s", moptAname, aname)
+			return nil, nil, linuxerr.EINVAL
+		}
+		fsopts.aname = path.Clean(aname)
 	}
 
 	// Parse the cache policy. For historical reasons, this defaults to the
@@ -527,9 +532,39 @@ func (fs *filesystem) initClientLisa(ctx context.Context) (*lisafs.Inode, error)
 
 	var rootInode *lisafs.Inode
 	ctx.UninterruptibleSleepStart(false)
-	fs.clientLisa, rootInode, err = lisafs.NewClient(sock, fs.opts.aname)
+	fs.clientLisa, rootInode, err = lisafs.NewClient(sock)
 	ctx.UninterruptibleSleepFinish(false)
-	return rootInode, err
+	if err != nil {
+		return nil, err
+	}
+	if fs.opts.aname == "/" {
+		return rootInode, nil
+	}
+
+	// Walk to the attach point from root inode.
+	rootFD := fs.clientLisa.NewFD(rootInode.ControlFD)
+	status, inodes, err := rootFD.WalkMultiple(ctx, strings.Split(fs.opts.aname, "/"))
+	rootFD.CloseBatched(ctx)
+	if err != nil {
+		return nil, err
+	}
+
+	// Close all intermediate FDs to the attach point.
+	numInodes := len(inodes)
+	for _, inode := range inodes[:numInodes-1] {
+		curFD := fs.clientLisa.NewFD(inode.ControlFD)
+		curFD.CloseBatched(ctx)
+	}
+
+	switch status {
+	case lisafs.WalkSuccess:
+		return &inodes[numInodes-1], nil
+	default:
+		last := fs.clientLisa.NewFD(inodes[numInodes-1].ControlFD)
+		last.CloseBatched(ctx)
+		log.Warningf("initClientLisa failed because walk to attach point %q failed: lisafs.WalkStatus = %v", fs.opts.aname, status)
+		return nil, unix.ENOENT
+	}
 }
 
 func (fs *filesystem) initClient(ctx context.Context) (*dentry, error) {

runsc/boot/fs.go

@@ -189,7 +189,7 @@ func compileMounts(spec *specs.Spec, conf *config.Config, vfs2Enabled bool) []sp
 }
 
 // goferMountData creates a slice of gofer mount data.
-func goferMountData(fd int, fa config.FileAccessType, attachPath string, vfs2 bool, lisafs bool) []string {
+func goferMountData(fd int, fa config.FileAccessType, vfs2 bool, lisafs bool) []string {
 	opts := []string{
 		"trans=fd",
 		"rfdno=" + strconv.Itoa(fd),
@@ -205,7 +205,6 @@ func goferMountData(fd int, fa config.FileAccessType, attachPath string, vfs2 bo
 	}
 	if vfs2 && lisafs {
 		opts = append(opts, "lisafs=true")
-		opts = append(opts, "aname="+attachPath)
 	}
 	return opts
 }
@@ -784,7 +783,7 @@ func (c *containerMounter) createRootMount(ctx context.Context, conf *config.Con
 	fd := c.fds.remove()
 	log.Infof("Mounting root over 9P, ioFD: %d", fd)
 	p9FS := mustFindFilesystem("9p")
-	opts := goferMountData(fd, conf.FileAccess, "/", false /* vfs2 */, false /* lisafs */)
+	opts := goferMountData(fd, conf.FileAccess, false /* vfs2 */, false /* lisafs */)
 
 	// We can't check for overlayfs here because sandbox is chroot'ed and gofer
 	// can only send mount options for specs.Mounts (specs.Root is missing
@@ -845,7 +844,7 @@ func (c *containerMounter) getMountNameAndOptions(conf *config.Config, m *specs.
 	case bind:
 		fd := c.fds.remove()
 		fsName = gofervfs2.Name
-		opts = goferMountData(fd, c.getMountAccessType(conf, m), m.Destination, conf.VFS2, conf.Lisafs)
+		opts = goferMountData(fd, c.getMountAccessType(conf, m), conf.VFS2, conf.Lisafs)
 		// If configured, add overlay to all writable mounts.
 		useOverlay = conf.Overlay && !mountFlags(m.Options).ReadOnly
 	case cgroupfs.Name:
@@ -1003,7 +1002,7 @@ func (c *containerMounter) createRestoreEnvironment(conf *config.Config) (*fs.Re
 
 	// Add root mount.
 	fd := c.fds.remove()
-	opts := goferMountData(fd, conf.FileAccess, "/", conf.VFS2, false /* lisafs */)
+	opts := goferMountData(fd, conf.FileAccess, conf.VFS2, false /* lisafs */)
 
 	mf := fs.MountSourceFlags{}
 	if c.root.Readonly || conf.Overlay {

runsc/boot/vfs.go

@@ -213,7 +213,7 @@ func (c *containerMounter) mountAll(conf *config.Config, procArgs *kernel.Create
 // createMountNamespaceVFS2 creates the container's root mount and namespace.
 func (c *containerMounter) createMountNamespaceVFS2(ctx context.Context, conf *config.Config, creds *auth.Credentials) (*vfs.MountNamespace, error) {
 	fd := c.fds.remove()
-	data := goferMountData(fd, conf.FileAccess, "/", true /* vfs2 */, conf.Lisafs)
+	data := goferMountData(fd, conf.FileAccess, true /* vfs2 */, conf.Lisafs)
 
 	// We can't check for overlayfs here because sandbox is chroot'ed and gofer
 	// can only send mount options for specs.Mounts (specs.Root is missing
@@ -520,7 +520,7 @@ func (c *containerMounter) getMountNameAndOptionsVFS2(conf *config.Config, m *mo
 			// but unlikely to be correct in this context.
 			return "", nil, false, fmt.Errorf("9P mount requires a connection FD")
 		}
-		data = goferMountData(m.fd, c.getMountAccessType(conf, m.mount), m.mount.Destination, true /* vfs2 */, conf.Lisafs)
+		data = goferMountData(m.fd, c.getMountAccessType(conf, m.mount), true /* vfs2 */, conf.Lisafs)
 		internalData = gofer.InternalFilesystemOptions{
 			UniqueID: m.mount.Destination,
 		}

runsc/cmd/gofer.go

@@ -189,8 +189,9 @@ func newSocket(ioFD int) *unet.Socket {
 
 func (g *Gofer) serveLisafs(spec *specs.Spec, conf *config.Config, root string) subcommands.ExitStatus {
 	type connectionConfig struct {
-		sock     *unet.Socket
-		readonly bool
+		sock      *unet.Socket
+		mountPath string
+		readonly  bool
 	}
 	cfgs := make([]connectionConfig, 0, len(spec.Mounts)+1)
 	server := fsgofer.NewLisafsServer(fsgofer.Config{
@@ -202,8 +203,9 @@ func (g *Gofer) serveLisafs(spec *specs.Spec, conf *config.Config, root string)
 
 	// Start with root mount, then add any other additional mount as needed.
 	cfgs = append(cfgs, connectionConfig{
-		sock:     newSocket(g.ioFDs[0]),
-		readonly: spec.Root.Readonly || conf.Overlay,
+		sock:      newSocket(g.ioFDs[0]),
+		mountPath: "/", // fsgofer process is always chroot()ed. So serve root.
+		readonly:  spec.Root.Readonly || conf.Overlay,
 	})
 	log.Infof("Serving %q mapped to %q on FD %d (ro: %t)", "/", root, g.ioFDs[0], cfgs[0].readonly)
 
@@ -221,8 +223,9 @@ func (g *Gofer) serveLisafs(spec *specs.Spec, conf *config.Config, root string)
 		}
 
 		cfgs = append(cfgs, connectionConfig{
-			sock:     newSocket(g.ioFDs[mountIdx]),
-			readonly: isReadonlyMount(m.Options) || conf.Overlay,
+			sock:      newSocket(g.ioFDs[mountIdx]),
+			mountPath: m.Destination,
+			readonly:  isReadonlyMount(m.Options) || conf.Overlay,
 		})
 
 		log.Infof("Serving %q mapped on FD %d (ro: %t)", m.Destination, g.ioFDs[mountIdx], cfgs[mountIdx].readonly)
@@ -235,7 +238,7 @@ func (g *Gofer) serveLisafs(spec *specs.Spec, conf *config.Config, root string)
 	cfgs = cfgs[:mountIdx]
 
 	for _, cfg := range cfgs {
-		conn, err := server.CreateConnection(cfg.sock, cfg.readonly)
+		conn, err := server.CreateConnection(cfg.sock, cfg.mountPath, cfg.readonly)
 		if err != nil {
 			Fatalf("starting connection on FD %d for gofer mount failed: %v", cfg.sock.FD(), err)
 		}

runsc/fsgofer/lisafs.go

@@ -50,9 +50,10 @@ func NewLisafsServer(config Config) *LisafsServer {
 }
 
 // Mount implements lisafs.ServerImpl.Mount.
-func (s *LisafsServer) Mount(c *lisafs.Connection) (*lisafs.ControlFD, linux.Statx, error) {
+func (s *LisafsServer) Mount(c *lisafs.Connection, mountNode *lisafs.Node) (*lisafs.ControlFD, linux.Statx, error) {
+	mountPath := mountNode.FilePath()
 	rootHostFD, err := tryOpen(func(flags int) (int, error) {
-		return unix.Open("/", flags, 0)
+		return unix.Open(mountPath, flags, 0)
 	})
 	if err != nil {
 		return nil, linux.Statx{}, err
@@ -67,7 +68,8 @@ func (s *LisafsServer) Mount(c *lisafs.Connection) (*lisafs.ControlFD, linux.Sta
 		hostFD:         rootHostFD,
 		writableHostFD: -1,
 	}
-	rootFD.ControlFD.Init(c, s.Root(), linux.FileMode(stat.Mode), rootFD)
+	mountNode.IncRef() // Ref is transferred to ControlFD.
+	rootFD.ControlFD.Init(c, mountNode, linux.FileMode(stat.Mode), rootFD)
 	return rootFD.FD(), stat, nil
 }