audit: Use struct net not pid_t to remember the network namespce to reply in

While reading through 3.14-rc1 I found a pretty siginficant mishandling of network namespaces in the recent audit changes.

In struct audit_netlink_list and audit_reply add a reference to the network namespace of the caller and remove the userspace pid of the caller. This cleanly remembers the callers network namespace, and removes a huge class of races and nasty failure modes that can occur when attempting to relook up the callers network namespace from a pid_t (including the caller's network namespace changing, pid wraparound, and the pid simply not being present).

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 kernel/audit.c       |   10 ++++++----
 kernel/audit.h       |    2 +-
 kernel/auditfilter.c |    3 ++-
 3 files changed, 9 insertions(+), 6 deletions(-)

kernel/audit.c +6 -4

index 34c5a2310fbf..1e5756f16f6f 100644
@@ -182,7 +182,7 @@ struct audit_buffer {
 
 struct audit_reply {
 	__u32 portid;
-	pid_t pid;
+	struct net *net;	
 	struct sk_buff *skb;
 };
 

@@ -500,7 +500,7 @@ int audit_send_list(void *_dest)
 {
 	struct audit_netlink_list *dest = _dest;
 	struct sk_buff *skb;
-	struct net *net = get_net_ns_by_pid(dest->pid);
+	struct net *net = dest->net;
 	struct audit_net *aunet = net_generic(net, audit_net_id);
 
 	/* wait for parent to finish and send an ACK */

@@ -510,6 +510,7 @@ int audit_send_list(void *_dest)
 	while ((skb = __skb_dequeue(&dest->q)) != NULL)
 		netlink_unicast(aunet->nlsk, skb, dest->portid, 0);
 
+	put_net(net);
 	kfree(dest);
 
 	return 0;

@@ -543,7 +544,7 @@ out_kfree_skb:
 static int audit_send_reply_thread(void *arg)
 {
 	struct audit_reply *reply = (struct audit_reply *)arg;
-	struct net *net = get_net_ns_by_pid(reply->pid);
+	struct net *net = reply->net;
 	struct audit_net *aunet = net_generic(net, audit_net_id);
 
 	mutex_lock(&audit_cmd_mutex);

@@ -552,6 +553,7 @@ static int audit_send_reply_thread(void *arg)
 	/* Ignore failure. It'll only happen if the sender goes away,
 	   because our timeout is set to infinite. */
 	netlink_unicast(aunet->nlsk , reply->skb, reply->portid, 0);
+	put_net(net);
 	kfree(reply);
 	return 0;
 }

@@ -583,8 +585,8 @@ static void audit_send_reply(__u32 portid, int seq, int type, int done,
 	if (!skb)
 		goto out;
 
+	reply->net = get_net(current->nsproxy->net_ns);
 	reply->portid = portid;
-	reply->pid = task_pid_vnr(current);
 	reply->skb = skb;
 
 	tsk = kthread_run(audit_send_reply_thread, reply, "audit_send_reply");

kernel/audit.h +1 -1

index 57cc64d67718..8df132214606 100644
@@ -247,7 +247,7 @@ extern void		    audit_panic(const char *message);
 
 struct audit_netlink_list {
 	__u32 portid;
-	pid_t pid;
+	struct net *net;
 	struct sk_buff_head q;
 };
 

kernel/auditfilter.c +2 -1

index 14a78cca384e..a5e3d73d73e4 100644
@@ -29,6 +29,7 @@
 #include <linux/sched.h>
 #include <linux/slab.h>
 #include <linux/security.h>
+#include <net/net_namespace.h>
 #include "audit.h"
 
 /*

@@ -1083,8 +1084,8 @@ int audit_list_rules_send(__u32 portid, int seq)
 	dest = kmalloc(sizeof(struct audit_netlink_list), GFP_KERNEL);
 	if (!dest)
 		return -ENOMEM;
+	dest->net = get_net(current->nsproxy->net_ns);
 	dest->portid = portid;
-	dest->pid = task_pid_vnr(current);
 	skb_queue_head_init(&dest->q);
 
 	mutex_lock(&audit_filter_mutex);

Signature · 1 line

1.7.5.4

Quoted 10 lines · Eric W. Biederman

While reading through 3.14-rc1 I found a pretty siginficant mishandling of network namespaces in the recent audit changes.

In struct audit_netlink_list and audit_reply add a reference to the network namespace of the caller and remove the userspace pid of the caller. This cleanly remembers the callers network namespace, and removes a huge class of races and nasty failure modes that can occur when attempting to relook up the callers network namespace from a pid_t (including the caller's network namespace changing, pid wraparound, and the pid simply not being present).

Ok, so I see that avoiding pid_t in struct audit_reply and struct audit_netlink_list is necessary. Why not switch to struct pid?

How does this patch solve a caller's network namespace changing?

Modify audit_send_reply to directly use a non-blocking send and to return an error on failure (if anyone cares).

Modify audit_list_rules_send to use audit_send_reply and give up if we can not send a packet.

Merge audit_list_rules into iaudit_list_rules_send as the code is now sufficiently simple to not justify to callers.

Kill audit_send_list, audit_send_reply_thread because using a separate thread for replies is not needed when sending packets syncrhonously.

Quoted 8 lines

Modify audit_send_reply to directly use a non-blocking send and to return an error on failure (if anyone cares).

Modify audit_list_rules_send to use audit_send_reply and give up if we can not send a packet.

Quoted 6 lines

Merge audit_list_rules into iaudit_list_rules_send as the code is now sufficiently simple to not justify to callers.

Kill audit_send_list, audit_send_reply_thread because using a separate thread for replies is not needed when sending packets syncrhonously.

Nothing much seems to be happening here?

In an earlier email you said "While reading through 3.14-rc1 I found a pretty siginficant mishandling of network namespaces in the recent audit changes." Were those recent audit changes a post-3.14 thing? And what is the user-visible effect of the mishandling?

From: Andrew Morton <[email protected]>
Date: Tue, 4 Mar 2014 13:30:04 -0800

Quoted 17 lines · On Fri, 28 Feb 2014 20:50:19 -0800 [email protected] (Eric W. Biederman)…

Quoted 8 lines

Modify audit_send_reply to directly use a non-blocking send and to return an error on failure (if anyone cares).

Modify audit_list_rules_send to use audit_send_reply and give up if we can not send a packet.

Quoted 6 lines

Merge audit_list_rules into iaudit_list_rules_send as the code is now sufficiently simple to not justify to callers.

Kill audit_send_list, audit_send_reply_thread because using a separate thread for replies is not needed when sending packets syncrhonously.

Nothing much seems to be happening here?

Quoted 4 lines

In an earlier email you said "While reading through 3.14-rc1 I found a pretty siginficant mishandling of network namespaces in the recent audit changes." Were those recent audit changes a post-3.14 thing? And what is the user-visible effect of the mishandling?

I took a quick look at this patch yesterday, and my only suspicion is that threads are created currently by this code purely to cons up a blockable context for the netlink code.

Perhaps it wants to avoid any netlink packet drops from being possible.

I'm not so sure.

Anyways, some investigation into perhaps figuring out the intentions of the original implementation would be nice.

Quoted 15 lines · David Miller

From: Andrew Morton <[email protected]>
Date: Tue, 4 Mar 2014 13:30:04 -0800

Quoted 12 lines · On Fri, 28 Feb 2014 20:50:19 -0800 [email protected] (Eric W. Biederman)…

Quoted 11 lines

Modify audit_send_reply to directly use a non-blocking send and to return an error on failure (if anyone cares).

Modify audit_list_rules_send to use audit_send_reply and give up if we can not send a packet.

Merge audit_list_rules into iaudit_list_rules_send as the code is now sufficiently simple to not justify to callers.

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

Kill audit_send_list, audit_send_reply_thread because using a separate thread for replies is not needed when sending packets syncrhonously.

Nothing much seems to be happening here?

Well you picked up the patch that fixes the worst of the bugs that I was complaining about. Beyond that I don't know what makes sense.

If we really want the ability to always appened to the queue of skb's is to just have a version of netlink_send_skb that ignores the queued limits. Of course an evil program then could force the generation of enough audit records to DOS the kernel, but we seem to be in that situation now. Shrug.

From: [email protected] (Eric W. Biederman)
Date: Tue, 04 Mar 2014 14:41:16 -0800

Quoted 5 lines

If we really want the ability to always appened to the queue of skb's is to just have a version of netlink_send_skb that ignores the queued limits. Of course an evil program then could force the generation of enough audit records to DOS the kernel, but we seem to be in that situation now. Shrug.

There is never a valid reason to bypass the socket limits.

It protects the system from things going out of control.

Netlink packet sends can fail, and audit should cope with that event instead of trying to bludgeon it into not happening.

The audit system has to be very reliable. It can't lose any event or record. The people that really depend on it would rather have access denied to the system than lose any event. This is the reason it goes to such lengths.

Quoted 13 lines · David Miller

From: [email protected] (Eric W. Biederman)
Date: Tue, 04 Mar 2014 14:41:16 -0800

Quoted 5 lines

If we really want the ability to always appened to the queue of skb's is to just have a version of netlink_send_skb that ignores the queued limits. Of course an evil program then could force the generation of enough audit records to DOS the kernel, but we seem to be in that situation now. Shrug.

There is never a valid reason to bypass the socket limits.

It protects the system from things going out of control.

Quoted 2 lines

Netlink packet sends can fail, and audit should cope with that event instead of trying to bludgeon it into not happening.

I am not sure what exactly is the problem with the code that was being patched. The audit code has been stable and working pretty well for everyone for the last 5-6 years. But lately there has been a lot of kernel code churn and I am concerned that the changes are being made without a complete understanding of the design goals.

The audit system has to be very reliable. It can't lose any event or record. The people that really depend on it would rather have access denied to the system than lose any event. This is the reason it goes to such lengths.

If I understand the patch, it looks like its affecting code that adds, deletes, or lists audit rules. This code is quite old and working well. It didn't at first. We found a lot of problems by writing scripts like:

#!/bin/bash
while [ 1 ] ;
do

        echo "Inserting syscalls..."
        sys=1
        while [ $sys -lt 100 ]
        do
                sys=`expr $sys + 1`
                echo "$sys"
                auditctl -a entry,always -S $sys
        done

        echo "Listing..."
        auditctl -l
        echo "Deleting..."
        auditctl -D

done

with another shell running:

#!/bin/bash
while [ 1 ] ;
do

	echo "Listing..."
	auditctl -l

done

and another shell running:

#!/bin/bash
while [ 1 ] ;
do

	echo "Disabling..."
	auditctl -e 0
	echo "Enabling..."
	auditctl -e 1

done

You can probably imagine more stress tests. But the proposed code should be well tested similar to this.

Quoted 4 lines · Steve Grubb

Quoted 3 lines · David Miller

From: [email protected] (Eric W. Biederman)
Date: Tue, 04 Mar 2014 14:41:16 -0800

Quoted 7 lines

Quoted 7 lines

Quoted 5 lines

If we really want the ability to always appened to the queue of skb's is to just have a version of netlink_send_skb that ignores the queued limits. Of course an evil program then could force the generation of enough audit records to DOS the kernel, but we seem to be in that situation now. Shrug.

There is never a valid reason to bypass the socket limits.

Audit is non-tolerant to failure and loss.

From: Eric Paris <[email protected]>
Date: Fri, 07 Mar 2014 17:52:02 -0500

Quoted 1 line

Audit is non-tolerant to failure and loss.

Netlink is not a loss-less transport.

From: Eric Paris <[email protected]>
Date: Fri, 07 Mar 2014 17:52:02 -0500

Quoted 1 line

Audit is non-tolerant to failure and loss.

Netlink is not a loss-less transport.

Quoted 6 lines

From: Eric Paris <[email protected]>
Date: Fri, 07 Mar 2014 17:52:02 -0500

Quoted 1 line

Audit is non-tolerant to failure and loss.

Netlink is not a loss-less transport.

Perhaps. But in all our testing over the years its been very good.

Linus,

Please pull the for-linus branch from the git tree:

git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git for-linus

HEAD: d211f177b28ec070c25b3d0b960aa55f352f731f audit: Update kdoc for audit_send_reply and audit_list_rules_send

Starting with 3.14-rc1 the audit code is faulty (think oopses and races) with respect to how it computes the network namespace of which socket to reply to, and I happened to notice by chance when reading through the code.

My efforts to get these fixes noticed by people who care about audit seem to have landed on deaf ears, so since these are namespace related I have put them in my tree.

The second user Eric patched, audit_send_list(), can grow without bound. The number of skb's is going to be the size of the number of audit rules that root loaded. We run the list of rules, generate an skb per rule, and add all of them to an skb_buff_head. We then pass the skb_buff_head to a kthread so that current will be able to read/drain the socket.

There really is no limit to how big the skb_buff_head could possibly grow. This doesn't necessarily absolutely have to be lossless but it can actually quite reasonably be a whole lot of data that needs to get sent. I know of no way to deliver unbounded lengths of data to the current task via netlink without blocking on more space in the socket. Even if the socket rmem was MAX_INT, how can we deliver more? The rule size is unbounded. How do I get an unbounded amount of data onto this side of the socket when I have to generate it all during the request...

Quoted 3 lines · Eric W. Biederman

My efforts to get these fixes noticed by people who care about audit seem to have landed on deaf ears, so since these are namespace related I have put them in my tree.

This commentary sounds like a pile of crap seeing as how there was a whole discussion around how to handle this stuff, which you were a part of. And that the audit tree already picked up these 2 patches.

In any case, since I haven't sent them to Linus and I'm glad that is done, so feel free to consider this me Acking the pull request.

From: Eric Paris <[email protected]>
Date: Fri, 07 Mar 2014 17:52:02 -0500

Quoted 5 lines

The second user Eric patched, audit_send_list(), can grow without bound. The number of skb's is going to be the size of the number of audit rules that root loaded. We run the list of rules, generate an skb per rule, and add all of them to an skb_buff_head. We then pass the skb_buff_head to a kthread so that current will be able to read/drain the socket.

Quoted 8 lines

There really is no limit to how big the skb_buff_head could possibly grow. This doesn't necessarily absolutely have to be lossless but it can actually quite reasonably be a whole lot of data that needs to get sent. I know of no way to deliver unbounded lengths of data to the current task via netlink without blocking on more space in the socket. Even if the socket rmem was MAX_INT, how can we deliver more? The rule size is unbounded. How do I get an unbounded amount of data onto this side of the socket when I have to generate it all during the request...

This is what netlink dumps are for. It is how we are able to dump routing tables with millions of routes to userspace.

By using normal netlink requests and netlink_unicast() for this, you are ignoring an entire mechanism in netlink designed specifically to handle this kind of situation.

Netlink dumps track state and build one or more SKBs (as necessary), one by one, to form the reply. It implements flow control, state tracking for iteration, optimized SKB sizing and allocation, etc.

While reading through 3.14-rc1 I found a pretty siginficant mishandling of network namespaces in the recent audit changes.

In struct audit_netlink_list and audit_reply add a reference to the network namespace of the caller and remove the userspace pid of the caller. This cleanly remembers the callers network namespace, and removes a huge class of races and nasty failure modes that can occur when attempting to relook up the callers network namespace from a pid_t (including the caller's network namespace changing, pid wraparound, and the pid simply not being present).

Signed-off-by "Eric W. Biederman" <[email protected]>

---
 kernel/audit.c       |   10 ++++++----
 kernel/audit.h       |    2 +-
 kernel/auditfilter.c |    3 ++-
 3 files changed, 9 insertions(+), 6 deletions(-)

kernel/audit.c +6 -4

index 34c5a2310fbf..1e5756f16f6f 100644
@@ -182,7 +182,7 @@ struct audit_buffer {
 
 struct audit_reply {
 	__u32 portid;
-	pid_t pid;
+	struct net *net;	
 	struct sk_buff *skb;
 };
 

@@ -500,7 +500,7 @@ int audit_send_list(void *_dest)
 {
 	struct audit_netlink_list *dest = _dest;
 	struct sk_buff *skb;
-	struct net *net = get_net_ns_by_pid(dest->pid);
+	struct net *net = dest->net;
 	struct audit_net *aunet = net_generic(net, audit_net_id);
 
 	/* wait for parent to finish and send an ACK */

@@ -510,6 +510,7 @@ int audit_send_list(void *_dest)
 	while ((skb = __skb_dequeue(&dest->q)) != NULL)
 		netlink_unicast(aunet->nlsk, skb, dest->portid, 0);
 
+	put_net(net);
 	kfree(dest);
 
 	return 0;

@@ -543,7 +544,7 @@ out_kfree_skb:
 static int audit_send_reply_thread(void *arg)
 {
 	struct audit_reply *reply = (struct audit_reply *)arg;
-	struct net *net = get_net_ns_by_pid(reply->pid);
+	struct net *net = reply->net;
 	struct audit_net *aunet = net_generic(net, audit_net_id);
 
 	mutex_lock(&audit_cmd_mutex);

@@ -552,6 +553,7 @@ static int audit_send_reply_thread(void *arg)
 	/* Ignore failure. It'll only happen if the sender goes away,
 	   because our timeout is set to infinite. */
 	netlink_unicast(aunet->nlsk , reply->skb, reply->portid, 0);
+	put_net(net);
 	kfree(reply);
 	return 0;
 }

@@ -583,8 +585,8 @@ static void audit_send_reply(__u32 portid, int seq, int type, int done,
 	if (!skb)
 		goto out;
 
+	reply->net = get_net(current->nsproxy->net_ns);
 	reply->portid = portid;
-	reply->pid = task_pid_vnr(current);
 	reply->skb = skb;
 
 	tsk = kthread_run(audit_send_reply_thread, reply, "audit_send_reply");

kernel/audit.h +1 -1

index 57cc64d67718..8df132214606 100644
@@ -247,7 +247,7 @@ extern void		    audit_panic(const char *message);
 
 struct audit_netlink_list {
 	__u32 portid;
-	pid_t pid;
+	struct net *net;
 	struct sk_buff_head q;
 };
 

kernel/auditfilter.c +2 -1

index 14a78cca384e..a5e3d73d73e4 100644
@@ -29,6 +29,7 @@
 #include <linux/sched.h>
 #include <linux/slab.h>
 #include <linux/security.h>
+#include <net/net_namespace.h>
 #include "audit.h"
 
 /*

@@ -1083,8 +1084,8 @@ int audit_list_rules_send(__u32 portid, int seq)
 	dest = kmalloc(sizeof(struct audit_netlink_list), GFP_KERNEL);
 	if (!dest)
 		return -ENOMEM;
+	dest->net = get_net(current->nsproxy->net_ns);
 	dest->portid = portid;
-	dest->pid = task_pid_vnr(current);
 	skb_queue_head_init(&dest->q);
 
 	mutex_lock(&audit_filter_mutex);

Signature · 1 line

1.7.5.4

In perverse cases of file descriptor passing the current network namespace of a process and the network namespace of a socket used by that socket may differ. Therefore use the network namespace of the appropiate socket to ensure replies always go to the appropiate socket.

Signed-off-by "Eric W. Biederman" <[email protected]>

---

This is an incremental change on top of my previous patch to guarantee that replies always happen in the appropriate network namespace.

 include/linux/audit.h |    3 ++-
 kernel/audit.c        |   21 ++++++++++-----------
 kernel/auditfilter.c  |    7 +++++--
 3 files changed, 17 insertions(+), 14 deletions(-)

include/linux/audit.h +2 -1

index aa865a9a4c4f..ec1464df4c60 100644
@@ -43,6 +43,7 @@ struct mq_attr;
 struct mqstat;
 struct audit_watch;
 struct audit_tree;
+struct sk_buff;
 
 struct audit_krule {
 	int			vers_ops;

@@ -463,7 +464,7 @@ extern int audit_filter_user(int type);
 extern int audit_filter_type(int type);
 extern int audit_rule_change(int type, __u32 portid, int seq,
 				void *data, size_t datasz);
-extern int audit_list_rules_send(__u32 portid, int seq);
+extern int audit_list_rules_send(struct sk_buff *request_skb, int seq);
 
 extern u32 audit_enabled;
 #else /* CONFIG_AUDIT */

kernel/audit.c +10 -11

index 1e5756f16f6f..32086bff5564 100644
@@ -570,9 +570,11 @@ static int audit_send_reply_thread(void *arg)
  * Allocates an skb, builds the netlink message, and sends it to the port id.
  * No failure notifications.
  */
-static void audit_send_reply(__u32 portid, int seq, int type, int done,
+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;
 	struct audit_reply *reply = kmalloc(sizeof(struct audit_reply),

@@ -585,7 +587,7 @@ static void audit_send_reply(__u32 portid, int seq, int type, int done,
 	if (!skb)
 		goto out;
 
-	reply->net = get_net(current->nsproxy->net_ns);
+	reply->net = get_net(net);
 	reply->portid = portid;
 	reply->skb = skb;
 

@@ -675,8 +677,7 @@ static int audit_get_feature(struct sk_buff *skb)
 
 	seq = nlmsg_hdr(skb)->nlmsg_seq;
 
-	audit_send_reply(NETLINK_CB(skb).portid, seq, AUDIT_GET, 0, 0,
-			 &af, sizeof(af));
+	audit_send_reply(skb, seq, AUDIT_GET, 0, 0, &af, sizeof(af));
 
 	return 0;
 }

@@ -796,8 +797,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 		s.backlog		= skb_queue_len(&audit_skb_queue);
 		s.version		= AUDIT_VERSION_LATEST;
 		s.backlog_wait_time	= audit_backlog_wait_time;
-		audit_send_reply(NETLINK_CB(skb).portid, seq, AUDIT_GET, 0, 0,
-				 &s, sizeof(s));
+		audit_send_reply(skb, seq, AUDIT_GET, 0, 0, &s, sizeof(s));
 		break;
 	}
 	case AUDIT_SET: {

@@ -907,7 +907,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 					   seq, data, nlmsg_len(nlh));
 		break;
 	case AUDIT_LIST_RULES:
-		err = audit_list_rules_send(NETLINK_CB(skb).portid, seq);
+		err = audit_list_rules_send(skb, seq);
 		break;
 	case AUDIT_TRIM:
 		audit_trim_trees();

@@ -972,8 +972,8 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 			memcpy(sig_data->ctx, ctx, len);
 			security_release_secctx(ctx, len);
 		}
-		audit_send_reply(NETLINK_CB(skb).portid, seq, AUDIT_SIGNAL_INFO,
-				0, 0, sig_data, sizeof(*sig_data) + len);
+		audit_send_reply(skb, seq, AUDIT_SIGNAL_INFO, 0, 0,
+				 sig_data, sizeof(*sig_data) + len);
 		kfree(sig_data);
 		break;
 	case AUDIT_TTY_GET: {

@@ -985,8 +985,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 		s.log_passwd = tsk->signal->audit_tty_log_passwd;
 		spin_unlock(&tsk->sighand->siglock);
 
-		audit_send_reply(NETLINK_CB(skb).portid, seq,
-				 AUDIT_TTY_GET, 0, 0, &s, sizeof(s));
+		audit_send_reply(skb, seq, AUDIT_TTY_GET, 0, 0, &s, sizeof(s));
 		break;
 	}
 	case AUDIT_TTY_SET: {

kernel/auditfilter.c +5 -2

index a5e3d73d73e4..e8d1c7c515d7 100644
@@ -30,6 +30,7 @@
 #include <linux/slab.h>
 #include <linux/security.h>
 #include <net/net_namespace.h>
+#include <net/sock.h>
 #include "audit.h"
 
 /*

@@ -1069,8 +1070,10 @@ int audit_rule_change(int type, __u32 portid, int seq, void *data,
  * @portid: target portid for netlink audit messages
  * @seq: netlink audit message sequence (serial) number
  */
-int audit_list_rules_send(__u32 portid, int seq)
+int audit_list_rules_send(struct sk_buff *request_skb, int seq)
 {
+	u32 portid = NETLINK_CB(request_skb).portid;
+	struct net *net = sock_net(NETLINK_CB(request_skb).sk);
 	struct task_struct *tsk;
 	struct audit_netlink_list *dest;
 	int err = 0;

@@ -1084,7 +1087,7 @@ int audit_list_rules_send(__u32 portid, int seq)
 	dest = kmalloc(sizeof(struct audit_netlink_list), GFP_KERNEL);
 	if (!dest)
 		return -ENOMEM;
-	dest->net = get_net(current->nsproxy->net_ns);
+	dest->net = get_net(net);
 	dest->portid = portid;
 	skb_queue_head_init(&dest->q);
 

Signature · 1 line

1.7.5.4

Quoted 5 lines · Eric Paris

Quoted 3 lines · Eric W. Biederman

My efforts to get these fixes noticed by people who care about audit seem to have landed on deaf ears, so since these are namespace related I have put them in my tree.

Quoted 3 lines

This commentary sounds like a pile of crap seeing as how there was a whole discussion around how to handle this stuff, which you were a part of. And that the audit tree already picked up these 2 patches.

My apologies for offending. Last night when I looked the audit tree had not been updated in 7 weeks, and Andrew Morton wasn't ready to push these fixes to Linus and was looking for someone who would. Keeping important bug fixes in my retransmit queue leaves me very grumpy.

Quoted 2 lines

In any case, since I haven't sent them to Linus and I'm glad that is done, so feel free to consider this me Acking the pull request.

Eric

Quoted 12 lines · Eric W. Biederman

While reading through 3.14-rc1 I found a pretty siginficant mishandling of network namespaces in the recent audit changes.

In struct audit_netlink_list and audit_reply add a reference to the network namespace of the caller and remove the userspace pid of the caller. This cleanly remembers the callers network namespace, and removes a huge class of races and nasty failure modes that can occur when attempting to relook up the callers network namespace from a pid_t (including the caller's network namespace changing, pid wraparound, and the pid simply not being present).

Quoted 1 line

Signed-off-by "Eric W. Biederman" <[email protected]>

Signed-off-by Richard Guy Briggs <[email protected]>

Quoted 101 lines

---
 kernel/audit.c       |   10 ++++++----
 kernel/audit.h       |    2 +-
 kernel/auditfilter.c |    3 ++-
 3 files changed, 9 insertions(+), 6 deletions(-)

kernel/audit.c +6 -4

index 34c5a2310fbf..1e5756f16f6f 100644
@@ -182,7 +182,7 @@ struct audit_buffer {
 
 struct audit_reply {
 	__u32 portid;
-	pid_t pid;
+	struct net *net;	
 	struct sk_buff *skb;
 };
 

@@ -500,7 +500,7 @@ int audit_send_list(void *_dest)
 {
 	struct audit_netlink_list *dest = _dest;
 	struct sk_buff *skb;
-	struct net *net = get_net_ns_by_pid(dest->pid);
+	struct net *net = dest->net;
 	struct audit_net *aunet = net_generic(net, audit_net_id);
 
 	/* wait for parent to finish and send an ACK */

@@ -510,6 +510,7 @@ int audit_send_list(void *_dest)
 	while ((skb = __skb_dequeue(&dest->q)) != NULL)
 		netlink_unicast(aunet->nlsk, skb, dest->portid, 0);
 
+	put_net(net);
 	kfree(dest);
 
 	return 0;

@@ -543,7 +544,7 @@ out_kfree_skb:
 static int audit_send_reply_thread(void *arg)
 {
 	struct audit_reply *reply = (struct audit_reply *)arg;
-	struct net *net = get_net_ns_by_pid(reply->pid);
+	struct net *net = reply->net;
 	struct audit_net *aunet = net_generic(net, audit_net_id);
 
 	mutex_lock(&audit_cmd_mutex);

@@ -552,6 +553,7 @@ static int audit_send_reply_thread(void *arg)
 	/* Ignore failure. It'll only happen if the sender goes away,
 	   because our timeout is set to infinite. */
 	netlink_unicast(aunet->nlsk , reply->skb, reply->portid, 0);
+	put_net(net);
 	kfree(reply);
 	return 0;
 }

@@ -583,8 +585,8 @@ static void audit_send_reply(__u32 portid, int seq, int type, int done,
 	if (!skb)
 		goto out;
 
+	reply->net = get_net(current->nsproxy->net_ns);
 	reply->portid = portid;
-	reply->pid = task_pid_vnr(current);
 	reply->skb = skb;
 
 	tsk = kthread_run(audit_send_reply_thread, reply, "audit_send_reply");

kernel/audit.h +1 -1

index 57cc64d67718..8df132214606 100644
@@ -247,7 +247,7 @@ extern void		    audit_panic(const char *message);
 
 struct audit_netlink_list {
 	__u32 portid;
-	pid_t pid;
+	struct net *net;
 	struct sk_buff_head q;
 };
 

kernel/auditfilter.c +2 -1

index 14a78cca384e..a5e3d73d73e4 100644
@@ -29,6 +29,7 @@
 #include <linux/sched.h>
 #include <linux/slab.h>
 #include <linux/security.h>
+#include <net/net_namespace.h>
 #include "audit.h"
 
 /*

@@ -1083,8 +1084,8 @@ int audit_list_rules_send(__u32 portid, int seq)
 	dest = kmalloc(sizeof(struct audit_netlink_list), GFP_KERNEL);
 	if (!dest)
 		return -ENOMEM;
+	dest->net = get_net(current->nsproxy->net_ns);
 	dest->portid = portid;
-	dest->pid = task_pid_vnr(current);
 	skb_queue_head_init(&dest->q);
 
 	mutex_lock(&audit_filter_mutex);

Signature · 1 line

1.7.5.4

- RGB

--
Richard Guy Briggs <[email protected]>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

Quoted 8 lines · Eric W. Biederman

In perverse cases of file descriptor passing the current network namespace of a process and the network namespace of a socket used by that socket may differ. Therefore use the network namespace of the appropiate socket to ensure replies always go to the appropiate socket.

Signed-off-by "Eric W. Biederman" <[email protected]>

Signed-off-by Richard Guy Briggs <[email protected]>

Quoted 143 lines

---

This is an incremental change on top of my previous patch to guarantee that replies always happen in the appropriate network namespace.

 include/linux/audit.h |    3 ++-
 kernel/audit.c        |   21 ++++++++++-----------
 kernel/auditfilter.c  |    7 +++++--
 3 files changed, 17 insertions(+), 14 deletions(-)

include/linux/audit.h +2 -1

index aa865a9a4c4f..ec1464df4c60 100644
@@ -43,6 +43,7 @@ struct mq_attr;
 struct mqstat;
 struct audit_watch;
 struct audit_tree;
+struct sk_buff;
 
 struct audit_krule {
 	int			vers_ops;

@@ -463,7 +464,7 @@ extern int audit_filter_user(int type);
 extern int audit_filter_type(int type);
 extern int audit_rule_change(int type, __u32 portid, int seq,
 				void *data, size_t datasz);
-extern int audit_list_rules_send(__u32 portid, int seq);
+extern int audit_list_rules_send(struct sk_buff *request_skb, int seq);
 
 extern u32 audit_enabled;
 #else /* CONFIG_AUDIT */

kernel/audit.c +10 -11

index 1e5756f16f6f..32086bff5564 100644
@@ -570,9 +570,11 @@ static int audit_send_reply_thread(void *arg)
  * Allocates an skb, builds the netlink message, and sends it to the port id.
  * No failure notifications.
  */
-static void audit_send_reply(__u32 portid, int seq, int type, int done,
+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;
 	struct audit_reply *reply = kmalloc(sizeof(struct audit_reply),

@@ -585,7 +587,7 @@ static void audit_send_reply(__u32 portid, int seq, int type, int done,
 	if (!skb)
 		goto out;
 
-	reply->net = get_net(current->nsproxy->net_ns);
+	reply->net = get_net(net);
 	reply->portid = portid;
 	reply->skb = skb;
 

@@ -675,8 +677,7 @@ static int audit_get_feature(struct sk_buff *skb)
 
 	seq = nlmsg_hdr(skb)->nlmsg_seq;
 
-	audit_send_reply(NETLINK_CB(skb).portid, seq, AUDIT_GET, 0, 0,
-			 &af, sizeof(af));
+	audit_send_reply(skb, seq, AUDIT_GET, 0, 0, &af, sizeof(af));
 
 	return 0;
 }

@@ -796,8 +797,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 		s.backlog		= skb_queue_len(&audit_skb_queue);
 		s.version		= AUDIT_VERSION_LATEST;
 		s.backlog_wait_time	= audit_backlog_wait_time;
-		audit_send_reply(NETLINK_CB(skb).portid, seq, AUDIT_GET, 0, 0,
-				 &s, sizeof(s));
+		audit_send_reply(skb, seq, AUDIT_GET, 0, 0, &s, sizeof(s));
 		break;
 	}
 	case AUDIT_SET: {

@@ -907,7 +907,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 					   seq, data, nlmsg_len(nlh));
 		break;
 	case AUDIT_LIST_RULES:
-		err = audit_list_rules_send(NETLINK_CB(skb).portid, seq);
+		err = audit_list_rules_send(skb, seq);
 		break;
 	case AUDIT_TRIM:
 		audit_trim_trees();

@@ -972,8 +972,8 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 			memcpy(sig_data->ctx, ctx, len);
 			security_release_secctx(ctx, len);
 		}
-		audit_send_reply(NETLINK_CB(skb).portid, seq, AUDIT_SIGNAL_INFO,
-				0, 0, sig_data, sizeof(*sig_data) + len);
+		audit_send_reply(skb, seq, AUDIT_SIGNAL_INFO, 0, 0,
+				 sig_data, sizeof(*sig_data) + len);
 		kfree(sig_data);
 		break;
 	case AUDIT_TTY_GET: {

@@ -985,8 +985,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
 		s.log_passwd = tsk->signal->audit_tty_log_passwd;
 		spin_unlock(&tsk->sighand->siglock);
 
-		audit_send_reply(NETLINK_CB(skb).portid, seq,
-				 AUDIT_TTY_GET, 0, 0, &s, sizeof(s));
+		audit_send_reply(skb, seq, AUDIT_TTY_GET, 0, 0, &s, sizeof(s));
 		break;
 	}
 	case AUDIT_TTY_SET: {

kernel/auditfilter.c +5 -2

index a5e3d73d73e4..e8d1c7c515d7 100644
@@ -30,6 +30,7 @@
 #include <linux/slab.h>
 #include <linux/security.h>
 #include <net/net_namespace.h>
+#include <net/sock.h>
 #include "audit.h"
 
 /*

@@ -1069,8 +1070,10 @@ int audit_rule_change(int type, __u32 portid, int seq, void *data,
  * @portid: target portid for netlink audit messages
  * @seq: netlink audit message sequence (serial) number
  */
-int audit_list_rules_send(__u32 portid, int seq)
+int audit_list_rules_send(struct sk_buff *request_skb, int seq)
 {
+	u32 portid = NETLINK_CB(request_skb).portid;
+	struct net *net = sock_net(NETLINK_CB(request_skb).sk);
 	struct task_struct *tsk;
 	struct audit_netlink_list *dest;
 	int err = 0;

@@ -1084,7 +1087,7 @@ int audit_list_rules_send(__u32 portid, int seq)
 	dest = kmalloc(sizeof(struct audit_netlink_list), GFP_KERNEL);
 	if (!dest)
 		return -ENOMEM;
-	dest->net = get_net(current->nsproxy->net_ns);
+	dest->net = get_net(net);
 	dest->portid = portid;
 	skb_queue_head_init(&dest->q);
 

Signature · 1 line

1.7.5.4

- RGB

--
Richard Guy Briggs <[email protected]>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545