public inbox for linux-btrfs@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] btrfs: relax conditions on user subvolume delete
@ 2020-08-07 21:59 Boris Burkov
  2020-08-08  0:31 ` kernel test robot
  0 siblings, 1 reply; 3+ messages in thread
From: Boris Burkov @ 2020-08-07 21:59 UTC (permalink / raw)
  To: Chris Mason, Josef Bacik, David Sterba
  Cc: linux-btrfs, kernel-team, Boris Burkov

With the user_subvol_rm_allowed mount flag set, it is possible for non
root users to delete subvolumes. Currently, the conditions to do so
diverge from the conditions for root. Specifically, there is an
additional check on the ability to write to the subvolume to be deleted,
not just its containing directory. As a result, we see weird behavior
like root being able to delete a read only subvolume it doesn't have
write permissions for, but a user not being able to delete a read only
subvolume it does otherwise have write permissions for.

Furthermore, the existing comments make allusions to rmdir being allowed
as a standard, and rmdir only checks permissions on the containing
directory, as in the root case.

To streamline this, make user subvolume deletes with the mount flag set
match the behavior for root. For increased security, it is simple to
just mount without user_subvol_rm_allowed.

To put it plainly, the risk is that a non-root user who has write access
to a parent directory will be able to delete a subvolume in the parent,
even if they don't have write access to the subvolume. Currently, only
root can do that.

Signed-off-by: Boris Burkov <boris@bur.io>
---
 fs/btrfs/ioctl.c | 30 +-----------------------------
 1 file changed, 1 insertion(+), 29 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index bd3511c5ca81..475b99241564 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2835,7 +2835,6 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
 	struct dentry *dentry;
 	struct inode *dir = d_inode(parent);
 	struct inode *inode;
-	struct btrfs_root *root = BTRFS_I(dir)->root;
 	struct btrfs_root *dest = NULL;
 	struct btrfs_ioctl_vol_args *vol_args = NULL;
 	struct btrfs_ioctl_vol_args_v2 *vol_args2 = NULL;
@@ -2964,37 +2963,10 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
 	inode = d_inode(dentry);
 	dest = BTRFS_I(inode)->root;
 	if (!capable(CAP_SYS_ADMIN)) {
-		/*
-		 * Regular user.  Only allow this with a special mount
-		 * option, when the user has write+exec access to the
-		 * subvol root, and when rmdir(2) would have been
-		 * allowed.
-		 *
-		 * Note that this is _not_ check that the subvol is
-		 * empty or doesn't contain data that we wouldn't
-		 * otherwise be able to delete.
-		 *
-		 * Users who want to delete empty subvols should try
-		 * rmdir(2).
-		 */
+		/* Regular user.  Only allow this with a special mount option */
 		err = -EPERM;
 		if (!btrfs_test_opt(fs_info, USER_SUBVOL_RM_ALLOWED))
 			goto out_dput;
-
-		/*
-		 * Do not allow deletion if the parent dir is the same
-		 * as the dir to be deleted.  That means the ioctl
-		 * must be called on the dentry referencing the root
-		 * of the subvol, not a random directory contained
-		 * within it.
-		 */
-		err = -EINVAL;
-		if (root == dest)
-			goto out_dput;
-
-		err = inode_permission(inode, MAY_WRITE | MAY_EXEC);
-		if (err)
-			goto out_dput;
 	}
 
 	/* check if subvolume may be deleted by a user */
-- 
2.24.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-08-10 17:23 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-08-07 21:59 [PATCH] btrfs: relax conditions on user subvolume delete Boris Burkov
2020-08-08  0:31 ` kernel test robot
2020-08-10 17:22   ` [PATCH v2] " Boris Burkov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox