From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Howells Subject: Re: NFS/credentials leak in 2.6.29-rc1 Date: Thu, 22 Jan 2009 07:08:33 +0000 Message-ID: <24419.1232608113@redhat.com> References: <20090121223710.GF4295@fieldses.org> <20090120235341.GA29017@fieldses.org> <20090120114649.GA15832@ioremap.net> <20090120151125.GB24266@fieldses.org> <20090120152304.GA28592@ioremap.net> <21428.1232540589@redhat.com> Cc: dhowells@redhat.com, Evgeniy Polyakov , Trond Myklebust , linux-fsdevel@vger.kernel.org To: "J. Bruce Fields" Return-path: Received: from mx2.redhat.com ([66.187.237.31]:52404 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752572AbZAVHJE (ORCPT ); Thu, 22 Jan 2009 02:09:04 -0500 In-Reply-To: <20090121223710.GF4295@fieldses.org> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: J. Bruce Fields wrote: > 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? The former would be preferable, if it transfers the reference on the creds to the task_struct, thus eliminating the need for a put_cred(). > 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. It'll remove a potential OOM condition. It's a minor optimisation, I think. David