PERFORCE change 98825 for review
Robert Watson
rwatson at FreeBSD.org
Thu Jun 8 22:10:29 UTC 2006
http://perforce.freebsd.org/chv.cgi?CH=98825
Change 98825 by rwatson at 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)
More information about the trustedbsd-cvs
mailing list