Skip to content

Commit

Permalink
Kernfs should not try to rename a file to itself.
Browse files Browse the repository at this point in the history
One precondition of VFS.PrepareRenameAt is that the `from` and `to` dentries
are not the same. Kernfs was not checking this, which could lead to a deadlock.

PiperOrigin-RevId: 359385974
  • Loading branch information
nlacasse authored and gvisor-bot committed Feb 24, 2021
1 parent 303c913 commit f5692f7
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 9 deletions.
23 changes: 14 additions & 9 deletions pkg/sentry/fsimpl/kernfs/filesystem.go
Original file line number Diff line number Diff line change
Expand Up @@ -668,28 +668,33 @@ func (fs *Filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa

// Can we create the dst dentry?
var dst *Dentry
pc := rp.Component()
if pc == "." || pc == ".." {
newName := rp.Component()
if newName == "." || newName == ".." {
if noReplace {
return syserror.EEXIST
}
return syserror.EBUSY
}
switch err := checkCreateLocked(ctx, rp.Credentials(), pc, dstDir); err {
switch err := checkCreateLocked(ctx, rp.Credentials(), newName, dstDir); err {
case nil:
// Ok, continue with rename as replacement.
case syserror.EEXIST:
if noReplace {
// Won't overwrite existing node since RENAME_NOREPLACE was requested.
return syserror.EEXIST
}
dst = dstDir.children[pc]
dst = dstDir.children[newName]
if dst == nil {
panic(fmt.Sprintf("Child %q for parent Dentry %+v disappeared inside atomic section?", pc, dstDir))
panic(fmt.Sprintf("Child %q for parent Dentry %+v disappeared inside atomic section?", newName, dstDir))
}
default:
return err
}

if srcDir == dstDir && oldName == newName {
return nil
}

var dstVFSD *vfs.Dentry
if dst != nil {
dstVFSD = dst.VFSDentry()
Expand All @@ -712,7 +717,7 @@ func (fs *Filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa
if err := virtfs.PrepareRenameDentry(mntns, srcVFSD, dstVFSD); err != nil {
return err
}
err = srcDir.inode.Rename(ctx, src.name, pc, src.inode, dstDir.inode)
err = srcDir.inode.Rename(ctx, src.name, newName, src.inode, dstDir.inode)
if err != nil {
virtfs.AbortRenameDentry(srcVFSD, dstVFSD)
return err
Expand All @@ -723,12 +728,12 @@ func (fs *Filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa
dstDir.IncRef() // child (src) takes a ref on the new parent.
}
src.parent = dstDir
src.name = pc
src.name = newName
if dstDir.children == nil {
dstDir.children = make(map[string]*Dentry)
}
replaced := dstDir.children[pc]
dstDir.children[pc] = src
replaced := dstDir.children[newName]
dstDir.children[newName] = src
var replaceVFSD *vfs.Dentry
if replaced != nil {
// deferDecRef so that fs.mu and dstDir.mu are unlocked by then.
Expand Down
14 changes: 14 additions & 0 deletions test/syscalls/linux/rename.cc
Original file line number Diff line number Diff line change
Expand Up @@ -424,6 +424,20 @@ TEST(RenameTest, SysfsPathEndingWithDots) {
SyscallFailsWithErrno(EBUSY));
}

TEST(RenameTest, SysfsFileToSelf) {
// If a non-root user tries to rename inside /sys then we get EPERM.
SKIP_IF(geteuid() != 0);
std::string const path = "/sys/devices/system/cpu/online";
EXPECT_THAT(rename(path.c_str(), path.c_str()), SyscallSucceeds());
}

TEST(RenameTest, SysfsDirectoryToSelf) {
// If a non-root user tries to rename inside /sys then we get EPERM.
SKIP_IF(geteuid() != 0);
std::string const path = "/sys/devices";
EXPECT_THAT(rename(path.c_str(), path.c_str()), SyscallSucceeds());
}

} // namespace

} // namespace testing
Expand Down

0 comments on commit f5692f7

Please sign in to comment.