Re: [PATCH v2 review 09/11] quota: Handle quota data stored in s_user_ns.

[Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org>]

On Tue, Jul 05, 2016 at 04:28:53PM -0500, Eric W. Biederman wrote:
> Dave Chinner <david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org> writes:
> 
> > On Tue, Jul 05, 2016 at 10:34:49AM -0500, Eric W. Biederman wrote:
> >> The more I think of it the more I think that sounds like wisdom.
> >> Dropping this patch and replacing it by one that just does:
> >> 
> >> diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
> >> index d8fb0fd3ff6f..9c9890fe18b7 100644
> >> --- a/fs/quota/dquot.c
> >> +++ b/fs/quota/dquot.c
> >> @@ -2273,6 +2273,11 @@ static int vfs_load_quota_inode(struct inode *inode, int type, int format_id,
> >>                 error = -EINVAL;
> >>                 goto out_fmt;
> >>         }
> >> +       /* Filesystems outside of init_user_ns not yet supported */
> >> +       if (sb->s_user_ns != &init_user_ns) {
> >> +               error = -EINVAL;
> >> +               goto out_fmt;
> >> +       }
> >>         /* Usage always has to be set... */
> >>         if (!(flags & DQUOT_USAGE_ENABLED)) {
> >>                 error = -EINVAL;
> >> 
> >> 
> >> seems a lot more appropriate at this point.  That is enough to give a
> >> great big hint there is something that needs to be done but won't
> >> embrittle the code with untested corner cases.
> >
> > You'll need to propagate that to all filesystems that have their own
> > quota implemenation, too.
> 
> All of the filesytems that have their own quota implementations omit the
> flag FS_USERNS_MOUNT in fs_flags in struct filesystem so they are
> protected.

Which is the same situation currently for every filesystem that
supports quotas, regardless of the implementation infrastructure.

> p.s.  I  expect the the generic quota implementation is simple enough
> that it is not particularly suseptible to problems caused by malicious
> data.  But I don't currently care enough to verify and test that
> assumption so this is very much the wrong time for me to be enabling the
> feature.

All the more reason you should be adding the same guard to all the
other filesystems....

All i'm asking you to do is to make this check in a way that all
filesystems that implement quotas will execute it. Don't leave
landmines with security implications around - make sure all
filesystems have the same protections.

-Dave.
-- 
Dave Chinner
david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org