Re: [PATCH 1/2] Btrfs: don't check the permission of the subvolume which we want to delete

[Miao Xie <miaox@cn.fujitsu.com>]

On Mon, 22 Oct 2012 19:00:18 +0200, Goffredo Baroncelli wrote:
> On 2012-10-22 13:38, Miao Xie wrote:
>> Step to reproduce:
>>  # mkfs.btrfs <disk>
>>  # mount -o user_subvol_rm_allowed <disk> <mnt>
>>  # mkdir <mnt>/dir0
>>  # chmod 777 <mnt>/dir0
>>  # btrfs sub snap <mnt> <mnt>/dir0/snap0
>>  # su -c "btrfs sub del <mnt>/dir0/snap0" -s /bin/bash nobody
>>  ERROR: cannot delete '<mnt>/dir0/snap0' - Permission denied
>>
>> This is because we checked the permission of the subvolume that we want to
>> delete, and found the user - nobody have no WRITE permission of this subvolume.
>>
>> I think we need not check the permission of the subvolume we want to delete,
>> because we have the right to clean up the directory since we have WRITE and
>> EXECUTE permission, just like rmdir command.
> 
> I think that removing a subvolume is a bit different than removing a
> directory.
> With "user_subvol_rm_allowed" allow an ordinary user to remove  things
> that an plain "rm -rf" is not allowed.
> 
> For example if inside a directories tree there is a directory with
> permission 700 (uid==root) an ordinary user is not able to remove this
> directory.
> Instead with the subvolumes (and the flag user_subvol_rm_allowed) an
> ordinary user would be allowed to do it.
> As mitigation it is required that user has WX permission on subvolume.
> IIRC this is what we discussed at time.
> 
> See the thread "[PATCH] Btrfs: allow subvol deletion by unprivileged
> user with -o user_subvol_rm_allowed"
> 
> http://www.mail-archive.com/linux-btrfs@vger.kernel.org/msg06525.html

I don't think this mitigation is reasonable, it will make the user be
confused. As I said in another mail:

>> I don't think we can identify "btrfs sub del" with "rm -rf", because "rm -rf"
>> will check the permission of the parent directory of each file/directory which
>> is going to be deleted, but "btrfs sub del" doesn't do it, it will see all the
>> file/directory in the subvolume as one, so I think it seems like a special
>> "rmdir".

So I think we needn't the permission of the subvolume which is going to be deleted.

Thanks
Miao

> 
> 
>>
>> Signed-off-by: Miao Xie <miaox@cn.fujitsu.com>
>> ---
>>  fs/btrfs/ioctl.c |    4 ----
>>  1 files changed, 0 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index f5a2e6c..29fb07c 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -2062,10 +2062,6 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
>>  		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 non-root user */
>>  		err = btrfs_may_delete(dir, dentry, 1);
>>  		if (err)
> 
>