From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ipmail05.adl6.internode.on.net ([150.101.137.143]:18714 "EHLO ipmail05.adl6.internode.on.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750886AbcGFGf5 (ORCPT ); Wed, 6 Jul 2016 02:35:57 -0400 Date: Wed, 6 Jul 2016 16:35:04 +1000 From: Dave Chinner To: "Eric W. Biederman" Cc: Jan Kara , Seth Forshee , Jann Horn , Linux API , Linux Containers , Andy Lutomirski , James Bottomley , Michael Kerrisk , linux-fsdevel@vger.kernel.org, Djalal Harouni Subject: Re: [PATCH v2 review 09/11] quota: Handle quota data stored in s_user_ns. Message-ID: <20160706063504.GH27480@dastard> References: <87ziq03qnj.fsf@x220.int.ebiederm.org> <20160702172035.19568-1-ebiederm@xmission.com> <20160702172035.19568-9-ebiederm@xmission.com> <87mvm03pxy.fsf_-_@x220.int.ebiederm.org> <20160704091100.GD5200@quack2.suse.cz> <87d1msumhy.fsf@x220.int.ebiederm.org> <20160705205721.GG27480@dastard> <8737nnrcyy.fsf@x220.int.ebiederm.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <8737nnrcyy.fsf@x220.int.ebiederm.org> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Tue, Jul 05, 2016 at 04:28:53PM -0500, Eric W. Biederman wrote: > Dave Chinner 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@fromorbit.com