On Wed, Feb 21, 2018 at 09:46:02AM +0100, Ingo Molnar wrote:
> AFAICS the primary problem appears to be this code path:
>
> audit_receive() -> audit_receive_msg() -> AUDIT_TTY_SET -> audit_log_common_recv_msg() -> audit_log_start()
>
> where we can arrive already holding the lock.
>
> I.e. recursive mutex, kinda.
I _think_ something like the below ought to work, but I've no idea how
to even begin testing audit.
---
kernel/audit.c | 31 ++++++++++++++++++++++++-------
1 file changed, 24 insertions(+), 7 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 227db99b0f19..24175754f79d 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -184,6 +184,9 @@ static char *audit_feature_names[2] = {
/* Serialize requests from userspace. */
DEFINE_MUTEX(audit_cmd_mutex);
+static struct audit_buffer *__audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
+ int type, bool recursive);
+
/* AUDIT_BUFSIZ is the size of the temporary buffer used for formatting
* audit records. Since printk uses a 1024 byte buffer, this buffer
* should be at least that large. */
@@ -357,7 +360,7 @@ static int audit_log_config_change(char *function_name, u32 new, u32 old,
struct audit_buffer *ab;
int rc = 0;
- ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
+ ab = __audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE, true);
if (unlikely(!ab))
return rc;
audit_log_format(ab, "%s=%u old=%u", function_name, new, old);
@@ -1024,7 +1027,7 @@ static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type)
return;
}
- *ab = audit_log_start(NULL, GFP_KERNEL, msg_type);
+ *ab = __audit_log_start(NULL, GFP_KERNEL, msg_type, true);
if (unlikely(!*ab))
return;
audit_log_format(*ab, "pid=%d uid=%u", pid, uid);
@@ -1057,7 +1060,7 @@ static void audit_log_feature_change(int which, u32 old_feature, u32 new_feature
if (audit_enabled == AUDIT_OFF)
return;
- ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_FEATURE_CHANGE);
+ ab = __audit_log_start(NULL, GFP_KERNEL, AUDIT_FEATURE_CHANGE, true);
audit_log_task_info(ab, current);
audit_log_format(ab, " feature=%s old=%u new=%u old_lock=%u new_lock=%u res=%d",
audit_feature_names[which], !!old_feature, !!new_feature,
@@ -1578,6 +1581,12 @@ static int __init audit_enable(char *str)
if (audit_default == AUDIT_OFF)
audit_initialized = AUDIT_DISABLED;
+ /*
+ * Normally audit_set_enabled() would need to be called under
+ * @audit_cmd_mutex, however since audit_do_config_change() will not in
+ * fact call audit_log_config_change() when 'audit_enabled ==
+ * AUDIT_OFF', we can use it here without issue.
+ */
if (audit_set_enabled(audit_default))
panic("audit: error setting audit state (%d)\n", audit_default);
@@ -1690,8 +1699,8 @@ static inline void audit_get_stamp(struct audit_context *ctx,
* will be written at syscall exit. If there is no associated task, then
* task context (ctx) should be NULL.
*/
-struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
- int type)
+static struct audit_buffer *__audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
+ int type, bool recursive)
{
struct audit_buffer *ab;
struct timespec64 t;
@@ -1703,6 +1712,9 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
if (unlikely(!audit_filter(type, AUDIT_FILTER_TYPE)))
return NULL;
+ if (recursive)
+ lockdep_assert_held(&audit_cmd_mutex);
+
/* NOTE: don't ever fail/sleep on these two conditions:
* 1. auditd generated record - since we need auditd to drain the
* queue; also, when we are checking for auditd, compare PIDs using
@@ -1710,8 +1722,7 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
* using a PID anchored in the caller's namespace
* 2. generator holding the audit_cmd_mutex - we don't want to block
* while holding the mutex */
- if (!(auditd_test_task(current) ||
- (current == __mutex_owner(&audit_cmd_mutex)))) {
+ if (!(auditd_test_task(current) || recursive)) {
long stime = audit_backlog_wait_time;
while (audit_backlog_limit &&
@@ -1753,6 +1764,12 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
return ab;
}
+struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask,
+ int type)
+{
+ return __audit_log_start(ctx, gfp_mask, type, false);
+}
+
/**
* audit_expand - expand skb in the audit buffer
* @ab: audit_buffer