Date: Mon, 12 Jan 2004 21:13:28 -0800 (PST) From: Robert Watson <rwatson@FreeBSD.org> To: Perforce Change Reviews <perforce@freebsd.org> Subject: PERFORCE change 45249 for review Message-ID: <200401130513.i0D5DSBu002924@repoman.freebsd.org>
next in thread | raw e-mail | index | archive | help
http://perforce.freebsd.org/chv.cgi?CH=45249 Change 45249 by rwatson@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);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200401130513.i0D5DSBu002924>