From ecd94ea7a693d49a0edce8607241a8e2ac22bfe0 Mon Sep 17 00:00:00 2001 From: Nicolas Lacasse Date: Mon, 15 Oct 2018 17:41:34 -0700 Subject: Clean up Rename and Unlink checks for EBUSY. - Change Dirent.Busy => Dirent.isMountPoint. The function body is unchanged, and it is no longer exported. - fs.MayDelete now checks that the victim is not the process root. This aligns with Linux's namei.c:may_delete(). - Fix "is-ancestor" checks to actually compare all ancestors, not just the parents. - Fix handling of paths that end in dots, which are handled differently in Rename vs. Unlink. PiperOrigin-RevId: 217239274 Change-Id: I7a0eb768e70a1b2915017ce54f7f95cbf8edf1fb --- pkg/sentry/fs/dirent.go | 75 ++++++++++++++++++++++++----------- pkg/sentry/syscalls/linux/sys_file.go | 43 +++++++++++--------- 2 files changed, 75 insertions(+), 43 deletions(-) (limited to 'pkg') diff --git a/pkg/sentry/fs/dirent.go b/pkg/sentry/fs/dirent.go index 7191c5c30..a42c03e98 100644 --- a/pkg/sentry/fs/dirent.go +++ b/pkg/sentry/fs/dirent.go @@ -1027,11 +1027,14 @@ func (d *Dirent) flush() { } } -// Busy indicates whether this Dirent is a mount point or root dirent. -func (d *Dirent) Busy() bool { +// isMountPoint returns true if the dirent is a mount point or the root. +func (d *Dirent) isMountPoint() bool { d.mu.Lock() defer d.mu.Unlock() + return d.isMountPointLocked() +} +func (d *Dirent) isMountPointLocked() bool { return d.mounted || d.parent == nil } @@ -1137,7 +1140,7 @@ func (d *Dirent) Remove(ctx context.Context, root *Dirent, name string) error { } // Remove cannot remove a mount point. - if child.Busy() { + if child.isMountPoint() { return syscall.EBUSY } @@ -1211,7 +1214,7 @@ func (d *Dirent) RemoveDirectory(ctx context.Context, root *Dirent, name string) } // Remove cannot remove a mount point. - if child.Busy() { + if child.isMountPoint() { return syscall.EBUSY } @@ -1457,12 +1460,20 @@ func MayDelete(ctx context.Context, root, dir *Dirent, name string) error { return mayDelete(ctx, dir, victim) } -func mayDelete(ctx context.Context, dir *Dirent, victim *Dirent) error { +func mayDelete(ctx context.Context, dir, victim *Dirent) error { if err := dir.Inode.CheckPermission(ctx, PermMask{Write: true, Execute: true}); err != nil { return err } - return checkSticky(ctx, dir, victim) + if err := checkSticky(ctx, dir, victim); err != nil { + return err + } + + if victim.IsRoot() { + return syserror.EBUSY + } + + return nil } // Rename atomically converts the child of oldParent named oldName to a @@ -1491,33 +1502,28 @@ func Rename(ctx context.Context, root *Dirent, oldParent *Dirent, oldName string return syscall.ENOENT } - // Check constraints on the object being renamed. + // renamed is the dirent that will be renamed to something else. renamed, err := oldParent.walk(ctx, root, oldName, false /* may unlock */) if err != nil { return err } defer renamed.DecRef() - // Make sure we have write permissions on old and new parent. + // Check that the renamed dirent is deletable. if err := mayDelete(ctx, oldParent, renamed); err != nil { return err } - if newParent != oldParent { - if err := newParent.Inode.CheckPermission(ctx, PermMask{Write: true, Execute: true}); err != nil { - return err - } + + // Check that the renamed dirent is not a mount point. + if renamed.isMountPointLocked() { + return syscall.EBUSY } // Source should not be an ancestor of the target. - if renamed == newParent { + if newParent.descendantOf(renamed) { return syscall.EINVAL } - // Is the thing we're trying to rename busy? - if renamed.Busy() { - return syscall.EBUSY - } - // Per rename(2): "... EACCES: ... or oldpath is a directory and does not // allow write permission (needed to update the .. entry)." if IsDir(renamed.Inode.StableAttr) { @@ -1526,21 +1532,42 @@ func Rename(ctx context.Context, root *Dirent, oldParent *Dirent, oldName string } } - // Check constraints on the object being replaced, if any. + // replaced is the dirent that is being overwritten by rename. replaced, err := newParent.walk(ctx, root, newName, false /* may unlock */) - if err == nil { + if err != nil { + if err != syserror.ENOENT { + return err + } + + // Make sure we can create a new child in the new parent. + if err := newParent.Inode.CheckPermission(ctx, PermMask{Write: true, Execute: true}); err != nil { + return err + } + } else { + // Check constraints on the dirent being replaced. + // NOTE: We don't want to keep replaced alive // across the Rename, so must call DecRef manually (no defer). + // Check that we can delete replaced. + if err := mayDelete(ctx, oldParent, renamed); err != nil { + replaced.DecRef() + return err + } + // Target should not be an ancestor of source. - if replaced == oldParent { + if oldParent.descendantOf(replaced) { replaced.DecRef() - // Why is this not EINVAL? See fs/namei.c. + + // Note that Linux returns EINVAL if the source is an + // ancestor of target, but ENOTEMPTY if the target is + // an ancestor of source (unless RENAME_EXCHANGE flag + // is present). See fs/namei.c:renameat2. return syscall.ENOTEMPTY } - // Is the thing we're trying to replace busy? - if replaced.Busy() { + // Check that replaced is not a mount point. + if replaced.isMountPointLocked() { replaced.DecRef() return syscall.EBUSY } diff --git a/pkg/sentry/syscalls/linux/sys_file.go b/pkg/sentry/syscalls/linux/sys_file.go index 015afda9b..64704bb88 100644 --- a/pkg/sentry/syscalls/linux/sys_file.go +++ b/pkg/sentry/syscalls/linux/sys_file.go @@ -1042,11 +1042,9 @@ func rmdirAt(t *kernel.Task, dirFD kdefs.FD, addr usermem.Addr) error { return err } - // Special case: rmdir rejects anything with '.' as last component. - // This would be handled by the busy check for the current working - // directory, but this is how it's done. - if (len(path) == 1 && path == ".") || (len(path) > 1 && path[len(path)-2:] == "/.") { - return syserror.EINVAL + // Special case: removing the root always returns EBUSY. + if path == "/" { + return syserror.EBUSY } return fileOpAt(t, dirFD, path, func(root *fs.Dirent, d *fs.Dirent, name string) error { @@ -1054,6 +1052,15 @@ func rmdirAt(t *kernel.Task, dirFD kdefs.FD, addr usermem.Addr) error { return syserror.ENOTDIR } + // Linux returns different ernos when the path ends in single + // dot vs. double dots. + switch name { + case ".": + return syserror.EINVAL + case "..": + return syserror.ENOTEMPTY + } + if err := fs.MayDelete(t, root, d, name); err != nil { return err } @@ -1829,27 +1836,25 @@ func renameAt(t *kernel.Task, oldDirFD kdefs.FD, oldAddr usermem.Addr, newDirFD return syserror.ENOTDIR } - // Root cannot be renamed to anything. - // - // TODO: This catches the case when the rename - // argument is exactly "/", but we should return EBUSY when - // renaming any mount point, or when the argument is not - // exactly "/" but still resolves to the root, like "/.." or - // "/bin/..". - if oldParent == root && oldName == "." { - return syscall.EBUSY + // Rename rejects paths that end in ".", "..", or empty (i.e. + // the root) with EBUSY. + switch oldName { + case "", ".", "..": + return syserror.EBUSY } + return fileOpAt(t, newDirFD, newPath, func(root *fs.Dirent, newParent *fs.Dirent, newName string) error { if !fs.IsDir(newParent.Inode.StableAttr) { return syserror.ENOTDIR } - // Nothing can be renamed to root. - // - // TODO: Same as above. - if newParent == root && newName == "." { - return syscall.EBUSY + // Rename rejects paths that end in ".", "..", or empty + // (i.e. the root) with EBUSY. + switch newName { + case "", ".", "..": + return syserror.EBUSY } + return fs.Rename(t, root, oldParent, oldName, newParent, newName) }) }) -- cgit v1.2.3