Fix a crash in the systrace found by form@

One is a kernel fix that changes the lockin and one is a userland fix that
prevents dereferencing a freed pointer.
From provos
deraadt@ ok

1 files changed, 57 insertions, 55 deletions

diff --git a/sys/dev/systrace.c b/sys/dev/systrace.c
index 26ca9132f1e..9f47728d877 100644
--- a/sys/dev/systrace.c
+++ b/sys/dev/systrace.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: systrace.c,v 1.27 2002/12/12 08:36:05 art Exp $	*/
+/*	$OpenBSD: systrace.c,v 1.28 2003/02/20 22:03:31 art Exp $	*/
 /*
  * Copyright 2002 Niels Provos <provos@citi.umich.edu>
  * All rights reserved.
@@ -645,6 +645,18 @@ systrace_fork(struct proc *oldproc, struct proc *p)
 	lockmgr(&fst->lock, LK_RELEASE, NULL, curproc);
 }
 
+#define REACQUIRE_LOCK	do { \
+	systrace_lock(); \
+	strp = p->p_systrace; \
+	if (strp == NULL) { \
+		systrace_unlock(); \
+		return (error); \
+	} \
+	fst = strp->parent; \
+	lockmgr(&fst->lock, LK_EXCLUSIVE, NULL, p); \
+	systrace_unlock(); \
+} while (0)
+
 int
 systrace_redirect(int code, struct proc *p, void *v, register_t *retval)
 {
@@ -750,6 +762,7 @@ systrace_redirect(int code, struct proc *p, void *v, register_t *retval)
 		fst = NULL;
 	}
 
+
 	if (error)
 		return (error);
 
@@ -759,101 +772,90 @@ systrace_redirect(int code, struct proc *p, void *v, register_t *retval)
 	oldgid = pc->p_rgid;
 		
 	/* Elevate privileges as desired */
-	if (issuser) {
-		systrace_lock();
-		if ((strp = p->p_systrace) != NULL) {
+	systrace_lock();
+	if ((strp = p->p_systrace) != NULL) {
+		if (issuser) {
 			if (ISSET(strp->flags, STR_PROC_SETEUID))
 				strp->saveuid = systrace_seteuid(p, strp->seteuid);
 			if (ISSET(strp->flags, STR_PROC_SETEGID))
 				strp->savegid = systrace_setegid(p, strp->setegid);
-		}
-		systrace_unlock();
-	} else
-		CLR(strp->flags, STR_PROC_SETEUID|STR_PROC_SETEGID);
+		} else
+			CLR(strp->flags, STR_PROC_SETEUID|STR_PROC_SETEGID);
+	}
+	systrace_unlock();
 				
 	error = (*callp->sy_call)(p, v, retval);
 
 	/* Return to old privileges */
+	systrace_lock();
+	if ((strp = p->p_systrace) == NULL) {
+		systrace_unlock();
+		return (error);
+	}
+
 	if (issuser) {
-		systrace_lock();
-		if ((strp = p->p_systrace) != NULL) {
-			if (ISSET(strp->flags, STR_PROC_SETEUID)) {
-				if (pc->pc_ucred->cr_uid == strp->seteuid)
-					systrace_seteuid(p, strp->saveuid);
-				CLR(strp->flags, STR_PROC_SETEUID);
-			}
-			if (ISSET(strp->flags, STR_PROC_SETEGID)) {
-				if (pc->pc_ucred->cr_gid == strp->setegid)
-					systrace_setegid(p, strp->savegid);
-				CLR(strp->flags, STR_PROC_SETEGID);
-			}
+		if (ISSET(strp->flags, STR_PROC_SETEUID)) {
+			if (pc->pc_ucred->cr_uid == strp->seteuid)
+				systrace_seteuid(p, strp->saveuid);
+			CLR(strp->flags, STR_PROC_SETEUID);
+		}
+		if (ISSET(strp->flags, STR_PROC_SETEGID)) {
+			if (pc->pc_ucred->cr_gid == strp->setegid)
+				systrace_setegid(p, strp->savegid);
+			CLR(strp->flags, STR_PROC_SETEGID);
 		}
-		systrace_unlock();
 	}
 
 	if (p->p_flag & P_SUGID) {
-		/* Stupid Locking not necessary */
-		if ((strp = p->p_systrace) == NULL ||
-		    (fst = strp->parent) == NULL)
-			return (error);
-		if (!fst->issuser)
+		if ((fst = strp->parent) == NULL || !fst->issuser) {
+			systrace_unlock();
 			return (error);
+		}
 	}
 
 	/* Report change in emulation */
-	systrace_lock();
-	strp = p->p_systrace;
 
 	/* See if we should force a report */
-	if (strp != NULL && ISSET(strp->flags, STR_PROC_REPORT)) {
+	if (ISSET(strp->flags, STR_PROC_REPORT)) {
 		CLR(strp->flags, STR_PROC_REPORT);
 		oldemul = NULL;
 	}
 
-	if (p->p_emul != oldemul && strp != NULL) {
-		fst = strp->parent;
-		lockmgr(&fst->lock, LK_EXCLUSIVE, NULL, p);
-		systrace_unlock();
+	/* Acquire lock */
+	fst = strp->parent;
+	lockmgr(&fst->lock, LK_EXCLUSIVE, NULL, p);
+	systrace_unlock();
 
+	if (p->p_emul != oldemul) {
 		/* Old policy is without meaning now */
 		if (strp->policy) {
 			systrace_closepolicy(fst, strp->policy);
 			strp->policy = NULL;
 		}
 		systrace_msg_emul(fst, strp);
-	} else
-		systrace_unlock();
+
+		REACQUIRE_LOCK;
+	}
 
 	/* Report if effective uid or gid changed */
 	if (olduid != p->p_cred->p_ruid ||
 	    oldgid != p->p_cred->p_rgid) {
-		systrace_lock();
-		if ((strp = p->p_systrace) == NULL) {
-			systrace_unlock();
-			goto nougid;
-		}
-
-		fst = strp->parent;
-		lockmgr(&fst->lock, LK_EXCLUSIVE, NULL, p);
-		systrace_unlock();
-
 		systrace_msg_ugid(fst, strp);
-	nougid:
-		;
+
+		REACQUIRE_LOCK;
 	}
 
 	/* Report result from system call */
-	systrace_lock();
-	if (report && (strp = p->p_systrace) != NULL) {
-		fst = strp->parent;
-		lockmgr(&fst->lock, LK_EXCLUSIVE, NULL, p);
-		systrace_unlock();
-
+	if (report) {
 		systrace_msg_result(fst, strp, error, code,
 		    callp->sy_argsize, v, retval);
-	} else
-		systrace_unlock();
 
+		/* not locked */
+		goto out;
+	}
+
+	lockmgr(&fst->lock, LK_RELEASE, NULL, curproc);
+ out:
 	return (error);
 }