diff mbox series

[v3,1/3] ext4: fix setattr project check upon fssetxattr ioctl

Message ID 1536710238-16858-1-git-send-email-wshilong1991@gmail.com
State Superseded
Headers show
Series [v3,1/3] ext4: fix setattr project check upon fssetxattr ioctl | expand

Commit Message

Wang Shilong Sept. 11, 2018, 11:57 p.m. UTC 
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>
Reviewed-by: Andreas Dilger <adilger@dilger.ca>
---
v2->v3: change function declaration to one line.
v1->v2: rename ext4_ioctl_setattr_check_projid()
to ext4_ioctl_check_project()
---
 fs/ext4/ioctl.c | 60 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 37 insertions(+), 23 deletions(-)

Comments

Theodore Ts'o Sept. 16, 2018, 3:55 a.m. UTC | #1 
On Wed, Sep 12, 2018 at 08:57:16AM +0900, Wang Shilong wrote:
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index a7074115d6f6..f81102bd3203 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -339,19 +339,14 @@ static int ext4_ioctl_setproject(struct file *filp, __u32 projid)
>  	if (projid_eq(kprojid, EXT4_I(inode)->i_projid))
>  		return 0;
>  
> -	err = mnt_want_write_file(filp);
> -	if (err)
> -		return err;
> -

A huge part of this patch is dropping the calls to
mnt_want_write_file() and mnt_drop_write_file().  What's the
justification for doing this?  The use of mnt_want_write_file() looks
necessary to me...

				- Ted
Wang Shilong Sept. 16, 2018, 4:02 a.m. UTC | #2 
> 在 2018年9月16日，上午11:55，Theodore Y. Ts'o <tytso@mit.edu> 写道：
> 
> On Wed, Sep 12, 2018 at 08:57:16AM +0900, Wang Shilong wrote:
>> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
>> index a7074115d6f6..f81102bd3203 100644
>> --- a/fs/ext4/ioctl.c
>> +++ b/fs/ext4/ioctl.c
>> @@ -339,19 +339,14 @@ static int ext4_ioctl_setproject(struct file *filp, __u32 projid)
>> 	if (projid_eq(kprojid, EXT4_I(inode)->i_projid))
>> 		return 0;
>> 
>> -	err = mnt_want_write_file(filp);
>> -	if (err)
>> -		return err;
>> -
> 
> A huge part of this patch is dropping the calls to
> mnt_want_write_file() and mnt_drop_write_file().  What's the
> justification for doing this?  The use of mnt_want_write_file() looks
> necessary to me…

Hi Ted,

     mnt_want_write_file() and inode_lock is held before this function called now.
Since both ioctl_set_flags and ext4_set_project() need call them.


> 
> 				- Ted
Theodore Ts'o Sept. 16, 2018, 12:20 p.m. UTC | #3 
On Sun, Sep 16, 2018 at 04:02:52AM +0000, Wang Shilong wrote:
> 
> 
> > 在 2018年9月16日，上午11:55，Theodore Y. Ts'o <tytso@mit.edu> 写道：
> > 
> > On Wed, Sep 12, 2018 at 08:57:16AM +0900, Wang Shilong wrote:
> >> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> >> index a7074115d6f6..f81102bd3203 100644
> >> --- a/fs/ext4/ioctl.c
> >> +++ b/fs/ext4/ioctl.c
> >> @@ -339,19 +339,14 @@ static int ext4_ioctl_setproject(struct file *filp, __u32 projid)
> >> 	if (projid_eq(kprojid, EXT4_I(inode)->i_projid))
> >> 		return 0;
> >> 
> >> -	err = mnt_want_write_file(filp);
> >> -	if (err)
> >> -		return err;
> >> -
> > 
> > A huge part of this patch is dropping the calls to
> > mnt_want_write_file() and mnt_drop_write_file().  What's the
> > justification for doing this?  The use of mnt_want_write_file() looks
> > necessary to me…
> 
> Hi Ted,
> 
>      mnt_want_write_file() and inode_lock is held before this function called now.
> Since both ioctl_set_flags and ext4_set_project() need call them.

I don't see any place in this patch where mnt_want_write_file() is
being called.  What am I missing?  And if there is a dependent patch
to this first patch in the patch series, please include it in this
patch series....  or least point it out after the --- list.

Thanks,

						- Ted
