linux-ext4 - Re: [f2fs-dev] [PATCH v3 2/2] f2fs: fix setattr project check upon fssetxattr ioctl

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <84A27250-3DEB-4E64-AA85-9C67915FD34C@ddn.com>
Date:   Wed, 12 Sep 2018 00:00:06 +0000
From:   Wang Shilong <wshilong@....com>
To:     Jaegeuk Kim <jaegeuk@...nel.org>
CC:     Wang Shilong <wangshilong1991@...il.com>,
        "linux-ext4@...r.kernel.org" <linux-ext4@...r.kernel.org>,
        "linux-f2fs-devel@...ts.sourceforge.net" 
        <linux-f2fs-devel@...ts.sourceforge.net>,
        Andreas Dilger <adilger@...ger.ca>,
        "dchinner@...hat.com" <dchinner@...hat.com>
Subject: Re: [f2fs-dev] [PATCH v3 2/2] f2fs: fix setattr project check upon
 fssetxattr ioctl



> 在 2018年9月12日，上午6:47，Jaegeuk Kim <jaegeuk@...nel.org> 写道：
> 
> On 09/11, Wang Shilong wrote:
>> From: Wang Shilong <wangshilong1991@...il.com>
>> 
>> Currently, project quota could be changed by fssetxattr
>> ioctl, and existed permission check inode_owner_or_capable()
>> is obviously not enough, just think that common users could
>> change project id of file, that could make users to
>> break project quota easily.
>> 
>> This patch try to follow same regular of xfs project
>> quota:
>> 
>> "Project Quota ID state is only allowed to change from
>> within the init namespace. Enforce that restriction only
>> if we are trying to change the quota ID state.
>> Everything else is allowed in user namespaces."
>> 
>> Besides that, check and set project id'state should
>> be an atomic operation, protect whole operation with
>> inode lock.
>> 
>> Signed-off-by: Wang Shilong <wshilong@....com>
>> ---
>> v2->v3: remove inode_lock() and mnt_want_write_file()
>> inside f2fs_ioc_setproject()
>> v1->v2: rename f2fs_ioctl_setattr_check_projid()
>> to f2fs_ioctl_check_project()
>> fs/f2fs/file.c | 61 ++++++++++++++++++++++++++++++++++++----------------------
>> 1 file changed, 38 insertions(+), 23 deletions(-)
>> 
>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>> index 5474aaa..f5170e6 100644
>> --- a/fs/f2fs/file.c
>> +++ b/fs/f2fs/file.c
>> @@ -2617,34 +2617,26 @@ static int f2fs_ioc_setproject(struct file *filp, __u32 projid)
>> 	if (projid_eq(kprojid, F2FS_I(inode)->i_projid))
>> 		return 0;
>> 
>> -	err = mnt_want_write_file(filp);
>> -	if (err)
>> -		return err;
>> -
>> 	err = -EPERM;
>> -	inode_lock(inode);
>> -
>> 	/* Is it quota file? Do not allow user to mess with it */
>> 	if (IS_NOQUOTA(inode))
>> -		goto out_unlock;
>> +		return err;
>> 
>> 	ipage = f2fs_get_node_page(sbi, inode->i_ino);
>> -	if (IS_ERR(ipage)) {
>> -		err = PTR_ERR(ipage);
>> -		goto out_unlock;
>> -	}
>> +	if (IS_ERR(ipage))
>> +		return PTR_ERR(ipage);
>> 
>> 	if (!F2FS_FITS_IN_INODE(F2FS_INODE(ipage), fi->i_extra_isize,
>> 								i_projid)) {
>> 		err = -EOVERFLOW;
>> 		f2fs_put_page(ipage, 1);
>> -		goto out_unlock;
>> +		return err;
>> 	}
>> 	f2fs_put_page(ipage, 1);
>> 
>> 	err = dquot_initialize(inode);
>> 	if (err)
>> -		goto out_unlock;
>> +		return err;
>> 
>> 	transfer_to[PRJQUOTA] = dqget(sb, make_kqid_projid(kprojid));
>> 	if (!IS_ERR(transfer_to[PRJQUOTA])) {
>> @@ -2658,9 +2650,6 @@ static int f2fs_ioc_setproject(struct file *filp, __u32 projid)
>> 	inode->i_ctime = current_time(inode);
>> out_dirty:
>> 	f2fs_mark_inode_dirty_sync(inode, true);
>> -out_unlock:
>> -	inode_unlock(inode);
>> -	mnt_drop_write_file(filp);
>> 	return err;
>> }
>> #else
>> @@ -2736,6 +2725,31 @@ static int f2fs_ioc_fsgetxattr(struct file *filp, unsigned long arg)
>> 	return 0;
>> }
>> 
>> +static int
>> +f2fs_ioctl_check_project(struct inode *inode, struct fsxattr *fa)
> 
> We're not following this coding style. Let me allow to declare like this.
> 
> static int f2fs_ioctl_…

Hi JK,

   Good point, I followed Lustre codes styles which was a bad
Example of this…LOL.

Thanks,
Shilong

> 
> Thanks,
> 
>> +{
>> +	/*
>> +	 * Project Quota ID state is only allowed to change from within the init
>> +	 * namespace. Enforce that restriction only if we are trying to change
>> +	 * the quota ID state. Everything else is allowed in user namespaces.
>> +	 */
>> +	if (current_user_ns() == &init_user_ns)
>> +		return 0;
>> +
>> +	if (__kprojid_val(F2FS_I(inode)->i_projid) != fa->fsx_projid)
>> +		return -EINVAL;
>> +
>> +	if (F2FS_I(inode)->i_flags & F2FS_PROJINHERIT_FL) {
>> +		if (!(fa->fsx_xflags & FS_XFLAG_PROJINHERIT))
>> +			return -EINVAL;
>> +	} else {
>> +		if (fa->fsx_xflags & FS_XFLAG_PROJINHERIT)
>> +			return -EINVAL;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> static int f2fs_ioc_fssetxattr(struct file *filp, unsigned long arg)
>> {
>> 	struct inode *inode = file_inode(filp);
>> @@ -2763,19 +2777,20 @@ static int f2fs_ioc_fssetxattr(struct file *filp, unsigned long arg)
>> 		return err;
>> 
>> 	inode_lock(inode);
>> +	err = f2fs_ioctl_check_project(inode, &fa);
>> +	if (err)
>> +		goto out;
>> 	flags = (fi->i_flags & ~F2FS_FL_XFLAG_VISIBLE) |
>> 				(flags & F2FS_FL_XFLAG_VISIBLE);
>> 	err = __f2fs_ioc_setflags(inode, flags);
>> -	inode_unlock(inode);
>> -	mnt_drop_write_file(filp);
>> 	if (err)
>> -		return err;
>> +		goto out;
>> 
>> 	err = f2fs_ioc_setproject(filp, fa.fsx_projid);
>> -	if (err)
>> -		return err;
>> -
>> -	return 0;
>> +out:
>> +	inode_unlock(inode);
>> +	mnt_drop_write_file(filp);
>> +	return err;
>> }
>> 
>> int f2fs_pin_file_control(struct inode *inode, bool inc)
>> -- 
>> 1.8.3.1
>> 
>> 
>> 
>> _______________________________________________
>> Linux-f2fs-devel mailing list
>> Linux-f2fs-devel@...ts.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel