From aff351f684cc0f5ee9b12a67e74caafa054b780e Mon Sep 17 00:00:00 2001 From: Ayush Ranjan Date: Mon, 23 Jan 2023 15:32:25 -0800 Subject: [PATCH] Handle terminal symlinks correctly in OpenAt() implementations. All filesystems except tmpfs had the same bug regarding handling symlinks in the terminal path component. When they tried to open(2) such a symlink file, these implementations called into stepLocked() with Then these implementations also called vfs.HandleSymlink() which would also advance rp (when rp.Done() was already true). This was tripping the invariants checks in sentry/vfs/debug.go. PiperOrigin-RevId: 504098263 --- pkg/sentry/fsimpl/gofer/filesystem.go | 42 ++++++------- pkg/sentry/fsimpl/kernfs/filesystem.go | 81 +++++++++++-------------- pkg/sentry/fsimpl/overlay/filesystem.go | 67 ++++++++++---------- pkg/sentry/fsimpl/tmpfs/filesystem.go | 76 +++++++++++------------ pkg/sentry/vfs/resolving_path.go | 24 +++++--- 5 files changed, 136 insertions(+), 154 deletions(-) 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 {