Rework the logic around quick checks for auditing that take place at

system-call entry and whenever audit arguments or return values are
captured:

1. Expose a single global, audit_syscalls_enabled, which controls
   whether the audit framework is entered, rather than exposing
   components of the policy -- e.g., if the trail is enabled,
   suspended, etc.

2. Introduce a new function audit_syscalls_enabled_update(), which is
   called to update audit_syscalls_enabled whenever an aspect of the
   policy changes, so that the value can be updated.

3. Remove a check of trail enablement/suspension from audit_new() --
   at the point where this function has been entered, we believe that
   system-call auditing is already in force, or we wouldn't get here,
   so simply proceed to more expensive policy checks.

4. Use an audit-provided global, audit_dtrace_enabled, rather than a
   dtaudit-provided global, to provide policy indicating whether
   dtaudit would like system calls to be audited.

5. Do some minor cosmetic renaming to clarify what various variables
   are for.

These changes collectively arrange it so that traditional audit
(trail, pipes) or the DTrace audit provider can enable system-call
probes without the other configured.  Otherwise, dtaudit cannot
capture system-call data without auditd(8) started.

Reviewed by:		gnn
Sponsored by:		DARPA, AFRL
Approved by:		re (gjb)
Differential Revision:	https://reviews.freebsd.org/D17348

sys/security/audit/audit.c

@@ -2,7 +2,7 @@
  * SPDX-License-Identifier: BSD-3-Clause
  *
  * Copyright (c) 1999-2005 Apple Inc.
- * Copyright (c) 2006-2007, 2016-2017 Robert N. M. Watson
+ * Copyright (c) 2006-2007, 2016-2018 Robert N. M. Watson
  * All rights reserved.
  *
  * Portions of this software were developed by BAE Systems, the University of
