Message ID | 1536623661-5912-1-git-send-email-wshilong1991@gmail.com |
---|---|
State | Superseded |
Headers | show |
Series | None | expand |
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) >
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
> 在 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
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)