diff --git a/pkg/sentry/fsimpl/gofer/filesystem.go b/pkg/sentry/fsimpl/gofer/filesystem.go index a156d6c1c..74c24f0a1 100644 --- a/pkg/sentry/fsimpl/gofer/filesystem.go +++ b/pkg/sentry/fsimpl/gofer/filesystem.go @@ -164,9 +164,7 @@ func (fs *filesystem) renameMuUnlockAndCheckCaching(ctx context.Context, ds **[] } // stepLocked resolves rp.Component() to an existing file, starting from the -// given directory. If the file at rp.Component is a symlink and -// mayFollowSymlinks is set, the symlink is resolved and the result returned to -// the caller (single step). +// given directory. // // Dentries which may become cached as a result of the traversal are appended // to *ds. @@ -214,10 +212,8 @@ func (fs *filesystem) stepLocked(ctx context.Context, rp *vfs.ResolvingPath, d * if err != nil { return nil, false, err } - if err := rp.HandleSymlink(target); err != nil { - return nil, false, err - } - return d, true, nil + followedSymlink, err := rp.HandleSymlink(target) + return d, followedSymlink, err } rp.Advance() return child, false, nil @@ -816,13 +812,13 @@ func (fs *filesystem) MknodAt(ctx context.Context, rp *vfs.ResolvingPath, opts v // to creating a synthetic one, i.e. one that is kept entirely in memory. // Check that we're not overriding an existing file with a synthetic one. - _, _, err := fs.stepLocked(ctx, rp, parent, false, ds) + _, _, err := fs.stepLocked(ctx, rp, parent, false /* mayFollowSymlinks */, ds) switch { case err == nil: // Step succeeded, another file exists. return nil, linuxerr.EEXIST case !linuxerr.Equals(linuxerr.ENOENT, err): - // Unexpected error. + // Schrödinger. File/Cat may or may not exist. return nil, err } @@ -913,7 +909,21 @@ afterTrailingSymlink: } // Determine whether or not we need to create a file. parent.dirMu.Lock() - child, _, err := fs.stepLocked(ctx, rp, parent, false /* mayFollowSymlinks */, &ds) + child, followedSymlink, err := fs.stepLocked(ctx, rp, parent, true /* mayFollowSymlinks */, &ds) + if followedSymlink { + parent.dirMu.Unlock() + if mustCreate { + // EEXIST must be returned if an existing symlink is opened with O_EXCL. + return nil, linuxerr.EEXIST + } + if err != nil { + // If followedSymlink && err != nil, then this symlink resolution error + // must be handled by the VFS layer. + return nil, err + } + start = parent + goto afterTrailingSymlink + } if linuxerr.Equals(linuxerr.ENOENT, err) && mayCreate { if parent.isSynthetic() { parent.dirMu.Unlock() @@ -930,18 +940,6 @@ afterTrailingSymlink: if mustCreate { return nil, linuxerr.EEXIST } - // Open existing child or follow symlink. - if child.isSymlink() && rp.ShouldFollowSymlink() { - target, err := child.readlink(ctx, rp.Mount()) - if err != nil { - return nil, err - } - if err := rp.HandleSymlink(target); err != nil { - return nil, err - } - start = parent - goto afterTrailingSymlink - } if rp.MustBeDir() && !child.isDir() { return nil, linuxerr.ENOTDIR } diff --git a/pkg/sentry/fsimpl/kernfs/filesystem.go b/pkg/sentry/fsimpl/kernfs/filesystem.go index 5dc4499b2..8bf4192dd 100644 --- a/pkg/sentry/fsimpl/kernfs/filesystem.go +++ b/pkg/sentry/fsimpl/kernfs/filesystem.go @@ -37,15 +37,14 @@ import ( // - !rp.Done(). // // Postcondition: Caller must call fs.processDeferredDecRefs*. -func (fs *Filesystem) stepExistingLocked(ctx context.Context, rp *vfs.ResolvingPath, d *Dentry, mayFollowSymlinks bool) (*Dentry, error) { +func (fs *Filesystem) stepExistingLocked(ctx context.Context, rp *vfs.ResolvingPath, d *Dentry) (*Dentry, bool, error) { if !d.isDir() { - return nil, linuxerr.ENOTDIR + return nil, false, linuxerr.ENOTDIR } // Directory searchable? if err := d.inode.CheckPermissions(ctx, rp.Credentials(), vfs.MayExec); err != nil { - return nil, err + return nil, false, err } -afterSymlink: name := rp.Component() // Revalidation must be skipped if name is "." or ".."; d or its parent // respectively can't be expected to transition from invalidated back to @@ -54,54 +53,49 @@ afterSymlink: // calls d_revalidate(), but walk_component() => handle_dots() does not. if name == "." { rp.Advance() - return d, nil + return d, false, nil } if name == ".." { if isRoot, err := rp.CheckRoot(ctx, d.VFSDentry()); err != nil { - return nil, err + return nil, false, err } else if isRoot || d.parent == nil { rp.Advance() - return d, nil + return d, false, nil } if err := rp.CheckMount(ctx, d.parent.VFSDentry()); err != nil { - return nil, err + return nil, false, err } rp.Advance() - return d.parent, nil + return d.parent, false, nil } if len(name) > linux.NAME_MAX { - return nil, linuxerr.ENAMETOOLONG + return nil, false, linuxerr.ENAMETOOLONG } d.dirMu.Lock() next, err := fs.revalidateChildLocked(ctx, rp.VirtualFilesystem(), d, name, d.children[name]) d.dirMu.Unlock() if err != nil { - return nil, err + return nil, false, err } if err := rp.CheckMount(ctx, next.VFSDentry()); err != nil { - return nil, err + return nil, false, err } // Resolve any symlink at current path component. - if mayFollowSymlinks && rp.ShouldFollowSymlink() && next.isSymlink() { + if rp.ShouldFollowSymlink() && next.isSymlink() { targetVD, targetPathname, err := next.inode.Getlink(ctx, rp.Mount()) if err != nil { - return nil, err + return nil, false, err } if targetVD.Ok() { - err := rp.HandleJump(targetVD) + followedTarget, err := rp.HandleJump(targetVD) fs.deferDecRefVD(ctx, targetVD) - if err != nil { - return nil, err - } - } else { - if err := rp.HandleSymlink(targetPathname); err != nil { - return nil, err - } + return d, followedTarget, err } - goto afterSymlink + followedSymlink, err := rp.HandleSymlink(targetPathname) + return d, followedSymlink, err } rp.Advance() - return next, nil + return next, false, nil } // revalidateChildLocked must be called after a call to parent.vfsd.Child(name) @@ -163,7 +157,7 @@ func (fs *Filesystem) walkExistingLocked(ctx context.Context, rp *vfs.ResolvingP d := rp.Start().Impl().(*Dentry) for !rp.Done() { var err error - d, err = fs.stepExistingLocked(ctx, rp, d, true /* mayFollowSymlinks */) + d, _, err = fs.stepExistingLocked(ctx, rp, d) if err != nil { return nil, err } @@ -190,7 +184,7 @@ func (fs *Filesystem) walkParentDirLocked(ctx context.Context, rp *vfs.Resolving d := rp.Start().Impl().(*Dentry) for !rp.Final() { var err error - d, err = fs.stepExistingLocked(ctx, rp, d, true /* mayFollowSymlinks */) + d, _, err = fs.stepExistingLocked(ctx, rp, d) if err != nil { return nil, err } @@ -561,7 +555,19 @@ afterTrailingSymlink: return nil, linuxerr.ENOENT } // Determine whether or not we need to create a file. - child, err := fs.stepExistingLocked(ctx, rp, parent, false /* mayFollowSymlinks */) + child, followedSymlink, err := fs.stepExistingLocked(ctx, rp, parent) + if followedSymlink { + if mustCreate { + // EEXIST must be returned if an existing symlink is opened with O_EXCL. + return nil, linuxerr.EEXIST + } + if err != nil { + // If followedSymlink && err != nil, then this symlink resolution error + // must be handled by the VFS layer. + return nil, err + } + goto afterTrailingSymlink + } if linuxerr.Equals(linuxerr.ENOENT, err) { // Already checked for searchability above; now check for writability. if err := parent.inode.CheckPermissions(ctx, rp.Credentials(), vfs.MayWrite); err != nil { @@ -595,25 +601,8 @@ afterTrailingSymlink: if mustCreate { return nil, linuxerr.EEXIST } - if rp.ShouldFollowSymlink() && child.isSymlink() { - targetVD, targetPathname, err := child.inode.Getlink(ctx, rp.Mount()) - if err != nil { - return nil, err - } - if targetVD.Ok() { - err := rp.HandleJump(targetVD) - fs.deferDecRefVD(ctx, targetVD) - if err != nil { - return nil, err - } - } else { - if err := rp.HandleSymlink(targetPathname); err != nil { - return nil, err - } - } - // rp.Final() may no longer be true since we now need to resolve the - // symlink target. - goto afterTrailingSymlink + if rp.MustBeDir() && !child.isDir() { + return nil, linuxerr.ENOTDIR } if err := child.inode.CheckPermissions(ctx, rp.Credentials(), ats); err != nil { return nil, err diff --git a/pkg/sentry/fsimpl/overlay/filesystem.go b/pkg/sentry/fsimpl/overlay/filesystem.go index ee7899b3a..68ca780a9 100644 --- a/pkg/sentry/fsimpl/overlay/filesystem.go +++ b/pkg/sentry/fsimpl/overlay/filesystem.go @@ -135,54 +135,51 @@ func (fs *filesystem) renameMuUnlockAndCheckDrop(ctx context.Context, ds **[]*de // - fs.renameMu must be locked. // - d.dirMu must be locked. // - !rp.Done(). -func (fs *filesystem) stepLocked(ctx context.Context, rp *vfs.ResolvingPath, d *dentry, mayFollowSymlinks bool, ds **[]*dentry) (*dentry, lookupLayer, error) { +func (fs *filesystem) stepLocked(ctx context.Context, rp *vfs.ResolvingPath, d *dentry, ds **[]*dentry) (*dentry, lookupLayer, bool, error) { if !d.isDir() { - return nil, lookupLayerNone, linuxerr.ENOTDIR + return nil, lookupLayerNone, false, linuxerr.ENOTDIR } if err := d.checkPermissions(rp.Credentials(), vfs.MayExec); err != nil { - return nil, lookupLayerNone, err + return nil, lookupLayerNone, false, err } -afterSymlink: name := rp.Component() if name == "." { rp.Advance() - return d, d.topLookupLayer(), nil + return d, d.topLookupLayer(), false, nil } if name == ".." { if isRoot, err := rp.CheckRoot(ctx, &d.vfsd); err != nil { - return nil, lookupLayerNone, err + return nil, lookupLayerNone, false, err } else if isRoot || d.parent == nil { rp.Advance() - return d, d.topLookupLayer(), nil + return d, d.topLookupLayer(), false, nil } if err := rp.CheckMount(ctx, &d.parent.vfsd); err != nil { - return nil, lookupLayerNone, err + return nil, lookupLayerNone, false, err } rp.Advance() - return d.parent, d.parent.topLookupLayer(), nil + return d.parent, d.parent.topLookupLayer(), false, nil } if uint64(len(name)) > fs.maxFilenameLen { - return nil, lookupLayerNone, linuxerr.ENAMETOOLONG + return nil, lookupLayerNone, false, linuxerr.ENAMETOOLONG } child, topLookupLayer, err := fs.getChildLocked(ctx, d, name, ds) if err != nil { - return nil, topLookupLayer, err + return nil, topLookupLayer, false, err } if err := rp.CheckMount(ctx, &child.vfsd); err != nil { - return nil, lookupLayerNone, err + return nil, lookupLayerNone, false, err } - if child.isSymlink() && mayFollowSymlinks && rp.ShouldFollowSymlink() { + if child.isSymlink() && rp.ShouldFollowSymlink() { target, err := child.readlink(ctx) if err != nil { - return nil, lookupLayerNone, err + return nil, lookupLayerNone, false, err } - if err := rp.HandleSymlink(target); err != nil { - return nil, topLookupLayer, err - } - goto afterSymlink // don't check the current directory again + followedSymlink, err := rp.HandleSymlink(target) + return d, topLookupLayer, followedSymlink, err } rp.Advance() - return child, topLookupLayer, nil + return child, topLookupLayer, false, nil } // Preconditions: @@ -447,7 +444,7 @@ func (ll lookupLayer) existsInOverlay() bool { func (fs *filesystem) walkParentDirLocked(ctx context.Context, rp *vfs.ResolvingPath, d *dentry, ds **[]*dentry) (*dentry, error) { for !rp.Final() { d.dirMu.Lock() - next, _, err := fs.stepLocked(ctx, rp, d, true /* mayFollowSymlinks */, ds) + next, _, _, err := fs.stepLocked(ctx, rp, d, ds) d.dirMu.Unlock() if err != nil { return nil, err @@ -467,7 +464,7 @@ func (fs *filesystem) resolveLocked(ctx context.Context, rp *vfs.ResolvingPath, d := rp.Start().Impl().(*dentry) for !rp.Done() { d.dirMu.Lock() - next, _, err := fs.stepLocked(ctx, rp, d, true /* mayFollowSymlinks */, ds) + next, _, _, err := fs.stepLocked(ctx, rp, d, ds) d.dirMu.Unlock() if err != nil { return nil, err @@ -855,7 +852,21 @@ afterTrailingSymlink: } // Determine whether or not we need to create a file. parent.dirMu.Lock() - child, topLookupLayer, err := fs.stepLocked(ctx, rp, parent, false /* mayFollowSymlinks */, &ds) + child, topLookupLayer, followedSymlink, err := fs.stepLocked(ctx, rp, parent, &ds) + if followedSymlink { + parent.dirMu.Unlock() + if mustCreate { + // EEXIST must be returned if an existing symlink is opened with O_EXCL. + return nil, linuxerr.EEXIST + } + if err != nil { + // If followedSymlink && err != nil, then this symlink resolution error + // must be handled by the VFS layer. + return nil, err + } + start = parent + goto afterTrailingSymlink + } if linuxerr.Equals(linuxerr.ENOENT, err) && mayCreate { fd, err := fs.createAndOpenLocked(ctx, rp, parent, &opts, &ds, topLookupLayer == lookupLayerUpperWhiteout) parent.dirMu.Unlock() @@ -865,21 +876,9 @@ afterTrailingSymlink: if err != nil { return nil, err } - // Open existing child or follow symlink. if mustCreate { return nil, linuxerr.EEXIST } - if child.isSymlink() && rp.ShouldFollowSymlink() { - target, err := child.readlink(ctx) - if err != nil { - return nil, err - } - if err := rp.HandleSymlink(target); err != nil { - return nil, err - } - start = parent - goto afterTrailingSymlink - } if rp.MustBeDir() && !child.isDir() { return nil, linuxerr.ENOTDIR } diff --git a/pkg/sentry/fsimpl/tmpfs/filesystem.go b/pkg/sentry/fsimpl/tmpfs/filesystem.go index 1dd8589f0..fa2d2a7b3 100644 --- a/pkg/sentry/fsimpl/tmpfs/filesystem.go +++ b/pkg/sentry/fsimpl/tmpfs/filesystem.go @@ -51,53 +51,50 @@ func (fs *filesystem) Sync(ctx context.Context) error { // Preconditions: // - filesystem.mu must be locked. // - !rp.Done(). -func stepLocked(ctx context.Context, rp *vfs.ResolvingPath, d *dentry) (*dentry, error) { +func stepLocked(ctx context.Context, rp *vfs.ResolvingPath, d *dentry) (*dentry, bool, error) { dir, ok := d.inode.impl.(*directory) if !ok { - return nil, linuxerr.ENOTDIR + return nil, false, linuxerr.ENOTDIR } if err := d.inode.checkPermissions(rp.Credentials(), vfs.MayExec); err != nil { - return nil, err + return nil, false, err } -afterSymlink: name := rp.Component() if name == "." { rp.Advance() - return d, nil + return d, false, nil } if name == ".." { if isRoot, err := rp.CheckRoot(ctx, &d.vfsd); err != nil { - return nil, err + return nil, false, err } else if isRoot || d.parent == nil { rp.Advance() - return d, nil + return d, false, nil } if err := rp.CheckMount(ctx, &d.parent.vfsd); err != nil { - return nil, err + return nil, false, err } rp.Advance() - return d.parent, nil + return d.parent, false, nil } if len(name) > d.inode.fs.maxFilenameLen { - return nil, linuxerr.ENAMETOOLONG + return nil, false, linuxerr.ENAMETOOLONG } child, ok := dir.childMap[name] if !ok { - return nil, linuxerr.ENOENT + return nil, false, linuxerr.ENOENT } if err := rp.CheckMount(ctx, &child.vfsd); err != nil { - return nil, err + return nil, false, err } if symlink, ok := child.inode.impl.(*symlink); ok && rp.ShouldFollowSymlink() { // Symlink traversal updates access time. child.inode.touchAtime(rp.Mount()) - if err := rp.HandleSymlink(symlink.target); err != nil { - return nil, err - } - goto afterSymlink // don't check the current directory again + followedSymlink, err := rp.HandleSymlink(symlink.target) + return d, followedSymlink, err } rp.Advance() - return child, nil + return child, false, nil } // walkParentDirLocked resolves all but the last path component of rp to an @@ -113,7 +110,7 @@ afterSymlink: // - !rp.Done(). func walkParentDirLocked(ctx context.Context, rp *vfs.ResolvingPath, d *dentry) (*directory, error) { for !rp.Final() { - next, err := stepLocked(ctx, rp, d) + next, _, err := stepLocked(ctx, rp, d) if err != nil { return nil, err } @@ -140,13 +137,13 @@ func resolveLocked(ctx context.Context, rp *vfs.ResolvingPath) (*dentry, error) // // Symlink traversal updates access time. d.inode.touchAtime(rp.Mount()) - if err := rp.HandleSymlink(symlink.target); err != nil { + if _, err := rp.HandleSymlink(symlink.target); err != nil { return nil, err } } else { // Path with multiple components, walk and resolve as required. for !rp.Done() { - next, err := stepLocked(ctx, rp, d) + next, _, err := stepLocked(ctx, rp, d) if err != nil { return nil, err } @@ -397,15 +394,21 @@ afterTrailingSymlink: return nil, linuxerr.EISDIR } name := rp.Component() - if name == "." || name == ".." { - return nil, linuxerr.EISDIR + child, followedSymlink, err := stepLocked(ctx, rp, &parentDir.dentry) + if followedSymlink { + if mustCreate { + // EEXIST must be returned if an existing symlink is opened with O_EXCL. + return nil, linuxerr.EEXIST + } + if err != nil { + // If followedSymlink && err != nil, then this symlink resolution error + // must be handled by the VFS layer. + return nil, err + } + start = &parentDir.dentry + goto afterTrailingSymlink } - if len(name) > fs.maxFilenameLen { - return nil, linuxerr.ENAMETOOLONG - } - // Determine whether or not we need to create a file. - child, ok := parentDir.childMap[name] - if !ok { + if linuxerr.Equals(linuxerr.ENOENT, err) { // Already checked for searchability above; now check for writability. if err := parentDir.inode.checkPermissions(rp.Credentials(), vfs.MayWrite); err != nil { return nil, err @@ -429,22 +432,11 @@ afterTrailingSymlink: parentDir.inode.touchCMtime() return fd, nil } - if mustCreate { - return nil, linuxerr.EEXIST - } - // Is the file mounted over? - if err := rp.CheckMount(ctx, &child.vfsd); err != nil { + if err != nil { return nil, err } - // Do we need to resolve a trailing symlink? - if symlink, ok := child.inode.impl.(*symlink); ok && rp.ShouldFollowSymlink() { - // Symlink traversal updates access time. - child.inode.touchAtime(rp.Mount()) - if err := rp.HandleSymlink(symlink.target); err != nil { - return nil, err - } - start = &parentDir.dentry - goto afterTrailingSymlink + if mustCreate { + return nil, linuxerr.EEXIST } if rp.MustBeDir() && !child.inode.isDir() { return nil, linuxerr.ENOTDIR diff --git a/pkg/sentry/vfs/resolving_path.go b/pkg/sentry/vfs/resolving_path.go index 3abbc37d8..710e87a93 100644 --- a/pkg/sentry/vfs/resolving_path.go +++ b/pkg/sentry/vfs/resolving_path.go @@ -327,23 +327,25 @@ func (rp *ResolvingPath) ShouldFollowSymlink() bool { // HandleSymlink is called when the current path component is a symbolic link // to the given target. If the calling Filesystem method should continue path // traversal, HandleSymlink updates the path component stream to reflect the -// symlink target and returns nil. Otherwise it returns a non-nil error. +// symlink target and returns nil. Otherwise it returns a non-nil error. It +// also returns whether the symlink was successfully followed, which can be +// true even when a non-nil error like resolveAbsSymlinkError is returned. // // Preconditions: !rp.Done(). // // Postconditions: If HandleSymlink returns a nil error, then !rp.Done(). -func (rp *ResolvingPath) HandleSymlink(target string) error { +func (rp *ResolvingPath) HandleSymlink(target string) (bool, error) { if rp.symlinks >= linux.MaxSymlinkTraversals { - return linuxerr.ELOOP + return false, linuxerr.ELOOP } if len(target) == 0 { - return linuxerr.ENOENT + return false, linuxerr.ENOENT } rp.symlinks++ targetPath := fspath.Parse(target) if targetPath.Absolute { rp.absSymlinkTarget = targetPath - return resolveAbsSymlinkError{} + return true, resolveAbsSymlinkError{} } // Consume the path component that represented the symlink. rp.Advance() @@ -354,7 +356,7 @@ func (rp *ResolvingPath) HandleSymlink(target string) error { } } rp.relpathPrepend(targetPath) - return nil + return true, nil } // Preconditions: path.HasComponents(). @@ -379,12 +381,14 @@ func (rp *ResolvingPath) relpathPrepend(path fspath.Path) { // the given VirtualDentry, like /proc/[pid]/fd/[fd]. If the calling Filesystem // method should continue path traversal, HandleJump updates the path // component stream to reflect the magic link target and returns nil. Otherwise -// it returns a non-nil error. +// it returns a non-nil error. It also returns whether the magic link was +// followed, which can be true even when a non-nil error like +// resolveMountRootOrJumpError is returned. // // Preconditions: !rp.Done(). -func (rp *ResolvingPath) HandleJump(target VirtualDentry) error { +func (rp *ResolvingPath) HandleJump(target VirtualDentry) (bool, error) { if rp.symlinks >= linux.MaxSymlinkTraversals { - return linuxerr.ELOOP + return false, linuxerr.ELOOP } rp.symlinks++ // Consume the path component that represented the magic link. @@ -394,7 +398,7 @@ func (rp *ResolvingPath) HandleJump(target VirtualDentry) error { target.IncRef() rp.nextMount = target.mount rp.nextStart = target.dentry - return resolveMountRootOrJumpError{} + return true, resolveMountRootOrJumpError{} } func (rp *ResolvingPath) handleError(ctx context.Context, err error) bool {