From mboxrd@z Thu Jan 1 00:00:00 1970 From: "J. Bruce Fields" Subject: Re: NFS/credentials leak in 2.6.29-rc1 Date: Wed, 21 Jan 2009 17:37:10 -0500 Message-ID: <20090121223710.GF4295@fieldses.org> References: <20090120235341.GA29017@fieldses.org> <20090120114649.GA15832@ioremap.net> <20090120151125.GB24266@fieldses.org> <20090120152304.GA28592@ioremap.net> <21428.1232540589@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Evgeniy Polyakov , Trond Myklebust , linux-fsdevel@vger.kernel.org To: David Howells Return-path: Received: from mail.fieldses.org ([141.211.133.115]:40401 "EHLO pickle.fieldses.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752922AbZAUWhR (ORCPT ); Wed, 21 Jan 2009 17:37:17 -0500 Content-Disposition: inline In-Reply-To: <21428.1232540589@redhat.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Wed, Jan 21, 2009 at 12:23:09PM +0000, David Howells wrote: > J. Bruce Fields wrote: > > > - Finally, we put_cred(override_creds(new)). That modifies > > current->cred again, putting the old value and getting the > > new. > > > > Hm. But that last part's not OK; aren't we still holding our own > > reference to new, in addition to the one that override_creds() just > > took? So I think we need the following? > > Yes, you're right. override_creds() takes an extra ref on the argument it is > passed, thus leaving the caller with their original reference intact. > > So really, you don't want to call override_creds() as that will cost you an > extra atomic_inc() and atomic_dec_and_test(). I recommend you replace: > > put_cred(override_creds(new)); > > with: > > revert_creds(new); > > I think that should do the right thing. It may look a bit odd, but it'll be > quicker. If you object to using revert_creds)( because of the name, we can > come up with an alternative name. If the only difference is just whether it takes a reference on the passed-in cred it might be clearest just to write set_creds(new); or set_creds(get_creds(new)); depending on which you want? In any case, yes, the revert_creds()/override_creds() names don't tell me much. > > Looking through nfsd_setuser(), one obvious bug: in the (flags & > > NFSEXP_ALLSQUASH) case, we never check the return value from the > > groups_alloc(0). If it returns NULL, we dereference it anyway. > > Since a zero-length groups list must be copied before writing, can I recommend > that we make groups_alloc(0) a special case that returns pointer to a > statically allocated groups list (after inc'ing the refcount) that represents > a zero-length list, thus meaning groups_alloc(0) will never fail? Is there a really big advantage to that? On the face of it it strikes me as a weird corner case that I'll trip over every time I look at this code. --b.