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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180911224701.GF55456@jaegeuk-macbookpro.roam.corp.google.com>
Date:   Tue, 11 Sep 2018 15:47:01 -0700
From:   Jaegeuk Kim <jaegeuk@...nel.org>
To:     Wang Shilong <wangshilong1991@...il.com>
Cc:     linux-ext4@...r.kernel.org, linux-f2fs-devel@...ts.sourceforge.net,
        adilger@...ger.ca, wshilong@....com, dchinner@...hat.com
Subject: Re: [f2fs-dev] [PATCH v3 2/2] f2fs: fix setattr project check upon
 fssetxattr ioctl

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_...

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