| Message ID | 20200807215954.697124-1-boris@bur.io (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: relax conditions on user subvolume delete | expand |
Hi Boris,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on kdave/for-next]
[also build test WARNING on v5.8 next-20200807]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Boris-Burkov/btrfs-relax-conditions-on-user-subvolume-delete/20200808-060125
base: https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next
config: i386-randconfig-s001-20200807 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.2-118-ge1578773-dirty
# save the attached .config to linux build tree
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from fs/btrfs/ioctl.c:29:
fs/btrfs/ctree.h:2265:8: warning: type qualifiers ignored on function return type [-Wignored-qualifiers]
2265 | size_t __const btrfs_get_num_csums(void);
| ^~~~~~~
In file included from fs/btrfs/ioctl.c:43:
fs/btrfs/sysfs.h:16:1: warning: type qualifiers ignored on function return type [-Wignored-qualifiers]
16 | const char * const btrfs_feature_set_name(enum btrfs_feature_set set);
| ^~~~~
fs/btrfs/ioctl.c: In function 'btrfs_ioctl_snap_destroy':
>> fs/btrfs/ioctl.c:2838:21: warning: variable 'dest' set but not used [-Wunused-but-set-variable]
2838 | struct btrfs_root *dest = NULL;
| ^~~~
vim +/dest +2838 fs/btrfs/ioctl.c
42e4b520c812da Tomohiro Misono 2018-05-21 2828
76dda93c6ae2c1 Yan, Zheng 2009-09-21 2829 static noinline int btrfs_ioctl_snap_destroy(struct file *file,
949964c928430a Marcos Paulo de Souza 2020-02-07 2830 void __user *arg,
949964c928430a Marcos Paulo de Souza 2020-02-07 2831 bool destroy_v2)
76dda93c6ae2c1 Yan, Zheng 2009-09-21 2832 {
54563d41a58be7 Al Viro 2013-09-01 2833 struct dentry *parent = file->f_path.dentry;
0b246afa62b0cf Jeff Mahoney 2016-06-22 2834 struct btrfs_fs_info *fs_info = btrfs_sb(parent->d_sb);
76dda93c6ae2c1 Yan, Zheng 2009-09-21 2835 struct dentry *dentry;
2b0143b5c986be David Howells 2015-03-17 2836 struct inode *dir = d_inode(parent);
76dda93c6ae2c1 Yan, Zheng 2009-09-21 2837 struct inode *inode;
76dda93c6ae2c1 Yan, Zheng 2009-09-21 @2838 struct btrfs_root *dest = NULL;
949964c928430a Marcos Paulo de Souza 2020-02-07 2839 struct btrfs_ioctl_vol_args *vol_args = NULL;
949964c928430a Marcos Paulo de Souza 2020-02-07 2840 struct btrfs_ioctl_vol_args_v2 *vol_args2 = NULL;
949964c928430a Marcos Paulo de Souza 2020-02-07 2841 char *subvol_name, *subvol_name_ptr = NULL;
949964c928430a Marcos Paulo de Souza 2020-02-07 2842 int subvol_namelen;
76dda93c6ae2c1 Yan, Zheng 2009-09-21 2843 int err = 0;
949964c928430a Marcos Paulo de Souza 2020-02-07 2844 bool destroy_parent = false;
76dda93c6ae2c1 Yan, Zheng 2009-09-21 2845
949964c928430a Marcos Paulo de Souza 2020-02-07 2846 if (destroy_v2) {
949964c928430a Marcos Paulo de Souza 2020-02-07 2847 vol_args2 = memdup_user(arg, sizeof(*vol_args2));
949964c928430a Marcos Paulo de Souza 2020-02-07 2848 if (IS_ERR(vol_args2))
949964c928430a Marcos Paulo de Souza 2020-02-07 2849 return PTR_ERR(vol_args2);
325c50e3cebb92 Jeff Mahoney 2016-09-21 2850
949964c928430a Marcos Paulo de Souza 2020-02-07 2851 if (vol_args2->flags & ~BTRFS_SUBVOL_DELETE_ARGS_MASK) {
949964c928430a Marcos Paulo de Souza 2020-02-07 2852 err = -EOPNOTSUPP;
949964c928430a Marcos Paulo de Souza 2020-02-07 2853 goto out;
949964c928430a Marcos Paulo de Souza 2020-02-07 2854 }
76dda93c6ae2c1 Yan, Zheng 2009-09-21 2855
949964c928430a Marcos Paulo de Souza 2020-02-07 2856 /*
949964c928430a Marcos Paulo de Souza 2020-02-07 2857 * If SPEC_BY_ID is not set, we are looking for the subvolume by
949964c928430a Marcos Paulo de Souza 2020-02-07 2858 * name, same as v1 currently does.
949964c928430a Marcos Paulo de Souza 2020-02-07 2859 */
949964c928430a Marcos Paulo de Souza 2020-02-07 2860 if (!(vol_args2->flags & BTRFS_SUBVOL_SPEC_BY_ID)) {
949964c928430a Marcos Paulo de Souza 2020-02-07 2861 vol_args2->name[BTRFS_SUBVOL_NAME_MAX] = 0;
949964c928430a Marcos Paulo de Souza 2020-02-07 2862 subvol_name = vol_args2->name;
949964c928430a Marcos Paulo de Souza 2020-02-07 2863
949964c928430a Marcos Paulo de Souza 2020-02-07 2864 err = mnt_want_write_file(file);
949964c928430a Marcos Paulo de Souza 2020-02-07 2865 if (err)
949964c928430a Marcos Paulo de Souza 2020-02-07 2866 goto out;
949964c928430a Marcos Paulo de Souza 2020-02-07 2867 } else {
949964c928430a Marcos Paulo de Souza 2020-02-07 2868 if (vol_args2->subvolid < BTRFS_FIRST_FREE_OBJECTID) {
76dda93c6ae2c1 Yan, Zheng 2009-09-21 2869 err = -EINVAL;
76dda93c6ae2c1 Yan, Zheng 2009-09-21 2870 goto out;
76dda93c6ae2c1 Yan, Zheng 2009-09-21 2871 }
76dda93c6ae2c1 Yan, Zheng 2009-09-21 2872
a561be7100cd61 Al Viro 2011-11-23 2873 err = mnt_want_write_file(file);
76dda93c6ae2c1 Yan, Zheng 2009-09-21 2874 if (err)
76dda93c6ae2c1 Yan, Zheng 2009-09-21 2875 goto out;
76dda93c6ae2c1 Yan, Zheng 2009-09-21 2876
949964c928430a Marcos Paulo de Souza 2020-02-07 2877 dentry = btrfs_get_dentry(fs_info->sb,
949964c928430a Marcos Paulo de Souza 2020-02-07 2878 BTRFS_FIRST_FREE_OBJECTID,
949964c928430a Marcos Paulo de Souza 2020-02-07 2879 vol_args2->subvolid, 0, 0);
949964c928430a Marcos Paulo de Souza 2020-02-07 2880 if (IS_ERR(dentry)) {
949964c928430a Marcos Paulo de Souza 2020-02-07 2881 err = PTR_ERR(dentry);
949964c928430a Marcos Paulo de Souza 2020-02-07 2882 goto out_drop_write;
949964c928430a Marcos Paulo de Souza 2020-02-07 2883 }
949964c928430a Marcos Paulo de Souza 2020-02-07 2884
949964c928430a Marcos Paulo de Souza 2020-02-07 2885 /*
949964c928430a Marcos Paulo de Souza 2020-02-07 2886 * Change the default parent since the subvolume being
949964c928430a Marcos Paulo de Souza 2020-02-07 2887 * deleted can be outside of the current mount point.
949964c928430a Marcos Paulo de Souza 2020-02-07 2888 */
949964c928430a Marcos Paulo de Souza 2020-02-07 2889 parent = btrfs_get_parent(dentry);
949964c928430a Marcos Paulo de Souza 2020-02-07 2890
949964c928430a Marcos Paulo de Souza 2020-02-07 2891 /*
949964c928430a Marcos Paulo de Souza 2020-02-07 2892 * At this point dentry->d_name can point to '/' if the
949964c928430a Marcos Paulo de Souza 2020-02-07 2893 * subvolume we want to destroy is outsite of the
949964c928430a Marcos Paulo de Souza 2020-02-07 2894 * current mount point, so we need to release the
949964c928430a Marcos Paulo de Souza 2020-02-07 2895 * current dentry and execute the lookup to return a new
949964c928430a Marcos Paulo de Souza 2020-02-07 2896 * one with ->d_name pointing to the
949964c928430a Marcos Paulo de Souza 2020-02-07 2897 * <mount point>/subvol_name.
949964c928430a Marcos Paulo de Souza 2020-02-07 2898 */
949964c928430a Marcos Paulo de Souza 2020-02-07 2899 dput(dentry);
949964c928430a Marcos Paulo de Souza 2020-02-07 2900 if (IS_ERR(parent)) {
949964c928430a Marcos Paulo de Souza 2020-02-07 2901 err = PTR_ERR(parent);
949964c928430a Marcos Paulo de Souza 2020-02-07 2902 goto out_drop_write;
949964c928430a Marcos Paulo de Souza 2020-02-07 2903 }
949964c928430a Marcos Paulo de Souza 2020-02-07 2904 dir = d_inode(parent);
949964c928430a Marcos Paulo de Souza 2020-02-07 2905
949964c928430a Marcos Paulo de Souza 2020-02-07 2906 /*
949964c928430a Marcos Paulo de Souza 2020-02-07 2907 * If v2 was used with SPEC_BY_ID, a new parent was
949964c928430a Marcos Paulo de Souza 2020-02-07 2908 * allocated since the subvolume can be outside of the
949964c928430a Marcos Paulo de Souza 2020-02-07 2909 * current mount point. Later on we need to release this
949964c928430a Marcos Paulo de Souza 2020-02-07 2910 * new parent dentry.
949964c928430a Marcos Paulo de Souza 2020-02-07 2911 */
949964c928430a Marcos Paulo de Souza 2020-02-07 2912 destroy_parent = true;
949964c928430a Marcos Paulo de Souza 2020-02-07 2913
949964c928430a Marcos Paulo de Souza 2020-02-07 2914 subvol_name_ptr = btrfs_get_subvol_name_from_objectid(
949964c928430a Marcos Paulo de Souza 2020-02-07 2915 fs_info, vol_args2->subvolid);
949964c928430a Marcos Paulo de Souza 2020-02-07 2916 if (IS_ERR(subvol_name_ptr)) {
949964c928430a Marcos Paulo de Souza 2020-02-07 2917 err = PTR_ERR(subvol_name_ptr);
949964c928430a Marcos Paulo de Souza 2020-02-07 2918 goto free_parent;
949964c928430a Marcos Paulo de Souza 2020-02-07 2919 }
949964c928430a Marcos Paulo de Souza 2020-02-07 2920 /* subvol_name_ptr is already NULL termined */
949964c928430a Marcos Paulo de Souza 2020-02-07 2921 subvol_name = (char *)kbasename(subvol_name_ptr);
949964c928430a Marcos Paulo de Souza 2020-02-07 2922 }
949964c928430a Marcos Paulo de Souza 2020-02-07 2923 } else {
949964c928430a Marcos Paulo de Souza 2020-02-07 2924 vol_args = memdup_user(arg, sizeof(*vol_args));
949964c928430a Marcos Paulo de Souza 2020-02-07 2925 if (IS_ERR(vol_args))
949964c928430a Marcos Paulo de Souza 2020-02-07 2926 return PTR_ERR(vol_args);
949964c928430a Marcos Paulo de Souza 2020-02-07 2927
949964c928430a Marcos Paulo de Souza 2020-02-07 2928 vol_args->name[BTRFS_PATH_NAME_MAX] = 0;
949964c928430a Marcos Paulo de Souza 2020-02-07 2929 subvol_name = vol_args->name;
949964c928430a Marcos Paulo de Souza 2020-02-07 2930
949964c928430a Marcos Paulo de Souza 2020-02-07 2931 err = mnt_want_write_file(file);
949964c928430a Marcos Paulo de Souza 2020-02-07 2932 if (err)
949964c928430a Marcos Paulo de Souza 2020-02-07 2933 goto out;
949964c928430a Marcos Paulo de Souza 2020-02-07 2934 }
949964c928430a Marcos Paulo de Souza 2020-02-07 2935
949964c928430a Marcos Paulo de Souza 2020-02-07 2936 subvol_namelen = strlen(subvol_name);
949964c928430a Marcos Paulo de Souza 2020-02-07 2937
949964c928430a Marcos Paulo de Souza 2020-02-07 2938 if (strchr(subvol_name, '/') ||
949964c928430a Marcos Paulo de Souza 2020-02-07 2939 strncmp(subvol_name, "..", subvol_namelen) == 0) {
949964c928430a Marcos Paulo de Souza 2020-02-07 2940 err = -EINVAL;
949964c928430a Marcos Paulo de Souza 2020-02-07 2941 goto free_subvol_name;
949964c928430a Marcos Paulo de Souza 2020-02-07 2942 }
949964c928430a Marcos Paulo de Souza 2020-02-07 2943
949964c928430a Marcos Paulo de Souza 2020-02-07 2944 if (!S_ISDIR(dir->i_mode)) {
949964c928430a Marcos Paulo de Souza 2020-02-07 2945 err = -ENOTDIR;
949964c928430a Marcos Paulo de Souza 2020-02-07 2946 goto free_subvol_name;
949964c928430a Marcos Paulo de Souza 2020-02-07 2947 }
521e0546c970c3 David Sterba 2014-04-15 2948
002354112f1e3c Al Viro 2016-05-26 2949 err = down_write_killable_nested(&dir->i_rwsem, I_MUTEX_PARENT);
002354112f1e3c Al Viro 2016-05-26 2950 if (err == -EINTR)
949964c928430a Marcos Paulo de Souza 2020-02-07 2951 goto free_subvol_name;
949964c928430a Marcos Paulo de Souza 2020-02-07 2952 dentry = lookup_one_len(subvol_name, parent, subvol_namelen);
76dda93c6ae2c1 Yan, Zheng 2009-09-21 2953 if (IS_ERR(dentry)) {
76dda93c6ae2c1 Yan, Zheng 2009-09-21 2954 err = PTR_ERR(dentry);
76dda93c6ae2c1 Yan, Zheng 2009-09-21 2955 goto out_unlock_dir;
76dda93c6ae2c1 Yan, Zheng 2009-09-21 2956 }
76dda93c6ae2c1 Yan, Zheng 2009-09-21 2957
2b0143b5c986be David Howells 2015-03-17 2958 if (d_really_is_negative(dentry)) {
76dda93c6ae2c1 Yan, Zheng 2009-09-21 2959 err = -ENOENT;
76dda93c6ae2c1 Yan, Zheng 2009-09-21 2960 goto out_dput;
76dda93c6ae2c1 Yan, Zheng 2009-09-21 2961 }
76dda93c6ae2c1 Yan, Zheng 2009-09-21 2962
2b0143b5c986be David Howells 2015-03-17 2963 inode = d_inode(dentry);
4260f7c7516f4c Sage Weil 2010-10-29 2964 dest = BTRFS_I(inode)->root;
4260f7c7516f4c Sage Weil 2010-10-29 2965 if (!capable(CAP_SYS_ADMIN)) {
42e1d11143844c Boris Burkov 2020-08-07 2966 /* Regular user. Only allow this with a special mount option */
4260f7c7516f4c Sage Weil 2010-10-29 2967 err = -EPERM;
0b246afa62b0cf Jeff Mahoney 2016-06-22 2968 if (!btrfs_test_opt(fs_info, USER_SUBVOL_RM_ALLOWED))
4260f7c7516f4c Sage Weil 2010-10-29 2969 goto out_dput;
5c39da5b6ca23e Miao Xie 2012-10-22 2970 }
4260f7c7516f4c Sage Weil 2010-10-29 2971
5c39da5b6ca23e Miao Xie 2012-10-22 2972 /* check if subvolume may be deleted by a user */
4260f7c7516f4c Sage Weil 2010-10-29 2973 err = btrfs_may_delete(dir, dentry, 1);
4260f7c7516f4c Sage Weil 2010-10-29 2974 if (err)
76dda93c6ae2c1 Yan, Zheng 2009-09-21 2975 goto out_dput;
76dda93c6ae2c1 Yan, Zheng 2009-09-21 2976
4a0cc7ca6c40b6 Nikolay Borisov 2017-01-10 2977 if (btrfs_ino(BTRFS_I(inode)) != BTRFS_FIRST_FREE_OBJECTID) {
4260f7c7516f4c Sage Weil 2010-10-29 2978 err = -EINVAL;
4260f7c7516f4c Sage Weil 2010-10-29 2979 goto out_dput;
4260f7c7516f4c Sage Weil 2010-10-29 2980 }
76dda93c6ae2c1 Yan, Zheng 2009-09-21 2981
5955102c9984fa Al Viro 2016-01-22 2982 inode_lock(inode);
f60a2364a4eee4 Misono Tomohiro 2018-04-18 2983 err = btrfs_delete_subvolume(dir, dentry);
5955102c9984fa Al Viro 2016-01-22 2984 inode_unlock(inode);
46008d9d3f0ef7 Amir Goldstein 2019-05-26 2985 if (!err) {
46008d9d3f0ef7 Amir Goldstein 2019-05-26 2986 fsnotify_rmdir(dir, dentry);
76dda93c6ae2c1 Yan, Zheng 2009-09-21 2987 d_delete(dentry);
46008d9d3f0ef7 Amir Goldstein 2019-05-26 2988 }
fa6ac8765c48a0 Liu Bo 2013-02-20 2989
76dda93c6ae2c1 Yan, Zheng 2009-09-21 2990 out_dput:
76dda93c6ae2c1 Yan, Zheng 2009-09-21 2991 dput(dentry);
76dda93c6ae2c1 Yan, Zheng 2009-09-21 2992 out_unlock_dir:
5955102c9984fa Al Viro 2016-01-22 2993 inode_unlock(dir);
949964c928430a Marcos Paulo de Souza 2020-02-07 2994 free_subvol_name:
949964c928430a Marcos Paulo de Souza 2020-02-07 2995 kfree(subvol_name_ptr);
949964c928430a Marcos Paulo de Souza 2020-02-07 2996 free_parent:
949964c928430a Marcos Paulo de Souza 2020-02-07 2997 if (destroy_parent)
949964c928430a Marcos Paulo de Souza 2020-02-07 2998 dput(parent);
002354112f1e3c Al Viro 2016-05-26 2999 out_drop_write:
2a79f17e4a641a Al Viro 2011-12-09 3000 mnt_drop_write_file(file);
76dda93c6ae2c1 Yan, Zheng 2009-09-21 3001 out:
949964c928430a Marcos Paulo de Souza 2020-02-07 3002 kfree(vol_args2);
76dda93c6ae2c1 Yan, Zheng 2009-09-21 3003 kfree(vol_args);
76dda93c6ae2c1 Yan, Zheng 2009-09-21 3004 return err;
76dda93c6ae2c1 Yan, Zheng 2009-09-21 3005 }
76dda93c6ae2c1 Yan, Zheng 2009-09-21 3006
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
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 */
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(-)