Correct a number of problems that were previously commented on:

- Correct audit_arg_socketaddr() argument name from so to sa.
- Assert arguments are non-NULL to many argument capture functions
  rather than testing them.  This may trip some bugs.
- Assert the process lock is held when auditing process
  information.
- Test currecord in several more places.
- Test validity of more arguments with kasserts, such as flag
  values when auditing vnode information.

Perforce change:	98825
Obtained from:		TrustedBSD Project

2 files changed, 40 insertions, 49 deletions

diff --git a/sys/security/audit/audit.h b/sys/security/audit/audit.h
index df74e9a..6043236 100644
--- a/sys/security/audit/audit.h
+++ b/sys/security/audit/audit.h
@@ -151,7 +151,7 @@ void	 audit_arg_pid(pid_t pid);
 void	 audit_arg_process(struct proc *p);
 void	 audit_arg_signum(u_int signum);
 void	 audit_arg_socket(int sodomain, int sotype, int soprotocol);
-void	 audit_arg_sockaddr(struct thread *td, struct sockaddr *so);
+void	 audit_arg_sockaddr(struct thread *td, struct sockaddr *sa);
 void	 audit_arg_auid(uid_t auid);
 void	 audit_arg_auditinfo(struct auditinfo *au_info);
 void	 audit_arg_upath(struct thread *td, char *upath, u_int64_t flags);
diff --git a/sys/security/audit/audit_arg.c b/sys/security/audit/audit_arg.c
index 32e317a..5da377f 100644
--- a/sys/security/audit/audit_arg.c
+++ b/sys/security/audit/audit_arg.c
@@ -357,13 +357,14 @@ audit_arg_process(struct proc *p)
 {
 	struct kaudit_record *ar;
 
+	KASSERT(p != NULL, ("audit_arg_process: p == NULL"));
+
+	PROC_LOCK_ASSERT(p, MA_OWNED);
+
 	ar = currecord();
-	if ((ar == NULL) || (p == NULL))
+	if (ar == NULL)
 		return;
 
-	/*
-	 * XXXAUDIT: PROC_LOCK_ASSERT(p);
-	 */
 	ar->k_ar.ar_arg_auid = p->p_au->ai_auid;
 	ar->k_ar.ar_arg_euid = p->p_ucred->cr_uid;
 	ar->k_ar.ar_arg_egid = p->p_ucred->cr_groups[0];
@@ -404,21 +405,21 @@ audit_arg_socket(int sodomain, int sotype, int soprotocol)
 	ARG_SET_VALID(ar, ARG_SOCKINFO);
 }
 
-/*
- * XXXAUDIT: Argument here should be 'sa' not 'so'.  Caller is responsible
- * for synchronizing access to the source of the address.
- */
 void