@@ -98,8 +98,12 @@ static SYSCTL_NODE(_security, OID_AUTO, audit, CTLFLAG_RW, 0,
  *
  * Define the audit control flags.
  */
-int __read_frequently	audit_enabled;
-int			audit_suspended;
+int			audit_trail_enabled;
+int			audit_trail_suspended;
+#ifdef KDTRACE_HOOKS
+u_int			audit_dtrace_enabled;
+#endif
+int __read_frequently	audit_syscalls_enabled;
 
 /*
  * Flags controlling behavior in low storage situations.  Should we panic if
@@ -198,6 +202,33 @@ static struct rwlock		audit_kinfo_lock;
 #define	KINFO_RUNLOCK()		rw_runlock(&audit_kinfo_lock)
 #define	KINFO_WUNLOCK()		rw_wunlock(&audit_kinfo_lock)
 
+/*
+ * Check various policies to see if we should enable system-call audit hooks.
+ * Note that despite the mutex being held, we want to assign a value exactly
+ * once, as checks of the flag are performed lock-free for performance
+ * reasons.  The mutex is used to get a consistent snapshot of policy state --
+ * e.g., safely accessing the two audit_trail flags.
+ */
+void
+audit_syscalls_enabled_update(void)
+{
+
+	mtx_lock(&audit_mtx);
+#ifdef KDTRACE_HOOKS
+	if (audit_dtrace_enabled)
+		audit_syscalls_enabled = 1;
+	else {
+#endif
+		if (audit_trail_enabled && !audit_trail_suspended)
+			audit_syscalls_enabled = 1;
+		else
+			audit_syscalls_enabled = 0;
+#ifdef KDTRACE_HOOKS
+	}
+#endif
+	mtx_unlock(&audit_mtx);
+}
+
 void
 audit_set_kinfo(struct auditinfo_addr *ak)
 {
@@ -303,8 +334,9 @@ static void
 audit_init(void)
 {
 
-	audit_enabled = 0;
-	audit_suspended = 0;
+	audit_trail_enabled = 0;
+	audit_trail_suspended = 0;
+	audit_syscalls_enabled = 0;
 	audit_panic_on_write_fail = 0;
 	audit_fail_stop = 0;
 	audit_in_failure = 0;
@@ -337,6 +369,9 @@ audit_init(void)
 	    sizeof(struct kaudit_record), audit_record_ctor,
 	    audit_record_dtor, NULL, NULL, UMA_ALIGN_PTR, 0);
 
+	/* First initialisation of audit_syscalls_enabled. */
+	audit_syscalls_enabled_update();
+
 	/* Initialize the BSM audit subsystem. */
 	kau_init();
 
@@ -378,24 +413,13 @@ currecord(void)
 }
 
 /*
- * XXXAUDIT: There are a number of races present in the code below due to
- * release and re-grab of the mutex.  The code should be revised to become
- * slightly less racy.
- *
  * XXXAUDIT: Shouldn't there be logic here to sleep waiting on available
  * pre_q space, suspending the system call until there is room?
  */
 struct kaudit_record *
 audit_new(int event, struct thread *td)
 {
 	struct kaudit_record *ar;
-	int no_record;
-
-	mtx_lock(&audit_mtx);
-	no_record = (audit_suspended || !audit_enabled);
-	mtx_unlock(&audit_mtx);
-	if (no_record)
-		return (NULL);
 
 	/*
 	 * Note: the number of outstanding uncommitted audit records is
@@ -529,9 +553,13 @@ audit_commit(struct kaudit_record *ar, int error, int retval)
 	/*
 	 * Note: it could be that some records initiated while audit was
 	 * enabled should still be committed?
+	 *
+	 * NB: The check here is not for audit_syscalls because any
+	 * DTrace-related obligations have been fulfilled above -- we're just
+	 * down to the trail and pipes now.
 	 */
 	mtx_lock(&audit_mtx);
-	if (audit_suspended || !audit_enabled) {
+	if (audit_trail_suspended || !audit_trail_enabled) {
 		audit_pre_q_len--;
 		mtx_unlock(&audit_mtx);
 		audit_free(ar);
@@ -557,6 +585,10 @@ audit_commit(struct kaudit_record *ar, int error, int retval)
  * responsible for deciding whether or not to audit the call (preselection),
  * and if so, allocating a per-thread audit record.  audit_new() will fill in
  * basic thread/credential properties.
+ *
+ * This function will be entered only if audit_syscalls_enabled was set in the
+ * macro wrapper for this function.  It could be cleared by the time this
+ * function runs, but that is an acceptable race.
  */
 void
 audit_syscall_enter(unsigned short code, struct thread *td)

sys/security/audit/audit.h

@@ -2,7 +2,7 @@
  * SPDX-License-Identifier: BSD-3-Clause
  *
  * Copyright (c) 1999-2005 Apple Inc.
- * Copyright (c) 2016-2017 Robert N. M. Watson
+ * Copyright (c) 2016-2018 Robert N. M. Watson
  * All rights reserved.
  *
  * This software was developed by BAE Systems, the University of Cambridge
@@ -55,14 +55,23 @@
 #include <sys/sysctl.h>
 
 /*
- * Audit subsystem condition flags.  The audit_enabled flag is set and
+ * Audit subsystem condition flags.  The audit_trail_enabled flag is set and
  * removed automatically as a result of configuring log files, and can be
  * observed but should not be directly manipulated.  The audit suspension
  * flag permits audit to be temporarily disabled without reconfiguring the
  * audit target.
+ *
+ * As DTrace can also request system-call auditing, a further
+ * audit_syscalls_enabled flag tracks whether newly entering system calls
+ * should be considered for auditing or not.
+ *
+ * XXXRW: Move trail flags to audit_private.h, as they no longer need to be
+ * visible outside the audit code...?
  */
-extern int	audit_enabled;
-extern int	audit_suspended;
+extern u_int	audit_dtrace_enabled;
+extern int	audit_trail_enabled;
+extern int	audit_trail_suspended;
+extern int	audit_syscalls_enabled;
 
 void	 audit_syscall_enter(unsigned short code, struct thread *td);
 void	 audit_syscall_exit(int error, struct thread *td);
@@ -139,7 +148,7 @@ void	 audit_thread_free(struct thread *td);
 
 /*
  * Define macros to wrap the audit_arg_* calls by checking the global
- * audit_enabled flag before performing the actual call.
+ * audit_syscalls_enabled flag before performing the actual call.
  */
 #define	AUDITING_TD(td)		((td)->td_pflags & TDP_AUDITREC)
 
@@ -369,15 +378,15 @@ void	 audit_thread_free(struct thread *td);
 } while (0)
 
 #define	AUDIT_SYSCALL_ENTER(code, td)	do {				\
-	if (audit_enabled) {						\
+	if (audit_syscalls_enabled) {					\
 		audit_syscall_enter(code, td);				\
 	}								\
 } while (0)
 
 /*
  * Wrap the audit_syscall_exit() function so that it is called only when
  * we have a audit record on the thread.  Audit records can persist after
- * auditing is disabled, so we don't just check audit_enabled here.
+ * auditing is disabled, so we don't just check audit_syscalls_enabled here.
  */
 #define	AUDIT_SYSCALL_EXIT(error, td)	do {				\
 	if (td->td_pflags & TDP_AUDITREC)				\

sys/security/audit/audit_dtrace.c

@@ -1,5 +1,5 @@
 /*-
- * Copyright (c) 2016 Robert N. M. Watson
+ * Copyright (c) 2016, 2018 Robert N. M. Watson
  * All rights reserved.
  *
  * This software was developed by BAE Systems, the University of Cambridge
@@ -147,8 +147,12 @@ static dtrace_provider_id_t	dtaudit_id;
  * maintain a global flag tracking whether any dtaudit probes are enabled.  If
  * not, don't bother doing all that work whenever potential queries about
  * events turn up during preselection or commit.
+ *
+ * NB: We used to maintain our own variable in dtaudit, but now use the
+ * centralized audit_dtrace_enabled variable imported from the audit code.
+ *
+ * static uint_t		dtaudit_probes_enabled;
  */
-static uint_t		dtaudit_probes_enabled;
 
 /*
  * Check dtaudit policy for the event to see whether this is an event we would
@@ -179,7 +183,7 @@ dtaudit_preselect(au_id_t auid, au_event_t event, au_class_t class)
 	 * NB: Lockless reads here may return a slightly stale value; this is
 	 * considered better than acquiring a lock, however.
 	 */
-	if (!dtaudit_probes_enabled)
+	if (!audit_dtrace_enabled)
 		return (NULL);
 	ene = au_evnamemap_lookup(event);
 	if (ene == NULL)
@@ -457,7 +461,8 @@ dtaudit_enable(void *arg, dtrace_id_t id, void *parg)
 		ene->ene_commit_probe_enabled = 1;
 	else
 		ene->ene_bsm_probe_enabled = 1;
-	refcount_acquire(&dtaudit_probes_enabled);
+	refcount_acquire(&audit_dtrace_enabled);
+	audit_syscalls_enabled_update();
 }
 
 static void
@@ -474,7 +479,8 @@ dtaudit_disable(void *arg, dtrace_id_t id, void *parg)
 		ene->ene_commit_probe_enabled = 0;
 	else
 		ene->ene_bsm_probe_enabled = 0;
-	(void)refcount_release(&dtaudit_probes_enabled);
+	(void)refcount_release(&audit_dtrace_enabled);
+	audit_syscalls_enabled_update();
 }
 
 static void

sys/security/audit/audit_private.h

@@ -2,7 +2,7 @@
  * SPDX-License-Identifier: BSD-3-Clause
  *
  * Copyright (c) 1999-2009 Apple Inc.
- * Copyright (c) 2016-2017 Robert N. M. Watson
+ * Copyright (c) 2016, 2018 Robert N. M. Watson
  * All rights reserved.
  *
  * Portions of this software were developed by BAE Systems, the University of
@@ -343,6 +343,13 @@ void			 audit_commit(struct kaudit_record *ar, int error,
 			    int retval);
 struct kaudit_record	*audit_new(int event, struct thread *td);
 
+/*
+ * Function to update the audit_syscalls_enabled flag, whose value is affected
+ * by configuration of the audit trail/pipe mechanism and DTrace.  Call this
+ * function when any of the inputs to that policy change.
+ */
+void	audit_syscalls_enabled_update(void);
+
 /*
  * Functions relating to the conversion of internal kernel audit records to
  * the BSM file format.

sys/security/audit/audit_syscalls.c

@@ -2,7 +2,7 @@
  * SPDX-License-Identifier: BSD-3-Clause
  *
  * Copyright (c) 1999-2009 Apple Inc.
- * Copyright (c) 2016 Robert N. M. Watson
+ * Copyright (c) 2016, 2018 Robert N. M. Watson
  * All rights reserved.
  *
  * Portions of this software were developed by BAE Systems, the University of
@@ -368,15 +368,15 @@ sys_auditon(struct thread *td, struct auditon_args *uap)
 	case A_OLDGETCOND:
 	case A_GETCOND:
 		if (uap->length == sizeof(udata.au_cond64)) {
-			if (audit_enabled && !audit_suspended)
+			if (audit_trail_enabled && !audit_trail_suspended)
 				udata.au_cond64 = AUC_AUDITING;
 			else
 				udata.au_cond64 = AUC_NOAUDIT;
 			break;
 		}
 		if (uap->length != sizeof(udata.au_cond))
 			return (EINVAL);
-		if (audit_enabled && !audit_suspended)
+		if (audit_trail_enabled && !audit_trail_suspended)
 			udata.au_cond = AUC_AUDITING;
 		else
 			udata.au_cond = AUC_NOAUDIT;
@@ -386,25 +386,27 @@ sys_auditon(struct thread *td, struct auditon_args *uap)
 	case A_SETCOND:
 		if (uap->length == sizeof(udata.au_cond64)) {
 			if (udata.au_cond64 == AUC_NOAUDIT)
-				audit_suspended = 1;
+				audit_trail_suspended = 1;
 			if (udata.au_cond64 == AUC_AUDITING)
-				audit_suspended = 0;
+				audit_trail_suspended = 0;
 			if (udata.au_cond64 == AUC_DISABLED) {
-				audit_suspended = 1;
+				audit_trail_suspended = 1;
 				audit_shutdown(NULL, 0);
 			}
+			audit_syscalls_enabled_update();
 			break;
 		}
 		if (uap->length != sizeof(udata.au_cond))
 			return (EINVAL);
 		if (udata.au_cond == AUC_NOAUDIT)
-			audit_suspended = 1;
+			audit_trail_suspended = 1;
 		if (udata.au_cond == AUC_AUDITING)
-			audit_suspended = 0;
+			audit_trail_suspended = 0;
 		if (udata.au_cond == AUC_DISABLED) {
-			audit_suspended = 1;
+			audit_trail_suspended = 1;
 			audit_shutdown(NULL, 0);
 		}
+		audit_syscalls_enabled_update();
 		break;
 
 	case A_GETCLASS:
@@ -826,10 +828,11 @@ sys_auditctl(struct thread *td, struct auditctl_args *uap)
 	crhold(cred);
 
 	/*
-	 * XXXAUDIT: Should audit_suspended actually be cleared by
+	 * XXXAUDIT: Should audit_trail_suspended actually be cleared by
 	 * audit_worker?
 	 */
-	audit_suspended = 0;
+	audit_trail_suspended = 0;
+	audit_syscalls_enabled_update();
 
 	audit_rotate_vnode(cred, vp);
 

sys/security/audit/audit_worker.c

@@ -2,7 +2,7 @@
  * SPDX-License-Identifier: BSD-3-Clause
  *
  * Copyright (c) 1999-2008 Apple Inc.
- * Copyright (c) 2006-2008, 2016 Robert N. M. Watson
+ * Copyright (c) 2006-2008, 2016, 2018 Robert N. M. Watson
  * All rights reserved.
  *
  * Portions of this software were developed by BAE Systems, the University of
@@ -305,7 +305,8 @@ audit_record_write(struct vnode *vp, struct ucred *cred, void *data,
 		    "Audit log space exhausted and fail-stop set.");
 	}
 	(void)audit_send_trigger(AUDIT_TRIGGER_NO_SPACE);
-	audit_suspended = 1;
+	audit_trail_suspended = 1;
+	audit_syscalls_enabled_update();
 
 	/* FALLTHROUGH */
 fail:
@@ -518,7 +519,8 @@ audit_rotate_vnode(struct ucred *cred, struct vnode *vp)
 	audit_vp = vp;
 	audit_size = vattr.va_size;
 	audit_file_rotate_wait = 0;
-	audit_enabled = (audit_vp != NULL);
+	audit_trail_enabled = (audit_vp != NULL);
+	audit_syscalls_enabled_update();
 	AUDIT_WORKER_UNLOCK();
 
 	/*