From 32a1dbaece7e37cea415e03cd426172249aa859e Mon Sep 17 00:00:00 2001 From: Richard Guy Briggs Date: Wed, 4 Nov 2015 08:23:50 -0500 Subject: audit: try harder to send to auditd upon netlink failure There are several reports of the kernel losing contact with auditd when it is, in fact, still running. When this happens, kernel syslogs show: "audit: *NO* daemon at audit_pid=" although auditd is still running, and is apparently happy, listening on the netlink socket. The pid in the "*NO* daemon" message matches the pid of the running auditd process. Restarting auditd solves this. The problem appears to happen randomly, and doesn't seem to be strongly correlated to the rate of audit events being logged. The problem happens fairly regularly (every few days), but not yet reproduced to order. On production kernels, BUG_ON() is a no-op, so any error will trigger this. Commit 34eab0a7cd45 ("audit: prevent an older auditd shutdown from orphaning a newer auditd startup") eliminates one possible cause. This isn't the case here, since the PID in the error message and the PID of the running auditd match. The primary expected cause of error here is -ECONNREFUSED when the audit daemon goes away, when netlink_getsockbyportid() can't find the auditd portid entry in the netlink audit table (or there is no receive function). If -EPERM is returned, that situation isn't likely to be resolved in a timely fashion without administrator intervention. In both cases, reset the audit_pid. This does not rule out a race condition. SELinux is expected to return zero since this isn't an INET or INET6 socket. Other LSMs may have other return codes. Log the error code for better diagnosis in the future. In the case of -ENOMEM, the situation could be temporary, based on local or general availability of buffers. -EAGAIN should never happen since the netlink audit (kernel) socket is set to MAX_SCHEDULE_TIMEOUT. -ERESTARTSYS and -EINTR are not expected since this kernel thread is not expected to receive signals. In these cases (or any other unexpected ones for now), report the error and re-schedule the thread, retrying up to 5 times. v2: Removed BUG_ON(). Moved comma in pr_*() statements. Removed audit_strerror() text. Reported-by: Vipin Rathor Reported-by: Signed-off-by: Richard Guy Briggs [PM: applied rgb's fixup patch to correct audit_log_lost() format issues] Signed-off-by: Paul Moore diff --git a/kernel/audit.c b/kernel/audit.c index 662c007..c4c98d8 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -407,16 +407,33 @@ static void audit_printk_skb(struct sk_buff *skb) static void kauditd_send_skb(struct sk_buff *skb) { int err; + int attempts = 0; +#define AUDITD_RETRIES 5 + +restart: /* take a reference in case we can't send it and we want to hold it */ skb_get(skb); err = netlink_unicast(audit_sock, skb, audit_nlk_portid, 0); if (err < 0) { - BUG_ON(err != -ECONNREFUSED); /* Shouldn't happen */ + pr_err("netlink_unicast sending to audit_pid=%d returned error: %d\n", + audit_pid, err); if (audit_pid) { - pr_err("*NO* daemon at audit_pid=%d\n", audit_pid); - audit_log_lost("auditd disappeared"); - audit_pid = 0; - audit_sock = NULL; + if (err == -ECONNREFUSED || err == -EPERM + || ++attempts >= AUDITD_RETRIES) { + char s[32]; + + snprintf(s, sizeof(s), "audit_pid=%d reset", audit_pid); + audit_log_lost(s); + audit_pid = 0; + audit_sock = NULL; + } else { + pr_warn("re-scheduling(#%d) write to audit_pid=%d\n", + attempts, audit_pid); + set_current_state(TASK_INTERRUPTIBLE); + schedule(); + __set_current_state(TASK_RUNNING); + goto restart; + } } /* we might get lucky and get this in the next auditd */ audit_hold_skb(skb); -- cgit v0.10.2 From 36734810488e618d48cc14782f7111b3dfaffb83 Mon Sep 17 00:00:00 2001 From: Yaowei Bai Date: Wed, 4 Nov 2015 08:23:51 -0500 Subject: audit: audit_dummy_context can be boolean This patch makes audit_dummy_context return bool due to this particular function only using either one or zero as its return value. No functional change. Signed-off-by: Yaowei Bai [PM: subject line tweak] Signed-off-by: Paul Moore diff --git a/include/linux/audit.h b/include/linux/audit.h index b2abc99..69844b6 100644 --- a/include/linux/audit.h +++ b/include/linux/audit.h @@ -143,7 +143,7 @@ extern void __audit_inode_child(const struct inode *parent, extern void __audit_seccomp(unsigned long syscall, long signr, int code); extern void __audit_ptrace(struct task_struct *t); -static inline int audit_dummy_context(void) +static inline bool audit_dummy_context(void) { void *p = current->audit_context; return !p || *(int *)p; @@ -345,9 +345,9 @@ static inline void audit_syscall_entry(int major, unsigned long a0, { } static inline void audit_syscall_exit(void *pt_regs) { } -static inline int audit_dummy_context(void) +static inline bool audit_dummy_context(void) { - return 1; + return true; } static inline struct filename *audit_reusename(const __user char *name) { -- cgit v0.10.2 From 9fcf836b215ca5685030ecab3e35ecc14ee3bcfb Mon Sep 17 00:00:00 2001 From: Yaowei Bai Date: Wed, 4 Nov 2015 08:23:51 -0500 Subject: audit: audit_string_contains_control can be boolean This patch makes audit_string_contains_control return bool to improve readability due to this particular function only using either one or zero as its return value. Signed-off-by: Yaowei Bai [PM: tweaked subject line] Signed-off-by: Paul Moore diff --git a/include/linux/audit.h b/include/linux/audit.h index 69844b6..20eba1e 100644 --- a/include/linux/audit.h +++ b/include/linux/audit.h @@ -457,7 +457,7 @@ extern struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp extern __printf(2, 3) void audit_log_format(struct audit_buffer *ab, const char *fmt, ...); extern void audit_log_end(struct audit_buffer *ab); -extern int audit_string_contains_control(const char *string, +extern bool audit_string_contains_control(const char *string, size_t len); extern void audit_log_n_hex(struct audit_buffer *ab, const unsigned char *buf, diff --git a/kernel/audit.c b/kernel/audit.c index c4c98d8..648036f 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -1583,14 +1583,14 @@ void audit_log_n_string(struct audit_buffer *ab, const char *string, * @string: string to be checked * @len: max length of the string to check */ -int audit_string_contains_control(const char *string, size_t len) +bool audit_string_contains_control(const char *string, size_t len) { const unsigned char *p; for (p = string; p < (const unsigned char *)string + len; p++) { if (*p == '"' || *p < 0x21 || *p > 0x7e) - return 1; + return true; } - return 0; + return false; } /** -- cgit v0.10.2 From 6f1b5d7afe1d737b7ca726e08e26f2e0367876d2 Mon Sep 17 00:00:00 2001 From: Yaowei Bai Date: Wed, 4 Nov 2015 08:23:51 -0500 Subject: audit: audit_tree_match can be boolean This patch makes audit_tree_match return bool to improve readability due to this particular function only using either one or zero as its return value. No functional change. Signed-off-by: Yaowei Bai [PM: tweaked the subject line] Signed-off-by: Paul Moore diff --git a/kernel/audit.h b/kernel/audit.h index dadf86a..de6cbb7 100644 --- a/kernel/audit.h +++ b/kernel/audit.h @@ -301,7 +301,7 @@ extern int audit_exe_compare(struct task_struct *tsk, struct audit_fsnotify_mark #ifdef CONFIG_AUDIT_TREE extern struct audit_chunk *audit_tree_lookup(const struct inode *); extern void audit_put_chunk(struct audit_chunk *); -extern int audit_tree_match(struct audit_chunk *, struct audit_tree *); +extern bool audit_tree_match(struct audit_chunk *, struct audit_tree *); extern int audit_make_tree(struct audit_krule *, char *, u32); extern int audit_add_tree_rule(struct audit_krule *); extern int audit_remove_tree_rule(struct audit_krule *); diff --git a/kernel/audit_tree.c b/kernel/audit_tree.c index 94ecdab..5efe9b29 100644 --- a/kernel/audit_tree.c +++ b/kernel/audit_tree.c @@ -197,13 +197,13 @@ struct audit_chunk *audit_tree_lookup(const struct inode *inode) return NULL; } -int audit_tree_match(struct audit_chunk *chunk, struct audit_tree *tree) +bool audit_tree_match(struct audit_chunk *chunk, struct audit_tree *tree) { int n; for (n = 0; n < chunk->count; n++) if (chunk->owners[n].owner == tree) - return 1; - return 0; + return true; + return false; } /* tagging and untagging inodes with trees */ -- cgit v0.10.2 From 725131efa52812973afda6ff3fbeec6cc22882a5 Mon Sep 17 00:00:00 2001 From: Scott Matheina Date: Wed, 4 Nov 2015 08:23:51 -0500 Subject: audit: fix comment block whitespace Signed-off-by: Scott Matheina [PM: fixed subject line] Signed-off-by: Paul Moore diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c index 7714d93..b8ff9e1 100644 --- a/kernel/auditfilter.c +++ b/kernel/auditfilter.c @@ -39,13 +39,13 @@ * Locking model: * * audit_filter_mutex: - * Synchronizes writes and blocking reads of audit's filterlist - * data. Rcu is used to traverse the filterlist and access - * contents of structs audit_entry, audit_watch and opaque - * LSM rules during filtering. If modified, these structures - * must be copied and replace their counterparts in the filterlist. - * An audit_parent struct is not accessed during filtering, so may - * be written directly provided audit_filter_mutex is held. + * Synchronizes writes and blocking reads of audit's filterlist + * data. Rcu is used to traverse the filterlist and access + * contents of structs audit_entry, audit_watch and opaque + * LSM rules during filtering. If modified, these structures + * must be copied and replace their counterparts in the filterlist. + * An audit_parent struct is not accessed during filtering, so may + * be written directly provided audit_filter_mutex is held. */ /* Audit filter lists, defined in */ -- cgit v0.10.2 From c5ea6efda6ff0fd591d6b7a2e1ba086b196dd864 Mon Sep 17 00:00:00 2001 From: Saurabh Sengar Date: Wed, 4 Nov 2015 08:23:52 -0500 Subject: audit: removing unused variable Variable rc in not required as it is just used for unchanged for return, and return is always 0 in the function. Signed-off-by: Saurabh Sengar [PM: fixed spelling errors in description] Signed-off-by: Paul Moore diff --git a/kernel/audit.c b/kernel/audit.c index 648036f..85570f3 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -703,23 +703,22 @@ static int audit_netlink_ok(struct sk_buff *skb, u16 msg_type) static int audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type) { - int rc = 0; uid_t uid = from_kuid(&init_user_ns, current_uid()); pid_t pid = task_tgid_nr(current); if (!audit_enabled && msg_type != AUDIT_USER_AVC) { *ab = NULL; - return rc; + return 0; } *ab = audit_log_start(NULL, GFP_KERNEL, msg_type); if (unlikely(!*ab)) - return rc; + return 0; audit_log_format(*ab, "pid=%d uid=%u", pid, uid); audit_log_session_info(*ab); audit_log_task_context(*ab); - return rc; + return 0; } int is_audit_feature_set(int i) -- cgit v0.10.2 From 233a68667cf4c134d07ef7e22bdd77786b5c7360 Mon Sep 17 00:00:00 2001 From: Paul Moore Date: Wed, 4 Nov 2015 08:23:52 -0500 Subject: audit: make audit_log_common_recv_msg() a void function It always returns zero and no one is checking the return value. Signed-off-by: Paul Moore diff --git a/kernel/audit.c b/kernel/audit.c index 85570f3..8a056a3 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -701,24 +701,22 @@ static int audit_netlink_ok(struct sk_buff *skb, u16 msg_type) return err; } -static int audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type) +static void audit_log_common_recv_msg(struct audit_buffer **ab, u16 msg_type) { uid_t uid = from_kuid(&init_user_ns, current_uid()); pid_t pid = task_tgid_nr(current); if (!audit_enabled && msg_type != AUDIT_USER_AVC) { *ab = NULL; - return 0; + return; } *ab = audit_log_start(NULL, GFP_KERNEL, msg_type); if (unlikely(!*ab)) - return 0; + return; audit_log_format(*ab, "pid=%d uid=%u", pid, uid); audit_log_session_info(*ab); audit_log_task_context(*ab); - - return 0; } int is_audit_feature_set(int i) -- cgit v0.10.2