Date: Wed, 5 Aug 2009 18:27:52 GMT From: Robert Watson <rwatson@FreeBSD.org> To: Perforce Change Reviews <perforce@freebsd.org> Subject: PERFORCE change 167043 for review Message-ID: <200908051827.n75IRqaZ084215@repoman.freebsd.org>
next in thread | raw e-mail | index | archive | help
http://perforce.freebsd.org/chv.cgi?CH=167043 Change 167043 by rwatson@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)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200908051827.n75IRqaZ084215>