PERFORCE change 45249 for review

[Robert Watson]

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

Change 45249 by rwatson at rwatson_paprika on 2004/01/12 21:13:10

	- Pass thread instead of socket in audit_arg_sockaddr(),
	  audit_arg_upath(), canon_path().
	- Use vn_fullpath() instead of vn_getpath() in canon_path() and
	  audit_arg_vnpath().  Note that we can resolve the XXX relating
	  to lack of locking assertions, because FreeBSD has those.
	- Note the improper locking in canon_path() for file descriptors
	  and vnodes.

Affected files ...

.. //depot/projects/trustedbsd/audit2/sys/security/audit/audit.c#11 edit
.. //depot/projects/trustedbsd/audit2/sys/security/audit/bsm_klib.c#8 edit
.. //depot/projects/trustedbsd/audit2/sys/security/audit/bsm_klib.h#4 edit
.. //depot/projects/trustedbsd/audit2/sys/security/audit/kern_audit.h#10 edit

Differences ...

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

@@ -1127,12 +1127,12 @@
 }
 
 void
-audit_arg_sockaddr(struct proc *p, struct sockaddr *so)
+audit_arg_sockaddr(struct thread *td, struct sockaddr *so)
 {
 	struct kaudit_record *ar;
 
 	ar = currecord();
-	if (ar == NULL || p == NULL || so == NULL)
+	if (ar == NULL || td == NULL || so == NULL)
 		return;
 
 	bcopy(so, &ar->k_ar.ar_arg_sockaddr, sizeof(ar->k_ar.ar_arg_sockaddr));
@@ -1144,7 +1144,7 @@
 		ar->k_ar.ar_valid_arg |= ARG_SADDRINET6;
 		break;
 	case AF_UNIX:
-		audit_arg_upath(p, ((struct sockaddr_un *)so)->sun_path, 
+		audit_arg_upath(td, ((struct sockaddr_un *)so)->sun_path, 
 				ARG_UPATH1);
 		ar->k_ar.ar_valid_arg |= ARG_SADDRUNIX;
 		break;
@@ -1301,12 +1301,12 @@
  * freed when the audit record is freed.
  */
 void
-audit_arg_upath(struct proc *p, char *upath, u_int64_t flags)
+audit_arg_upath(struct thread *td, char *upath, u_int64_t flags)
 {
 	struct kaudit_record *ar;
 	char **pathp;
 
-	if (p == NULL || upath == NULL) 
+	if (td == NULL || upath == NULL) 
 		return;		/* nothing to do! */
 
 	if ((flags & (ARG_UPATH1 | ARG_UPATH2)) == 0)
@@ -1328,7 +1328,7 @@
 	if (*pathp == NULL)
 		pathp = malloc(MAXPATHLEN, M_AUDIT, M_WAITOK);
 
-	canon_path(p, upath, *pathp);
+	canon_path(td, upath, *pathp);
 
 	if (flags & ARG_UPATH1)
 		ar->k_ar.ar_valid_arg |= ARG_UPATH1;
@@ -1356,7 +1356,6 @@
 	struct kaudit_record *ar;
 	struct vattr vattr;
 	int error;
-	int len;
 	char **pathp;
 	struct vnode_au_info *vnp;
 	struct thread *td;
@@ -1389,9 +1388,15 @@
 	if (*pathp == NULL)
 		pathp = malloc(MAXPATHLEN, M_AUDIT, M_WAITOK);
 
-	/* Copy the path looked up by the vn_getpath() function */
-	len = MAXPATHLEN;
-	vn_getpath(vp, *pathp, &len);
+	/*
+	 * Copy the path looked up by the vn_getpath() function.
+	 *
+	 * XXX: Note that in FreeBSD, vn_fullpath() is unreliable, so if
+	 * it fails, we just have a zero-length string.  Perhaps instead
+	 * we should not include a path token...?
+	 */
+	if (vn_fullpath(NULL, vp, *pathp, MAXPATHLEN) != 0)
+		(*pathp)[0] = '\0';
 	if (flags & ARG_VNODE1)
 		ar->k_ar.ar_valid_arg |= ARG_KPATH1;
 	else

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

@@ -722,14 +722,17 @@
  * written to the audit log. So we will leave the filename starting
  * with '/' in the audit log in this case.
  */
-void canon_path(struct proc *p, char *path, char *cpath)
+void canon_path(struct thread *td, char *path, char *cpath)
 {
 	char *bufp;
 	int len;
 	struct vnode *vnp;
 	struct filedesc *fdp;
 
-	fdp = p->p_fd;
+	/*
+	 * XXX: file descriptor locking!
+	 */
+	fdp = td->td_proc->p_fd;
 	bufp = path;
 	if (*(path) == '/') {
 		while (*(bufp) == '/') 
@@ -749,13 +752,21 @@
 		bufp = path;
 	}
 	if (vnp != NULL) {
-		len = MAXPATHLEN;
-		vn_getpath(vnp, cpath, &len);
-		/* The length returned by vn_getpath() is two greater than the 
-		 * number of characters in the string.
+		/*
+		 * XXX: Should lock vnode!
+		 */
+		/*
+		 * XXX: vn_fullpath() on FreeBSD is "less reliable"
+		 * than vn_getpath() on Darwin, so this will need more
+		 * attention in the future.  Also, the question and
+		 * string bounding here seems a bit questionable and
+		 * will also require attention.
 		 */
-		if (len < MAXPATHLEN)
-			cpath[len-2] = '/';	
+		vn_lock(vnp, LK_EXCLUSIVE | LK_RETRY, td);
+		if (vn_fullpath(NULL, vnp, cpath, MAXPATHLEN) != 0)
+			cpath[0] = '\0';
+		VOP_UNLOCK(vnp, 0, td);
+		len = strlen(cpath);
 		strncpy(cpath + len-1, bufp, MAXPATHLEN - len);
 	} else {
 		strncpy(cpath, bufp, MAXPATHLEN);

==== //depot/projects/trustedbsd/audit2/sys/security/audit/bsm_klib.h#4 (text+ko) ====

@@ -34,7 +34,7 @@
 int au_preselect(au_event_t event, au_mask_t *mask_p, int sorf);
 au_event_t flags_to_openevent(int oflags);
 void fill_vattr(struct vattr *v, struct vnode_au_info *vn_info);
-void canon_path(struct proc *p, char *path, char *cpath);
+void	canon_path(struct thread *td, char *path, char *cpath);
 
 int	msgctl_to_event(int cmd);
 int	semctl_to_event(int cmd);

==== //depot/projects/trustedbsd/audit2/sys/security/audit/kern_audit.h#10 (text+ko) ====

@@ -141,10 +141,10 @@
 void			 audit_arg_signum(u_int signum);
 void			 audit_arg_socket(int sodomain, int sotype, 
 						int soprotocol);
-void			 audit_arg_sockaddr(struct proc *p, 
+void			 audit_arg_sockaddr(struct thread *td,
 						struct sockaddr *so);
 void			 audit_arg_auid(uid_t auid);
-void			 audit_arg_upath(struct proc *p, char *upath, 
+void			 audit_arg_upath(struct thread *td, char *upath, 
 					 u_int64_t flags);
 void			 audit_arg_vnpath(struct vnode *vp, u_int64_t flags);
 void			 audit_arg_text(char *text);
To Unsubscribe: send mail to majordomo at trustedbsd.org
with "unsubscribe trustedbsd-cvs" in the body of the message