From a9d1620877748375cf60b43ef3fa5f61ab6d9f24 Mon Sep 17 00:00:00 2001 From: Paul Moore Date: Tue, 2 May 2017 10:16:05 -0400 Subject: audit: combine audit_receive() and audit_receive_skb() There is no reason to have both of these functions, combine the two. Signed-off-by: Paul Moore Reviewed-by: Richard Guy Briggs --- kernel/audit.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) (limited to 'kernel/audit.c') diff --git a/kernel/audit.c b/kernel/audit.c index a871bf80fde1..eff602c1aa79 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -1378,11 +1378,14 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh) return err < 0 ? err : 0; } -/* - * Get message from skb. Each message is processed by audit_receive_msg. - * Malformed skbs with wrong length are discarded silently. +/** + * audit_receive - receive messages from a netlink control socket + * @skb: the message buffer + * + * Parse the provided skb and deal with any messages that may be present, + * malformed skbs are discarded. */ -static void audit_receive_skb(struct sk_buff *skb) +static void audit_receive(struct sk_buff *skb) { struct nlmsghdr *nlh; /* @@ -1395,6 +1398,7 @@ static void audit_receive_skb(struct sk_buff *skb) nlh = nlmsg_hdr(skb); len = skb->len; + mutex_lock(&audit_cmd_mutex); while (nlmsg_ok(nlh, len)) { err = audit_receive_msg(skb, nlh); /* if err or if this message says it wants a response */ @@ -1403,13 +1407,6 @@ static void audit_receive_skb(struct sk_buff *skb) nlh = nlmsg_next(nlh, &len); } -} - -/* Receive messages from netlink socket. */ -static void audit_receive(struct sk_buff *skb) -{ - mutex_lock(&audit_cmd_mutex); - audit_receive_skb(skb); mutex_unlock(&audit_cmd_mutex); } -- cgit v1.2.3 From 45a0642b4d021a2f50d5db9c191b5bfe60bfa1c7 Mon Sep 17 00:00:00 2001 From: Paul Moore Date: Tue, 2 May 2017 10:16:05 -0400 Subject: audit: kernel generated netlink traffic should have a portid of 0 We were setting the portid incorrectly in the netlink message headers, fix that to always be 0 (nlmsg_pid = 0). Signed-off-by: Paul Moore Reviewed-by: Richard Guy Briggs --- include/linux/audit.h | 3 +-- kernel/audit.c | 23 ++++++----------------- kernel/audit.h | 3 +-- kernel/auditfilter.c | 14 ++++++-------- 4 files changed, 14 insertions(+), 29 deletions(-) (limited to 'kernel/audit.c') diff --git a/include/linux/audit.h b/include/linux/audit.h index 504e784b7ffa..cc0497c39472 100644 --- a/include/linux/audit.h +++ b/include/linux/audit.h @@ -163,8 +163,7 @@ extern void audit_log_task_info(struct audit_buffer *ab, extern int audit_update_lsm_rules(void); /* Private API (for audit.c only) */ -extern int audit_rule_change(int type, __u32 portid, int seq, - void *data, size_t datasz); +extern int audit_rule_change(int type, int seq, void *data, size_t datasz); extern int audit_list_rules_send(struct sk_buff *request_skb, int seq); extern u32 audit_enabled; diff --git a/kernel/audit.c b/kernel/audit.c index eff602c1aa79..b40f3c4727e1 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -250,14 +250,6 @@ static struct sock *audit_get_sk(const struct net *net) return aunet->sk; } -static void audit_set_portid(struct audit_buffer *ab, __u32 portid) -{ - if (ab) { - struct nlmsghdr *nlh = nlmsg_hdr(ab->skb); - nlh->nlmsg_pid = portid; - } -} - void audit_panic(const char *message) { switch (audit_failure) { @@ -816,7 +808,7 @@ int audit_send_list(void *_dest) return 0; } -struct sk_buff *audit_make_reply(__u32 portid, int seq, int type, int done, +struct sk_buff *audit_make_reply(int seq, int type, int done, int multi, const void *payload, int size) { struct sk_buff *skb; @@ -829,7 +821,7 @@ struct sk_buff *audit_make_reply(__u32 portid, int seq, int type, int done, if (!skb) return NULL; - nlh = nlmsg_put(skb, portid, seq, t, size, flags); + nlh = nlmsg_put(skb, 0, seq, t, size, flags); if (!nlh) goto out_kfree_skb; data = nlmsg_data(nlh); @@ -873,7 +865,6 @@ static int audit_send_reply_thread(void *arg) static void audit_send_reply(struct sk_buff *request_skb, int seq, int type, int done, int multi, const void *payload, int size) { - u32 portid = NETLINK_CB(request_skb).portid; struct net *net = sock_net(NETLINK_CB(request_skb).sk); struct sk_buff *skb; struct task_struct *tsk; @@ -883,12 +874,12 @@ static void audit_send_reply(struct sk_buff *request_skb, int seq, int type, int if (!reply) return; - skb = audit_make_reply(portid, seq, type, done, multi, payload, size); + skb = audit_make_reply(seq, type, done, multi, payload, size); if (!skb) goto out; reply->net = get_net(net); - reply->portid = portid; + reply->portid = NETLINK_CB(request_skb).portid; reply->skb = skb; tsk = kthread_run(audit_send_reply_thread, reply, "audit_send_reply"); @@ -1072,7 +1063,7 @@ static int audit_replace(pid_t pid) { struct sk_buff *skb; - skb = audit_make_reply(0, 0, AUDIT_REPLACE, 0, 0, &pid, sizeof(pid)); + skb = audit_make_reply(0, AUDIT_REPLACE, 0, 0, &pid, sizeof(pid)); if (!skb) return -ENOMEM; return auditd_send_unicast_skb(skb); @@ -1242,7 +1233,6 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh) size--; audit_log_n_untrustedstring(ab, data, size); } - audit_set_portid(ab, NETLINK_CB(skb).portid); audit_log_end(ab); } break; @@ -1256,8 +1246,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh) audit_log_end(ab); return -EPERM; } - err = audit_rule_change(msg_type, NETLINK_CB(skb).portid, - seq, data, nlmsg_len(nlh)); + err = audit_rule_change(msg_type, seq, data, nlmsg_len(nlh)); break; case AUDIT_LIST_RULES: err = audit_list_rules_send(skb, seq); diff --git a/kernel/audit.h b/kernel/audit.h index 0d87f8ab8778..18f3c2deeccf 100644 --- a/kernel/audit.h +++ b/kernel/audit.h @@ -237,8 +237,7 @@ extern int audit_uid_comparator(kuid_t left, u32 op, kuid_t right); extern int audit_gid_comparator(kgid_t left, u32 op, kgid_t right); extern int parent_len(const char *path); extern int audit_compare_dname_path(const char *dname, const char *path, int plen); -extern struct sk_buff *audit_make_reply(__u32 portid, int seq, int type, - int done, int multi, +extern struct sk_buff *audit_make_reply(int seq, int type, int done, int multi, const void *payload, int size); extern void audit_panic(const char *message); diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c index 239d11c3122c..0b0aa5854dac 100644 --- a/kernel/auditfilter.c +++ b/kernel/auditfilter.c @@ -1033,7 +1033,7 @@ out: } /* List rules using struct audit_rule_data. */ -static void audit_list_rules(__u32 portid, int seq, struct sk_buff_head *q) +static void audit_list_rules(int seq, struct sk_buff_head *q) { struct sk_buff *skb; struct audit_krule *r; @@ -1048,15 +1048,15 @@ static void audit_list_rules(__u32 portid, int seq, struct sk_buff_head *q) data = audit_krule_to_data(r); if (unlikely(!data)) break; - skb = audit_make_reply(portid, seq, AUDIT_LIST_RULES, - 0, 1, data, + skb = audit_make_reply(seq, AUDIT_LIST_RULES, 0, 1, + data, sizeof(*data) + data->buflen); if (skb) skb_queue_tail(q, skb); kfree(data); } } - skb = audit_make_reply(portid, seq, AUDIT_LIST_RULES, 1, 1, NULL, 0); + skb = audit_make_reply(seq, AUDIT_LIST_RULES, 1, 1, NULL, 0); if (skb) skb_queue_tail(q, skb); } @@ -1085,13 +1085,11 @@ static void audit_log_rule_change(char *action, struct audit_krule *rule, int re /** * audit_rule_change - apply all rules to the specified message type * @type: audit message type - * @portid: target port id for netlink audit messages * @seq: netlink audit message sequence (serial) number * @data: payload data * @datasz: size of payload data */ -int audit_rule_change(int type, __u32 portid, int seq, void *data, - size_t datasz) +int audit_rule_change(int type, int seq, void *data, size_t datasz) { int err = 0; struct audit_entry *entry; @@ -1150,7 +1148,7 @@ int audit_list_rules_send(struct sk_buff *request_skb, int seq) skb_queue_head_init(&dest->q); mutex_lock(&audit_filter_mutex); - audit_list_rules(portid, seq, &dest->q); + audit_list_rules(seq, &dest->q); mutex_unlock(&audit_filter_mutex); tsk = kthread_run(audit_send_list, dest, "audit_send_list"); -- cgit v1.2.3 From b6c7c115c2ce679ac536f0adf0ff518fcd939196 Mon Sep 17 00:00:00 2001 From: Paul Moore Date: Tue, 2 May 2017 10:16:05 -0400 Subject: audit: store the auditd PID as a pid struct instead of pid_t This is arguably the right thing to do, and will make it easier when we start supporting multiple audit daemons in different namespaces. Signed-off-by: Paul Moore --- kernel/audit.c | 84 +++++++++++++++++++++++++++++++++++++++------------------- kernel/audit.h | 2 +- 2 files changed, 58 insertions(+), 28 deletions(-) (limited to 'kernel/audit.c') diff --git a/kernel/audit.c b/kernel/audit.c index b40f3c4727e1..a2f7803a68d0 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -58,6 +58,7 @@ #include #include #include +#include #include @@ -117,7 +118,7 @@ struct audit_net { * or the included spinlock for writing. */ static struct auditd_connection { - int pid; + struct pid *pid; u32 portid; struct net *net; spinlock_t lock; @@ -220,17 +221,40 @@ struct audit_reply { * Description: * Return 1 if the task is a registered audit daemon, 0 otherwise. */ -int auditd_test_task(const struct task_struct *task) +int auditd_test_task(struct task_struct *task) { int rc; rcu_read_lock(); - rc = (auditd_conn.pid && task->tgid == auditd_conn.pid ? 1 : 0); + rc = (auditd_conn.pid && auditd_conn.pid == task_tgid(task) ? 1 : 0); rcu_read_unlock(); return rc; } +/** + * auditd_pid_vnr - Return the auditd PID relative to the namespace + * @auditd: the auditd connection + * + * Description: + * Returns the PID in relation to the namespace, 0 on failure. This function + * takes the RCU read lock internally, but if the caller needs to protect the + * auditd_connection pointer it should take the RCU read lock as well. + */ +static pid_t auditd_pid_vnr(const struct auditd_connection *auditd) +{ + pid_t pid; + + rcu_read_lock(); + if (!auditd || !auditd->pid) + pid = 0; + else + pid = pid_vnr(auditd->pid); + rcu_read_unlock(); + + return pid; +} + /** * audit_get_sk - Return the audit socket for the given network namespace * @net: the destination network namespace @@ -428,12 +452,17 @@ static int audit_set_failure(u32 state) * This function will obtain and drop network namespace references as * necessary. */ -static void auditd_set(int pid, u32 portid, struct net *net) +static void auditd_set(struct pid *pid, u32 portid, struct net *net) { unsigned long flags; spin_lock_irqsave(&auditd_conn.lock, flags); - auditd_conn.pid = pid; + if (auditd_conn.pid) + put_pid(auditd_conn.pid); + if (pid) + auditd_conn.pid = get_pid(pid); + else + auditd_conn.pid = NULL; auditd_conn.portid = portid; if (auditd_conn.net) put_net(auditd_conn.net); @@ -1059,11 +1088,13 @@ static int audit_set_feature(struct sk_buff *skb) return 0; } -static int audit_replace(pid_t pid) +static int audit_replace(struct pid *pid) { + pid_t pvnr; struct sk_buff *skb; - skb = audit_make_reply(0, AUDIT_REPLACE, 0, 0, &pid, sizeof(pid)); + pvnr = pid_vnr(pid); + skb = audit_make_reply(0, AUDIT_REPLACE, 0, 0, &pvnr, sizeof(pvnr)); if (!skb) return -ENOMEM; return auditd_send_unicast_skb(skb); @@ -1093,9 +1124,9 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh) memset(&s, 0, sizeof(s)); s.enabled = audit_enabled; s.failure = audit_failure; - rcu_read_lock(); - s.pid = auditd_conn.pid; - rcu_read_unlock(); + /* NOTE: use pid_vnr() so the PID is relative to the current + * namespace */ + s.pid = auditd_pid_vnr(&auditd_conn); s.rate_limit = audit_rate_limit; s.backlog_limit = audit_backlog_limit; s.lost = atomic_read(&audit_lost); @@ -1121,36 +1152,36 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh) return err; } if (s.mask & AUDIT_STATUS_PID) { - /* NOTE: we are using task_tgid_vnr() below because - * the s.pid value is relative to the namespace - * of the caller; at present this doesn't matter - * much since you can really only run auditd - * from the initial pid namespace, but something - * to keep in mind if this changes */ - int new_pid = s.pid; + /* NOTE: we are using the vnr PID functions below + * because the s.pid value is relative to the + * namespace of the caller; at present this + * doesn't matter much since you can really only + * run auditd from the initial pid namespace, but + * something to keep in mind if this changes */ + pid_t new_pid = s.pid; pid_t auditd_pid; - pid_t requesting_pid = task_tgid_vnr(current); + struct pid *req_pid = task_tgid(current); + + /* sanity check - PID values must match */ + if (new_pid != pid_vnr(req_pid)) + return -EINVAL; /* test the auditd connection */ - audit_replace(requesting_pid); + audit_replace(req_pid); - rcu_read_lock(); - auditd_pid = auditd_conn.pid; + auditd_pid = auditd_pid_vnr(&auditd_conn); /* only the current auditd can unregister itself */ - if ((!new_pid) && (requesting_pid != auditd_pid)) { - rcu_read_unlock(); + if ((!new_pid) && (new_pid != auditd_pid)) { audit_log_config_change("audit_pid", new_pid, auditd_pid, 0); return -EACCES; } /* replacing a healthy auditd is not allowed */ if (auditd_pid && new_pid) { - rcu_read_unlock(); audit_log_config_change("audit_pid", new_pid, auditd_pid, 0); return -EEXIST; } - rcu_read_unlock(); if (audit_enabled != AUDIT_OFF) audit_log_config_change("audit_pid", new_pid, @@ -1158,8 +1189,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh) if (new_pid) { /* register a new auditd connection */ - auditd_set(new_pid, - NETLINK_CB(skb).portid, + auditd_set(req_pid, NETLINK_CB(skb).portid, sock_net(NETLINK_CB(skb).sk)); /* try to process any backlog */ wake_up_interruptible(&kauditd_wait); diff --git a/kernel/audit.h b/kernel/audit.h index 18f3c2deeccf..4987ea2a4702 100644 --- a/kernel/audit.h +++ b/kernel/audit.h @@ -218,7 +218,7 @@ extern void audit_log_name(struct audit_context *context, struct audit_names *n, const struct path *path, int record_num, int *call_panic); -extern int auditd_test_task(const struct task_struct *task); +extern int auditd_test_task(struct task_struct *task); #define AUDIT_INODE_BUCKETS 32 extern struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS]; -- cgit v1.2.3 From 2115bb250f260089743e26decfb5f271ba71ca37 Mon Sep 17 00:00:00 2001 From: Deepa Dinamani Date: Tue, 2 May 2017 10:16:05 -0400 Subject: audit: Use timespec64 to represent audit timestamps struct timespec is not y2038 safe. Audit timestamps are recorded in string format into an audit buffer for a given context. These mark the entry timestamps for the syscalls. Use y2038 safe struct timespec64 to represent the times. The log strings can handle this transition as strings can hold upto 1024 characters. Signed-off-by: Deepa Dinamani Reviewed-by: Arnd Bergmann Acked-by: Paul Moore Acked-by: Richard Guy Briggs Signed-off-by: Paul Moore --- include/linux/audit.h | 4 ++-- kernel/audit.c | 10 +++++----- kernel/audit.h | 2 +- kernel/auditsc.c | 6 +++--- 4 files changed, 11 insertions(+), 11 deletions(-) (limited to 'kernel/audit.c') diff --git a/include/linux/audit.h b/include/linux/audit.h index cc0497c39472..2150bdccfbab 100644 --- a/include/linux/audit.h +++ b/include/linux/audit.h @@ -331,7 +331,7 @@ static inline void audit_ptrace(struct task_struct *t) /* Private API (for audit.c only) */ extern unsigned int audit_serial(void); extern int auditsc_get_stamp(struct audit_context *ctx, - struct timespec *t, unsigned int *serial); + struct timespec64 *t, unsigned int *serial); extern int audit_set_loginuid(kuid_t loginuid); static inline kuid_t audit_get_loginuid(struct task_struct *tsk) @@ -510,7 +510,7 @@ static inline void __audit_seccomp(unsigned long syscall, long signr, int code) static inline void audit_seccomp(unsigned long syscall, long signr, int code) { } static inline int auditsc_get_stamp(struct audit_context *ctx, - struct timespec *t, unsigned int *serial) + struct timespec64 *t, unsigned int *serial) { return 0; } diff --git a/kernel/audit.c b/kernel/audit.c index a2f7803a68d0..41efd2ad1931 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -1638,10 +1638,10 @@ unsigned int audit_serial(void) } static inline void audit_get_stamp(struct audit_context *ctx, - struct timespec *t, unsigned int *serial) + struct timespec64 *t, unsigned int *serial) { if (!ctx || !auditsc_get_stamp(ctx, t, serial)) { - *t = CURRENT_TIME; + ktime_get_real_ts64(t); *serial = audit_serial(); } } @@ -1665,7 +1665,7 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, int type) { struct audit_buffer *ab; - struct timespec t; + struct timespec64 t; unsigned int uninitialized_var(serial); if (audit_initialized != AUDIT_INITIALIZED) @@ -1718,8 +1718,8 @@ struct audit_buffer *audit_log_start(struct audit_context *ctx, gfp_t gfp_mask, } audit_get_stamp(ab->ctx, &t, &serial); - audit_log_format(ab, "audit(%lu.%03lu:%u): ", - t.tv_sec, t.tv_nsec/1000000, serial); + audit_log_format(ab, "audit(%llu.%03lu:%u): ", + (unsigned long long)t.tv_sec, t.tv_nsec/1000000, serial); return ab; } diff --git a/kernel/audit.h b/kernel/audit.h index 4987ea2a4702..ddfce2ea4891 100644 --- a/kernel/audit.h +++ b/kernel/audit.h @@ -112,7 +112,7 @@ struct audit_context { enum audit_state state, current_state; unsigned int serial; /* serial number for record */ int major; /* syscall number */ - struct timespec ctime; /* time of syscall entry */ + struct timespec64 ctime; /* time of syscall entry */ unsigned long argv[4]; /* syscall arguments */ long return_code;/* syscall return code */ u64 prio; diff --git a/kernel/auditsc.c b/kernel/auditsc.c index 1c2333155893..b2dcbe637b7c 100644 --- a/kernel/auditsc.c +++ b/kernel/auditsc.c @@ -1532,7 +1532,7 @@ void __audit_syscall_entry(int major, unsigned long a1, unsigned long a2, return; context->serial = 0; - context->ctime = CURRENT_TIME; + ktime_get_real_ts64(&context->ctime); context->in_syscall = 1; context->current_state = state; context->ppid = 0; @@ -1941,13 +1941,13 @@ EXPORT_SYMBOL_GPL(__audit_inode_child); /** * auditsc_get_stamp - get local copies of audit_context values * @ctx: audit_context for the task - * @t: timespec to store time recorded in the audit_context + * @t: timespec64 to store time recorded in the audit_context * @serial: serial value that is recorded in the audit_context * * Also sets the context as auditable. */ int auditsc_get_stamp(struct audit_context *ctx, - struct timespec *t, unsigned int *serial) + struct timespec64 *t, unsigned int *serial) { if (!ctx->in_syscall) return 0; -- cgit v1.2.3 From 8cc96382d9a7fe1746286670dd5140c3b12638ae Mon Sep 17 00:00:00 2001 From: Paul Moore Date: Tue, 2 May 2017 10:16:05 -0400 Subject: audit: use kmem_cache to manage the audit_buffer cache The audit subsystem implemented its own buffer cache mechanism which is a bit silly these days when we could use the kmem_cache construct. Some credit is due to Florian Westphal for originally proposing that we remove the audit cache implementation in favor of simple kmalloc()/kfree() calls, but I would rather have a dedicated slab cache to ease debugging and future stats/performance work. Cc: Florian Westphal Reviewed-by: Richard Guy Briggs Signed-off-by: Paul Moore --- kernel/audit.c | 66 +++++++++++++++------------------------------------------- 1 file changed, 17 insertions(+), 49 deletions(-) (limited to 'kernel/audit.c') diff --git a/kernel/audit.c b/kernel/audit.c index 41efd2ad1931..10bc2bad2adf 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -59,6 +59,7 @@ #include #include #include +#include #include @@ -152,12 +153,7 @@ static atomic_t audit_lost = ATOMIC_INIT(0); /* Hash for inode-based rules */ struct list_head audit_inode_hash[AUDIT_INODE_BUCKETS]; -/* The audit_freelist is a list of pre-allocated audit buffers (if more - * than AUDIT_MAXFREE are in use, the audit buffer is freed instead of - * being placed on the freelist). */ -static DEFINE_SPINLOCK(audit_freelist_lock); -static int audit_freelist_count; -static LIST_HEAD(audit_freelist); +static struct kmem_cache *audit_buffer_cache; /* queue msgs to send via kauditd_task */ static struct sk_buff_head audit_queue; @@ -192,17 +188,12 @@ DEFINE_MUTEX(audit_cmd_mutex); * should be at least that large. */ #define AUDIT_BUFSIZ 1024 -/* AUDIT_MAXFREE is the number of empty audit_buffers we keep on the - * audit_freelist. Doing so eliminates many kmalloc/kfree calls. */ -#define AUDIT_MAXFREE (2*NR_CPUS) - /* The audit_buffer is used when formatting an audit record. The caller * locks briefly to get the record off the freelist or to allocate the * buffer, and locks briefly to send the buffer to the netlink layer or * to place it on a transmit queue. Multiple audit_buffers can be in * use simultaneously. */ struct audit_buffer { - struct list_head list; struct sk_buff *skb; /* formatted skb ready to send */ struct audit_context *ctx; /* NULL or associated context */ gfp_t gfp_mask; @@ -1486,6 +1477,10 @@ static int __init audit_init(void) if (audit_initialized == AUDIT_DISABLED) return 0; + audit_buffer_cache = kmem_cache_create("audit_buffer", + sizeof(struct audit_buffer), + 0, SLAB_PANIC, NULL); + memset(&auditd_conn, 0, sizeof(auditd_conn)); spin_lock_init(&auditd_conn.lock); @@ -1554,60 +1549,33 @@ __setup("audit_backlog_limit=", audit_backlog_limit_set); static void audit_buffer_free(struct audit_buffer *ab) { - unsigned long flags; - if (!ab) return; kfree_skb(ab->skb); - spin_lock_irqsave(&audit_freelist_lock, flags); - if (audit_freelist_count > AUDIT_MAXFREE) - kfree(ab); - else { - audit_freelist_count++; - list_add(&ab->list, &audit_freelist); - } - spin_unlock_irqrestore(&audit_freelist_lock, flags); + kmem_cache_free(audit_buffer_cache, ab); } -static struct audit_buffer * audit_buffer_alloc(struct audit_context *ctx, - gfp_t gfp_mask, int type) +static struct audit_buffer *audit_buffer_alloc(struct audit_context *ctx, + gfp_t gfp_mask, int type) { - unsigned long flags; - struct audit_buffer *ab = NULL; - struct nlmsghdr *nlh; - - spin_lock_irqsave(&audit_freelist_lock, flags); - if (!list_empty(&audit_freelist)) { - ab = list_entry(audit_freelist.next, - struct audit_buffer, list); - list_del(&ab->list); - --audit_freelist_count; - } - spin_unlock_irqrestore(&audit_freelist_lock, flags); - - if (!ab) { - ab = kmalloc(sizeof(*ab), gfp_mask); - if (!ab) - goto err; - } + struct audit_buffer *ab; - ab->ctx = ctx; - ab->gfp_mask = gfp_mask; + ab = kmem_cache_alloc(audit_buffer_cache, gfp_mask); + if (!ab) + return NULL; ab->skb = nlmsg_new(AUDIT_BUFSIZ, gfp_mask); if (!ab->skb) goto err; + if (!nlmsg_put(ab->skb, 0, 0, type, 0, 0)) + goto err; - nlh = nlmsg_put(ab->skb, 0, 0, type, 0, 0); - if (!nlh) - goto out_kfree_skb; + ab->ctx = ctx; + ab->gfp_mask = gfp_mask; return ab; -out_kfree_skb: - kfree_skb(ab->skb); - ab->skb = NULL; err: audit_buffer_free(ab); return NULL; -- cgit v1.2.3 From 48d0e023af9799cd7220335baf8e3ba61eeafbeb Mon Sep 17 00:00:00 2001 From: Paul Moore Date: Tue, 2 May 2017 10:16:05 -0400 Subject: audit: fix the RCU locking for the auditd_connection structure Cong Wang correctly pointed out that the RCU read locking of the auditd_connection struct was wrong, this patch correct this by adopting a more traditional, and correct RCU locking model. This patch is heavily based on an earlier prototype by Cong Wang. Cc: # 4.11.x- Reported-by: Cong Wang Signed-off-by: Cong Wang Signed-off-by: Paul Moore --- kernel/audit.c | 157 ++++++++++++++++++++++++++++++++++++--------------------- 1 file changed, 100 insertions(+), 57 deletions(-) (limited to 'kernel/audit.c') diff --git a/kernel/audit.c b/kernel/audit.c index 10bc2bad2adf..a7c6a50477aa 100644 --- a/kernel/audit.c +++ b/kernel/audit.c @@ -112,18 +112,19 @@ struct audit_net { * @pid: auditd PID * @portid: netlink portid * @net: the associated network namespace - * @lock: spinlock to protect write access + * @rcu: RCU head * * Description: * This struct is RCU protected; you must either hold the RCU lock for reading - * or the included spinlock for writing. + * or the associated spinlock for writing. */ static struct auditd_connection { struct pid *pid; u32 portid; struct net *net; - spinlock_t lock; -} auditd_conn; + struct rcu_head rcu; +} *auditd_conn = NULL; +static DEFINE_SPINLOCK(auditd_conn_lock); /* If audit_rate_limit is non-zero, limit the rate of sending audit records * to that number per second. This prevents DoS attacks, but results in @@ -215,9 +216,11 @@ struct audit_reply { int auditd_test_task(struct task_struct *task) { int rc; + struct auditd_connection *ac; rcu_read_lock(); - rc = (auditd_conn.pid && auditd_conn.pid == task_tgid(task) ? 1 : 0); + ac = rcu_dereference(auditd_conn); + rc = (ac && ac->pid == task_tgid(task) ? 1 : 0); rcu_read_unlock(); return rc; @@ -225,22 +228,21 @@ int auditd_test_task(struct task_struct *task) /** * auditd_pid_vnr - Return the auditd PID relative to the namespace - * @auditd: the auditd connection * * Description: - * Returns the PID in relation to the namespace, 0 on failure. This function - * takes the RCU read lock internally, but if the caller needs to protect the - * auditd_connection pointer it should take the RCU read lock as well. + * Returns the PID in relation to the namespace, 0 on failure. */ -static pid_t auditd_pid_vnr(const struct auditd_connection *auditd) +static pid_t auditd_pid_vnr(void) { pid_t pid; + const struct auditd_connection *ac; rcu_read_lock(); - if (!auditd || !auditd->pid) + ac = rcu_dereference(auditd_conn); + if (!ac || !ac->pid) pid = 0; else - pid = pid_vnr(auditd->pid); + pid = pid_vnr(ac->pid); rcu_read_unlock(); return pid; @@ -433,6 +435,24 @@ static int audit_set_failure(u32 state) return audit_do_config_change("audit_failure", &audit_failure, state); } +/** + * auditd_conn_free - RCU helper to release an auditd connection struct + * @rcu: RCU head + * + * Description: + * Drop any references inside the auditd connection tracking struct and free + * the memory. + */ + static void auditd_conn_free(struct rcu_head *rcu) + { + struct auditd_connection *ac; + + ac = container_of(rcu, struct auditd_connection, rcu); + put_pid(ac->pid); + put_net(ac->net); + kfree(ac); + } + /** * auditd_set - Set/Reset the auditd connection state * @pid: auditd PID @@ -441,27 +461,33 @@ static int audit_set_failure(u32 state) * * Description: * This function will obtain and drop network namespace references as - * necessary. + * necessary. Returns zero on success, negative values on failure. */ -static void auditd_set(struct pid *pid, u32 portid, struct net *net) +static int auditd_set(struct pid *pid, u32 portid, struct net *net) { unsigned long flags; + struct auditd_connection *ac_old, *ac_new; - spin_lock_irqsave(&auditd_conn.lock, flags); - if (auditd_conn.pid) - put_pid(auditd_conn.pid); - if (pid) - auditd_conn.pid = get_pid(pid); - else - auditd_conn.pid = NULL; - auditd_conn.portid = portid; - if (auditd_conn.net) - put_net(auditd_conn.net); - if (net) - auditd_conn.net = get_net(net); - else - auditd_conn.net = NULL; - spin_unlock_irqrestore(&auditd_conn.lock, flags); + if (!pid || !net) + return -EINVAL; + + ac_new = kzalloc(sizeof(*ac_new), GFP_KERNEL); + if (!ac_new) + return -ENOMEM; + ac_new->pid = get_pid(pid); + ac_new->portid = portid; + ac_new->net = get_net(net); + + spin_lock_irqsave(&auditd_conn_lock, flags); + ac_old = rcu_dereference_protected(auditd_conn, + lockdep_is_held(&auditd_conn_lock)); + rcu_assign_pointer(auditd_conn, ac_new); + spin_unlock_irqrestore(&auditd_conn_lock, flags); + + if (ac_old) + call_rcu(&ac_old->rcu, auditd_conn_free); + + return 0; } /** @@ -556,13 +582,19 @@ static void kauditd_retry_skb(struct sk_buff *skb) */ static void auditd_reset(void) { + unsigned long flags; struct sk_buff *skb; + struct auditd_connection *ac_old; /* if it isn't already broken, break the connection */ - rcu_read_lock(); - if (auditd_conn.pid) - auditd_set(0, 0, NULL); - rcu_read_unlock(); + spin_lock_irqsave(&auditd_conn_lock, flags); + ac_old = rcu_dereference_protected(auditd_conn, + lockdep_is_held(&auditd_conn_lock)); + rcu_assign_pointer(auditd_conn, NULL); + spin_unlock_irqrestore(&auditd_conn_lock, flags); + + if (ac_old) + call_rcu(&ac_old->rcu, auditd_conn_free); /* flush all of the main and retry queues to the hold queue */ while ((skb = skb_dequeue(&audit_retry_queue))) @@ -588,6 +620,7 @@ static int auditd_send_unicast_skb(struct sk_buff *skb) u32 portid; struct net *net; struct sock *sk; + struct auditd_connection *ac; /* NOTE: we can't call netlink_unicast while in the RCU section so * take a reference to the network namespace and grab local @@ -597,15 +630,15 @@ static int auditd_send_unicast_skb(struct sk_buff *skb) * section netlink_unicast() should safely return an error */ rcu_read_lock(); - if (!auditd_conn.pid) { + ac = rcu_dereference(auditd_conn); + if (!ac) { rcu_read_unlock(); rc = -ECONNREFUSED; goto err; } - net = auditd_conn.net; - get_net(net); + net = get_net(ac->net); sk = audit_get_sk(net); - portid = auditd_conn.portid; + portid = ac->portid; rcu_read_unlock(); rc = netlink_unicast(sk, skb, portid, 0); @@ -740,6 +773,7 @@ static int kauditd_thread(void *dummy) u32 portid = 0; struct net *net = NULL; struct sock *sk = NULL; + struct auditd_connection *ac; #define UNICAST_RETRIES 5 @@ -747,14 +781,14 @@ static int kauditd_thread(void *dummy) while (!kthread_should_stop()) { /* NOTE: see the lock comments in auditd_send_unicast_skb() */ rcu_read_lock(); - if (!auditd_conn.pid) { + ac = rcu_dereference(auditd_conn); + if (!ac) { rcu_read_unlock(); goto main_queue; } - net = auditd_conn.net; - get_net(net); + net = get_net(ac->net); sk = audit_get_sk(net); - portid = auditd_conn.portid; + portid = ac->portid; rcu_read_unlock(); /* attempt to flush the hold queue */ @@ -1117,7 +1151,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh) s.failure = audit_failure; /* NOTE: use pid_vnr() so the PID is relative to the current * namespace */ - s.pid = auditd_pid_vnr(&auditd_conn); + s.pid = auditd_pid_vnr(); s.rate_limit = audit_rate_limit; s.backlog_limit = audit_backlog_limit; s.lost = atomic_read(&audit_lost); @@ -1160,7 +1194,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh) /* test the auditd connection */ audit_replace(req_pid); - auditd_pid = auditd_pid_vnr(&auditd_conn); + auditd_pid = auditd_pid_vnr(); /* only the current auditd can unregister itself */ if ((!new_pid) && (new_pid != auditd_pid)) { audit_log_config_change("audit_pid", new_pid, @@ -1174,19 +1208,30 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh) return -EEXIST; } - if (audit_enabled != AUDIT_OFF) - audit_log_config_change("audit_pid", new_pid, - auditd_pid, 1); - if (new_pid) { /* register a new auditd connection */ - auditd_set(req_pid, NETLINK_CB(skb).portid, - sock_net(NETLINK_CB(skb).sk)); + err = auditd_set(req_pid, + NETLINK_CB(skb).portid, + sock_net(NETLINK_CB(skb).sk)); + if (audit_enabled != AUDIT_OFF) + audit_log_config_change("audit_pid", + new_pid, + auditd_pid, + err ? 0 : 1); + if (err) + return err; + /* try to process any backlog */ wake_up_interruptible(&kauditd_wait); - } else + } else { + if (audit_enabled != AUDIT_OFF) + audit_log_config_change("audit_pid", + new_pid, + auditd_pid, 1); + /* unregister the auditd connection */ auditd_reset(); + } } if (s.mask & AUDIT_STATUS_RATE_LIMIT) { err = audit_set_rate_limit(s.rate_limit); @@ -1454,10 +1499,11 @@ static void __net_exit audit_net_exit(struct net *net) { struct audit_net *aunet = net_generic(net, audit_net_id); - rcu_read_lock(); - if (net == auditd_conn.net) - auditd_reset(); - rcu_read_unlock(); + /* NOTE: you would think that we would want to check the auditd + * connection and potentially reset it here if it lives in this + * namespace, but since the auditd connection tracking struct holds a + * reference to this namespace (see auditd_set()) we are only ever + * going to get here after that connection has been released */ netlink_kernel_release(aunet->sk); } @@ -1481,9 +1527,6 @@ static int __init audit_init(void) sizeof(struct audit_buffer), 0, SLAB_PANIC, NULL); - memset(&auditd_conn, 0, sizeof(auditd_conn)); - spin_lock_init(&auditd_conn.lock); - skb_queue_head_init(&audit_queue); skb_queue_head_init(&audit_retry_queue); skb_queue_head_init(&audit_hold_queue); -- cgit v1.2.3