making crdup()/crcopy() safe??
John Baldwin
jhb at freebsd.org
Tue Dec 20 17:31:01 UTC 2011
On Tuesday, December 20, 2011 10:38:34 am Rick Macklem wrote:
> John Baldwin wrote:
> > On Monday, December 19, 2011 8:21:45 pm Rick Macklem wrote:
> > > Hi,
> > >
> > > A recent NFS client crash:
> > > http://glebius.int.ru/tmp/nfs_panic.jpg
> > > appears to have happened because some field is
> > > bogus when crfree() is called. I've asked Gleb
> > > to disassemble crfree() for me, so I can try and
> > > see exactly which field causes the crash, however...
> > >
> > > Basically, the code:
> > > newcred = crdup(cred);
> > > - does read with newcred
> > > crfree(newcred); <-- which crashes at 0x65 into
> > > crfree()
> > >
> > > Looking at crdup(), it calls crcopy(), which copies
> > > 4 pointers and then ref. counts them:
> > > cr_uidinfo, cr_ruidinfo, cr_prison and cr_loginclass
> > >
> > > It seems some lock should be held while crcopy() does this,
> > > so that the pointers don't get deref'd during the copy/ref. count?
> > > (Or is there some rule that guarantees these won't change. ie. No
> > > no calls to change_euid() or similar.)
> > >
> > > Is there such a lock and should crdup() use it?
> >
> > In general the caller of crdup is expected to hold a reference on cred
> > or some
> > other lock to ensure that cred remains valid and cannot be free'd
> > while it is
> > being duplicated. There is no global lock that crdup can hold for
> > that,
> > instead the caller is required to guarantee that.
> >
> Well, I think it does hold a reference on cred. I think the problem is
> that this doesn't stop another process from changing the pointer fields:
> cr_uidinfo, cr_ruidinfo, cr_loginclass and cr_prison
No, a credential with more than one reference is immutable and can not be
changed.
> For example, change_euid() replaces cr_uidinfo. There is something called
> cr_copysafe(), which assumes PROC_LOCK(p) is held. However, for the case
> that crashed, it is an iod read-ahead thread, so I don't think I know
> what the correct "p" argument is? It also appears that PROC_LOCK(p) is
> used to lock change_euid(), when it replaces cr_uidinfo with a different
> pointer. (Basically, it appears that the cr_uidinfo, cr_ruidinfo,
> cr_loginclass and cr_prison are protected by PROC_LOCK(p), but it isn't
> obvious to me that the NFS iod thread can know what the correct "p" is.
> In fact, that process may have already exited, since the "cred" is
refenenced
> by b_rcred for the buffer in the buffer cache that is being read-ahead.
No, change_euid() only operates on a private credential where the caller knows
that it holds the only reference to the credential. The various system calls
like setuid(), etc. all allocate a new credential, grab the PROC_LOCK to
protect what p_ucred refers to (and to serialize updates to p_ucred for a
given process), copy the existing credential from p_ucred into the new
credential, update the new credential as appropriate, then change p_ucred to
point to the new credential before dropping the PROC_LOCK. The old credential
then has its reference count dropped (since p_ucred no longer references it)
via crfree(). However, that old credential is not changed and will remain
immutable until the last reference is dropped and it is freed.
> For my NFS client case, I need a "new" cred, but it has to have cr_uidinfo
> etc filled in, since the kernel rpc does a crdup() and the cr_uidinfo
> field is used in socket calls further down. Basically, the NFS client
> fills in uid, gid-list for the "new" cred and doesn't care about other
> fields, except whatever the kernel rpc and socket ops care about.
>
> Would it be ok if, instead of using crdup(), I create the "new" cred via:
> cr = crget();
> - do the same as crcopy(), except for the pointer fields, which would be
> set as follows
> cr->cr_uidinfo = uifind(0); /* This means that chgsbsize() will record
> * the change for uid 0, but since this is
> * an iod thread for the NFS client, that
> * seems ok?
> */
> cr->cr_ruidinfo = uifind(0);
> cr->cr_loginclass = loginclass_find("default");
> /* For cr_prison, I think what crcopy() does is safe, since cr_prison
> * doesn't normally get changed after a process does I/O, I think?
> * Alternately, it could be set to &prison0. Does setting it to &prison0
> * break anything?
> */
> prison_hold(cr->cr_prison);
>
> crfree() does check for these fields being non-NULL before freeing them,
> but crdup() does not check for the NULL case before incrementing ref cnts
> on them. If crdup() were changed to check for non-NULL, then I think the
> only one of the above that would need to be set is cr_uidinfo, since that
> appears to be the only one used by socket ops.
>
> Comments? Am I missing something? Thanks, rick
Using crdup() should be fine. Your old credential should not be changed out
from under you since you hold a reference to it. I think there is some other
bug that trashed your temporary credential before it was free'd.
--
John Baldwin
More information about the freebsd-current
mailing list