PERFORCE change 45390 for review

[Robert Watson]

http://perforce.freebsd.org/chv.cgi?CH=45390

Change 45390 by rwatson at rwatson_tislabs on 2004/01/15 08:17:28

	- Assert Giant in audit_write(): can't do the VFS thing without it.
	
	- audit() system call is MPSAFE, at least at this layer.  Need to
	  check the BSM routines.
	
	- auditon() is MPSAFE (unimplemented).
	
	- auditsvc() is MPSAFE (unimplemented).
	
	- getauid() is MPSAFE: cache the value protected by the proc lock
	  in a local variable and copyout after releasing the lock.
	
	- setauid() is MPSAFE: copyin to a stack variable, then update
	  holding the process lock.  Audit the argument from the stack
	  variable rather than doing it while holding the process lock.
	
	- getaudit() is MPSAFE: cache the value protected by the proc lock
	  in a local variable, and copyout after releasing the lock.
	
	- setaudit() is MPSAFE: copyin to a stack variable, then update
	  holding the process lock.  XXX: Missing audit of argument here?
	
	- setaudit_addr() is MPSAFE (unimplemented).
	
	- auditctl() is MPSAFE: grab Giant around VFS operations.  XXX:
	  Missing audit of argument here?
	
	- audit_arg_sockaddr(): XXX: Socket locking will be needed here.
	
	- Assert Giant and vnode lock in audit_arg_vnpath().  Remove
	  comment about Darwin not having vnode locking assertions, as
	  FreeBSD does have them.

Affected files ...

.. //depot/projects/trustedbsd/audit2/sys/security/audit/audit.c#13 edit

Differences ...

==== //depot/projects/trustedbsd/audit2/sys/security/audit/audit.c#13 (text+ko) ====

@@ -164,6 +164,8 @@
 	int ret;
 	struct au_record *bsm;
 
+	mtx_assert(&Giant, MA_OWNED);
+
 	/* 
 	 * If there is a user audit record attached to the kernel record,
 	 * then write the user record.
@@ -482,6 +484,8 @@
  * Begin system calls.            *
  **********************************/
 /*
+ * MPSAFE
+ *
  * System call to allow a user space application to submit a BSM audit
  * record to the kernel for inclusion in the audit log. This function
  * does little verification on the audit record that is submitted.
@@ -538,6 +542,8 @@
 }
 
 /*
+ * MPSAFE
+ *
  *  System call to manipulate auditing.
  */
 /* ARGSUSED */
@@ -553,6 +559,8 @@
 }
 
 /*
+ * MPSAFE
+ *
  *  System call to pass in file descriptor for audit log.
  */
 /* ARGSUSED */
@@ -568,6 +576,8 @@
 }
 
 /* 
+ * MPSAFE
+ *
  * System calls to manage the user audit information.
  * XXXAUDIT May need to lock the proc structure.
  */
@@ -576,75 +586,100 @@
 getauid(struct thread *td, struct getauid_args *uap)
 {
 	int error;
+	au_id_t id;
 
 	error = suser(td);
 	if (error)
 		return (error);
 
-	error = copyout((void *)&td->td_proc->p_au->ai_auid, (void *)uap->auid, 
-				sizeof(*uap->auid));
-	if (error)
-		return (error);
-
-	return (0);
+	/*
+	 * XXX:
+	 * Integer read on static pointer dereference: doesn't need locking?
+	 */
+	PROC_LOCK(td->td_proc);
+	id = td->td_proc->p_au->ai_auid;
+	PROC_UNLOCK(td->td_proc);
+	return (copyout(&id, uap->auid, sizeof(id)));
 }
 
+/* MPSAFE */
 /* ARGSUSED */
 int
 setauid(struct thread *td, struct setauid_args *uap)
 {
 	int error;
+	au_id_t id;
 
 	error = suser(td);
 	if (error)
 		return (error);
 
-	error = copyin((void *)uap->auid, (void *)&td->td_proc->p_au->ai_auid, 
-				sizeof(td->td_proc->p_au->ai_auid));
+	error = copyin(uap->auid, &id, sizeof(id));
 	if (error)
 		return (error);
 
-	audit_arg_auid(td->td_proc->p_au->ai_auid);
+	audit_arg_auid(id);
+
+	/*
+	 * XXX:
+	 * Integer write on static pointer dereference: doesn't need locking?
+	 */
+	PROC_LOCK(td->td_proc);
+	td->td_proc->p_au->ai_auid = id;
+	PROC_UNLOCK(td->td_proc);
+
 	return (0);
 }
 
 /*
+ * MPSAFE
  *  System calls to get and set process audit information.
  */
 /* ARGSUSED */
 int
 getaudit(struct thread *td, struct getaudit_args *uap)
 {
+	struct auditinfo ai;
 	int error;
 
 	error = suser(td);
 	if (error)
 		return (error);
-	error = copyout((void *)td->td_proc->p_au, (void *)uap->auditinfo, 
-				sizeof(*uap->auditinfo));
-	if (error)
-		return (error);
+
+	PROC_LOCK(td->td_proc);
+	ai = *td->td_proc->p_au;
+	PROC_UNLOCK(td->td_proc);
 
-	return (0);
+	return (copyout(&ai, uap->auditinfo, sizeof(ai)));
 }
 
