[PATCH] ext4: forbid U32_MAX project ID

[PATCH] ext4: forbid U32_MAX project ID

[Wang Shilong]

From: Wang Shilong <wshilong@ddn.com>

U32_MAX is reserved for special purpose,
qid_has_mapping() will return false if projid is
4294967295, dqget() will return NULL for it.

So U32_MAX is unsupported Project ID, fix to forbid
it.

Signed-off-by: Wang Shilong <wshilong@ddn.com>
---
 fs/ext4/ioctl.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 31627f7dc5cd..f3a8d962c291 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -744,6 +744,9 @@ int ext4_fileattr_set(struct user_namespace *mnt_userns,
 	u32 flags = fa->flags;
 	int err = -EOPNOTSUPP;
 
+	if (fa->fsx_projid >= U32_MAX)
+		return -EINVAL;
+
 	ext4_fc_start_update(inode);
 	if (flags & ~EXT4_FL_USER_VISIBLE)
 		goto out;
-- 
2.27.0

Re: [PATCH] ext4: forbid U32_MAX project ID

[Dave Chinner]

On Fri, Jun 25, 2021 at 08:40:33AM -0400, Wang Shilong wrote:
> From: Wang Shilong <wshilong@ddn.com>
> 
> U32_MAX is reserved for special purpose,
> qid_has_mapping() will return false if projid is
> 4294967295, dqget() will return NULL for it.
> 
> So U32_MAX is unsupported Project ID, fix to forbid
> it.

Actually, it's INVALID_PROJID, not U32_MAX, and we already have a
check function for that:

static inline bool projid_valid(kprojid_t projid)
{
        return !projid_eq(projid, INVALID_PROJID);
}

> Signed-off-by: Wang Shilong <wshilong@ddn.com>
> ---
>  fs/ext4/ioctl.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index 31627f7dc5cd..f3a8d962c291 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -744,6 +744,9 @@ int ext4_fileattr_set(struct user_namespace *mnt_userns,
>  	u32 flags = fa->flags;
>  	int err = -EOPNOTSUPP;
>  
> +	if (fa->fsx_projid >= U32_MAX)
> +		return -EINVAL;
> +

This should actually be calling qid_valid() or projid_valid(),
and it should be in generic code because multiple filesystems
support project quotas.  i.e this should be checked in
fileattr_set_prepare(), not in ext4 specific code.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] ext4: forbid U32_MAX project ID

[Wang Shilong]

On Mon, Jun 28, 2021 at 6:42 AM Dave Chinner <david@fromorbit.com> wrote:
>
> On Fri, Jun 25, 2021 at 08:40:33AM -0400, Wang Shilong wrote:
> > From: Wang Shilong <wshilong@ddn.com>
> >
> > U32_MAX is reserved for special purpose,
> > qid_has_mapping() will return false if projid is
> > 4294967295, dqget() will return NULL for it.
> >
> > So U32_MAX is unsupported Project ID, fix to forbid
> > it.
>
> Actually, it's INVALID_PROJID, not U32_MAX, and we already have a
> check function for that:
>
> static inline bool projid_valid(kprojid_t projid)
> {
>         return !projid_eq(projid, INVALID_PROJID);
> }
>

I was not aware of this, thanks for pointing it out.

> > Signed-off-by: Wang Shilong <wshilong@ddn.com>
> > ---
> >  fs/ext4/ioctl.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> > index 31627f7dc5cd..f3a8d962c291 100644
> > --- a/fs/ext4/ioctl.c
> > +++ b/fs/ext4/ioctl.c
> > @@ -744,6 +744,9 @@ int ext4_fileattr_set(struct user_namespace *mnt_userns,
> >       u32 flags = fa->flags;
> >       int err = -EOPNOTSUPP;
> >
> > +     if (fa->fsx_projid >= U32_MAX)
> > +             return -EINVAL;
> > +
>
> This should actually be calling qid_valid() or projid_valid(),
> and it should be in generic code because multiple filesystems
> support project quotas.  i.e this should be checked in
> fileattr_set_prepare(), not in ext4 specific code.

I tried to fix ext4/f2fs, i am not sure about XFS, it looks to me XFS
implemented quota mostly by itself.

Anyway, let me fix this in generic code.


>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com

Re: [PATCH] ext4: forbid U32_MAX project ID

[Dave Chinner]

On Mon, Jun 28, 2021 at 07:13:04AM +0800, Wang Shilong wrote:
> On Mon, Jun 28, 2021 at 6:42 AM Dave Chinner <david@fromorbit.com> wrote:
> >
> > On Fri, Jun 25, 2021 at 08:40:33AM -0400, Wang Shilong wrote:
> > > From: Wang Shilong <wshilong@ddn.com>
> > >
> > > U32_MAX is reserved for special purpose,
> > > qid_has_mapping() will return false if projid is
> > > 4294967295, dqget() will return NULL for it.
> > >
> > > So U32_MAX is unsupported Project ID, fix to forbid
> > > it.
> >
> > Actually, it's INVALID_PROJID, not U32_MAX, and we already have a
> > check function for that:
> >
> > static inline bool projid_valid(kprojid_t projid)
> > {
> >         return !projid_eq(projid, INVALID_PROJID);
> > }
> >
> 
> I was not aware of this, thanks for pointing it out.
> 
> > > Signed-off-by: Wang Shilong <wshilong@ddn.com>
> > > ---
> > >  fs/ext4/ioctl.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> > > index 31627f7dc5cd..f3a8d962c291 100644
> > > --- a/fs/ext4/ioctl.c
> > > +++ b/fs/ext4/ioctl.c
> > > @@ -744,6 +744,9 @@ int ext4_fileattr_set(struct user_namespace *mnt_userns,
> > >       u32 flags = fa->flags;
> > >       int err = -EOPNOTSUPP;
> > >
> > > +     if (fa->fsx_projid >= U32_MAX)
> > > +             return -EINVAL;
> > > +
> >
> > This should actually be calling qid_valid() or projid_valid(),
> > and it should be in generic code because multiple filesystems
> > support project quotas.  i.e this should be checked in
> > fileattr_set_prepare(), not in ext4 specific code.
> 
> I tried to fix ext4/f2fs, i am not sure about XFS, it looks to me XFS
> implemented quota mostly by itself.

Yes, XFS is where project quotas originally come from - XFS has had
project quotas since quotas were first implemented in XFS back in
1995, long before it was ported to Linux. Ext4 and other filesystems
are very recent Linux re-implementations, hence the different quota
infrastructure. As it is, XFS project quotas can be queried and
controlled through the same generic linux quota APIs ithat ext4 uses
as well as it's own....

But the above change is not in quota code - you're changing code in
the FS_IOC_FSSETXATTR ioctl call (again, originally XFS code, but
lifted to the VFS level so ext4 et al can manipulate project
quotas). Hence parameter validity checks for parameters need to be
done at the VFS layers...

> Anyway, let me fix this in generic code.

Thanks!

Cheers,

-Dave.
-- 
Dave Chinner
david@fromorbit.com