Add additional sanity checks for walk.

PiperOrigin-RevId: 212058684
Change-Id: I319709b9ffcfccb3231bac98df345d2a20eca24b

6 files changed, 239 insertions, 80 deletions

diff --git a/pkg/p9/file.go b/pkg/p9/file.go
index ae726f0b9..9723fa24d 100644
--- a/pkg/p9/file.go
+++ b/pkg/p9/file.go
@@ -20,10 +20,13 @@ import (
 	"gvisor.googlesource.com/gvisor/pkg/fd"
 )
 
-// Attacher is provided by the user.
+// Attacher is provided by the server.
 type Attacher interface {
 	// Attach returns a new File.
-	Attach(attachName string) (File, error)
+	//
+	// The client-side attach will be translate to a series of walks from
+	// the file returned by this Attach call.
+	Attach() (File, error)
 }
 
 // File is a set of operations corresponding to a single node.
diff --git a/pkg/p9/handlers.go b/pkg/p9/handlers.go
index 7da9eff5f..ea41f97c7 100644
--- a/pkg/p9/handlers.go
+++ b/pkg/p9/handlers.go
@@ -17,6 +17,7 @@ package p9
 import (
 	"io"
 	"os"
+	"path"
 	"strings"
 	"sync/atomic"
 	"syscall"
@@ -94,39 +95,6 @@ func isSafeName(name string) bool {
 }
 
 // handle implements handler.handle.
-func (t *Twalk) handle(cs *connState) message {
-	// Check the names.
-	for _, name := range t.Names {
-		if !isSafeName(name) {
-			return newErr(syscall.EINVAL)
-		}
-	}
-
-	// Lookup the FID.
-	ref, ok := cs.LookupFID(t.FID)
-	if !ok {
-		return newErr(syscall.EBADF)
-	}
-	defer ref.DecRef()
-
-	// Has it been opened already?
-	if _, opened := ref.OpenFlags(); opened {
-		return newErr(syscall.EBUSY)
-	}
-
-	// Do the walk.
-	qids, sf, err := ref.file.Walk(t.Names)
-	if err != nil {
-		return newErr(err)
-	}
-
-	// Install the new FID.
-	cs.InsertFID(t.NewFID, &fidRef{file: sf})
-
-	return &Rwalk{QIDs: qids}
-}
-
-// handle implements handler.handle.
 func (t *Tclunk) handle(cs *connState) message {
 	if !cs.DeleteFID(t.FID) {
 		return newErr(syscall.EBADF)
@@ -175,14 +143,57 @@ func (t *Tattach) handle(cs *connState) message {
 		return newErr(syscall.EINVAL)
 	}
 
-	// Do the attach.
-	sf, err := cs.server.attacher.Attach(t.Auth.AttachName)
+	// Must provide an absolute path.
+	if path.IsAbs(t.Auth.AttachName) {
+		// Trim off the leading / if the path is absolute. We always
+		// treat attach paths as absolute and call attach with the root
+		// argument on the server file for clarity.
+		t.Auth.AttachName = t.Auth.AttachName[1:]
+	}
+
+	// Do the attach on the root.
+	sf, err := cs.server.attacher.Attach()
 	if err != nil {
 		return newErr(err)
 	}
-	cs.InsertFID(t.FID, &fidRef{file: sf})
+	_, valid, attr, err := sf.GetAttr(AttrMaskAll())
+	if err != nil {
+		sf.Close() // Drop file.
+		return newErr(err)
+	}
+	if !valid.Mode {
+		sf.Close() // Drop file.
+		return newErr(syscall.EINVAL)
+	}
+
+	// Build a transient reference.
+	root := &fidRef{
+		file:     sf,
+		refs:     1,
+		walkable: attr.Mode.IsDir(),
+	}
+	defer root.DecRef()
+
+	// Attach the root?
+	if len(t.Auth.AttachName) == 0 {
+		cs.InsertFID(t.FID, root)
+		return &Rattach{}
+	}
+
+	// We want the same traversal checks to apply on attach, so always
+	// attach at the root and use the regular walk paths.
+	names := strings.Split(t.Auth.AttachName, "/")
+	_, target, _, attr, err := doWalk(cs, root, names)
+	if err != nil {
+		return newErr(err)
+	}
+
+	// Insert the FID.
+	cs.InsertFID(t.FID, &fidRef{
+		file:     target,
+		walkable: attr.Mode.IsDir(),
+	})
 
-	// Return an empty QID.
 	return &Rattach{}
 }
 
@@ -678,15 +689,104 @@ func (t *Tflushf) handle(cs *connState) message {
 	return &Rflushf{}
 }
 
-// handle implements handler.handle.
-func (t *Twalkgetattr) handle(cs *connState) message {
+// walkOne walks zero or one path elements.
+//
+// The slice passed as qids is append and returned.
+func walkOne(qids []QID, from File, names []string) ([]QID, File, AttrMask, Attr, error) {
+	if len(names) > 1 {
+		// We require exactly zero or one elements.
+		return nil, nil, AttrMask{}, Attr{}, syscall.EINVAL
+	}
+	var localQIDs []QID
+	localQIDs, sf, valid, attr, err := from.WalkGetAttr(names)
+	if err == syscall.ENOSYS {
+		localQIDs, sf, err = from.Walk(names)
+		if err != nil {
+			// No way to walk this element.
+			return nil, nil, AttrMask{}, Attr{}, err
+		}
+		// Need a manual getattr.
+		_, valid, attr, err = sf.GetAttr(AttrMaskAll())
+		if err != nil {
+			// Don't leak the file.
+			sf.Close()
+		}
+	}
+	if err != nil {
+		// Error walking, don't return anything.
+		return nil, nil, AttrMask{}, Attr{}, err
+	}
+	if len(localQIDs) != 1 {
+		// Expected a single QID.
+		sf.Close()
+		return nil, nil, AttrMask{}, Attr{}, syscall.EINVAL
+	}
+	return append(qids, localQIDs...), sf, valid, attr, nil
+}
+
+// doWalk walks from a given fidRef.
+//
+// This enforces that all intermediate nodes are walkable (directories).
+func doWalk(cs *connState, ref *fidRef, names []string) (qids []QID, sf File, valid AttrMask, attr Attr, err error) {
 	// Check the names.
-	for _, name := range t.Names {
+	for _, name := range names {
 		if !isSafeName(name) {
-			return newErr(syscall.EINVAL)
+			err = syscall.EINVAL
+			return
+		}
+	}
+
+	// Has it been opened already?
+	if _, opened := ref.OpenFlags(); opened {
+		err = syscall.EBUSY
+		return
+	}
+
+	// Is this an empty list? Handle specially. We don't actually need to
+	// validate anything since this is always permitted.
+	if len(names) == 0 {
+		return walkOne(nil, ref.file, nil)
+	}
+
+	// Is it walkable?
+	if !ref.walkable {
+		err = syscall.EINVAL
+		return
+	}
+
+	from := ref.file // Start at the passed ref.
+
+	// Do the walk, one element at a time.
+	for i := 0; i < len(names); i++ {
+		qids, sf, valid, attr, err = walkOne(qids, from, names[i:i+1])
+
+		// Close the intermediate file. Note that we don't close the
+		// first file because in that case we are walking from the
+		// existing reference.
+		if i > 0 {
+			from.Close()
+		}
+		from = sf // Use the new file.
+
+		// Was there an error walking?
+		if err != nil {
+			return nil, nil, AttrMask{}, Attr{}, err
+		}
+
+		// We won't allow beyond past symlinks; stop here if this isn't
+		// a proper directory and we have additional paths to walk.
+		if !valid.Mode || (!attr.Mode.IsDir() && i < len(names)-1) {
+			from.Close() // Not using the file object.
+			return nil, nil, AttrMask{}, Attr{}, syscall.EINVAL
 		}
 	}
 
+	// Success.
+	return qids, sf, valid, attr, nil
+}
+
+// handle implements handler.handle.
+func (t *Twalk) handle(cs *connState) message {
 	// Lookup the FID.
 	ref, ok := cs.LookupFID(t.FID)
 	if !ok {
@@ -694,26 +794,41 @@ func (t *Twalkgetattr) handle(cs *connState) message {
 	}
 	defer ref.DecRef()
 
-	// Has it been opened already?
-	if _, opened := ref.OpenFlags(); opened {
-		return newErr(syscall.EBUSY)
+	// Do the walk.
+	qids, sf, _, attr, err := doWalk(cs, ref, t.Names)
+	if err != nil {
+		return newErr(err)
 	}
 
-	// Do the walk.
-	qids, sf, valid, attr, err := ref.file.WalkGetAttr(t.Names)
-	if err == syscall.ENOSYS {
-		qids, sf, err = ref.file.Walk(t.Names)
-		if err != nil {
-			return newErr(err)
-		}
-		_, valid, attr, err = sf.GetAttr(AttrMaskAll())
+	// Install the new FID.
+	cs.InsertFID(t.NewFID, &fidRef{
+		file:     sf,
+		walkable: attr.Mode.IsDir(),
+	})
+
+	return &Rwalk{QIDs: qids}
+}
+
+// handle implements handler.handle.
+func (t *Twalkgetattr) handle(cs *connState) message {
+	// Lookup the FID.
+	ref, ok := cs.LookupFID(t.FID)
+	if !ok {
+		return newErr(syscall.EBADF)
 	}
+	defer ref.DecRef()
+
+	// Do the walk.
+	qids, sf, valid, attr, err := doWalk(cs, ref, t.Names)
 	if err != nil {
 		return newErr(err)
 	}
 
 	// Install the new FID.
-	cs.InsertFID(t.NewFID, &fidRef{file: sf})
+	cs.InsertFID(t.NewFID, &fidRef{
+		file:     sf,
+		walkable: attr.Mode.IsDir(),
+	})
 
 	return &Rwalkgetattr{QIDs: qids, Valid: valid, Attr: attr}
 }
diff --git a/pkg/p9/local_server/local_server.go b/pkg/p9/local_server/local_server.go
index b4db44e27..cef3701a7 100644
--- a/pkg/p9/local_server/local_server.go
+++ b/pkg/p9/local_server/local_server.go
@@ -70,8 +70,8 @@ func (l *local) info() (p9.QID, os.FileInfo, error) {
 }
 
 // Attach implements p9.Attacher.Attach.
-func (l *local) Attach(name string) (p9.File, error) {
-	return &local{path: path.Clean(name)}, nil
+func (l *local) Attach() (p9.File, error) {
+	return &local{path: "/"}, nil
 }
 
 // Walk implements p9.File.Walk.
diff --git a/pkg/p9/p9test/client_test.go b/pkg/p9/p9test/client_test.go
index 8e35d6017..34ddccd8b 100644
--- a/pkg/p9/p9test/client_test.go
+++ b/pkg/p9/p9test/client_test.go
@@ -50,8 +50,16 @@ func TestDonateFD(t *testing.T) {
 
 	// Craft attacher to attach to the mocked file which will return our
 	// temporary file.
-	fileMock := &FileMock{OpenMock: OpenMock{File: f}}
-	attacher := &AttachMock{File: fileMock}
+	fileMock := &FileMock{
+		OpenMock: OpenMock{File: f},
+		GetAttrMock: GetAttrMock{
+			// The mode must be valid always.
+			Valid: p9.AttrMask{Mode: true},
+		},
+	}
+	attacher := &AttachMock{
+		File: fileMock,
+	}
 
 	// Make socket pair.
 	serverSocket, clientSocket, err := unet.SocketPair(false)
@@ -139,15 +147,14 @@ func TestClient(t *testing.T) {
 				a.Called = false
 				a.File = sf
 				a.Err = nil
+				// The attached root must have a valid mode.
+				sf.GetAttrMock.Attr = p9.Attr{Mode: p9.ModeDirectory}
+				sf.GetAttrMock.Valid = p9.AttrMask{Mode: true}
 				var err error
-				sfFile, err = c.Attach("foo")
+				sfFile, err = c.Attach("")
 				if !a.Called {
 					t.Errorf("Attach never Called?")
 				}
-				if a.AttachName != "foo" {
-					// This wasn't carried through?
-					t.Errorf("attachName got %v wanted foo", a.AttachName)
-				}
 				return err
 			},
 		},
@@ -155,6 +162,8 @@ func TestClient(t *testing.T) {
 			name: "bad-walk",
 			want: sentinelErr,
 			fn: func(c *p9.Client) error {
+				// Walk only called when WalkGetAttr not available.
+				sf.WalkGetAttrMock.Err = syscall.ENOSYS
 				sf.WalkMock.File = d
 				sf.WalkMock.Err = sentinelErr
 				_, _, err := sfFile.Walk([]string{"foo", "bar"})
@@ -164,21 +173,39 @@ func TestClient(t *testing.T) {
 		{
 			name: "walk-to-dir",
 			fn: func(c *p9.Client) error {
+				// Walk only called when WalkGetAttr not available.
+				sf.WalkGetAttrMock.Err = syscall.ENOSYS
 				sf.WalkMock.Called = false
+				sf.WalkMock.Names = nil
 				sf.WalkMock.File = d
 				sf.WalkMock.Err = nil
 				sf.WalkMock.QIDs = []p9.QID{{Type: 1}}
+				// All intermediate values must be directories.
+				d.WalkGetAttrMock.Err = syscall.ENOSYS
+				d.WalkMock.Called = false
+				d.WalkMock.Names = nil
+				d.WalkMock.File = d // Walk to self.
+				d.WalkMock.Err = nil
+				d.WalkMock.QIDs = []p9.QID{{Type: 1}}
+				d.GetAttrMock.Attr = p9.Attr{Mode: p9.ModeDirectory}
+				d.GetAttrMock.Valid = p9.AttrMask{Mode: true}
 				var qids []p9.QID
 				var err error
 				qids, _, err = sfFile.Walk([]string{"foo", "bar"})
 				if !sf.WalkMock.Called {
 					t.Errorf("Walk never Called?")
 				}
-				if !reflect.DeepEqual(sf.WalkMock.Names, []string{"foo", "bar"}) {
-					t.Errorf("got names %v wanted []{foo, bar}", sf.WalkMock.Names)
+				if !d.GetAttrMock.Called {
+					t.Errorf("GetAttr never Called?")
 				}
-				if len(qids) != 1 || qids[0].Type != 1 {
-					t.Errorf("got qids %v wanted []{{Type: 1}}", qids)
+				if !reflect.DeepEqual(sf.WalkMock.Names, []string{"foo"}) {
+					t.Errorf("got names %v wanted []{foo}", sf.WalkMock.Names)
+				}
+				if !reflect.DeepEqual(d.WalkMock.Names, []string{"bar"}) {
+					t.Errorf("got names %v wanted []{bar}", d.WalkMock.Names)
+				}
+				if len(qids) != 2 || qids[len(qids)-1].Type != 1 {
+					t.Errorf("got qids %v wanted []{..., {Type: 1}}", qids)
 				}
 				return err
 			},
@@ -187,11 +214,20 @@ func TestClient(t *testing.T) {
 			name: "walkgetattr-to-dir",
 			fn: func(c *p9.Client) error {
 				sf.WalkGetAttrMock.Called = false
+				sf.WalkGetAttrMock.Names = nil
 				sf.WalkGetAttrMock.File = d
 				sf.WalkGetAttrMock.Err = nil
 				sf.WalkGetAttrMock.QIDs = []p9.QID{{Type: 1}}
-				sf.WalkGetAttrMock.Attr = p9.Attr{UID: 1}
+				sf.WalkGetAttrMock.Attr = p9.Attr{Mode: p9.ModeDirectory, UID: 1}
 				sf.WalkGetAttrMock.Valid = p9.AttrMask{Mode: true}
+				// See above.
+				d.WalkGetAttrMock.Called = false
+				d.WalkGetAttrMock.Names = nil
+				d.WalkGetAttrMock.File = d // Walk to self.
+				d.WalkGetAttrMock.Err = nil
+				d.WalkGetAttrMock.QIDs = []p9.QID{{Type: 1}}
+				d.WalkGetAttrMock.Attr = p9.Attr{Mode: p9.ModeDirectory, UID: 1}
+				d.WalkGetAttrMock.Valid = p9.AttrMask{Mode: true}
 				var qids []p9.QID
 				var err error
 				var mask p9.AttrMask
@@ -200,11 +236,14 @@ func TestClient(t *testing.T) {
 				if !sf.WalkGetAttrMock.Called {
 					t.Errorf("Walk never Called?")
 				}
-				if !reflect.DeepEqual(sf.WalkGetAttrMock.Names, []string{"foo", "bar"}) {
-					t.Errorf("got names %v wanted []{foo, bar}", sf.WalkGetAttrMock.Names)
+				if !reflect.DeepEqual(sf.WalkGetAttrMock.Names, []string{"foo"}) {
+					t.Errorf("got names %v wanted []{foo}", sf.WalkGetAttrMock.Names)
+				}
+				if !reflect.DeepEqual(d.WalkGetAttrMock.Names, []string{"bar"}) {
+					t.Errorf("got names %v wanted []{bar}", d.WalkGetAttrMock.Names)
 				}
-				if len(qids) != 1 || qids[0].Type != 1 {
-					t.Errorf("got qids %v wanted []{{Type: 1}}", qids)
+				if len(qids) != 2 || qids[len(qids)-1].Type != 1 {
+					t.Errorf("got qids %v wanted []{..., {Type: 1}}", qids)
 				}
 				if !reflect.DeepEqual(attr, sf.WalkGetAttrMock.Attr) {
 					t.Errorf("got attrs %s wanted %s", attr, sf.WalkGetAttrMock.Attr)
diff --git a/pkg/p9/p9test/mocks.go b/pkg/p9/p9test/mocks.go
index e10f206dd..9d039ac63 100644
--- a/pkg/p9/p9test/mocks.go
+++ b/pkg/p9/p9test/mocks.go
@@ -71,7 +71,8 @@ type WalkGetAttrMock struct {
 
 // WalkGetAttr implements p9.File.WalkGetAttr.
 func (w *WalkGetAttrMock) WalkGetAttr(names []string) ([]p9.QID, p9.File, p9.AttrMask, p9.Attr, error) {
-	w.Called, w.Names = true, names
+	w.Called = true
+	w.Names = append(w.Names, names...)
 	return w.QIDs, w.File, w.Valid, w.Attr, w.Err
 }
 
@@ -300,17 +301,14 @@ func (r *ReadlinkMock) Readlink() (string, error) {
 type AttachMock struct {
 	Called bool
 
-	// Args.
-	AttachName string
-
 	// Return.
 	File p9.File
 	Err  error
 }
 
 // Attach implements p9.Attacher.Attach.
-func (a *AttachMock) Attach(attachName string) (p9.File, error) {
-	a.Called, a.AttachName = true, attachName
+func (a *AttachMock) Attach() (p9.File, error) {
+	a.Called = true
 	return a.File, a.Err
 }
 
@@ -329,7 +327,8 @@ type WalkMock struct {
 
 // Walk implements p9.File.Walk.
 func (w *WalkMock) Walk(names []string) ([]p9.QID, p9.File, error) {
-	w.Called, w.Names = true, names
+	w.Called = true
+	w.Names = append(w.Names, names...)
 	return w.QIDs, w.File, w.Err
 }
 
diff --git a/pkg/p9/server.go b/pkg/p9/server.go
index 2965ae16e..28a273ac6 100644
--- a/pkg/p9/server.go
+++ b/pkg/p9/server.go
@@ -97,6 +97,9 @@ type fidRef struct {
 	// This is updated in handlers.go.
 	opened bool
 
+	// walkable indicates this fidRef may be walked.
+	walkable bool
+
 	// openFlags is the mode used in the open.
 	//
 	// This is updated in handlers.go.