+/* MPSAFE */
 /* ARGSUSED */
 int
 setaudit(struct thread *td, struct setaudit_args *uap)
 {
+	struct auditinfo ai;
 	int error;
 
 	error = suser(td);
 	if (error)
 		return (error);
-	error = copyin((void *)uap->auditinfo, (void *)td->td_proc->p_au, 
-				sizeof(*td->td_proc->p_au));
+
+	error = copyin(uap->auditinfo, &ai, sizeof(ai));
 	if (error)
 		return (error);
 
+	/*
+	 * XXX: Shouldn't the arguments here be audited?
+	 */
+	PROC_LOCK(td->td_proc);
+	*td->td_proc->p_au = ai;
+	PROC_UNLOCK(td->td_proc);
+
 	return (0);
 }
 
+/* MPSAFE */
 /* ARGSUSED */
 int
 getaudit_addr(struct thread *td, struct getaudit_addr_args *uap)
@@ -657,6 +692,7 @@
 	return (ENOSYS);
 }
 
+/* MPSAFE */
 /* ARGSUSED */
 int
 setaudit_addr(struct thread *td, struct setaudit_addr_args *uap)
@@ -670,6 +706,7 @@
 }
 
 /*
+ * MPSAFE
  * Syscall to manage audit files.
  *
  * XXX: Should generate an audit event.
@@ -696,26 +733,29 @@
 	 * credential.
 	 */
 	if (uap->path != NULL) {
+		mtx_lock(&Giant);
 		NDINIT(&nd, LOOKUP, FOLLOW | LOCKLEAF, UIO_USERSPACE,
 		    uap->path, td);
 		flags = audit_open_flags;
 		error = vn_open(&nd, &flags, 0, -1);
-		if (error)
-			goto out;
+		if (error) {
+			mtx_unlock(&Giant);
+			return (error);
+		}
 		VOP_UNLOCK(nd.ni_vp, 0, td);
 		vp = nd.ni_vp;
 		if (vp->v_type != VREG) {
 			vn_close(vp, audit_close_flags, td->td_ucred, td);
-			error = EINVAL;
-			goto out;
+			mtx_lock(&Giant);
+			return (EINVAL);
 		}
 		cred = td->td_ucred;
 		crhold(cred);
+		mtx_unlock(&Giant);
 	}
 
 	audit_rotate_vnode(cred, vp);
-out:
-	return (error);
+	return (0);
 }
 
 /**********************************
@@ -1135,6 +1175,9 @@
 	if (ar == NULL || td == NULL || so == NULL)
 		return;
 
+	/*
+	 * XXX: Socket locking?
+	 */
 	bcopy(so, &ar->k_ar.ar_arg_sockaddr, sizeof(ar->k_ar.ar_arg_sockaddr));
 	switch (so->sa_family) {
 	case AF_INET:
@@ -1367,6 +1410,9 @@
 	if (vp == NULL)
 		return;
 
+	mtx_assert(&Giant, MA_OWNED);
+	ASSERT_VOP_LOCKED(vp, "audit_arg_vnpath")
+
 	ar = currecord();
 	if (ar == NULL)	/* This will be the case for unaudited system calls */
 		return;
@@ -1406,10 +1452,6 @@
 	else
 		ar->k_ar.ar_valid_arg |= ARG_KPATH2;
 
-	/*
-	 * XXX: We'd assert the vnode lock here, only Darwin doesn't
-	 * appear to have vnode locking assertions.
-	 */
 	error = VOP_GETATTR(vp, &vattr, td->td_ucred, td);
 	if (error) {
 		/* XXX: How to handle this case? */
To Unsubscribe: send mail to majordomo at trustedbsd.org
with "unsubscribe trustedbsd-cvs" in the body of the message