Sage Weil
2010-Oct-25 21:24 UTC
[PATCH] Btrfs: allow subvol deletion by unprivileged user with -o user_subvol_rm_allowed
Add a mount option user_subvol_rm_allowed that allows users to delete a (potentially non-empty!) subvol when they would otherwise we allowed to do an rmdir(2). We duplicate the may_delete() checks from the core VFS code to implement identical security checks (minus the directory size check). Signed-off-by: Sage Weil <sage@newdream.net> --- fs/btrfs/ctree.h | 1 + fs/btrfs/ioctl.c | 109 +++++++++++++++++++++++++++++++++++++++++++++++++++-- fs/btrfs/super.c | 6 ++- 3 files changed, 110 insertions(+), 6 deletions(-) diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h index 5ac2bca..140003e 100644 --- a/fs/btrfs/ctree.h +++ b/fs/btrfs/ctree.h @@ -1196,6 +1196,7 @@ struct btrfs_root { #define BTRFS_MOUNT_NOSSD (1 << 9) #define BTRFS_MOUNT_DISCARD (1 << 10) #define BTRFS_MOUNT_FORCE_COMPRESS (1 << 11) +#define BTRFS_MOUNT_USER_SUBVOL_RM_ALLOWED (1 << 12) #define btrfs_clear_opt(o, opt) ((o) &= ~BTRFS_MOUNT_##opt) #define btrfs_set_opt(o, opt) ((o) |= BTRFS_MOUNT_##opt) diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 906e3b3..919b23f 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -409,6 +409,76 @@ fail: return ret; } +/* copy of check_sticky in fs/namei.c() +* It''s inline, so penalty for filesystems that don''t use sticky bit is +* minimal. +*/ +static inline int btrfs_check_sticky(struct inode *dir, struct inode *inode) +{ + uid_t fsuid = current_fsuid(); + + if (!(dir->i_mode & S_ISVTX)) + return 0; + if (inode->i_uid == fsuid) + return 0; + if (dir->i_uid == fsuid) + return 0; + return !capable(CAP_FOWNER); +} + +/* copy of may_delete in fs/namei.c() + * Check whether we can remove a link victim from directory dir, check + * whether the type of victim is right. + * 1. We can''t do it if dir is read-only (done in permission()) + * 2. We should have write and exec permissions on dir + * 3. We can''t remove anything from append-only dir + * 4. We can''t do anything with immutable dir (done in permission()) + * 5. If the sticky bit on dir is set we should either + * a. be owner of dir, or + * b. be owner of victim, or + * c. have CAP_FOWNER capability + * 6. If the victim is append-only or immutable we can''t do antyhing with + * links pointing to it. + * 7. If we were asked to remove a directory and victim isn''t one - ENOTDIR. + * 8. If we were asked to remove a non-directory and victim isn''t one - EISDIR. + * 9. We can''t remove a root or mountpoint. + * 10. We don''t allow removal of NFS sillyrenamed files; it''s handled by + * nfs_async_unlink(). + */ + +static int btrfs_may_delete(struct inode *dir,struct dentry *victim,int isdir) +{ + int error; + + if (!victim->d_inode) + return -ENOENT; + + BUG_ON(victim->d_parent->d_inode != dir); + audit_inode_child(victim, dir); + + error = inode_permission(dir, MAY_WRITE | MAY_EXEC); + if (error) + return error; + if (IS_APPEND(dir)) + return -EPERM; + if (btrfs_check_sticky(dir, victim->d_inode)|| + IS_APPEND(victim->d_inode)|| + IS_IMMUTABLE(victim->d_inode) || IS_SWAPFILE(victim->d_inode)) + return -EPERM; + if (isdir) { + if (!S_ISDIR(victim->d_inode->i_mode)) + return -ENOTDIR; + if (IS_ROOT(victim)) + return -EBUSY; + } else if (S_ISDIR(victim->d_inode->i_mode)) + return -EISDIR; + if (IS_DEADDIR(dir)) + return -ENOENT; + if (victim->d_flags & DCACHE_NFSFS_RENAMED) + return -EBUSY; + return 0; +} + /* copy of may_create in fs/namei.c() */ static inline int btrfs_may_create(struct inode *dir, struct dentry *child) { @@ -1288,9 +1358,6 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file, int ret; int err = 0; - if (!capable(CAP_SYS_ADMIN)) - return -EPERM; - vol_args = memdup_user(arg, sizeof(*vol_args)); if (IS_ERR(vol_args)) return PTR_ERR(vol_args); @@ -1320,13 +1387,45 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file, } inode = dentry->d_inode; + dest = BTRFS_I(inode)->root; + if (!capable(CAP_SYS_ADMIN)){ + /* + * Regular user. Only allow this with a special mount + * option, 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). + */ + err = -EPERM; + if (!btrfs_test_opt(root, 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; + + /* check if subvolume may be deleted by a non-root user */ + err = btrfs_may_delete(dir, dentry, 1); + if (err) + goto out_dput; + } + if (inode->i_ino != BTRFS_FIRST_FREE_OBJECTID) { err = -EINVAL; goto out_dput; } - dest = BTRFS_I(inode)->root; - mutex_lock(&inode->i_mutex); err = d_invalidate(dentry); if (err) diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c index 4da2680..8ff5a3a 100644 --- a/fs/btrfs/super.c +++ b/fs/btrfs/super.c @@ -68,7 +68,7 @@ enum { Opt_nodatacow, Opt_max_inline, Opt_alloc_start, Opt_nobarrier, Opt_ssd, Opt_nossd, Opt_ssd_spread, Opt_thread_pool, Opt_noacl, Opt_compress, Opt_compress_force, Opt_notreelog, Opt_ratio, Opt_flushoncommit, - Opt_discard, Opt_err, + Opt_discard, Opt_user_subvol_rm_allowed, Opt_err, }; static match_table_t tokens = { @@ -92,6 +92,7 @@ static match_table_t tokens = { {Opt_flushoncommit, "flushoncommit"}, {Opt_ratio, "metadata_ratio=%d"}, {Opt_discard, "discard"}, + {Opt_user_subvol_rm_allowed, "user_subvol_rm_allowed"}, {Opt_err, NULL}, }; @@ -235,6 +236,9 @@ int btrfs_parse_options(struct btrfs_root *root, char *options) case Opt_discard: btrfs_set_opt(info->mount_opt, DISCARD); break; + case Opt_user_subvol_rm_allowed: + btrfs_set_opt(info->mount_opt, USER_SUBVOL_RM_ALLOWED); + break; case Opt_err: printk(KERN_INFO "btrfs: unrecognized mount option " "''%s''\n", p); -- 1.6.6.1 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at vger.kernel.org/majordomo-info.html
Goffredo Baroncelli
2010-Oct-26 17:51 UTC
Re: [PATCH] Btrfs: allow subvol deletion by unprivileged user with -o user_subvol_rm_allowed
Hi sage, On Monday, 25 October, 2010, Sage Weil wrote:> Add a mount option user_subvol_rm_allowed that allows users to delete a > (potentially non-empty!) subvol when they would otherwise we allowed to do > an rmdir(2). We duplicate the may_delete() checks from the core VFS code > to implement identical security checks (minus the directory size check). > > Signed-off-by: Sage Weil <sage@newdream.net> > --- > fs/btrfs/ctree.h | 1 + > fs/btrfs/ioctl.c | 109+++++++++++++++++++++++++++++++++++++++++++++++++++--> fs/btrfs/super.c | 6 ++- > 3 files changed, 110 insertions(+), 6 deletions(-) > > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h > index 5ac2bca..140003e 100644 > --- a/fs/btrfs/ctree.h > +++ b/fs/btrfs/ctree.h > @@ -1196,6 +1196,7 @@ struct btrfs_root { > #define BTRFS_MOUNT_NOSSD (1 << 9) > #define BTRFS_MOUNT_DISCARD (1 << 10) > #define BTRFS_MOUNT_FORCE_COMPRESS (1 << 11) > +#define BTRFS_MOUNT_USER_SUBVOL_RM_ALLOWED (1 << 12) > > #define btrfs_clear_opt(o, opt) ((o) &= ~BTRFS_MOUNT_##opt) > #define btrfs_set_opt(o, opt) ((o) |= BTRFS_MOUNT_##opt) > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index 906e3b3..919b23f 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -409,6 +409,76 @@ fail: > return ret; > } > > +/* copy of check_sticky in fs/namei.c() > +* It''s inline, so penalty for filesystems that don''t use sticky bit is > +* minimal. > +*/ > +static inline int btrfs_check_sticky(struct inode *dir, struct inode*inode)> +{ > + uid_t fsuid = current_fsuid(); > + > + if (!(dir->i_mode & S_ISVTX)) > + return 0; > + if (inode->i_uid == fsuid) > + return 0; > + if (dir->i_uid == fsuid) > + return 0; > + return !capable(CAP_FOWNER); > +} > + > +/* copy of may_delete in fs/namei.c() > + * Check whether we can remove a link victim from directory dir, check > + * whether the type of victim is right. > + * 1. We can''t do it if dir is read-only (done in permission()) > + * 2. We should have write and exec permissions on dir > + * 3. We can''t remove anything from append-only dir > + * 4. We can''t do anything with immutable dir (done in permission()) > + * 5. If the sticky bit on dir is set we should either > + * a. be owner of dir, or > + * b. be owner of victim, or > + * c. have CAP_FOWNER capability > + * 6. If the victim is append-only or immutable we can''t do antyhing with > + * links pointing to it. > + * 7. If we were asked to remove a directory and victim isn''t one -ENOTDIR.> + * 8. If we were asked to remove a non-directory and victim isn''t one -EISDIR.> + * 9. We can''t remove a root or mountpoint. > + * 10. We don''t allow removal of NFS sillyrenamed files; it''s handled by > + * nfs_async_unlink(). > + */ > + > +static int btrfs_may_delete(struct inode *dir,struct dentry *victim,intisdir)> +{ > + int error; > + > + if (!victim->d_inode) > + return -ENOENT; > + > + BUG_ON(victim->d_parent->d_inode != dir); > + audit_inode_child(victim, dir); > + > + error = inode_permission(dir, MAY_WRITE | MAY_EXEC); > + if (error) > + return error; > + if (IS_APPEND(dir)) > + return -EPERM; > + if (btrfs_check_sticky(dir, victim->d_inode)|| > + IS_APPEND(victim->d_inode)|| > + IS_IMMUTABLE(victim->d_inode) || IS_SWAPFILE(victim->d_inode)) > + return -EPERM; > + if (isdir) { > + if (!S_ISDIR(victim->d_inode->i_mode)) > + return -ENOTDIR; > + if (IS_ROOT(victim)) > + return -EBUSY; > + } else if (S_ISDIR(victim->d_inode->i_mode)) > + return -EISDIR; > + if (IS_DEADDIR(dir)) > + return -ENOENT; > + if (victim->d_flags & DCACHE_NFSFS_RENAMED) > + return -EBUSY; > + return 0; > +} > + > /* copy of may_create in fs/namei.c() */ > static inline int btrfs_may_create(struct inode *dir, struct dentry *child) > { > @@ -1288,9 +1358,6 @@ static noinline int btrfs_ioctl_snap_destroy(structfile *file,> int ret; > int err = 0; > > - if (!capable(CAP_SYS_ADMIN)) > - return -EPERM; > - > vol_args = memdup_user(arg, sizeof(*vol_args)); > if (IS_ERR(vol_args)) > return PTR_ERR(vol_args); > @@ -1320,13 +1387,45 @@ static noinline int btrfs_ioctl_snap_destroy(structfile *file,> } > > inode = dentry->d_inode; > + dest = BTRFS_I(inode)->root; > + if (!capable(CAP_SYS_ADMIN)){ > + /* > + * Regular user. Only allow this with a special mount > + * option, 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). > + */ > + err = -EPERM; > + if (!btrfs_test_opt(root, 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; > + > + /* check if subvolume may be deleted by a non-root user */ > + err = btrfs_may_delete(dir, dentry, 1); > + if (err) > + goto out_dput;If I read correctly, an user now is capable to remove a "not owned" subvolume. Is this the intended behavior ? I am not arguing against, but I want to highlight this fact.> + } > + > if (inode->i_ino != BTRFS_FIRST_FREE_OBJECTID) { > err = -EINVAL; > goto out_dput; > } > > - dest = BTRFS_I(inode)->root; > - > mutex_lock(&inode->i_mutex); > err = d_invalidate(dentry); > if (err) > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c > index 4da2680..8ff5a3a 100644 > --- a/fs/btrfs/super.c > +++ b/fs/btrfs/super.c > @@ -68,7 +68,7 @@ enum { > Opt_nodatacow, Opt_max_inline, Opt_alloc_start, Opt_nobarrier,Opt_ssd,> Opt_nossd, Opt_ssd_spread, Opt_thread_pool, Opt_noacl, Opt_compress, > Opt_compress_force, Opt_notreelog, Opt_ratio, Opt_flushoncommit, > - Opt_discard, Opt_err, > + Opt_discard, Opt_user_subvol_rm_allowed, Opt_err, > }; > > static match_table_t tokens = { > @@ -92,6 +92,7 @@ static match_table_t tokens = { > {Opt_flushoncommit, "flushoncommit"}, > {Opt_ratio, "metadata_ratio=%d"}, > {Opt_discard, "discard"}, > + {Opt_user_subvol_rm_allowed, "user_subvol_rm_allowed"}, > {Opt_err, NULL}, > }; > > @@ -235,6 +236,9 @@ int btrfs_parse_options(struct btrfs_root *root, char*options)> case Opt_discard: > btrfs_set_opt(info->mount_opt, DISCARD); > break; > + case Opt_user_subvol_rm_allowed: > + btrfs_set_opt(info->mount_opt,USER_SUBVOL_RM_ALLOWED);> + break; > case Opt_err: > printk(KERN_INFO "btrfs: unrecognized mount option " > "''%s''\n", p); > -- > 1.6.6.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at vger.kernel.org/majordomo-info.html >-- gpg key@ keyserver.linux.it: Goffredo Baroncelli (ghigo) <kreijack@inwind.it> Key fingerprint = 4769 7E51 5293 D36C 814E C054 BF04 F161 3DC5 0512 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at vger.kernel.org/majordomo-info.html
Sage Weil
2010-Oct-26 18:32 UTC
Re: [PATCH] Btrfs: allow subvol deletion by unprivileged user with -o user_subvol_rm_allowed
On Tue, 26 Oct 2010, Goffredo Baroncelli wrote:> > inode = dentry->d_inode; > > + dest = BTRFS_I(inode)->root; > > + if (!capable(CAP_SYS_ADMIN)){ > > + /* > > + * Regular user. Only allow this with a special mount > > + * option, 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). > > + */ > > + err = -EPERM; > > + if (!btrfs_test_opt(root, 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; > > + > > + /* check if subvolume may be deleted by a non-root user */ > > + err = btrfs_may_delete(dir, dentry, 1); > > + if (err) > > + goto out_dput; > > If I read correctly, an user now is capable to remove a "not owned" subvolume. > Is this the intended behavior ? > I am not arguing against, but I want to highlight this fact.Good point. This was mirroring the rmdir(2) checks, but given that we can remove a non-empty subvol, we should add + err = inode_permission(inode, MAY_WRITE | MAY_EXEC); + if (err) + goto out_dput; as well. I''ll resend. Thanks! sage> > > > + } > > + > > if (inode->i_ino != BTRFS_FIRST_FREE_OBJECTID) { > > err = -EINVAL; > > goto out_dput; > > } > > > > - dest = BTRFS_I(inode)->root; > > - > > mutex_lock(&inode->i_mutex); > > err = d_invalidate(dentry); > > if (err) > > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c > > index 4da2680..8ff5a3a 100644 > > --- a/fs/btrfs/super.c > > +++ b/fs/btrfs/super.c > > @@ -68,7 +68,7 @@ enum { > > Opt_nodatacow, Opt_max_inline, Opt_alloc_start, Opt_nobarrier, > Opt_ssd, > > Opt_nossd, Opt_ssd_spread, Opt_thread_pool, Opt_noacl, Opt_compress, > > Opt_compress_force, Opt_notreelog, Opt_ratio, Opt_flushoncommit, > > - Opt_discard, Opt_err, > > + Opt_discard, Opt_user_subvol_rm_allowed, Opt_err, > > }; > > > > static match_table_t tokens = { > > @@ -92,6 +92,7 @@ static match_table_t tokens = { > > {Opt_flushoncommit, "flushoncommit"}, > > {Opt_ratio, "metadata_ratio=%d"}, > > {Opt_discard, "discard"}, > > + {Opt_user_subvol_rm_allowed, "user_subvol_rm_allowed"}, > > {Opt_err, NULL}, > > }; > > > > @@ -235,6 +236,9 @@ int btrfs_parse_options(struct btrfs_root *root, char > *options) > > case Opt_discard: > > btrfs_set_opt(info->mount_opt, DISCARD); > > break; > > + case Opt_user_subvol_rm_allowed: > > + btrfs_set_opt(info->mount_opt, > USER_SUBVOL_RM_ALLOWED); > > + break; > > case Opt_err: > > printk(KERN_INFO "btrfs: unrecognized mount option " > > "''%s''\n", p); > > -- > > 1.6.6.1 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at vger.kernel.org/majordomo-info.html > > > > > -- > gpg key@ keyserver.linux.it: Goffredo Baroncelli (ghigo) <kreijack@inwind.it> > Key fingerprint = 4769 7E51 5293 D36C 814E C054 BF04 F161 3DC5 0512 > >-- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at vger.kernel.org/majordomo-info.html
Maybe Matching Threads
- [PATCH V2] Btrfs: fix subvolume mount by name problem when default mount subvolume is set
- [PATCH 0/6] Btrfs commit fixes, async subvol operations
- [PATCH] btrfs: flushoncommit mount option
- [PATCH] btrfs: rename the option to nospace_cache
- [PATCH] Btrfs: add "nocompress" mount option