Wang Shilong Sept. 16, 2018, 12:25 p.m. UTC | #4 
> 在 2018年9月16日，下午8:20，Theodore Y. Ts'o <tytso@mit.edu> 写道：
> 
> On Sun, Sep 16, 2018 at 04:02:52AM +0000, Wang Shilong wrote:
>> 
>> 
>>> 在 2018年9月16日，上午11:55，Theodore Y. Ts'o <tytso@mit.edu> 写道：
>>> 
>>> On Wed, Sep 12, 2018 at 08:57:16AM +0900, Wang Shilong wrote:
>>>> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
>>>> index a7074115d6f6..f81102bd3203 100644
>>>> --- a/fs/ext4/ioctl.c
>>>> +++ b/fs/ext4/ioctl.c
>>>> @@ -339,19 +339,14 @@ static int ext4_ioctl_setproject(struct file *filp, __u32 projid)
>>>> 	if (projid_eq(kprojid, EXT4_I(inode)->i_projid))
>>>> 		return 0;
>>>> 
>>>> -	err = mnt_want_write_file(filp);
>>>> -	if (err)
>>>> -		return err;
>>>> -
>>> 
>>> A huge part of this patch is dropping the calls to
>>> mnt_want_write_file() and mnt_drop_write_file().  What's the
>>> justification for doing this?  The use of mnt_want_write_file() looks
>>> necessary to me…
>> 
>> Hi Ted,
>> 
>>     mnt_want_write_file() and inode_lock is held before this function called now.
>> Since both ioctl_set_flags and ext4_set_project() need call them.
> 
> I don't see any place in this patch where mnt_want_write_file() is
> being called.  What am I missing?  And if there is a dependent patch
> to this first patch in the patch series, please include it in this
> patch series....  or least point it out after the --- list.

Sorry, I might not explain it clearly, here is current codes after applying patch:

039                 err = mnt_want_write_file(filp);          ———————>called here, this is originally there for ext4_ioctl_setflags()
1040                 if (err)
1041                         return err;
1042 
1043                 inode_lock(inode);
1044                 err = ext4_ioctl_setattr_check_projid(inode, &fa);
1045                 if (err)
1046                         goto out;
1047                 flags = (ei->i_flags & ~EXT4_FL_XFLAG_VISIBLE) |
1048                          (flags & EXT4_FL_XFLAG_VISIBLE);
1049                 err = ext4_ioctl_setflags(inode, flags);
1050                 if (err)
1051                         goto out;
1052                 err = ext4_ioctl_setproject(filp, fa.fsx_projid);
1053 out:
1054                 inode_unlock(inode);
1055                 mnt_drop_write_file(filp);          ————————>dropped here.
1056                 return err; 
1057         }

Thanks,
Shilong

> 
> Thanks,
> 
> 						- Ted
Wang Shilong Sept. 17, 2018, 5:18 a.m. UTC | #5 
> 在 2018年9月16日，下午8:25，Wang Shilong <wshilong@ddn.com> 写道：
> 
> 
> 
>> 在 2018年9月16日，下午8:20，Theodore Y. Ts'o <tytso@mit.edu> 写道：
>> 
>> On Sun, Sep 16, 2018 at 04:02:52AM +0000, Wang Shilong wrote:
>>> 
>>> 
>>>> 在 2018年9月16日，上午11:55，Theodore Y. Ts'o <tytso@mit.edu> 写道：
>>>> 
>>>> On Wed, Sep 12, 2018 at 08:57:16AM +0900, Wang Shilong wrote:
>>>>> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
>>>>> index a7074115d6f6..f81102bd3203 100644
>>>>> --- a/fs/ext4/ioctl.c
>>>>> +++ b/fs/ext4/ioctl.c
>>>>> @@ -339,19 +339,14 @@ static int ext4_ioctl_setproject(struct file *filp, __u32 projid)
>>>>> 	if (projid_eq(kprojid, EXT4_I(inode)->i_projid))
>>>>> 		return 0;
>>>>> 
>>>>> -	err = mnt_want_write_file(filp);
>>>>> -	if (err)
>>>>> -		return err;
>>>>> -
>>>> 
>>>> A huge part of this patch is dropping the calls to
>>>> mnt_want_write_file() and mnt_drop_write_file().  What's the
>>>> justification for doing this?  The use of mnt_want_write_file() looks
>>>> necessary to me…
>>> 
>>> Hi Ted,
>>> 
>>>    mnt_want_write_file() and inode_lock is held before this function called now.
>>> Since both ioctl_set_flags and ext4_set_project() need call them.
>> 
>> I don't see any place in this patch where mnt_want_write_file() is
>> being called.  What am I missing?  And if there is a dependent patch
>> to this first patch in the patch series, please include it in this
>> patch series....  or least point it out after the --- list.
> 
> Sorry, I might not explain it clearly, here is current codes after applying patch:
> 
> 039                 err = mnt_want_write_file(filp);          ———————>called here, this is originally there for ext4_ioctl_setflags()
> 1040                 if (err)
> 1041                         return err;
> 1042 
> 1043                 inode_lock(inode);
> 1044                 err = ext4_ioctl_setattr_check_projid(inode, &fa);
> 1045                 if (err)
> 1046                         goto out;
> 1047                 flags = (ei->i_flags & ~EXT4_FL_XFLAG_VISIBLE) |
> 1048                          (flags & EXT4_FL_XFLAG_VISIBLE);
> 1049                 err = ext4_ioctl_setflags(inode, flags);
> 1050                 if (err)
> 1051                         goto out;
> 1052                 err = ext4_ioctl_setproject(filp, fa.fsx_projid);
> 1053 out:
> 1054                 inode_unlock(inode);
> 1055                 mnt_drop_write_file(filp);          ————————>dropped here.
> 1056                 return err; 
> 1057         }
> 
> Thanks,
> Shilong

