From owner-p4-projects@FreeBSD.ORG Thu Jun 8 21:52:51 2006 Return-Path: X-Original-To: p4-projects@freebsd.org Delivered-To: p4-projects@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 32767) id 6DBB916A4A7; Thu, 8 Jun 2006 21:52:51 +0000 (UTC) X-Original-To: perforce@freebsd.org Delivered-To: perforce@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 3392816A492 for ; Thu, 8 Jun 2006 21:52:51 +0000 (UTC) (envelope-from bb+lists.freebsd.perforce@cyrus.watson.org) Received: from repoman.freebsd.org (repoman.freebsd.org [216.136.204.115]) by mx1.FreeBSD.org (Postfix) with ESMTP id 46016452CC for ; Thu, 8 Jun 2006 20:06:17 +0000 (GMT) (envelope-from bb+lists.freebsd.perforce@cyrus.watson.org) Received: from repoman.freebsd.org (localhost [127.0.0.1]) by repoman.freebsd.org (8.13.6/8.13.6) with ESMTP id k58K4JZd062312 for ; Thu, 8 Jun 2006 20:04:19 GMT (envelope-from bb+lists.freebsd.perforce@cyrus.watson.org) Received: (from perforce@localhost) by repoman.freebsd.org (8.13.6/8.13.4/Submit) id k58K4I3e062298 for perforce@freebsd.org; Thu, 8 Jun 2006 20:04:18 GMT (envelope-from bb+lists.freebsd.perforce@cyrus.watson.org) Date: Thu, 8 Jun 2006 20:04:18 GMT Message-Id: <200606082004.k58K4I3e062298@repoman.freebsd.org> X-Authentication-Warning: repoman.freebsd.org: perforce set sender to bb+lists.freebsd.perforce@cyrus.watson.org using -f From: Robert Watson To: Perforce Change Reviews Cc: Subject: PERFORCE change 98825 for review X-BeenThere: p4-projects@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: p4 projects tree changes List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 08 Jun 2006 21:52:51 -0000 http://perforce.freebsd.org/chv.cgi?CH=98825 Change 98825 by rwatson@rwatson_sesame on 2006/06/08 20:03:34 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. Affected files ... .. //depot/projects/trustedbsd/audit3/sys/security/audit/audit.h#16 edit .. //depot/projects/trustedbsd/audit3/sys/security/audit/audit_arg.c#17 edit Differences ... ==== //depot/projects/trustedbsd/audit3/sys/security/audit/audit.h#16 (text+ko) ==== @@ -153,7 +153,7 @@ 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); ==== //depot/projects/trustedbsd/audit3/sys/security/audit/audit_arg.c#17 (text+ko) ==== @@ -357,13 +357,14 @@ { 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 @@ 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 @@ 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 @@ { 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, @@ -501,6 +499,8 @@ int first; struct sbuf sb; + KASSERT(iov != NULL, ("audit_arg_iovec: iov == NULL")); + ar = currecord(); if (ar == NULL) return; @@ -635,9 +635,10 @@ 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: @@ -653,14 +654,8 @@ 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; @@ -681,6 +676,7 @@ pcb->inp_lport; ARG_SET_VALID(ar, ARG_SOCKINFO); } + SOCK_UNLOCK(so); break; default: @@ -704,8 +700,12 @@ 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? @@ -715,10 +715,6 @@ 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 @@ -755,13 +751,10 @@ 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 @@ -775,17 +768,10 @@ 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; @@ -794,7 +780,7 @@ 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; @@ -821,10 +807,17 @@ 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)