[PATCH v3 2/2] f2fs: fix setattr project check upon fssetxattr ioctl

[PATCH v3 2/2] f2fs: fix setattr project check upon fssetxattr ioctl

[Wang Shilong]

From: Wang Shilong <wangshilong1991@gmail.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@ddn.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)
+{
+	/*
+	 * 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

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

[Chao Yu]

On 2018/9/11 7:54, Wang Shilong wrote:
> From: Wang Shilong <wangshilong1991@gmail.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@ddn.com>
> ---
> v2->v3: remove inode_lock() and mnt_want_write_file()
> inside f2fs_ioc_setproject()

I missed to check that, my bad.

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,

> 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)
> +{
> +	/*
> +	 * 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)
> 

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

[Jaegeuk Kim]

On 09/11, Wang Shilong wrote:
> From: Wang Shilong <wangshilong1991@gmail.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@ddn.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_...

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@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

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

[Wang Shilong]

> 在 2018年9月12日，上午6:47，Jaegeuk Kim <jaegeuk@kernel.org> 写道：
> 
> On 09/11, Wang Shilong wrote:
>> From: Wang Shilong <wangshilong1991@gmail.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@ddn.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@lists.sourceforge.net
>> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel