Date: Thu, 15 Jan 2004 08:18:07 -0800 (PST) From: Robert Watson <rwatson@FreeBSD.org> To: Perforce Change Reviews <perforce@freebsd.org> Subject: PERFORCE change 45390 for review Message-ID: <200401151618.i0FGI7O9097403@repoman.freebsd.org>
next in thread | raw e-mail | index | archive | help
http://perforce.freebsd.org/chv.cgi?CH=45390 Change 45390 by rwatson@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? */
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200401151618.i0FGI7O9097403>