PERFORCE change 167044 for review
Robert Watson
rwatson at FreeBSD.org
Wed Aug 5 19:12:43 UTC 2009
http://perforce.freebsd.org/chv.cgi?CH=167044
Change 167044 by rwatson at rwatson_cinnamon on 2009/08/05 19:08:03
Various questions, comments, and annotations.
Affected files ...
.. //depot/projects/soc2009/marinosi_appaudit/src/sys/security/audit/audit.c#10 edit
.. //depot/projects/soc2009/marinosi_appaudit/src/sys/security/audit/audit.h#3 edit
.. //depot/projects/soc2009/marinosi_appaudit/src/sys/security/audit/audit_private.h#6 edit
.. //depot/projects/soc2009/marinosi_appaudit/src/sys/security/audit/audit_slice.h#10 edit
Differences ...
==== //depot/projects/soc2009/marinosi_appaudit/src/sys/security/audit/audit.c#10 (text) ====
@@ -92,7 +92,11 @@
/* Audit slice ptr -helper */
struct audit_slice *as_ptr = NULL;
-/* Audit slices queue */
+/*
+ * Audit slices queue.
+ *
+ * XXXRW: What synchronizes access to this list?
+ */
struct audit_slice_queue audit_slice_q;
/*
@@ -209,6 +213,9 @@
* base(no reason to be in the queue). We want the queue initialized
* once, even if there are no other slices except the base one and
* thus this is happening in audit_init().
+ *
+ * XXXRW: Why might audit_init() ever be called with a non-NULL base
+ * slice?
*/
TAILQ_INIT(&audit_slice_q);
if ( audit_base_slice == NULL ) {
@@ -220,6 +227,10 @@
as = audit_base_slice;
}
+ /*
+ * XXXRW: as should always be non-NULL here so should make this
+ * unconditional.
+ */
if ( as != NULL )
audit_slice_init(as, "base_slice");
@@ -250,6 +261,8 @@
*
* XXXRW: In FreeBSD 7.x and 8.x, this fails to wait for the record queue to
* drain before returning, which could lead to lost records on shutdown.
+ *
+ * XXXRW: Presumably we need this to iterate over all slices?
*/
void
audit_shutdown(void *arg, int howto)
@@ -314,6 +327,9 @@
uma_zfree(audit_record_zone, ar);
}
+/*
+ * XXXRW: Should this accept an audit slice argument?
+ */
void
audit_commit(struct kaudit_record *ar, int error, int retval)
{
@@ -639,17 +655,34 @@
struct audit_slice *as = NULL;
int err;
+ /*
+ * XXXRW: With M_WAITOK, malloc(9) never fails, so no error handling
+ * needed here.
+ *
+ * XXXRW: It might be desirable to wrap all of this in an sx lock
+ * to serialize the creation of audit slices, probably the same lock
+ * used to serialize access to audit_slice_q. That would prevent
+ * attempts to remove a slice while it was still being added, etc.
+ */
err = 0;
as = malloc(sizeof(*as), M_AUDITSLICE, M_WAITOK | M_ZERO);
if ( as == NULL )
err = 1; /* Failed to allocate slice */
+ /*
+ * XXXRW: Locking needed here. Possibly we should fully initialize
+ * the slice before inserting it on the list?
+ */
as_ptr = as;
TAILQ_INSERT_TAIL(&audit_slice_q, as, as_q);
/* Initialize the base slice */
audit_slice_init(as, name);
+ /*
+ * XXXRW: Possibly start worker before creating the device?
+ */
+
/* Create the special device node */
audit_slice_cdev_init(as);
@@ -664,6 +697,8 @@
void
audit_slice_init(struct audit_slice *as, char *name)
{
+
+ /* XXXRW: Does the caller validate the length limit? */
strcpy(as->as_name, name);
/*
@@ -698,6 +733,10 @@
audit_kinfo.ai_termid.at_type = AU_IPv4;
audit_kinfo.ai_termid.at_addr[0] = INADDR_ANY;
+ /*
+ * XXXRW: KINFO_LOCK_INIT() should be in audit_init() rather than
+ * here conditionally?
+ */
mtx_init(&(as->audit_mtx), "audit_mtx", NULL, MTX_DEF);
if ( as == audit_base_slice )
KINFO_LOCK_INIT();
@@ -712,10 +751,25 @@
/*
* audit_slice_destroy() is called through A_REMOVESLICE command of auditon()
* syscall to remove an existing slice ( except the base one!)
+ *
+ * XXXRW: likewise, should acquire an sx lock around all of this to avoid
+ * simultaneous add/remove being a problem.
+ *
+ * XXXR: Does the caller validate that as != audit_base_slice? Perhaps
+ * audit_slice_destroy() should do the name->slice lookup and potentially
+ * return an error here.
*/
void
audit_slice_destroy(struct audit_slice *as)
{
+
+ /*
+ * XXXRW: Should either assert the record queue is empty, or drain
+ * it, depending on invariants.
+ *
+ * XXXRW: Need to mtx_destroy the lock, cv_destroy the condition
+ * variables?
+ */
if (as != NULL) {
TAILQ_REMOVE(&audit_slice_q, as, as_q);
destroy_dev(as->as_dev);
@@ -735,7 +789,14 @@
int error;
-
+ /*
+ * XXXRW: Audit slices other than the base should probably never
+ * touch td->td_ar, so the below should unconditionally allocate the
+ * container record.
+ *
+ * XXXRW: Asser that this isn't audit_base_slice, since we'll handle
+ * that improperly?
+ */
/* struct thread uninitialized! */
if (ar == NULL) {
@@ -746,6 +807,10 @@
*
* XXXAUDIT: Maybe AUE_AUDIT in the system call context and
* special pre-select handling?
+ *
+ * XXXRW: Not sure we need to use td->td_ar here at all? The
+ * base slice may already be using it to record the write
+ * syscall.
*/
td->td_ar = audit_new(AUE_NULL, td, as);
if (td->td_ar == NULL)
@@ -755,6 +820,11 @@
ar = td->td_ar;
}
+ /*
+ * XXXRW: This error value seems never to be used? Possibly we
+ * should validate the record before calling audit_new, and return
+ * the error to the caller so it can return to userspace?
+ */
/* Verify the record. */
if (bsm_rec_verify(rec) == 0)
error = EINVAL;
==== //depot/projects/soc2009/marinosi_appaudit/src/sys/security/audit/audit.h#3 (text) ====
@@ -52,6 +52,8 @@
* observed but should not be directly manipulated. The audit suspension
* flag permits audit to be temporarily disabled without reconfiguring the
* audit target.
+ *
+ * XXXRW: These are no longer needed if they're per-slice.
*/
extern int audit_enabled;
extern int audit_suspended;
@@ -190,6 +192,11 @@
audit_arg_ ## op (args); \
} while (0)
+
+/*
+ * XXXRW: Perhaps we should have audit_base_enabled or such as a global to
+ * avoid an extra pointer deref for every syscall?
+ */
#define AUDIT_SYSCALL_ENTER(code, td) do { \
if (audit_base_slice->audit_enabled) { \
audit_syscall_enter(code, td); \
==== //depot/projects/soc2009/marinosi_appaudit/src/sys/security/audit/audit_private.h#6 (text) ====
@@ -138,6 +138,11 @@
* Helper data structure that keeps the data that are needed for new audit
* slice creation/modification/removal.This structure will be used with the
* auditon() syscall for all the audit slices except the base.
+ *
+ * XXXRW: Don't expose kernel data structures/pointers to userspace -- for
+ * example, audit_cred and audit_vp. I wonder if we should have an
+ * auditon_slice(2) system call that accepts regular auditon(2) arguments
+ * with the addition of a string pointer identifying the slice to operate on?
*/
struct au_slice_data {
char as_name[AUDIT_SLICE_NAME_LEN];
==== //depot/projects/soc2009/marinosi_appaudit/src/sys/security/audit/audit_slice.h#10 (text+ko) ====
@@ -69,6 +69,11 @@
*/
int audit_panic_on_write_fail;
int audit_fail_stop;
+
+ /*
+ * XXXRW: Maybe these should remain global rather than per-slice, as
+ * they apply only to the base slice?
+ */
int audit_argv;
int audit_arge;
More information about the p4-projects
mailing list