linux-ext4 - Re: [PATCH v2 1/2] ext4: 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] [thread-next>] [day] [month] [year] [list]

Message-Id: <9572CE12-24D8-45D9-BB85-22F97588D0BA@dilger.ca>
Date:   Sun, 9 Sep 2018 22:58:24 -0600
From:   Andreas Dilger <adilger@...ger.ca>
To:     Wang Shilong <wangshilong1991@...il.com>
Cc:     Ext4 Developers List <linux-ext4@...r.kernel.org>,
        linux-f2fs-devel@...ts.sourceforge.net, dchinner@...hat.com,
        wshilong@....com
Subject: Re: [PATCH v2 1/2] ext4: fix setattr project check upon fssetxattr
 ioctl

On Sep 9, 2018, at 3:15 AM, Wang Shilong <wangshilong1991@...il.com> 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>

Reviewed-by: Andreas Dilger <adilger@...ger.ca>

> ---
> v1->v2: rename ext4_ioctl_setattr_check_projid()
> to ext4_ioctl_check_project()
> ---
> fs/ext4/ioctl.c | 61 +++++++++++++++++++++++++++++++++++----------------------
> 1 file changed, 38 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index a707411..6ef989a 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,31 @@ 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 +1040,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);
> --
> 1.8.3.1
> 


Cheers, Andreas






Download attachment "signature.asc" of type "application/pgp-signature" (874 bytes)