From owner-p4-projects@FreeBSD.ORG Wed Aug 5 19:12:42 2009 Return-Path: Delivered-To: p4-projects@freebsd.org Received: by hub.freebsd.org (Postfix, from userid 32767) id 0BFB9106567A; Wed, 5 Aug 2009 19:12:42 +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 B88B2106564A for ; Wed, 5 Aug 2009 19:12:41 +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 A56158FC1A for ; Wed, 5 Aug 2009 19:12:41 +0000 (UTC) Received: from repoman.freebsd.org (localhost [127.0.0.1]) by repoman.freebsd.org (8.14.3/8.14.3) with ESMTP id n75J8a1w088956 for ; Wed, 5 Aug 2009 19:08:36 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 n75J8atq088954 for perforce@freebsd.org; Wed, 5 Aug 2009 19:08:36 GMT (envelope-from bb+lists.freebsd.perforce@cyrus.watson.org) Date: Wed, 5 Aug 2009 19:08:36 GMT Message-Id: <200908051908.n75J8atq088954@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 167044 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 19:12:43 -0000 http://perforce.freebsd.org/chv.cgi?CH=167044 Change 167044 by rwatson@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;