On Sep 4, 2018, at 8:54 AM, Wang Shilong <wangshilong1991@xxxxxxxxx> wrote:
>
> 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@xxxxxxx>
> ---
> fs/ext4/ioctl.c | 62 ++++++++++++++++++++++++++++++++++++---------------------
> 1 file changed, 39 insertions(+), 23 deletions(-)
>
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index a707411..e860df9 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,32 @@ static long ext4_ioctl_group_add(struct file *file,
> return err;
> }
>
> +static int
> +ext4_ioctl_setattr_check_projid(struct inode *inode,
One minor complaint is the function name "ext4_ioctl_setattr_check_projid()"
does not match the existing function name "ext4_ioctl_setproject()". It would
be more consistent to name this "ext4_ioctl_check_project()", or maybe better
to name the current function "ext4_ioctl_set_projid()"?
Also, given that "check_projid()" is used before "setproject()", it makes sense
to locate "check_projid()" before "setproject()" in the code as well?
Cheers, Andreas
> + 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 +1041,19 @@ long ext4_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> return err;
>
> inode_lock(inode);
> + err = ext4_ioctl_setattr_check_projid(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);
Cheers, Andreas
Attachment:
signature.asc
Description: Message signed with OpenPGP
