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@xxxxxxxxxxxxx> 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@xxxxxxxx>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