-audit_arg_sockaddr(struct thread *td, struct sockaddr *so)
+audit_arg_sockaddr(struct thread *td, struct sockaddr *sa)
 {
 	struct kaudit_record *ar;
 
+	KASSERT(td != NULL, ("audit_arg_sockaddr: td == NULL"));
+	KASSERT(sa != NULL, ("audit_arg_sockaddr: sa == NULL"));
+
 	ar = currecord();
-	if (ar == NULL || td == NULL || so == NULL)
+	if (ar == NULL)
 		return;
 
-	bcopy(so, &ar->k_ar.ar_arg_sockaddr, sizeof(ar->k_ar.ar_arg_sockaddr));
-	switch (so->sa_family) {
+	bcopy(sa, &ar->k_ar.ar_arg_sockaddr,
+	    sizeof(ar->k_ar.ar_arg_sockaddr));
+	switch (sa->sa_family) {
 	case AF_INET:
 		ARG_SET_VALID(ar, ARG_SADDRINET);
 		break;
@@ -428,7 +429,7 @@ audit_arg_sockaddr(struct thread *td, struct sockaddr *so)
 		break;
 
 	case AF_UNIX:
-		audit_arg_upath(td, ((struct sockaddr_un *)so)->sun_path,
+		audit_arg_upath(td, ((struct sockaddr_un *)sa)->sun_path,
 				ARG_UPATH1);
 		ARG_SET_VALID(ar, ARG_SADDRUNIX);
 		break;
@@ -472,17 +473,14 @@ audit_arg_text(char *text)
 {
 	struct kaudit_record *ar;
 
+	KASSERT(text != NULL, ("audit_arg_text: text == NULL"));
+
 	ar = currecord();
 	if (ar == NULL)
 		return;
 
-	/*
-	 * XXXAUDIT: Why do we accept a possibly NULL string here?
-	 */
 	/* Invalidate the text string */
 	ar->k_ar.ar_valid_arg &= (ARG_ALL ^ ARG_TEXT);
-	if (text == NULL)
-		return;
 
 	if (ar->k_ar.ar_arg_text == NULL)
 		ar->k_ar.ar_arg_text = malloc(MAXPATHLEN, M_AUDITTEXT,
@@ -600,9 +598,10 @@ audit_arg_file(struct proc *p, struct file *fp)
 	struct vnode *vp;
 	int vfslocked;
 
-	/*
-	 * XXXAUDIT: Why is the (ar == NULL) test only in the socket case?
-	 */
+	ar = currecord();
+	if (ar == NULL)
+		return;
+
 	switch (fp->f_type) {
 	case DTYPE_VNODE:
 	case DTYPE_FIFO:
@@ -618,14 +617,8 @@ audit_arg_file(struct proc *p, struct file *fp)
 		break;
 
 	case DTYPE_SOCKET:
-		ar = currecord();
-		if (ar == NULL)
-			return;
-
-		/*
-		 * XXXAUDIT: Socket locking?  Inpcb locking?
-		 */
 		so = (struct socket *)fp->f_data;
+		SOCK_LOCK(so);
 		if (INP_CHECK_SOCKAF(so, PF_INET)) {
 			if (so->so_pcb == NULL)
 				return;
@@ -646,6 +639,7 @@ audit_arg_file(struct proc *p, struct file *fp)
 			    pcb->inp_lport;
 			ARG_SET_VALID(ar, ARG_SOCKINFO);
 		}
+		SOCK_UNLOCK(so);
 		break;
 
 	default:
@@ -669,8 +663,12 @@ audit_arg_upath(struct thread *td, char *upath, u_int64_t flag)
 	struct kaudit_record *ar;
 	char **pathp;
 
-	if (td == NULL || upath == NULL)
-		return;		/* nothing to do! */
+	KASSERT(td != NULL, ("audit_arg_upath: td == NULL"));
+	KASSERT(upath != NULL, ("audit_arg_upath: upath == NULL"));
+
+	ar = currecord();
+	if (ar == NULL)
+		return;
 
 	/*
 	 * XXXAUDIT: Witness warning for possible sleep here?
@@ -680,10 +678,6 @@ audit_arg_upath(struct thread *td, char *upath, u_int64_t flag)
 	KASSERT((flag != ARG_UPATH1) || (flag != ARG_UPATH2),
 	    ("audit_arg_upath: flag %llu", (unsigned long long)flag));
 
-	ar = currecord();
-	if (ar == NULL)
-		return;
-
 	if (flag == ARG_UPATH1)
 		pathp = &ar->k_ar.ar_arg_upath1;
 	else
@@ -720,13 +714,10 @@ audit_arg_vnode(struct vnode *vp, u_int64_t flags)
 	struct vattr vattr;
 	int error;
 	struct vnode_au_info *vnp;
-	struct thread *td;
 
-	/*
-	 * XXXAUDIT: Why is vp possibly NULL here?
-	 */
-	if (vp == NULL)
-		return;
+	KASSERT(vp != NULL, ("audit_arg_vnode: vp == NULL"));
+	KASSERT((flags == ARG_VNODE1) || (flags == ARG_VNODE2),
+	    ("audit_arg_vnode: flags %jd", (intmax_t)flags));
 
 	/*
 	 * Assume that if the caller is calling audit_arg_vnode() on a
@@ -740,17 +731,10 @@ audit_arg_vnode(struct vnode *vp, u_int64_t flags)
 		return;
 
 	/*
-	 * XXXAUDIT: KASSERT argument validity instead?
-	 *
 	 * XXXAUDIT: The below clears, and then resets the flags for valid
 	 * arguments.  Ideally, either the new vnode is used, or the old one
 	 * would be.
 	 */
-	if ((flags & (ARG_VNODE1 | ARG_VNODE2)) == 0)
-		return;
-
-	td = curthread;
-
 	if (flags & ARG_VNODE1) {
 		ar->k_ar.ar_valid_arg &= (ARG_ALL ^ ARG_VNODE1);
 		vnp = &ar->k_ar.ar_arg_vnode1;
@@ -759,7 +743,7 @@ audit_arg_vnode(struct vnode *vp, u_int64_t flags)
 		vnp = &ar->k_ar.ar_arg_vnode2;
 	}
 
-	error = VOP_GETATTR(vp, &vattr, td->td_ucred, td);
+	error = VOP_GETATTR(vp, &vattr, curthread->td_ucred, curthread);
 	if (error) {
 		/* XXX: How to handle this case? */
 		return;
@@ -786,10 +770,17 @@ audit_arg_vnode(struct vnode *vp, u_int64_t flags)
 void
 audit_sysclose(struct thread *td, int fd)
 {
+	struct kaudit_record *ar;
 	struct vnode *vp;
 	struct file *fp;
 	int vfslocked;
 
+	KASSERT(td != NULL, ("audit_sysclose: td == NULL"));
+
+	ar = currecord();
+	if (ar == NULL)
+		return;
+
 	audit_arg_fd(fd);
 
 	if (getvnode(td->td_proc->p_fd, fd, &fp) != 0)