From owner-p4-projects@FreeBSD.ORG Wed Aug 5 18:27:53 2009 Return-Path: Delivered-To: p4-projects@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 32767) id BD9F1106566C; Wed, 5 Aug 2009 18:27:52 +0000 (UTC) Delivered-To: perforce@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 64FD11065672 for ; Wed, 5 Aug 2009 18:27:52 +0000 (UTC) (envelope-from bb+lists.freebsd.perforce@cyrus.watson.org) Received: from repoman.freebsd.org (repoman.freebsd.org [IPv6:2001:4f8:fff6::29]) by mx1.freebsd.org (Postfix) with ESMTP id 4962C8FC0A for ; Wed, 5 Aug 2009 18:27:52 +0000 (UTC) (envelope-from bb+lists.freebsd.perforce@cyrus.watson.org) Received: from repoman.freebsd.org (localhost [127.0.0.1]) by repoman.freebsd.org (8.14.3/8.14.3) with ESMTP id n75IRqjB084217 for ; Wed, 5 Aug 2009 18:27:52 GMT (envelope-from bb+lists.freebsd.perforce@cyrus.watson.org) Received: (from perforce@localhost) by repoman.freebsd.org (8.14.3/8.14.3/Submit) id n75IRqaZ084215 for perforce@freebsd.org; Wed, 5 Aug 2009 18:27:52 GMT (envelope-from bb+lists.freebsd.perforce@cyrus.watson.org) Date: Wed, 5 Aug 2009 18:27:52 GMT Message-Id: <200908051827.n75IRqaZ084215@repoman.freebsd.org> X-Authentication-Warning: repoman.freebsd.org: perforce set sender to bb+lists.freebsd.perforce@cyrus.watson.org using -f From: Robert Watson To: Perforce Change Reviews Cc: Subject: PERFORCE change 167043 for review X-BeenThere: p4-projects@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: p4 projects tree changes List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 05 Aug 2009 18:27:53 -0000 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)