From 3f42b2da94eb30bbbb55fc40ddbfc7ce7ba6f1b8 Mon Sep 17 00:00:00 2001 From: Ayush Ranjan Date: Fri, 28 Jan 2022 13:45:15 -0800 Subject: [PATCH] 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 | 7 +-- pkg/lisafs/connection.go | 30 ++++++++----- pkg/lisafs/connection_test.go | 13 +++--- pkg/lisafs/handlers.go | 75 ++++++++++++++++--------------- pkg/lisafs/message.go | 20 --------- pkg/lisafs/server.go | 11 ++--- pkg/lisafs/testsuite/testsuite.go | 4 +- pkg/sentry/fsimpl/gofer/gofer.go | 41 +++++++++++++++-- runsc/boot/fs.go | 9 ++-- runsc/boot/vfs.go | 4 +- runsc/cmd/gofer.go | 17 ++++--- runsc/fsgofer/lisafs.go | 8 ++-- 12 files changed, 131 insertions(+), 108 deletions(-) diff --git a/pkg/lisafs/client.go b/pkg/lisafs/client.go index 933e7a0bb..4e4f12513 100644 --- a/pkg/lisafs/client.go +++ b/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 } diff --git a/pkg/lisafs/connection.go b/pkg/lisafs/connection.go index f53b4ccf1..38d769e7d 100644 --- a/pkg/lisafs/connection.go +++ b/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) diff --git a/pkg/lisafs/connection_test.go b/pkg/lisafs/connection_test.go index 2d3b40692..1d1e430de 100644 --- a/pkg/lisafs/connection_test.go +++ b/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) } diff --git a/pkg/lisafs/handlers.go b/pkg/lisafs/handlers.go index 8eb8b2437..02a1701d3 100644 --- a/pkg/lisafs/handlers.go +++ b/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, diff --git a/pkg/lisafs/message.go b/pkg/lisafs/message.go index 9da87e771..d0370647c 100644 --- a/pkg/lisafs/message.go +++ b/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 diff --git a/pkg/lisafs/server.go b/pkg/lisafs/server.go index de02aec06..681532960 100644 --- a/pkg/lisafs/server.go +++ b/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. diff --git a/pkg/lisafs/testsuite/testsuite.go b/pkg/lisafs/testsuite/testsuite.go index 83883b184..936d70f77 100644 --- a/pkg/lisafs/testsuite/testsuite.go +++ b/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) } diff --git a/pkg/sentry/fsimpl/gofer/gofer.go b/pkg/sentry/fsimpl/gofer/gofer.go index 0399e2233..3fd65ee7c 100644 --- a/pkg/sentry/fsimpl/gofer/gofer.go +++ b/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) { diff --git a/runsc/boot/fs.go b/runsc/boot/fs.go index f02818743..3ef746b54 100644 --- a/runsc/boot/fs.go +++ b/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 { diff --git a/runsc/boot/vfs.go b/runsc/boot/vfs.go index a2b18abb9..aaf7e5672 100644 --- a/runsc/boot/vfs.go +++ b/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, } diff --git a/runsc/cmd/gofer.go b/runsc/cmd/gofer.go index c65e0267a..7c22439c7 100644 --- a/runsc/cmd/gofer.go +++ b/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) } diff --git a/runsc/fsgofer/lisafs.go b/runsc/fsgofer/lisafs.go index e7b1ff508..b2408787b 100644 --- a/runsc/fsgofer/lisafs.go +++ b/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 }