PERFORCE change 167043 for review

[Robert Watson]

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

Change 167043 by rwatson at rwatson_cinnamon on 2009/08/05 18:27:20

	Add a few comments/annotations in the audit slice code about
	possibly design choices.

Affected files ...

.. //depot/projects/soc2009/marinosi_appaudit/src/sys/security/audit/audit_slice.c#6 edit

Differences ...

==== //depot/projects/soc2009/marinosi_appaudit/src/sys/security/audit/audit_slice.c#6 (text+ko) ====

@@ -90,7 +90,7 @@
 /*
  * Audit slice's device open method.  Explicit privilege check isn't used as 
  * this allows file permissions on the special device to be used to grant 
- * audit review access.  Those file permissions should be managed carefully.
+ * audit submit access.  Those file permissions should be managed carefully.
  */
 static int
 audit_slice_dev_open(struct cdev *dev, int oflags, int devtype, 
@@ -107,7 +107,16 @@
 	as = as_ptr;
 	dev->si_drv1 = as;
 
-	/* Only one process may open the device at a time. */
+	/*
+	 * Only one process may open the device at a time.
+	 *
+	 * XXXRW: It would be desirable to allow concurrent writers to block
+	 * or otherwise be serialized in order to avoid application threads
+	 * having to loop waiting for the device to be free while another
+	 * thread is writing a record.  Because we want to support buffered
+	 * writes spread over multiple syscalls, this is a little tricky --
+	 * we should think more about what the right way to handle this is.
+	 */
 	mtx_lock(&(as->as_dev_mtx));
 	if (!as->as_dev_isopen) {
 		error = 0;
@@ -170,6 +179,13 @@
 	audit_slice_dev_buf = malloc(sizeof(*audit_slice_dev_buf),
 			M_TEMP, M_WAITOK);
 
+	/*
+	 * XXXRW: This seems to handle multiple records/system call, but not
+	 * multiple system calls/record.  To handle the latter, we need to
+	 * have a buffer that potentially spans system calls that we can copy
+	 * into until we have a complete record which we can then submit to
+	 * audit.
+	 */
 	while (uio->uio_resid > 0) {
 		c = MIN((int)uio->uio_resid, sizeof(*audit_slice_dev_buf));
 		if ( c == (int)uio->uio_resid )
@@ -183,6 +199,10 @@
 		/*
 		 * Store the actual record's size. Add some checks before
 		 * this.
+		 *
+		 * XXXRW: for example, perhaps we shouldn't accept records
+		 * longer than MAX_AUDIT_RECORD_SIZE, or less than
+		 * sizeof(*audit_slice_dev_buf).
 		 */
 		recsz = be32dec (audit_slice_dev_buf->rec_byte_count);
 		as_rec = (void *)malloc((unsigned long)recsz, M_AUDITBSM, 
@@ -200,6 +220,9 @@
 		audit_slice_commit_rec( as_rec, as);
 	}
 
+	/*
+	 * XXXRW: Only free as_rec if we allocated it.
+	 */
 	free(audit_slice_dev_buf, M_TEMP);
 	free(as_rec, M_AUDITBSM);
 
@@ -207,7 +230,12 @@
 }
 
 /*
- * Ioctl method
+ * Ioctl method.
+ *
+ * XXXRW: Possibly we should allow querying per-slice audit properties here,
+ * such as event masks so that userspace can decide whether an event is
+ * wanted -- i.e., similar to what we do with auditon(2) to query the current
+ * mask.
  */
 static int
 audit_slice_dev_ioctl(struct cdev *dev, u_long cmd, caddr_t data,  int flag, 
@@ -219,6 +247,9 @@
 
 /*
  * Poll method.(if needed)
+ *
+ * XXXRW: Interesting design question -- probably, it's OK to submit a record
+ * and then block waiting for it to commit.
  */
 static int
 audit_slice_dev_poll(struct cdev *dev, int events, struct thread *td)