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

[Jan Kara <jack@suse.cz>]

On Wed 06-07-16 16:35:04, Dave Chinner wrote:
> On Tue, Jul 05, 2016 at 04:28:53PM -0500, Eric W. Biederman wrote:
> > Dave Chinner <david@fromorbit.com> 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.

Well, I'm not sure I follow you here. VFS quotas are a generic code used by
a few filesystems. So I can imagine that someone would decide to enable
FS_USERNS_MOUNT for one of those filesystems without thinking about quotas
and then Eric's check would trigger and possibly save use from some
problems.

When someone decides to enable FS_USERNS_MOUNT for XFS, he will have
presumably made sure all parts of XFS are safe, including its quota
implementation.

I don't want to stop you or Eric in adding an extra check in XFS, I just
have hard time to see how that check would trigger and how XFS quota is
different from other XFS parts...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR