Date: Mon, 07 May 2001 13:03:40 -0700 (PDT) From: John Baldwin <jhb@FreeBSD.org> To: Robert Watson <rwatson@FreeBSD.org> Cc: arch@FreeBSD.org Subject: RE: Patch to eliminate struct pcred Message-ID: <XFMail.010507130340.jhb@FreeBSD.org> In-Reply-To: <Pine.NEB.3.96L.1010506235944.43785B-100000@fledge.watson.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 07-May-01 Robert Watson wrote:
> Index: compat/svr4/svr4_misc.c
> ===================================================================
> RCS file: /home/ncvs/src/sys/compat/svr4/svr4_misc.c,v
> retrieving revision 1.30
> diff -u -r1.30 svr4_misc.c
> --- compat/svr4/svr4_misc.c 2001/05/01 08:11:52 1.30
> +++ compat/svr4/svr4_misc.c 2001/05/06 00:43:54
> @@ -1294,13 +1294,8 @@
> /*
> * Free up credentials.
> */
> - PROC_LOCK(q);
> - if (--q->p_cred->p_refcnt == 0) {
> - crfree(q->p_ucred);
> - uifree(q->p_cred->p_uidinfo);
> - FREE(q->p_cred, M_SUBPROC);
> - q->p_cred = NULL;
> - }
> + crfree(q->p_ucred);
> + q->p_ucred = NULL;
Removing the proc lock here looks suspicious, but I think it might mirror a
change I just made to kern_exit.c in wait1(), in which case it is ok.
> Index: kern/kern_exec.c
> ===================================================================
> RCS file: /home/ncvs/src/sys/kern/kern_exec.c,v
> retrieving revision 1.126
> diff -u -r1.126 kern_exec.c
> --- kern/kern_exec.c 2001/05/01 08:12:56 1.126
> +++ kern/kern_exec.c 2001/05/06 16:25:06
> @@ -104,8 +104,9 @@
> register struct execve_args *uap;
> {
> struct nameidata nd, *ndp;
> + struct ucred *oldcred = p->p_ucred, *newcred;
> register_t *stack_base;
> - int error, len, i;
> + int error, len, i, intrace;
> struct image_params image_params, *imgp;
> struct vattr attr;
> int (*img_first) __P((struct image_params *));
> @@ -272,23 +273,31 @@
> p->p_flag &= ~P_PPWAIT;
> wakeup((caddr_t)p->p_pptr);
> }
> + intrace = p->p_flag & P_TRACED;
> + PROC_UNLOCK(p);
This unlock is busted since we then try to unlock this lock again later on
since you didn't remove the other unlocks. Also, this whole caching of the
intrace flag is bogus too. If you read a value and release the lock, then you
have now lost the ability to safely make decisions on the value you just read.
You have to hold the lock over both reading the value and deciding what to do
based on that value so that the entire thing is an "atomic" operation. For
now, I would just revert the intrace changes to check the flag directly like
the code does now and not add in this proc unlock.
> /*
> + * XXX: Note, the whole execve() is incredibly racey right now
> + * with regards to debugging and privilege/credential management.
> + * This MUST be fixed prior to any release.
> + */
> +
> + /*
> * Implement image setuid/setgid.
> *
> * Don't honor setuid/setgid if the filesystem prohibits it or if
> * the process is being traced.
> */
> - if ((((attr.va_mode & VSUID) && p->p_ucred->cr_uid != attr.va_uid) ||
> - ((attr.va_mode & VSGID) && p->p_ucred->cr_gid != attr.va_gid)) &&
> - (imgp->vp->v_mount->mnt_flag & MNT_NOSUID) == 0 &&
> - (p->p_flag & P_TRACED) == 0) {
> + newcred = NULL;
> + if ((((attr.va_mode & VSUID) && oldcred->cr_uid != attr.va_uid) ||
> + ((attr.va_mode & VSGID) && oldcred->cr_gid != attr.va_gid)) &&
> + (imgp->vp->v_mount->mnt_flag & MNT_NOSUID) == 0 && intrace == 0) {
> PROC_UNLOCK(p);
> /*
> * Turn off syscall tracing for set-id programs, except for
> * root.
> */
> - if (p->p_tracep && suser(p)) {
> + if (p->p_tracep && suser_xxx(oldcred, NULL, PRISON_ROOT)) {
> p->p_traceflag = 0;
> vrele(p->p_tracep);
> p->p_tracep = NULL;
> @@ -296,25 +305,42 @@
> /*
> * Set the new credentials.
> */
> - p->p_ucred = crcopy(p->p_ucred);
> + newcred = crdup(p->p_ucred);
> if (attr.va_mode & VSUID)
> - change_euid(p, attr.va_uid);
> + change_euid(newcred, attr.va_uid);
> if (attr.va_mode & VSGID)
> - p->p_ucred->cr_gid = attr.va_gid;
> + change_egid(newcred, attr.va_gid);
> setsugid(p);
> setugidsafety(p);
> } else {
> - if (p->p_ucred->cr_uid == p->p_cred->p_ruid &&
> - p->p_ucred->cr_gid == p->p_cred->p_rgid)
> - p->p_flag &= ~P_SUGID;
> + if (oldcred->cr_uid == oldcred->cr_ruid &&
> + oldcred->cr_gid == oldcred->cr_rgid)
> + p->p_flag &= ~P_SUGID; /* XXX locking */
> PROC_UNLOCK(p);
> }
>
> /*
> * Implement correct POSIX saved-id behavior.
> + *
> + * XXX: determine whether tests and sets should occur on old or
> + * new credentials.
> */
> - p->p_cred->p_svuid = p->p_ucred->cr_uid;
> - p->p_cred->p_svgid = p->p_ucred->cr_gid;
> + if (p->p_ucred->cr_svuid != p->p_ucred->cr_uid ||
> + p->p_ucred->cr_svgid != p->p_ucred->cr_gid) {
> + if (newcred != NULL)
> + newcred = crdup(p->p_ucred);
> +
> + change_svuid(newcred, p->p_ucred->cr_uid);
> + change_svgid(newcred, p->p_ucred->cr_gid);
> + }
> +
> + if (newcred != NULL) {
> + struct ucred *oldcred;
> +
> + oldcred = p->p_ucred;
> + p->p_ucred = newcred;
> + crfree(oldcred);
> + }
>
> /*
> * Store the vp for use in procfs
--
John Baldwin <jhb@FreeBSD.org> -- http://www.FreeBSD.org/~jhb/
PGP Key: http://www.baldwin.cx/~john/pgpkey.asc
"Power Users Use the Power to Serve!" - http://www.FreeBSD.org/
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-arch" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?XFMail.010507130340.jhb>