Just a general ping for this, make sure you did not miss reply.
Since I saw you sent new pull request.

Is there anything more I could do? 


> 
>> 
>> Thanks,
>> 
>> 						- Ted
>
diff mbox series

Patch

diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index a7074115d6f6..f81102bd3203 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -339,19 +339,14 @@  static int ext4_ioctl_setproject(struct file *filp, __u32 projid)
 	if (projid_eq(kprojid, EXT4_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 (ext4_is_quota_file(inode))
-		goto out_unlock;
+		return err;
 
 	err = ext4_get_inode_loc(inode, &iloc);
 	if (err)
-		goto out_unlock;
+		return err;
 
 	raw_inode = ext4_raw_inode(&iloc);
 	if (!EXT4_FITS_IN_INODE(raw_inode, ei, i_projid)) {
@@ -359,7 +354,7 @@  static int ext4_ioctl_setproject(struct file *filp, __u32 projid)
 					      EXT4_SB(sb)->s_want_extra_isize,
 					      &iloc);
 		if (err)
-			goto out_unlock;
+			return err;
 	} else {
 		brelse(iloc.bh);
 	}
@@ -369,10 +364,8 @@  static int ext4_ioctl_setproject(struct file *filp, __u32 projid)
 	handle = ext4_journal_start(inode, EXT4_HT_QUOTA,
 		EXT4_QUOTA_INIT_BLOCKS(sb) +
 		EXT4_QUOTA_DEL_BLOCKS(sb) + 3);
-	if (IS_ERR(handle)) {
-		err = PTR_ERR(handle);
-		goto out_unlock;
-	}
+	if (IS_ERR(handle))
+		return PTR_ERR(handle);
 
 	err = ext4_reserve_inode_write(handle, inode, &iloc);
 	if (err)
@@ -400,9 +393,6 @@  static int ext4_ioctl_setproject(struct file *filp, __u32 projid)
 		err = rc;
 out_stop:
 	ext4_journal_stop(handle);
-out_unlock:
-	inode_unlock(inode);
-	mnt_drop_write_file(filp);
 	return err;
 }
 #else
@@ -626,6 +616,30 @@  static long ext4_ioctl_group_add(struct file *file,
 	return err;
 }
 
+static int ext4_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(EXT4_I(inode)->i_projid) != fa->fsx_projid)
+		return -EINVAL;
+
+	if (ext4_test_inode_flag(inode, EXT4_INODE_PROJINHERIT)) {
+		if (!(fa->fsx_xflags & FS_XFLAG_PROJINHERIT))
+			return -EINVAL;
+	} else {
+		if (fa->fsx_xflags & FS_XFLAG_PROJINHERIT)
+			return -EINVAL;
+	}
+
+	return 0;
+}
+
 long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
 	struct inode *inode = file_inode(filp);
@@ -1025,19 +1039,19 @@  long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 			return err;
 
 		inode_lock(inode);
+		err = ext4_ioctl_check_project(inode, &fa);
+		if (err)
+			goto out;
 		flags = (ei->i_flags & ~EXT4_FL_XFLAG_VISIBLE) |
 			 (flags & EXT4_FL_XFLAG_VISIBLE);
 		err = ext4_ioctl_setflags(inode, flags);
-		inode_unlock(inode);
-		mnt_drop_write_file(filp);
 		if (err)
-			return err;
-
+			goto out;
 		err = ext4_ioctl_setproject(filp, fa.fsx_projid);
-		if (err)
-			return err;
-
-		return 0;
+out:
+		inode_unlock(inode);
+		mnt_drop_write_file(filp);
+		return err;
 	}
 	case EXT4_IOC_SHUTDOWN:
 		return ext4_shutdown(sb, arg);