Re: [PATCH v3] fs: forbid invalid project ID

[Dave Chinner <david@fromorbit.com>]

On Fri, Jul 02, 2021 at 10:02:43AM -0400, Wang Shilong wrote:
> fileattr_set_prepare() should check if project ID
> is valid, otherwise dqget() will return NULL for
> such project ID quota.
> 
> Signed-off-by: Wang Shilong <wshilong@ddn.com>
> ---
> v2->v3: move check before @fsx_projid is accessed
> and use make_kprojid() helper.
> 
> v1->v2: try to fix in the VFS
> ---
>  fs/ioctl.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 1e2204fa9963..d7edc92df473 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -806,6 +806,8 @@ static int fileattr_set_prepare(struct inode *inode,
>  	if (err)
>  		return err;
>  
> +	if (!projid_valid(make_kprojid(&init_user_ns, fa->fsx_projid)))
> +		return -EINVAL;
>  	/*
>  	 * Project Quota ID state is only allowed to change from within the init
>  	 * namespace. Enforce that restriction only if we are trying to change

I still don't think this is correct - read the comment directly
below where you put this. That was the code block I was referring to
early:

        /*
         * 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) {
                if (old_ma->fsx_projid != fa->fsx_projid)
                        return -EINVAL;
                if ((old_ma->fsx_xflags ^ fa->fsx_xflags) &
                                FS_XFLAG_PROJINHERIT)
                        return -EINVAL;
        }

IOWs: if we are not changing the projid, then we should not be
checking it for validity because of the way the whole get/set
interface works.

The reason for this is that this interface is a get/set pair, where
you first have to get the values from the filesystem, then modify
them and send the set back to the filesystem. If the filesystem
sends out fsx_projid = -1 (for whatever reason), the caller must
send that same value back into the filesystem if they are not
modifying the project ID. Hence we have to accept any projid from
the caller that matches the current filesystem value, regardless of
whether it is an invalid value or not.

Therefore, we should only be checking if fa->fsx_projid is valid if
it is different to the current filesystem value *and* we are allowed
to change it. So:

        /*
         * 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) {
                if (old_ma->fsx_projid != fa->fsx_projid)
                        return -EINVAL;
                if ((old_ma->fsx_xflags ^ fa->fsx_xflags) &
                                FS_XFLAG_PROJINHERIT)
                        return -EINVAL;
        } else {

		/*
		 * Caller is allowed to change the project ID. If it is being
		 * changed, make sure that the new value is valid.
		 */
                if (old_ma->fsx_projid != fa->fsx_projid &&
		    !projid_valid(make_kprojid(&init_user_ns, fa->fsx_projid)))
			return -EINVAL;
	}

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com