Re: NFS/credentials leak in 2.6.29-rc1

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

On Tue, Jan 20, 2009 at 06:23:04PM +0300, Evgeniy Polyakov wrote:
> On Tue, Jan 20, 2009 at 10:11:25AM -0500, J. Bruce Fields (bfields@fieldses.org) wrote:
> > This doesn't look familiar, no; thanks for the report.  I guess we
> > should take a careful look at the recent changes to fs/nfsd/auth.c?
> 
> If creds are allocated in nfsd_setuser() and never freed? groups_alloc()
> can also explain size-256 slab grew, so this may be the place where
> things are allocated, but why they are not freed?
> This may also explain why I did not see this for the large sequential
> IO, since number of requests to the server was noticebly smaller, than
> in random IO test.

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.

But that's unrelated.  For the cred reference counting:

	- revert_creds(get_cred(current->real_cred)) modifies
	  current->cred, putting the old value and getting the new.  OK.
	- new = prepare_creds() creates a new object with count 1.  All
	  subsequent exits from the function go through the oom or error
	  labels, which put new. OK.
	- Each of the three ALLSQUASH/ROOTSQUASH/else paths create a new
	  reference to a groups_info struct in gi, which is
	  unconditionally assigned to new by set_groups().  set_groups()
	  takes its own reference on gi, then we put the original
	  reference in the following put_group_info().  OK.
	- 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?

--b.

diff --git a/fs/nfsd/auth.c b/fs/nfsd/auth.c
index c903e04..9966e9e 100644
--- a/fs/nfsd/auth.c
+++ b/fs/nfsd/auth.c
@@ -85,6 +85,7 @@ int nfsd_setuser(struct svc_rqst *rqstp, struct svc_export *exp)
 		new->cap_effective = cap_raise_nfsd_set(new->cap_effective,
 							new->cap_permitted);
 	put_cred(override_creds(new));
+	put_cred(new);
 	return 0;
 
 oom: