Re: NFS/credentials leak in 2.6.29-rc1

["J. Bruce Fields" <bfields@fieldses.org>]

On Wed, Jan 21, 2009 at 12:23:09PM +0000, David Howells wrote:
> J. Bruce Fields <bfields@fieldses.org> 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.