[PATCH] netlink: have netlink per-protocol bind function return an error code.

[PATCH] netlink: have netlink per-protocol bind function return an error code.

[Richard Guy Briggs]

Have the netlink per-protocol optional bind function return an int error code
rather than void to signal a failure.

This will enable netlink protocols to perform extra checks including
capabilities and permissions verifications when updating memberships in
multicast groups.

In netlink_bind() and netlink_setsockopt() the call to the per-protocol bind
function was moved above the multicast group update to prevent any access to
the multicast socket groups before checking with the per-protocol bind
function.  This will enable the per-protocol bind function to be used to check
permissions which could be denied before making them available, and to avoid
the messy job of undoing the addition should the per-protocol bind function
fail.

The netfilter subsystem seems to be the only one currently using the
per-protocol bind function.

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
In particular, the audit subsystem (NETLINK_AUDIT protocol) could benefit by
being able to check specific capabilities for each multicast group before
granting membership to the requesting socket.  Currently, all NETLINK_AUDIT
sockets must have the capability CAP_NET_ADMIN.  No other capabilities are
required to join a multicast group.  This capability is too broad allowing
access to this socket by many applications that must not have access to this
information.  It is proposed to add capability CAP_AUDIT_READ to allow this
access while dropping the exessively broad capability CAP_NET_ADMIN.

There has also been some interest expressed by IETF ForCES folk.
---
 include/linux/netlink.h   |    2 +-
 net/netfilter/nfnetlink.c |    3 ++-
 net/netlink/af_netlink.c  |   30 +++++++++++++++++-------------
 net/netlink/af_netlink.h  |    4 ++--
 4 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index 7a6c396..4402653 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -45,7 +45,7 @@ struct netlink_kernel_cfg {
 	unsigned int	flags;
 	void		(*input)(struct sk_buff *skb);
 	struct mutex	*cb_mutex;
-	void		(*bind)(int group);
+	int		(*bind)(int group);
 	bool		(*compare)(struct net *net, struct sock *sk);
 };
 
diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
index 75619f9..10a4cf5 100644
--- a/net/netfilter/nfnetlink.c
+++ b/net/netfilter/nfnetlink.c
@@ -392,7 +392,7 @@ static void nfnetlink_rcv(struct sk_buff *skb)
 }
 
 #ifdef CONFIG_MODULES
-static void nfnetlink_bind(int group)
+static int nfnetlink_bind(int group)
 {
 	const struct nfnetlink_subsystem *ss;
 	int type = nfnl_group2type[group];
@@ -403,6 +403,7 @@ static void nfnetlink_bind(int group)
 	if (!ss) {
 		request_module("nfnetlink-subsys-%d", type);
 	}
+	return 0;
 }
 #endif
 
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index bca50b9..4224dc5 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1198,7 +1198,7 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
 	struct module *module = NULL;
 	struct mutex *cb_mutex;
 	struct netlink_sock *nlk;
-	void (*bind)(int group);
+	int (*bind)(int group);
 	int err = 0;
 
 	sock->state = SS_UNCONNECTED;
@@ -1441,6 +1441,17 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 	if (!nladdr->nl_groups && (nlk->groups == NULL || !(u32)nlk->groups[0]))
 		return 0;
 
+	if (nlk->netlink_bind && nladdr->nl_groups) {
+		int i;
+
+		for (i = 0; i < nlk->ngroups; i++)
+			if (test_bit(i, (long unsigned int *)&nladdr->nl_groups)) {
+				err = nlk->netlink_bind(i);
+				if (err)
+					return err;
+			}
+	}
+
 	netlink_table_grab();
 	netlink_update_subscriptions(sk, nlk->subscriptions +
 					 hweight32(nladdr->nl_groups) -
@@ -1449,15 +1460,6 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 	netlink_update_listeners(sk);
 	netlink_table_ungrab();
 
-	if (nlk->netlink_bind && nlk->groups[0]) {
-		int i;
-
-		for (i=0; i<nlk->ngroups; i++) {
-			if (test_bit(i, nlk->groups))
-				nlk->netlink_bind(i);
-		}
-	}
-
 	return 0;
 }
 
@@ -2095,14 +2097,16 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
 			return err;
 		if (!val || val - 1 >= nlk->ngroups)
 			return -EINVAL;
+		if (nlk->netlink_bind) {
+			err = nlk->netlink_bind(val);
+			if (err)
+				return err;
+		}
 		netlink_table_grab();
 		netlink_update_socket_mc(nlk, val,
 					 optname == NETLINK_ADD_MEMBERSHIP);
 		netlink_table_ungrab();
 
-		if (nlk->netlink_bind)
-			nlk->netlink_bind(val);
-
 		err = 0;
 		break;
 	}
diff --git a/net/netlink/af_netlink.h b/net/netlink/af_netlink.h
index acbd774..0edb8d5 100644
--- a/net/netlink/af_netlink.h
+++ b/net/netlink/af_netlink.h
@@ -37,7 +37,7 @@ struct netlink_sock {
 	struct mutex		*cb_mutex;
 	struct mutex		cb_def_mutex;
 	void			(*netlink_rcv)(struct sk_buff *skb);
-	void			(*netlink_bind)(int group);
+	int			(*netlink_bind)(int group);
 	struct module		*module;
 #ifdef CONFIG_NETLINK_MMAP
 	struct mutex		pg_vec_lock;
@@ -73,7 +73,7 @@ struct netlink_table {
 	unsigned int		groups;
 	struct mutex		*cb_mutex;
 	struct module		*module;
-	void			(*bind)(int group);
+	int			(*bind)(int group);
 	bool			(*compare)(struct net *net, struct sock *sock);
 	int			registered;
 };
-- 
1.7.1

Re: [PATCH] netlink: have netlink per-protocol bind function return an error code.

[David Miller]

From: Richard Guy Briggs <rgb@redhat.com>
Date: Fri, 21 Mar 2014 12:39:11 -0400

> @@ -1441,6 +1441,17 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
>  	if (!nladdr->nl_groups && (nlk->groups == NULL || !(u32)nlk->groups[0]))
>  		return 0;
>  
> +	if (nlk->netlink_bind && nladdr->nl_groups) {
> +		int i;
> +
> +		for (i = 0; i < nlk->ngroups; i++)
> +			if (test_bit(i, (long unsigned int *)&nladdr->nl_groups)) {
> +				err = nlk->netlink_bind(i);
> +				if (err)
> +					return err;
> +			}
> +	}
> +

You can't just leave a partially set of completed bindings in place.

It's not valid to leave half-baked state like this.

If you return an error, all of the binding state changes must be
completely undone.

If you can't find a way to do this cleanly, you'll need to find
a way for the audit code to not return an error.

Re: [PATCH] netlink: have netlink per-protocol bind function return an error code.

[Richard Guy Briggs]

On 14/03/23, David Miller wrote:
> From: Richard Guy Briggs <rgb@redhat.com>
> Date: Fri, 21 Mar 2014 12:39:11 -0400
> 
> > @@ -1441,6 +1441,17 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
> >  	if (!nladdr->nl_groups && (nlk->groups == NULL || !(u32)nlk->groups[0]))
> >  		return 0;
> >  
> > +	if (nlk->netlink_bind && nladdr->nl_groups) {
> > +		int i;
> > +
> > +		for (i = 0; i < nlk->ngroups; i++)
> > +			if (test_bit(i, (long unsigned int *)&nladdr->nl_groups)) {
> > +				err = nlk->netlink_bind(i);
> > +				if (err)
> > +					return err;
> > +			}
> > +	}
> > +
> 
> You can't just leave a partially set of completed bindings in place.

In the general case, I agree.

> It's not valid to leave half-baked state like this.

In the one existing case (netfilter), it adds a module that is never
unloaded.  (refcounts are bumped up and down, but I don't see an
auto-reap based on cleared multicast group subscriptions.)  For that
matter, netlink_realloc_groups() isn't reversed on error either.

In the proposed case (audit) it is only a permissions check, so there is
nothing to undo.

So, I was being lazy looking at the existing situation.

> If you return an error, all of the binding state changes must be
> completely undone.

Is it time to add a ".unbind = netlink_unbind" to struct proto_ops
netlink_ops?  (I am only half serious here...)

> If you can't find a way to do this cleanly, you'll need to find
> a way for the audit code to not return an error.

Fair enough.  I'll go back and look at updating subscriptions and
listeners first and undoing those actions if the bind fails.  In the
case of netlink_setsockopt() it is just one to undo, which is easy.
netlink_bind() is a bit more complex, but doable.

The whole purpose here was to add a way for each protocol to be able to
add its own permissions check and signal a way for netlink to refuse the
subscription if the userspace process doesn't have the required
permissions, so not returning an error defeats that whole purpose.

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
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

Re: [PATCH] netlink: have netlink per-protocol bind function return an error code.

[Richard Guy Briggs]

On 14/03/24, Richard Guy Briggs wrote:
> On 14/03/23, David Miller wrote:
> > From: Richard Guy Briggs <rgb@redhat.com>
> > Date: Fri, 21 Mar 2014 12:39:11 -0400
> > 
> > > @@ -1441,6 +1441,17 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
> > >  	if (!nladdr->nl_groups && (nlk->groups == NULL || !(u32)nlk->groups[0]))
> > >  		return 0;
> > >  
> > > +	if (nlk->netlink_bind && nladdr->nl_groups) {
> > > +		int i;
> > > +
> > > +		for (i = 0; i < nlk->ngroups; i++)
> > > +			if (test_bit(i, (long unsigned int *)&nladdr->nl_groups)) {
> > > +				err = nlk->netlink_bind(i);
> > > +				if (err)
> > > +					return err;
> > > +			}
> > > +	}
> > > +
> > 
> > You can't just leave a partially set of completed bindings in place.
> 
> In the general case, I agree.
> 
> > It's not valid to leave half-baked state like this.
> 
> In the one existing case (netfilter), it adds a module that is never
> unloaded.  (refcounts are bumped up and down, but I don't see an
> auto-reap based on cleared multicast group subscriptions.)  For that
> matter, netlink_realloc_groups() isn't reversed on error either.

Ok, in netlink_bind(), netlink_insert()/netlink_autobind() also need
to be undone with netlink_remove() if nlk->portid was not set.

> In the proposed case (audit) it is only a permissions check, so there is
> nothing to undo.
> 
> So, I was being lazy looking at the existing situation.
> 
> > If you return an error, all of the binding state changes must be
> > completely undone.
> 
> Is it time to add a ".unbind = netlink_unbind" to struct proto_ops
> netlink_ops?  (I am only half serious here...)

At this stage, that function would be a no-op for netfilter and audit.
Are there any out-of-tree users of this per-protocol bind function?

> > If you can't find a way to do this cleanly, you'll need to find
> > a way for the audit code to not return an error.
> 
> Fair enough.  I'll go back and look at updating subscriptions and
> listeners first and undoing those actions if the bind fails.  In the
> case of netlink_setsockopt() it is just one to undo, which is easy.
> netlink_bind() is a bit more complex, but doable.
> 
> The whole purpose here was to add a way for each protocol to be able to
> add its own permissions check and signal a way for netlink to refuse the
> subscription if the userspace process doesn't have the required
> permissions, so not returning an error defeats that whole purpose.
> 
> - RGB

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
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

[PATCH][v3] netlink: have netlink per-protocol bind function return an error code.

[Richard Guy Briggs]

Have the netlink per-protocol optional bind function return an int error code
rather than void to signal a failure.

This will enable netlink protocols to perform extra checks including
capabilities and permissions verifications when updating memberships in
multicast groups.

In netlink_bind() and netlink_setsockopt() the call to the per-protocol bind
function was moved above the multicast group update to prevent any access to
the multicast socket groups before checking with the per-protocol bind
function.  This will enable the per-protocol bind function to be used to check
permissions which could be denied before making them available, and to avoid
the messy job of undoing the addition should the per-protocol bind function
fail.

The netfilter subsystem seems to be the only one currently using the
per-protocol bind function.

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
In particular, the audit subsystem (NETLINK_AUDIT protocol) could benefit by
being able to check specific capabilities for each multicast group before
granting membership to the requesting socket.  Currently, all NETLINK_AUDIT
sockets must have the capability CAP_NET_ADMIN.  No other capabilities are
required to join a multicast group.  This capability is too broad allowing
access to this socket by many applications that must not have access to this
information.  It is proposed to add capability CAP_AUDIT_READ to allow this
access while dropping the exessively broad capability CAP_NET_ADMIN.

There has also been some interest expressed by IETF ForCES folk.
---
 include/linux/netlink.h   |    3 ++-
 net/netfilter/nfnetlink.c |    3 ++-
 net/netlink/af_netlink.c  |   39 ++++++++++++++++++++++++++-------------
 net/netlink/af_netlink.h  |    6 ++++--
 4 files changed, 34 insertions(+), 17 deletions(-)

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index 7a6c396..0ba2edd 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -45,7 +45,8 @@ struct netlink_kernel_cfg {
 	unsigned int	flags;
 	void		(*input)(struct sk_buff *skb);
 	struct mutex	*cb_mutex;
-	void		(*bind)(int group);
+	int		(*bind)(int group);
+	void		(*unbind)(int group);
 	bool		(*compare)(struct net *net, struct sock *sk);
 };
 
diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
index 1da5ef1..14c8ac8 100644
--- a/net/netfilter/nfnetlink.c
+++ b/net/netfilter/nfnetlink.c
@@ -392,7 +392,7 @@ static void nfnetlink_rcv(struct sk_buff *skb)
 }
 
 #ifdef CONFIG_MODULES
-static void nfnetlink_bind(int group)
+static int nfnetlink_bind(int group)
 {
 	const struct nfnetlink_subsystem *ss;
 	int type = nfnl_group2type[group];
@@ -402,6 +402,7 @@ static void nfnetlink_bind(int group)
 	rcu_read_unlock();
 	if (!ss)
 		request_module("nfnetlink-subsys-%d", type);
+	return 0;
 }
 #endif
 
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index bca50b9..301dfe9 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1198,7 +1198,8 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
 	struct module *module = NULL;
 	struct mutex *cb_mutex;
 	struct netlink_sock *nlk;
-	void (*bind)(int group);
+	int (*bind)(int group);
+	void (*unbind)(int group);
 	int err = 0;
 
 	sock->state = SS_UNCONNECTED;
@@ -1224,6 +1225,7 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
 		err = -EPROTONOSUPPORT;
 	cb_mutex = nl_table[protocol].cb_mutex;
 	bind = nl_table[protocol].bind;
+	unbind = nl_table[protocol].unbind;
 	netlink_unlock_table();
 
 	if (err < 0)
@@ -1240,6 +1242,7 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
 	nlk = nlk_sk(sock->sk);
 	nlk->module = module;
 	nlk->netlink_bind = bind;
+	nlk->netlink_unbind = unbind;
 out:
 	return err;
 
@@ -1293,6 +1296,7 @@ static int netlink_release(struct socket *sock)
 			kfree_rcu(old, rcu);
 			nl_table[sk->sk_protocol].module = NULL;
 			nl_table[sk->sk_protocol].bind = NULL;
+			nl_table[sk->sk_protocol].unbind = NULL;
 			nl_table[sk->sk_protocol].flags = 0;
 			nl_table[sk->sk_protocol].registered = 0;
 		}
@@ -1441,6 +1445,22 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 	if (!nladdr->nl_groups && (nlk->groups == NULL || !(u32)nlk->groups[0]))
 		return 0;
 
+	if (nlk->netlink_bind && nladdr->nl_groups) {
+		int i;
+
+		for (i = 0; i < nlk->ngroups; i++)
+			if (test_bit(i, (long unsigned int *)&nladdr->nl_groups)) {
+				err = nlk->netlink_bind(i);
+				if (err)
+					if (!nlk->portid)
+						netlink_remove(sk);
+					for (int undo = 0; undo < i; undo++)
+						if (nlk->netlink_unbind)
+							nlk->netlink_unbind(undo);
+					return err;
+			}
+	}
+
 	netlink_table_grab();
 	netlink_update_subscriptions(sk, nlk->subscriptions +
 					 hweight32(nladdr->nl_groups) -
@@ -1449,15 +1469,6 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 	netlink_update_listeners(sk);
 	netlink_table_ungrab();
 
-	if (nlk->netlink_bind && nlk->groups[0]) {
-		int i;
-
-		for (i=0; i<nlk->ngroups; i++) {
-			if (test_bit(i, nlk->groups))
-				nlk->netlink_bind(i);
-		}
-	}
-
 	return 0;
 }
 
@@ -2095,14 +2106,16 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
 			return err;
 		if (!val || val - 1 >= nlk->ngroups)
 			return -EINVAL;
+		if (nlk->netlink_bind) {
+			err = nlk->netlink_bind(val);
+			if (err)
+				return err;
+		}
 		netlink_table_grab();
 		netlink_update_socket_mc(nlk, val,
 					 optname == NETLINK_ADD_MEMBERSHIP);
 		netlink_table_ungrab();
 
-		if (nlk->netlink_bind)
-			nlk->netlink_bind(val);
-
 		err = 0;
 		break;
 	}
diff --git a/net/netlink/af_netlink.h b/net/netlink/af_netlink.h
index acbd774..2aeae8a 100644
--- a/net/netlink/af_netlink.h
+++ b/net/netlink/af_netlink.h
@@ -37,7 +37,8 @@ struct netlink_sock {
 	struct mutex		*cb_mutex;
 	struct mutex		cb_def_mutex;
 	void			(*netlink_rcv)(struct sk_buff *skb);
-	void			(*netlink_bind)(int group);
+	int			(*netlink_bind)(int group);
+	void			(*netlink_unbind)(int group);
 	struct module		*module;
 #ifdef CONFIG_NETLINK_MMAP
 	struct mutex		pg_vec_lock;
@@ -73,7 +74,8 @@ struct netlink_table {
 	unsigned int		groups;
 	struct mutex		*cb_mutex;
 	struct module		*module;
-	void			(*bind)(int group);
+	int			(*bind)(int group);
+	void			(*unbind)(int group);
 	bool			(*compare)(struct net *net, struct sock *sock);
 	int			registered;
 };
-- 
1.7.1

[PATCH][v4] netlink: have netlink per-protocol bind function return an error code.

[Richard Guy Briggs]

Have the netlink per-protocol optional bind function return an int error code
rather than void to signal a failure.

This will enable netlink protocols to perform extra checks including
capabilities and permissions verifications when updating memberships in
multicast groups.

In netlink_bind() and netlink_setsockopt() the call to the per-protocol bind
function was moved above the multicast group update to prevent any access to
the multicast socket groups before checking with the per-protocol bind
function.  This will enable the per-protocol bind function to be used to check
permissions which could be denied before making them available, and to avoid
the messy job of undoing the addition should the per-protocol bind function
fail.

The netfilter subsystem seems to be the only one currently using the
per-protocol bind function.

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
In particular, the audit subsystem (NETLINK_AUDIT protocol) could benefit by
being able to check specific capabilities for each multicast group before
granting membership to the requesting socket.  Currently, all NETLINK_AUDIT
sockets must have the capability CAP_NET_ADMIN.  No other capabilities are
required to join a multicast group.  This capability is too broad allowing
access to this socket by many applications that must not have access to this
information.  It is proposed to add capability CAP_AUDIT_READ to allow this
access while dropping the exessively broad capability CAP_NET_ADMIN.

There has also been some interest expressed by IETF ForCES folk.

v4: fixed a logic error (missing braces) and a C99 error.

v3: added netlink_unbind() option and unwound netlink_insert().
---
 include/linux/netlink.h   |    3 ++-
 net/netfilter/nfnetlink.c |    3 ++-
 net/netlink/af_netlink.c  |   39 ++++++++++++++++++++++++++-------------
 net/netlink/af_netlink.h  |    6 ++++--
 4 files changed, 34 insertions(+), 17 deletions(-)

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index 7a6c396..0ba2edd 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -45,7 +45,8 @@ struct netlink_kernel_cfg {
 	unsigned int	flags;
 	void		(*input)(struct sk_buff *skb);
 	struct mutex	*cb_mutex;
-	void		(*bind)(int group);
+	int		(*bind)(int group);
+	void		(*unbind)(int group);
 	bool		(*compare)(struct net *net, struct sock *sk);
 };
 
diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
index 1da5ef1..14c8ac8 100644
--- a/net/netfilter/nfnetlink.c
+++ b/net/netfilter/nfnetlink.c
@@ -392,7 +392,7 @@ static void nfnetlink_rcv(struct sk_buff *skb)
 }
 
 #ifdef CONFIG_MODULES
-static void nfnetlink_bind(int group)
+static int nfnetlink_bind(int group)
 {
 	const struct nfnetlink_subsystem *ss;
 	int type = nfnl_group2type[group];
@@ -402,6 +402,7 @@ static void nfnetlink_bind(int group)
 	rcu_read_unlock();
 	if (!ss)
 		request_module("nfnetlink-subsys-%d", type);
+	return 0;
 }
 #endif
 
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index bca50b9..301dfe9 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1198,7 +1198,8 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
 	struct module *module = NULL;
 	struct mutex *cb_mutex;
 	struct netlink_sock *nlk;
-	void (*bind)(int group);
+	int (*bind)(int group);
+	void (*unbind)(int group);
 	int err = 0;
 
 	sock->state = SS_UNCONNECTED;
@@ -1224,6 +1225,7 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
 		err = -EPROTONOSUPPORT;
 	cb_mutex = nl_table[protocol].cb_mutex;
 	bind = nl_table[protocol].bind;
+	unbind = nl_table[protocol].unbind;
 	netlink_unlock_table();
 
 	if (err < 0)
@@ -1240,6 +1242,7 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
 	nlk = nlk_sk(sock->sk);
 	nlk->module = module;
 	nlk->netlink_bind = bind;
+	nlk->netlink_unbind = unbind;
 out:
 	return err;
 
@@ -1293,6 +1296,7 @@ static int netlink_release(struct socket *sock)
 			kfree_rcu(old, rcu);
 			nl_table[sk->sk_protocol].module = NULL;
 			nl_table[sk->sk_protocol].bind = NULL;
+			nl_table[sk->sk_protocol].unbind = NULL;
 			nl_table[sk->sk_protocol].flags = 0;
 			nl_table[sk->sk_protocol].registered = 0;
 		}
@@ -1441,6 +1445,22 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 	if (!nladdr->nl_groups && (nlk->groups == NULL || !(u32)nlk->groups[0]))
 		return 0;
 
+	if (nlk->netlink_bind && nladdr->nl_groups) {
+		int i;
+
+		for (i = 0; i < nlk->ngroups; i++)
+			if (test_bit(i, (long unsigned int *)&nladdr->nl_groups)) {
+				err = nlk->netlink_bind(i);
+				if (err)
+					if (!nlk->portid)
+						netlink_remove(sk);
+					for (int undo = 0; undo < i; undo++)
+						if (nlk->netlink_unbind)
+							nlk->netlink_unbind(undo);
+					return err;
+			}
+	}
+
 	netlink_table_grab();
 	netlink_update_subscriptions(sk, nlk->subscriptions +
 					 hweight32(nladdr->nl_groups) -
@@ -1449,15 +1469,6 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 	netlink_update_listeners(sk);
 	netlink_table_ungrab();
 
-	if (nlk->netlink_bind && nlk->groups[0]) {
-		int i;
-
-		for (i=0; i<nlk->ngroups; i++) {
-			if (test_bit(i, nlk->groups))
-				nlk->netlink_bind(i);
-		}
-	}
-
 	return 0;
 }
 
@@ -2095,14 +2106,16 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
 			return err;
 		if (!val || val - 1 >= nlk->ngroups)
 			return -EINVAL;
+		if (nlk->netlink_bind) {
+			err = nlk->netlink_bind(val);
+			if (err)
+				return err;
+		}
 		netlink_table_grab();
 		netlink_update_socket_mc(nlk, val,
 					 optname == NETLINK_ADD_MEMBERSHIP);
 		netlink_table_ungrab();
 
-		if (nlk->netlink_bind)
-			nlk->netlink_bind(val);
-
 		err = 0;
 		break;
 	}
diff --git a/net/netlink/af_netlink.h b/net/netlink/af_netlink.h
index acbd774..2aeae8a 100644
--- a/net/netlink/af_netlink.h
+++ b/net/netlink/af_netlink.h
@@ -37,7 +37,8 @@ struct netlink_sock {
 	struct mutex		*cb_mutex;
 	struct mutex		cb_def_mutex;
 	void			(*netlink_rcv)(struct sk_buff *skb);
-	void			(*netlink_bind)(int group);
+	int			(*netlink_bind)(int group);
+	void			(*netlink_unbind)(int group);
 	struct module		*module;
 #ifdef CONFIG_NETLINK_MMAP
 	struct mutex		pg_vec_lock;
@@ -73,7 +74,8 @@ struct netlink_table {
 	unsigned int		groups;
 	struct mutex		*cb_mutex;
 	struct module		*module;
-	void			(*bind)(int group);
+	int			(*bind)(int group);
+	void			(*unbind)(int group);
 	bool			(*compare)(struct net *net, struct sock *sock);
 	int			registered;
 };
-- 
1.7.1

[PATCH][v5] netlink: have netlink per-protocol bind function return an error code.

[Richard Guy Briggs]

Have the netlink per-protocol optional bind function return an int error code
rather than void to signal a failure.

This will enable netlink protocols to perform extra checks including
capabilities and permissions verifications when updating memberships in
multicast groups.

In netlink_bind() and netlink_setsockopt() the call to the per-protocol bind
function was moved above the multicast group update to prevent any access to
the multicast socket groups before checking with the per-protocol bind
function.  This will enable the per-protocol bind function to be used to check
permissions which could be denied before making them available, and to avoid
the messy job of undoing the addition should the per-protocol bind function
fail.

The netfilter subsystem seems to be the only one currently using the
per-protocol bind function.

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
In particular, the audit subsystem (NETLINK_AUDIT protocol) could benefit by
being able to check specific capabilities for each multicast group before
granting membership to the requesting socket.  Currently, all NETLINK_AUDIT
sockets must have the capability CAP_NET_ADMIN.  No other capabilities are
required to join a multicast group.  This capability is too broad allowing
access to this socket by many applications that must not have access to this
information.  It is proposed to add capability CAP_AUDIT_READ to allow this
access while dropping the exessively broad capability CAP_NET_ADMIN.

There has also been some interest expressed by IETF ForCES folk.

v5: *really* fixed a logic error (missing braces) and a C99 error.

v3: added netlink_unbind() option and unwound netlink_insert().
---
 include/linux/netlink.h   |    3 ++-
 net/netfilter/nfnetlink.c |    3 ++-
 net/netlink/af_netlink.c  |   39 ++++++++++++++++++++++++++-------------
 net/netlink/af_netlink.h  |    6 ++++--
 4 files changed, 34 insertions(+), 17 deletions(-)

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index 7a6c396..0ba2edd 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -45,7 +45,8 @@ struct netlink_kernel_cfg {
 	unsigned int	flags;
 	void		(*input)(struct sk_buff *skb);
 	struct mutex	*cb_mutex;
-	void		(*bind)(int group);
+	int		(*bind)(int group);
+	void		(*unbind)(int group);
 	bool		(*compare)(struct net *net, struct sock *sk);
 };
 
diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
index 1da5ef1..14c8ac8 100644
--- a/net/netfilter/nfnetlink.c
+++ b/net/netfilter/nfnetlink.c
@@ -392,7 +392,7 @@ static void nfnetlink_rcv(struct sk_buff *skb)
 }
 
 #ifdef CONFIG_MODULES
-static void nfnetlink_bind(int group)
+static int nfnetlink_bind(int group)
 {
 	const struct nfnetlink_subsystem *ss;
 	int type = nfnl_group2type[group];
@@ -402,6 +402,7 @@ static void nfnetlink_bind(int group)
 	rcu_read_unlock();
 	if (!ss)
 		request_module("nfnetlink-subsys-%d", type);
+	return 0;
 }
 #endif
 
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index bca50b9..301dfe9 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1198,7 +1198,8 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
 	struct module *module = NULL;
 	struct mutex *cb_mutex;
 	struct netlink_sock *nlk;
-	void (*bind)(int group);
+	int (*bind)(int group);
+	void (*unbind)(int group);
 	int err = 0;
 
 	sock->state = SS_UNCONNECTED;
@@ -1224,6 +1225,7 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
 		err = -EPROTONOSUPPORT;
 	cb_mutex = nl_table[protocol].cb_mutex;
 	bind = nl_table[protocol].bind;
+	unbind = nl_table[protocol].unbind;
 	netlink_unlock_table();
 
 	if (err < 0)
@@ -1240,6 +1242,7 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
 	nlk = nlk_sk(sock->sk);
 	nlk->module = module;
 	nlk->netlink_bind = bind;
+	nlk->netlink_unbind = unbind;
 out:
 	return err;
 
@@ -1293,6 +1296,7 @@ static int netlink_release(struct socket *sock)
 			kfree_rcu(old, rcu);
 			nl_table[sk->sk_protocol].module = NULL;
 			nl_table[sk->sk_protocol].bind = NULL;
+			nl_table[sk->sk_protocol].unbind = NULL;
 			nl_table[sk->sk_protocol].flags = 0;
 			nl_table[sk->sk_protocol].registered = 0;
 		}
@@ -1441,6 +1445,22 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 	if (!nladdr->nl_groups && (nlk->groups == NULL || !(u32)nlk->groups[0]))
 		return 0;
 
+	if (nlk->netlink_bind && nladdr->nl_groups) {
+		int i;
+
+		for (i = 0; i < nlk->ngroups; i++)
+			if (test_bit(i, (long unsigned int *)&nladdr->nl_groups)) {
+				err = nlk->netlink_bind(i);
+				if (err)
+					if (!nlk->portid)
+						netlink_remove(sk);
+					for (int undo = 0; undo < i; undo++)
+						if (nlk->netlink_unbind)
+							nlk->netlink_unbind(undo);
+					return err;
+			}
+	}
+
 	netlink_table_grab();
 	netlink_update_subscriptions(sk, nlk->subscriptions +
 					 hweight32(nladdr->nl_groups) -
@@ -1449,15 +1469,6 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 	netlink_update_listeners(sk);
 	netlink_table_ungrab();
 
-	if (nlk->netlink_bind && nlk->groups[0]) {
-		int i;
-
-		for (i=0; i<nlk->ngroups; i++) {
-			if (test_bit(i, nlk->groups))
-				nlk->netlink_bind(i);
-		}
-	}
-
 	return 0;
 }
 
@@ -2095,14 +2106,16 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
 			return err;
 		if (!val || val - 1 >= nlk->ngroups)
 			return -EINVAL;
+		if (nlk->netlink_bind) {
+			err = nlk->netlink_bind(val);
+			if (err)
+				return err;
+		}
 		netlink_table_grab();
 		netlink_update_socket_mc(nlk, val,
 					 optname == NETLINK_ADD_MEMBERSHIP);
 		netlink_table_ungrab();
 
-		if (nlk->netlink_bind)
-			nlk->netlink_bind(val);
-
 		err = 0;
 		break;
 	}
diff --git a/net/netlink/af_netlink.h b/net/netlink/af_netlink.h
index acbd774..2aeae8a 100644
--- a/net/netlink/af_netlink.h
+++ b/net/netlink/af_netlink.h
@@ -37,7 +37,8 @@ struct netlink_sock {
 	struct mutex		*cb_mutex;
 	struct mutex		cb_def_mutex;
 	void			(*netlink_rcv)(struct sk_buff *skb);
-	void			(*netlink_bind)(int group);
+	int			(*netlink_bind)(int group);
+	void			(*netlink_unbind)(int group);
 	struct module		*module;
 #ifdef CONFIG_NETLINK_MMAP
 	struct mutex		pg_vec_lock;
@@ -73,7 +74,8 @@ struct netlink_table {
 	unsigned int		groups;
 	struct mutex		*cb_mutex;
 	struct module		*module;
-	void			(*bind)(int group);
+	int			(*bind)(int group);
+	void			(*unbind)(int group);
 	bool			(*compare)(struct net *net, struct sock *sock);
 	int			registered;
 };
-- 
1.7.1

Re: [PATCH][v5] netlink: have netlink per-protocol bind function return an error code.

[David Miller]

From: Richard Guy Briggs <rgb@redhat.com>
Date: Mon, 24 Mar 2014 16:59:23 -0400

> +				if (err)
> +					if (!nlk->portid)
> +						netlink_remove(sk);
> +					for (int undo = 0; undo < i; undo++)
> +						if (nlk->netlink_unbind)
> +							nlk->netlink_unbind(undo);
> +					return err;

Take a good long stare at that code block for a while.

Looks like you forgot the braces to delineate the code block.

This also means you really haven't tested this patch :-)

Please also DO NOT declare local variables in a for() statement,
this is not c++.

v6 superceded it [was: Re: [PATCH][v5] netlink: have netlink per-protocol bind function return an error code.]

[Richard Guy Briggs]

On 14/03/26, David Miller wrote:
> From: Richard Guy Briggs <rgb@redhat.com>
> Date: Mon, 24 Mar 2014 16:59:23 -0400
> 
> > +				if (err)
> > +					if (!nlk->portid)
> > +						netlink_remove(sk);
> > +					for (int undo = 0; undo < i; undo++)
> > +						if (nlk->netlink_unbind)
> > +							nlk->netlink_unbind(undo);
> > +					return err;
> 
> Take a good long stare at that code block for a while.
> 
> Looks like you forgot the braces to delineate the code block.

Did you notice all the attempts to send a corrected patch?  v6 finally
got it right (sent Monday).  Sorry for all the noise.  Serve me right
for not sitting on my hands before sending the email for a bit while I
checked it.

> This also means you really haven't tested this patch :-)

It was tested, but the patched code wasn't actually checked in.  It was
still in my tree.  I ran "git commit --amend" more than once, forgetting
the "-a", and didn't carefully enough inspect the resulting patch.

> Please also DO NOT declare local variables in a for() statement,
> this is not c++.

It is above the for now (also in v6).

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
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

[PATCH][v6] netlink: have netlink per-protocol bind function return an error code.

[Richard Guy Briggs]

Have the netlink per-protocol optional bind function return an int error code
rather than void to signal a failure.

This will enable netlink protocols to perform extra checks including
capabilities and permissions verifications when updating memberships in
multicast groups.

In netlink_bind() and netlink_setsockopt() the call to the per-protocol bind
function was moved above the multicast group update to prevent any access to
the multicast socket groups before checking with the per-protocol bind
function.  This will enable the per-protocol bind function to be used to check
permissions which could be denied before making them available, and to avoid
the messy job of undoing the addition should the per-protocol bind function
fail.

The netfilter subsystem seems to be the only one currently using the
per-protocol bind function.

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
In particular, the audit subsystem (NETLINK_AUDIT protocol) could benefit by
being able to check specific capabilities for each multicast group before
granting membership to the requesting socket.  Currently, all NETLINK_AUDIT
sockets must have the capability CAP_NET_ADMIN.  No other capabilities are
required to join a multicast group.  This capability is too broad allowing
access to this socket by many applications that must not have access to this
information.  It is proposed to add capability CAP_AUDIT_READ to allow this
access while dropping the exessively broad capability CAP_NET_ADMIN.

There has also been some interest expressed by IETF ForCES folk.

v6: *really* fixed a logic error (missing braces) and a C99 error.

v3: added netlink_unbind() option and unwound netlink_insert().
---
 include/linux/netlink.h   |    3 ++-
 net/netfilter/nfnetlink.c |    3 ++-
 net/netlink/af_netlink.c  |   41 ++++++++++++++++++++++++++++-------------
 net/netlink/af_netlink.h  |    6 ++++--
 4 files changed, 36 insertions(+), 17 deletions(-)

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index 7a6c396..0ba2edd 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -45,7 +45,8 @@ struct netlink_kernel_cfg {
 	unsigned int	flags;
 	void		(*input)(struct sk_buff *skb);
 	struct mutex	*cb_mutex;
-	void		(*bind)(int group);
+	int		(*bind)(int group);
+	void		(*unbind)(int group);
 	bool		(*compare)(struct net *net, struct sock *sk);
 };
 
diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
index 1da5ef1..14c8ac8 100644
--- a/net/netfilter/nfnetlink.c
+++ b/net/netfilter/nfnetlink.c
@@ -392,7 +392,7 @@ static void nfnetlink_rcv(struct sk_buff *skb)
 }
 
 #ifdef CONFIG_MODULES
-static void nfnetlink_bind(int group)
+static int nfnetlink_bind(int group)
 {
 	const struct nfnetlink_subsystem *ss;
 	int type = nfnl_group2type[group];
@@ -402,6 +402,7 @@ static void nfnetlink_bind(int group)
 	rcu_read_unlock();
 	if (!ss)
 		request_module("nfnetlink-subsys-%d", type);
+	return 0;
 }
 #endif
 
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index bca50b9..45dccff 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1198,7 +1198,8 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
 	struct module *module = NULL;
 	struct mutex *cb_mutex;
 	struct netlink_sock *nlk;
-	void (*bind)(int group);
+	int (*bind)(int group);
+	void (*unbind)(int group);
 	int err = 0;
 
 	sock->state = SS_UNCONNECTED;
@@ -1224,6 +1225,7 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
 		err = -EPROTONOSUPPORT;
 	cb_mutex = nl_table[protocol].cb_mutex;
 	bind = nl_table[protocol].bind;
+	unbind = nl_table[protocol].unbind;
 	netlink_unlock_table();
 
 	if (err < 0)
@@ -1240,6 +1242,7 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
 	nlk = nlk_sk(sock->sk);
 	nlk->module = module;
 	nlk->netlink_bind = bind;
+	nlk->netlink_unbind = unbind;
 out:
 	return err;
 
@@ -1293,6 +1296,7 @@ static int netlink_release(struct socket *sock)
 			kfree_rcu(old, rcu);
 			nl_table[sk->sk_protocol].module = NULL;
 			nl_table[sk->sk_protocol].bind = NULL;
+			nl_table[sk->sk_protocol].unbind = NULL;
 			nl_table[sk->sk_protocol].flags = 0;
 			nl_table[sk->sk_protocol].registered = 0;
 		}
@@ -1441,6 +1445,24 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 	if (!nladdr->nl_groups && (nlk->groups == NULL || !(u32)nlk->groups[0]))
 		return 0;
 
+	if (nlk->netlink_bind && nladdr->nl_groups) {
+		int i;
+
+		for (i = 0; i < nlk->ngroups; i++)
+			if (test_bit(i, (long unsigned int *)&nladdr->nl_groups)) {
+				err = nlk->netlink_bind(i);
+				if (err) {
+					int undo;
+					if (!nlk->portid)
+						netlink_remove(sk);
+					for (undo = 0; undo < i; undo++)
+						if (nlk->netlink_unbind)
+							nlk->netlink_unbind(undo);
+					return err;
+				}
+			}
+	}
+
 	netlink_table_grab();
 	netlink_update_subscriptions(sk, nlk->subscriptions +
 					 hweight32(nladdr->nl_groups) -
@@ -1449,15 +1471,6 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 	netlink_update_listeners(sk);
 	netlink_table_ungrab();
 
-	if (nlk->netlink_bind && nlk->groups[0]) {
-		int i;
-
-		for (i=0; i<nlk->ngroups; i++) {
-			if (test_bit(i, nlk->groups))
-				nlk->netlink_bind(i);
-		}
-	}
-
 	return 0;
 }
 
@@ -2095,14 +2108,16 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
 			return err;
 		if (!val || val - 1 >= nlk->ngroups)
 			return -EINVAL;
+		if (nlk->netlink_bind) {
+			err = nlk->netlink_bind(val);
+			if (err)
+				return err;
+		}
 		netlink_table_grab();
 		netlink_update_socket_mc(nlk, val,
 					 optname == NETLINK_ADD_MEMBERSHIP);
 		netlink_table_ungrab();
 
-		if (nlk->netlink_bind)
-			nlk->netlink_bind(val);
-
 		err = 0;
 		break;
 	}
diff --git a/net/netlink/af_netlink.h b/net/netlink/af_netlink.h
index acbd774..2aeae8a 100644
--- a/net/netlink/af_netlink.h
+++ b/net/netlink/af_netlink.h
@@ -37,7 +37,8 @@ struct netlink_sock {
 	struct mutex		*cb_mutex;
 	struct mutex		cb_def_mutex;
 	void			(*netlink_rcv)(struct sk_buff *skb);
-	void			(*netlink_bind)(int group);
+	int			(*netlink_bind)(int group);
+	void			(*netlink_unbind)(int group);
 	struct module		*module;
 #ifdef CONFIG_NETLINK_MMAP
 	struct mutex		pg_vec_lock;
@@ -73,7 +74,8 @@ struct netlink_table {
 	unsigned int		groups;
 	struct mutex		*cb_mutex;
 	struct module		*module;
-	void			(*bind)(int group);
+	int			(*bind)(int group);
+	void			(*unbind)(int group);
 	bool			(*compare)(struct net *net, struct sock *sock);
 	int			registered;
 };
-- 
1.7.1

Re: [PATCH][v6] netlink: have netlink per-protocol bind function return an error code.

[David Miller]

From: Richard Guy Briggs <rgb@redhat.com>
Date: Tue, 25 Mar 2014 08:50:56 -0400

> @@ -1441,6 +1445,24 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
>  	if (!nladdr->nl_groups && (nlk->groups == NULL || !(u32)nlk->groups[0]))
>  		return 0;
>  
> +	if (nlk->netlink_bind && nladdr->nl_groups) {
> +		int i;
> +
> +		for (i = 0; i < nlk->ngroups; i++)
 ...
> +				if (err) {
> +					int undo;
> +					if (!nlk->portid)
> +						netlink_remove(sk);

Please put an empty line between local variable declarations and code, just as it
is a few lines up.

Re: [PATCH][v6] netlink: have netlink per-protocol bind function return an error code.

[Patrick McHardy]

On Tue, Mar 25, 2014 at 08:50:56AM -0400, Richard Guy Briggs wrote:
> +	if (nlk->netlink_bind && nladdr->nl_groups) {
> +		int i;
> +
> +		for (i = 0; i < nlk->ngroups; i++)
> +			if (test_bit(i, (long unsigned int *)&nladdr->nl_groups)) {
> +				err = nlk->netlink_bind(i);
> +				if (err) {
> +					int undo;
> +					if (!nlk->portid)
> +						netlink_remove(sk);
> +					for (undo = 0; undo < i; undo++)
> +						if (nlk->netlink_unbind)
> +							nlk->netlink_unbind(undo);

Do we really need 7 levels of indentation? You could save at least
one by using

if (!test_bit(...)
	continue;
...

Or maybe simply move this to a helper function.



> +					return err;
> +				}
> +			}
> +	}
> +
>  	netlink_table_grab();
>  	netlink_update_subscriptions(sk, nlk->subscriptions +
>  					 hweight32(nladdr->nl_groups) -

unbind [was: Re: [PATCH] netlink: have netlink per-protocol bind function return] an error code.

[Richard Guy Briggs]

On 14/03/24, Richard Guy Briggs wrote:
> On 14/03/24, Richard Guy Briggs wrote:
> > On 14/03/23, David Miller wrote:
> > > From: Richard Guy Briggs <rgb@redhat.com>
> > > Date: Fri, 21 Mar 2014 12:39:11 -0400

Ok, sorry for all the noise.  I had a problem between chair and keyboard
which can explain the number of useless revs of this patch...  v6 is
current with potentially v7 with the inline hunk below...

> > > > @@ -1441,6 +1441,17 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
> > > >  	if (!nladdr->nl_groups && (nlk->groups == NULL || !(u32)nlk->groups[0]))
> > > >  		return 0;
> > > >  
> > > > +	if (nlk->netlink_bind && nladdr->nl_groups) {
> > > > +		int i;
> > > > +
> > > > +		for (i = 0; i < nlk->ngroups; i++)
> > > > +			if (test_bit(i, (long unsigned int *)&nladdr->nl_groups)) {
> > > > +				err = nlk->netlink_bind(i);
> > > > +				if (err)
> > > > +					return err;
> > > > +			}
> > > > +	}
> > > > +
> > > 
> > > You can't just leave a partially set of completed bindings in place.
> > 
> > In the general case, I agree.
> > 
> > > It's not valid to leave half-baked state like this.
> > 
> > In the one existing case (netfilter), it adds a module that is never
> > unloaded.  (refcounts are bumped up and down, but I don't see an
> > auto-reap based on cleared multicast group subscriptions.)  For that
> > matter, netlink_realloc_groups() isn't reversed on error either.
> 
> Ok, in netlink_bind(), netlink_insert()/netlink_autobind() also need
> to be undone with netlink_remove() if nlk->portid was not set.

This is also true with netlink_setsockopt() in the
NETLINK_{ADD,DROP}_MEMBERSHIP case, where on ADD we should call
netlink_bind() but on DROP we should *not* call it and instead call
netlink_unbind() afterwards, perhaps such as:

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 45dccff..3354d54 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2108,7 +2108,7 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
 			return err;
 		if (!val || val - 1 >= nlk->ngroups)
 			return -EINVAL;
-		if (nlk->netlink_bind) {
+		if (optname == NETLINK_ADD_MEMBERSHIP && nlk->netlink_bind) {
 			err = nlk->netlink_bind(val);
 			if (err)
 				return err;
@@ -2117,6 +2117,8 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
 		netlink_update_socket_mc(nlk, val,
 					 optname == NETLINK_ADD_MEMBERSHIP);
 		netlink_table_ungrab();
+		if (optname == NETLINK_DROP_MEMBERSHIP && nlk->netlink_unbind)
+			nlk->netlink_unbind(val);
 
 		err = 0;
 		break;

> > In the proposed case (audit) it is only a permissions check, so there is
> > nothing to undo.
> > 
> > So, I was being lazy looking at the existing situation.
> > 
> > > If you return an error, all of the binding state changes must be
> > > completely undone.
> > 
> > Is it time to add a ".unbind = netlink_unbind" to struct proto_ops
> > netlink_ops?  (I am only half serious here...)
> 
> At this stage, that function would be a no-op for netfilter and audit.
> Are there any out-of-tree users of this per-protocol bind function?
> 
> > > If you can't find a way to do this cleanly, you'll need to find
> > > a way for the audit code to not return an error.
> > 
> > Fair enough.  I'll go back and look at updating subscriptions and
> > listeners first and undoing those actions if the bind fails.  In the
> > case of netlink_setsockopt() it is just one to undo, which is easy.
> > netlink_bind() is a bit more complex, but doable.

So netlink_setsockopt() was right, but ADD/DROP need slightly different
treatment.

netlink_bind() just needs to undo netlink_bind() on error.

> > The whole purpose here was to add a way for each protocol to be able to
> > add its own permissions check and signal a way for netlink to refuse the
> > subscription if the userspace process doesn't have the required
> > permissions, so not returning an error defeats that whole purpose.

We could do all we wanted in audit, but that would still not signal
netlink to block that subscription.

> > - RGB
> 
> - RGB

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
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

[PATCH 0/3] netlink: per-protocol bind fixup/enhancement set

[Richard Guy Briggs]

This set provides a way for per-protocol bind functions to signal an error and
to be able to clean up after themselves.

The first patch has already been accepted, but is included just in case to
avoid a merge error.

The second patch adds the per-protocol bind return code to signal to the
netlink code that no further processing should be done and to undo the work
already done.  This rev has fixed DaveM's last issue and flattened the
intentation as requested by Patrick McHardy by two by reworking the logic.

The third provides a way per protocol to undo actions on DROP.

Thanks for the feedback.

Richard Guy Briggs (3):
  netlink: simplify nfnetlink_bind
  netlink: have netlink per-protocol bind function return an error code.
  netlink: implement unbind to netlink_setsockopt NETLINK_DROP_MEMBERSHIP

 include/linux/netlink.h   |    3 ++-
 net/netfilter/nfnetlink.c |   10 ++++------
 net/netlink/af_netlink.c  |   44 +++++++++++++++++++++++++++++++-------------
 net/netlink/af_netlink.h  |    6 ++++--
 4 files changed, 41 insertions(+), 22 deletions(-)

[PATCH 1/3] netlink: simplify nfnetlink_bind

[Richard Guy Briggs]

Remove duplicity and simplify code flow by moving the rcu_read_unlock() above
the condition and let the flow control exit naturally at the end of the
function.

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 net/netfilter/nfnetlink.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
index 046aa13..1da5ef1 100644
--- a/net/netfilter/nfnetlink.c
+++ b/net/netfilter/nfnetlink.c
@@ -399,12 +399,9 @@ static void nfnetlink_bind(int group)
 
 	rcu_read_lock();
 	ss = nfnetlink_get_subsys(type);
-	if (!ss) {
-		rcu_read_unlock();
-		request_module("nfnetlink-subsys-%d", type);
-		return;
-	}
 	rcu_read_unlock();
+	if (!ss)
+		request_module("nfnetlink-subsys-%d", type);
 }
 #endif
 
-- 
1.7.1

[PATCH 2/3][v7] netlink: have netlink per-protocol bind function return an error code.

[Richard Guy Briggs]

Have the netlink per-protocol optional bind function return an int error code
rather than void to signal a failure.

This will enable netlink protocols to perform extra checks including
capabilities and permissions verifications when updating memberships in
multicast groups.

In netlink_bind() and netlink_setsockopt() the call to the per-protocol bind
function was moved above the multicast group update to prevent any access to
the multicast socket groups before checking with the per-protocol bind
function.  This will enable the per-protocol bind function to be used to check
permissions which could be denied before making them available, and to avoid
the messy job of undoing the addition should the per-protocol bind function
fail.

The netfilter subsystem seems to be the only one currently using the
per-protocol bind function.

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
In particular, the audit subsystem (NETLINK_AUDIT protocol) could benefit by
being able to check specific capabilities for each multicast group before
granting membership to the requesting socket.  Currently, all NETLINK_AUDIT
sockets must have the capability CAP_NET_ADMIN.  No other capabilities are
required to join a multicast group.  This capability is too broad allowing
access to this socket by many applications that must not have access to this
information.  It is proposed to add capability CAP_AUDIT_READ to allow this
access while dropping the exessively broad capability CAP_NET_ADMIN.

There has also been some interest expressed by IETF ForCES folk.

v7: fix formatting and rework logic to flatten indentation.

v6: *really* fixed a logic error (missing braces) and a C99 error.

v3: added netlink_unbind() option and unwound netlink_insert().
---
 include/linux/netlink.h   |    3 ++-
 net/netfilter/nfnetlink.c |    3 ++-
 net/netlink/af_netlink.c  |   42 +++++++++++++++++++++++++++++-------------
 net/netlink/af_netlink.h  |    6 ++++--
 4 files changed, 37 insertions(+), 17 deletions(-)

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index 7a6c396..0ba2edd 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -45,7 +45,8 @@ struct netlink_kernel_cfg {
 	unsigned int	flags;
 	void		(*input)(struct sk_buff *skb);
 	struct mutex	*cb_mutex;
-	void		(*bind)(int group);
+	int		(*bind)(int group);
+	void		(*unbind)(int group);
 	bool		(*compare)(struct net *net, struct sock *sk);
 };
 
diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
index 1da5ef1..14c8ac8 100644
--- a/net/netfilter/nfnetlink.c
+++ b/net/netfilter/nfnetlink.c
@@ -392,7 +392,7 @@ static void nfnetlink_rcv(struct sk_buff *skb)
 }
 
 #ifdef CONFIG_MODULES
-static void nfnetlink_bind(int group)
+static int nfnetlink_bind(int group)
 {
 	const struct nfnetlink_subsystem *ss;
 	int type = nfnl_group2type[group];
@@ -402,6 +402,7 @@ static void nfnetlink_bind(int group)
 	rcu_read_unlock();
 	if (!ss)
 		request_module("nfnetlink-subsys-%d", type);
+	return 0;
 }
 #endif
 
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index bca50b9..9ae6e0f 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1198,7 +1198,8 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
 	struct module *module = NULL;
 	struct mutex *cb_mutex;
 	struct netlink_sock *nlk;
-	void (*bind)(int group);
+	int (*bind)(int group);
+	void (*unbind)(int group);
 	int err = 0;
 
 	sock->state = SS_UNCONNECTED;
@@ -1224,6 +1225,7 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
 		err = -EPROTONOSUPPORT;
 	cb_mutex = nl_table[protocol].cb_mutex;
 	bind = nl_table[protocol].bind;
+	unbind = nl_table[protocol].unbind;
 	netlink_unlock_table();
 
 	if (err < 0)
@@ -1240,6 +1242,7 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
 	nlk = nlk_sk(sock->sk);
 	nlk->module = module;
 	nlk->netlink_bind = bind;
+	nlk->netlink_unbind = unbind;
 out:
 	return err;
 
@@ -1293,6 +1296,7 @@ static int netlink_release(struct socket *sock)
 			kfree_rcu(old, rcu);
 			nl_table[sk->sk_protocol].module = NULL;
 			nl_table[sk->sk_protocol].bind = NULL;
+			nl_table[sk->sk_protocol].unbind = NULL;
 			nl_table[sk->sk_protocol].flags = 0;
 			nl_table[sk->sk_protocol].registered = 0;
 		}
@@ -1441,6 +1445,25 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 	if (!nladdr->nl_groups && (nlk->groups == NULL || !(u32)nlk->groups[0]))
 		return 0;
 
+	if (nlk->netlink_bind && nladdr->nl_groups) {
+		int i;
+
+		for (i = 0; i < nlk->ngroups; i++)
+			int undo;
+
+			if (!test_bit(i, (long unsigned int *)&nladdr->nl_groups))
+				continue;
+			err = nlk->netlink_bind(i);
+			if (!err)
+				continue;
+			if (!nlk->portid)
+				netlink_remove(sk);
+			for (undo = 0; undo < i; undo++)
+				if (nlk->netlink_unbind)
+					nlk->netlink_unbind(undo);
+			return err;
+	}
+
 	netlink_table_grab();
 	netlink_update_subscriptions(sk, nlk->subscriptions +
 					 hweight32(nladdr->nl_groups) -
@@ -1449,15 +1472,6 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 	netlink_update_listeners(sk);
 	netlink_table_ungrab();
 
-	if (nlk->netlink_bind && nlk->groups[0]) {
-		int i;
-
-		for (i=0; i<nlk->ngroups; i++) {
-			if (test_bit(i, nlk->groups))
-				nlk->netlink_bind(i);
-		}
-	}
-
 	return 0;
 }
 
@@ -2095,14 +2109,16 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
 			return err;
 		if (!val || val - 1 >= nlk->ngroups)
 			return -EINVAL;
+		if (nlk->netlink_bind) {
+			err = nlk->netlink_bind(val);
+			if (err)
+				return err;
+		}
 		netlink_table_grab();
 		netlink_update_socket_mc(nlk, val,
 					 optname == NETLINK_ADD_MEMBERSHIP);
 		netlink_table_ungrab();
 
-		if (nlk->netlink_bind)
-			nlk->netlink_bind(val);
-
 		err = 0;
 		break;
 	}
diff --git a/net/netlink/af_netlink.h b/net/netlink/af_netlink.h
index acbd774..2aeae8a 100644
--- a/net/netlink/af_netlink.h
+++ b/net/netlink/af_netlink.h
@@ -37,7 +37,8 @@ struct netlink_sock {
 	struct mutex		*cb_mutex;
 	struct mutex		cb_def_mutex;
 	void			(*netlink_rcv)(struct sk_buff *skb);
-	void			(*netlink_bind)(int group);
+	int			(*netlink_bind)(int group);
+	void			(*netlink_unbind)(int group);
 	struct module		*module;
 #ifdef CONFIG_NETLINK_MMAP
 	struct mutex		pg_vec_lock;
@@ -73,7 +74,8 @@ struct netlink_table {
 	unsigned int		groups;
 	struct mutex		*cb_mutex;
 	struct module		*module;
-	void			(*bind)(int group);
+	int			(*bind)(int group);
+	void			(*unbind)(int group);
 	bool			(*compare)(struct net *net, struct sock *sock);
 	int			registered;
 };
-- 
1.7.1

[PATCH 3/3] netlink: implement unbind to netlink_setsockopt NETLINK_DROP_MEMBERSHIP

[Richard Guy Briggs]

Call the per-protocol unbind function rather than bind function on
NETLINK_DROP_MEMBERSHIP in netlink_setsockopt().

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 net/netlink/af_netlink.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 9ae6e0f..3938138 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2109,7 +2109,7 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
 			return err;
 		if (!val || val - 1 >= nlk->ngroups)
 			return -EINVAL;
-		if (nlk->netlink_bind) {
+		if (optname == NETLINK_ADD_MEMBERSHIP && nlk->netlink_bind) {
 			err = nlk->netlink_bind(val);
 			if (err)
 				return err;
@@ -2118,6 +2118,8 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
 		netlink_update_socket_mc(nlk, val,
 					 optname == NETLINK_ADD_MEMBERSHIP);
 		netlink_table_ungrab();
+		if (optname == NETLINK_DROP_MEMBERSHIP && nlk->netlink_unbind)
+			nlk->netlink_unbind(val);
 
 		err = 0;
 		break;
-- 
1.7.1

Re: [PATCH 0/3] netlink: per-protocol bind fixup/enhancement set

[David Miller]

From: Richard Guy Briggs <rgb@redhat.com>
Date: Tue,  1 Apr 2014 10:14:55 -0400

> This set provides a way for per-protocol bind functions to signal an error and
> to be able to clean up after themselves.
> 
> The first patch has already been accepted, but is included just in case to
> avoid a merge error.
> 
> The second patch adds the per-protocol bind return code to signal to the
> netlink code that no further processing should be done and to undo the work
> already done.  This rev has fixed DaveM's last issue and flattened the
> intentation as requested by Patrick McHardy by two by reworking the logic.
> 
> The third provides a way per protocol to undo actions on DROP.
> 
> Thanks for the feedback.

I would like to defer this to the next merge window.

I'd also like to see how the AUDIT code is going to use this, provide
the user in your next submission.

Right now the only user is nfnetlink and it's merely to do a
(sub-)module request.

Therefore it's no surprise that we've never had any real well thought
out semantics defined for the bind method, and it's also why we never
thought of adding an unbind method before.

Re: [PATCH 0/3] netlink: per-protocol bind fixup/enhancement set

[Richard Guy Briggs]

On 14/04/01, David Miller wrote:
> From: Richard Guy Briggs <rgb@redhat.com>
> Date: Tue,  1 Apr 2014 10:14:55 -0400
> 
> > This set provides a way for per-protocol bind functions to signal an error and
> > to be able to clean up after themselves.
> > 
> > The first patch has already been accepted, but is included just in case to
> > avoid a merge error.
> > 
> > The second patch adds the per-protocol bind return code to signal to the
> > netlink code that no further processing should be done and to undo the work
> > already done.  This rev has fixed DaveM's last issue and flattened the
> > intentation as requested by Patrick McHardy by two by reworking the logic.
> > 
> > The third provides a way per protocol to undo actions on DROP.
> > 
> > Thanks for the feedback.
> 
> I would like to defer this to the next merge window.

I was hoping to get it into this merge window, but but I agree it is a
bit late for that.  If I had succeeded in posting it to the correct list
address back in February it wouldn't be late.

> I'd also like to see how the AUDIT code is going to use this, provide
> the user in your next submission.

That context was already posted here:
	https://www.redhat.com/archives/linux-audit/2014-February/msg00102.html
	https://lkml.org/lkml/2014/2/19/481

I discovered later I used a stale list address for netdev and didn't Cc
you directly, so you likely would have missed it.

> Right now the only user is nfnetlink and it's merely to do a
> (sub-)module request.
> 
> Therefore it's no surprise that we've never had any real well thought
> out semantics defined for the bind method, and it's also why we never
> thought of adding an unbind method before.

No problem.  It was recommended I resend patch 3/5 of that set,
isolated, to get it reviewed here.  These recent changes to that patch
should not affect patches 1, 2, 4, 5 of that original patch context.

Does that help?

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
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

Re: [PATCH 0/3] netlink: per-protocol bind fixup/enhancement set

[David Miller]

From: Richard Guy Briggs <rgb@redhat.com>
Date: Tue, 1 Apr 2014 18:12:15 -0400

> Does that help?

It will when you include in your resubmission when net-next
opens back up, I don't have time to look at it right now as
I'm working on the current merge window fevorishly.

[PATCH 0/6] audit: implement multicast socket for journald

[Richard Guy Briggs]

This is a patch set Eric Paris and I have been working on to add a restricted
capability read-only netlink multicast socket to kernel audit to enable
userspace clients such as systemd/journald to receive audit logs, in addition
to the bidirectional auditd userspace client.

Currently, auditd has the CAP_AUDIT_CONTROL and CAP_AUDIT_WRITE capabilities
(but uses CAP_NET_ADMIN).  The CAP_AUDIT_READ capability will be added for use
by read-only AUDIT_NLGRP_READLOG multicast group clients to the kaudit
subsystem.  This will remove the dependence on CAP_NET_ADMIN for the multicast
read-only socket.


Patches 1-3 provide a way for per-protocol bind functions to
signal an error and to be able to clean up after themselves.

The first netfilter cleanup patch has already been accepted by a netfilter
maintainer, though I don't see it upstream yet, so it is included for
completeness.

The second patch adds the per-protocol bind function return code to signal to
the netlink code that no further processing should be done and to undo the work
already done.  This rev fixes a bug introduced by flattening the code in the
last posting.

The third provides a way per protocol to undo bind actions on DROP.


Patches 4-6 implement the audit multicast socket with capability checking.

The fourth patch adds the bind function capability check to multicast join
requests for audit.

The fifth patch adds the audit log read multicast group.  An assumption has
been made that systemd/journald reside in the initial network namespace.  This
could be changed to check the actual network namespace of systemd/journald
should this assumption no longer be true since audit now supports all network
namespaces.  This version of the patch now directly sends the broadcast when
the packet is ready rather than waiting until it passes the queue.

The sixth checks if any clients actually exist before sending.


Since the net tree is busier than the audit tree, conflicts are more likely and
the audit patches depend on the net patches, it is proposed to have the net
tree carry this entire patchset for 3.16.  Are the net maintainers ok with this?


https://bugzilla.redhat.com/show_bug.cgi?id=887992 

First posted:   https://www.redhat.com/archives/linux-audit/2013-January/msg00008.html
                https://lkml.org/lkml/2013/1/27/279

Please find source for a test program at: 
	http://people.redhat.com/rbriggs/audit-multicast-listen/


Richard Guy Briggs (6):
  netlink: simplify nfnetlink_bind
  netlink: have netlink per-protocol bind function return an error
    code.
  netlink: implement unbind to netlink_setsockopt
    NETLINK_DROP_MEMBERSHIP
  audit: add netlink audit protocol bind to check capabilities on
    multicast join
  audit: add netlink multicast group for log read
  audit: send multicast messages only if there are listeners

 include/linux/netlink.h             |    3 +-
 include/uapi/linux/audit.h          |    8 ++++
 include/uapi/linux/capability.h     |    7 +++-
 kernel/audit.c                      |   64 ++++++++++++++++++++++++++++++++--
 net/netfilter/nfnetlink.c           |   10 ++---
 net/netlink/af_netlink.c            |   45 +++++++++++++++++-------
 net/netlink/af_netlink.h            |    6 ++-
 security/selinux/include/classmap.h |    2 +-
 8 files changed, 117 insertions(+), 28 deletions(-)

[PATCH 1/6] netlink: simplify nfnetlink_bind

[Richard Guy Briggs]

Remove duplicity and simplify code flow by moving the rcu_read_unlock() above
the condition and let the flow control exit naturally at the end of the
function.

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 net/netfilter/nfnetlink.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
index e8138da..0df800a 100644
--- a/net/netfilter/nfnetlink.c
+++ b/net/netfilter/nfnetlink.c
@@ -407,12 +407,9 @@ static void nfnetlink_bind(int group)
 
 	rcu_read_lock();
 	ss = nfnetlink_get_subsys(type);
-	if (!ss) {
-		rcu_read_unlock();
-		request_module("nfnetlink-subsys-%d", type);
-		return;
-	}
 	rcu_read_unlock();
+	if (!ss)
+		request_module("nfnetlink-subsys-%d", type);
 }
 #endif
 
-- 
1.7.1

[PATCH 2/6] netlink: have netlink per-protocol bind function return an error code.

[Richard Guy Briggs]

Have the netlink per-protocol optional bind function return an int error code
rather than void to signal a failure.

This will enable netlink protocols to perform extra checks including
capabilities and permissions verifications when updating memberships in
multicast groups.

In netlink_bind() and netlink_setsockopt() the call to the per-protocol bind
function was moved above the multicast group update to prevent any access to
the multicast socket groups before checking with the per-protocol bind
function.  This will enable the per-protocol bind function to be used to check
permissions which could be denied before making them available, and to avoid
the messy job of undoing the addition should the per-protocol bind function
fail.

The netfilter subsystem seems to be the only one currently using the
per-protocol bind function.

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
In particular, the audit subsystem (NETLINK_AUDIT protocol) could benefit by
being able to check specific capabilities for each multicast group before
granting membership to the requesting socket.  Currently, all NETLINK_AUDIT
sockets must have the capability CAP_NET_ADMIN.  No other capabilities are
required to join a multicast group.  This capability is too broad allowing
access to this socket by many applications that must not have access to this
information.  It is proposed to add capability CAP_AUDIT_READ to allow this
access while dropping the exessively broad capability CAP_NET_ADMIN.

There has also been some interest expressed by IETF ForCES folk.
---
 include/linux/netlink.h   |    3 ++-
 net/netfilter/nfnetlink.c |    3 ++-
 net/netlink/af_netlink.c  |   43 ++++++++++++++++++++++++++++++-------------
 net/netlink/af_netlink.h  |    6 ++++--
 4 files changed, 38 insertions(+), 17 deletions(-)

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index aad8eea..5146ce0 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -45,7 +45,8 @@ struct netlink_kernel_cfg {
 	unsigned int	flags;
 	void		(*input)(struct sk_buff *skb);
 	struct mutex	*cb_mutex;
-	void		(*bind)(int group);
+	int		(*bind)(int group);
+	void		(*unbind)(int group);
 	bool		(*compare)(struct net *net, struct sock *sk);
 };
 
diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
index 0df800a..6e42dcf 100644
--- a/net/netfilter/nfnetlink.c
+++ b/net/netfilter/nfnetlink.c
@@ -400,7 +400,7 @@ static void nfnetlink_rcv(struct sk_buff *skb)
 }
 
 #ifdef CONFIG_MODULES
-static void nfnetlink_bind(int group)
+static int nfnetlink_bind(int group)
 {
 	const struct nfnetlink_subsystem *ss;
 	int type = nfnl_group2type[group];
@@ -410,6 +410,7 @@ static void nfnetlink_bind(int group)
 	rcu_read_unlock();
 	if (!ss)
 		request_module("nfnetlink-subsys-%d", type);
+	return 0;
 }
 #endif
 
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 894cda0..3f43e5a 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1206,7 +1206,8 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
 	struct module *module = NULL;
 	struct mutex *cb_mutex;
 	struct netlink_sock *nlk;
-	void (*bind)(int group);
+	int (*bind)(int group);
+	void (*unbind)(int group);
 	int err = 0;
 
 	sock->state = SS_UNCONNECTED;
@@ -1232,6 +1233,7 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
 		err = -EPROTONOSUPPORT;
 	cb_mutex = nl_table[protocol].cb_mutex;
 	bind = nl_table[protocol].bind;
+	unbind = nl_table[protocol].unbind;
 	netlink_unlock_table();
 
 	if (err < 0)
@@ -1248,6 +1250,7 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
 	nlk = nlk_sk(sock->sk);
 	nlk->module = module;
 	nlk->netlink_bind = bind;
+	nlk->netlink_unbind = unbind;
 out:
 	return err;
 
@@ -1301,6 +1304,7 @@ static int netlink_release(struct socket *sock)
 			kfree_rcu(old, rcu);
 			nl_table[sk->sk_protocol].module = NULL;
 			nl_table[sk->sk_protocol].bind = NULL;
+			nl_table[sk->sk_protocol].unbind = NULL;
 			nl_table[sk->sk_protocol].flags = 0;
 			nl_table[sk->sk_protocol].registered = 0;
 		}
@@ -1449,6 +1453,26 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 	if (!nladdr->nl_groups && (nlk->groups == NULL || !(u32)nlk->groups[0]))
 		return 0;
 
+	if (nlk->netlink_bind && nladdr->nl_groups) {
+		int i;
+
+		for (i = 0; i < nlk->ngroups; i++) {
+			int undo;
+
+			if (!test_bit(i, (long unsigned int *)&nladdr->nl_groups))
+				continue;
+			err = nlk->netlink_bind(i);
+			if (!err)
+				continue;
+			if (!nlk->portid)
+				netlink_remove(sk);
+			for (undo = 0; undo < i; undo++)
+				if (nlk->netlink_unbind)
+					nlk->netlink_unbind(undo);
+			return err;
+		}
+	}
+
 	netlink_table_grab();
 	netlink_update_subscriptions(sk, nlk->subscriptions +
 					 hweight32(nladdr->nl_groups) -
@@ -1457,15 +1481,6 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 	netlink_update_listeners(sk);
 	netlink_table_ungrab();
 
-	if (nlk->netlink_bind && nlk->groups[0]) {
-		int i;
-
-		for (i = 0; i < nlk->ngroups; i++) {
-			if (test_bit(i, nlk->groups))
-				nlk->netlink_bind(i);
-		}
-	}
-
 	return 0;
 }
 
@@ -2103,14 +2118,16 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
 			return err;
 		if (!val || val - 1 >= nlk->ngroups)
 			return -EINVAL;
+		if (nlk->netlink_bind) {
+			err = nlk->netlink_bind(val);
+			if (err)
+				return err;
+		}
 		netlink_table_grab();
 		netlink_update_socket_mc(nlk, val,
 					 optname == NETLINK_ADD_MEMBERSHIP);
 		netlink_table_ungrab();
 
-		if (nlk->netlink_bind)
-			nlk->netlink_bind(val);
-
 		err = 0;
 		break;
 	}
diff --git a/net/netlink/af_netlink.h b/net/netlink/af_netlink.h
index ed13a79..0b59d44 100644
--- a/net/netlink/af_netlink.h
+++ b/net/netlink/af_netlink.h
@@ -38,7 +38,8 @@ struct netlink_sock {
 	struct mutex		*cb_mutex;
 	struct mutex		cb_def_mutex;
 	void			(*netlink_rcv)(struct sk_buff *skb);
-	void			(*netlink_bind)(int group);
+	int			(*netlink_bind)(int group);
+	void			(*netlink_unbind)(int group);
 	struct module		*module;
 #ifdef CONFIG_NETLINK_MMAP
 	struct mutex		pg_vec_lock;
@@ -74,7 +75,8 @@ struct netlink_table {
 	unsigned int		groups;
 	struct mutex		*cb_mutex;
 	struct module		*module;
-	void			(*bind)(int group);
+	int			(*bind)(int group);
+	void			(*unbind)(int group);
 	bool			(*compare)(struct net *net, struct sock *sock);
 	int			registered;
 };
-- 
1.7.1

Re: [PATCH 2/6] netlink: have netlink per-protocol bind function return an error code.

[David Miller]

From: Richard Guy Briggs <rgb@redhat.com>
Date: Fri, 18 Apr 2014 13:34:06 -0400

> @@ -1449,6 +1453,26 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
>  	if (!nladdr->nl_groups && (nlk->groups == NULL || !(u32)nlk->groups[0]))
>  		return 0;
>  
> +	if (nlk->netlink_bind && nladdr->nl_groups) {
> +		int i;
> +
> +		for (i = 0; i < nlk->ngroups; i++) {
> +			int undo;
> +
> +			if (!test_bit(i, (long unsigned int *)&nladdr->nl_groups))
> +				continue;
> +			err = nlk->netlink_bind(i);
> +			if (!err)
> +				continue;
> +			if (!nlk->portid)
> +				netlink_remove(sk);
> +			for (undo = 0; undo < i; undo++)
> +				if (nlk->netlink_unbind)
> +					nlk->netlink_unbind(undo);
> +			return err;
> +		}
> +	}
> +

It took me a while to figure out why you need to do the netlink_remove() in
the error path.

I think it's really asking for trouble to allow the socket to have temporary
visibility if we end up signalling an error.

It seems safest if we only do the autobind/insert once we are absolutely
certain that the bind() will fully succeed.  This means that you have to
do this bind validation loop before autobind/insert.

Please make this change and resubmit this series, thanks.

Re: [PATCH 2/6] netlink: have netlink per-protocol bind function return an error code.

[Richard Guy Briggs]

On 14/04/22, David Miller wrote:
> From: Richard Guy Briggs <rgb@redhat.com>
> Date: Fri, 18 Apr 2014 13:34:06 -0400
> 
> > @@ -1449,6 +1453,26 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
> >  	if (!nladdr->nl_groups && (nlk->groups == NULL || !(u32)nlk->groups[0]))
> >  		return 0;
> >  
> > +	if (nlk->netlink_bind && nladdr->nl_groups) {
> > +		int i;
> > +
> > +		for (i = 0; i < nlk->ngroups; i++) {
> > +			int undo;
> > +
> > +			if (!test_bit(i, (long unsigned int *)&nladdr->nl_groups))
> > +				continue;
> > +			err = nlk->netlink_bind(i);
> > +			if (!err)
> > +				continue;
> > +			if (!nlk->portid)
> > +				netlink_remove(sk);
> > +			for (undo = 0; undo < i; undo++)
> > +				if (nlk->netlink_unbind)
> > +					nlk->netlink_unbind(undo);
> > +			return err;
> > +		}
> > +	}
> > +
> 
> It took me a while to figure out why you need to do the netlink_remove() in
> the error path.
> 
> I think it's really asking for trouble to allow the socket to have temporary
> visibility if we end up signalling an error.

The risk seems small, but I agree it is sloppy.

> It seems safest if we only do the autobind/insert once we are absolutely
> certain that the bind() will fully succeed.  This means that you have to
> do this bind validation loop before autobind/insert.

Ok, done.  This revision ends up being a bit cleaner than the previous
one because I've refactored out the unbind loop into netlink_unbind()
due to it needing to be called if the netlink_insert/autobind() fails as
well.  In the factorization process I noticed that unrequested groups
were being unnecessarily unbound.  It also provoked assigning the var
"groups" from "nladdr->nl_groups" making things easier to read.

> Please make this change and resubmit this series, thanks.

Compiled and tested.  New patchset to follow.

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
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

[PATCH 0/6][v2] audit: implement multicast socket for journald

[Richard Guy Briggs]

This is a patch set Eric Paris and I have been working on to add a restricted
capability read-only netlink multicast socket to kernel audit to enable
userspace clients such as systemd/journald to receive audit logs, in addition
to the bidirectional auditd userspace client.

Currently, auditd has the CAP_AUDIT_CONTROL and CAP_AUDIT_WRITE capabilities
(but uses CAP_NET_ADMIN).  The CAP_AUDIT_READ capability will be added for use
by read-only AUDIT_NLGRP_READLOG multicast group clients to the kaudit
subsystem.  This will remove the dependence on CAP_NET_ADMIN for the multicast
read-only socket.


Patches 1-3 provide a way for per-protocol bind functions to
signal an error and to be able to clean up after themselves.

The first netfilter cleanup patch has already been accepted by a netfilter
maintainer, though I don't see it upstream yet, so it is included for
completeness.

The second patch adds the per-protocol bind function return code to signal to
the netlink code that no further processing should be done and to undo the work
already done.
V1: This rev fixes a bug introduced by flattening the code in the last posting.
*V2: This rev moves the per-protocol bind call above the socket exposure call
and refactors out the unbind procedure.

The third provides a way per protocol to undo bind actions on DROP.


Patches 4-6 implement the audit multicast socket with capability checking.

The fourth patch adds the bind function capability check to multicast join
requests for audit.

The fifth patch adds the audit log read multicast group.  An assumption has
been made that systemd/journald reside in the initial network namespace.  This
could be changed to check the actual network namespace of systemd/journald
should this assumption no longer be true since audit now supports all network
namespaces.  This version of the patch now directly sends the broadcast when
the packet is ready rather than waiting until it passes the queue.

The sixth checks if any clients actually exist before sending.


Since the net tree is busier than the audit tree, conflicts are more likely and
the audit patches depend on the net patches, it is proposed to have the net
tree carry this entire patchset for 3.16.  Are the net maintainers ok with this?


https://bugzilla.redhat.com/show_bug.cgi?id=887992 

First posted:   https://www.redhat.com/archives/linux-audit/2013-January/msg00008.html
                https://lkml.org/lkml/2013/1/27/279

Please find source for a test program at: 
	http://people.redhat.com/rbriggs/audit-multicast-listen/


Richard Guy Briggs (6):
  netlink: simplify nfnetlink_bind
  netlink: have netlink per-protocol bind function return an error
    code.
  netlink: implement unbind to netlink_setsockopt
    NETLINK_DROP_MEMBERSHIP
  audit: add netlink audit protocol bind to check capabilities on
    multicast join
  audit: add netlink multicast group for log read
  audit: send multicast messages only if there are listeners

 include/linux/netlink.h             |    3 +-
 include/uapi/linux/audit.h          |    8 ++++
 include/uapi/linux/capability.h     |    7 +++-
 kernel/audit.c                      |   64 ++++++++++++++++++++++++++++++--
 net/netfilter/nfnetlink.c           |   10 ++---
 net/netlink/af_netlink.c            |   70 +++++++++++++++++++++++++----------
 net/netlink/af_netlink.h            |    6 ++-
 security/selinux/include/classmap.h |    2 +-
 8 files changed, 135 insertions(+), 35 deletions(-)

[PATCH 1/6][v2] netlink: simplify nfnetlink_bind

[Richard Guy Briggs]

Remove duplicity and simplify code flow by moving the rcu_read_unlock() above
the condition and let the flow control exit naturally at the end of the
function.

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 net/netfilter/nfnetlink.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
index e8138da..0df800a 100644
--- a/net/netfilter/nfnetlink.c
+++ b/net/netfilter/nfnetlink.c
@@ -407,12 +407,9 @@ static void nfnetlink_bind(int group)
 
 	rcu_read_lock();
 	ss = nfnetlink_get_subsys(type);
-	if (!ss) {
-		rcu_read_unlock();
-		request_module("nfnetlink-subsys-%d", type);
-		return;
-	}
 	rcu_read_unlock();
+	if (!ss)
+		request_module("nfnetlink-subsys-%d", type);
 }
 #endif
 
-- 
1.7.1

[PATCH 2/6][v2] netlink: have netlink per-protocol bind function return an error code.

[Richard Guy Briggs]

Have the netlink per-protocol optional bind function return an int error code
rather than void to signal a failure.

This will enable netlink protocols to perform extra checks including
capabilities and permissions verifications when updating memberships in
multicast groups.

In netlink_bind() and netlink_setsockopt() the call to the per-protocol bind
function was moved above the multicast group update to prevent any access to
the multicast socket groups before checking with the per-protocol bind
function.  This will enable the per-protocol bind function to be used to check
permissions which could be denied before making them available, and to avoid
the messy job of undoing the addition should the per-protocol bind function
fail.

The netfilter subsystem seems to be the only one currently using the
per-protocol bind function.

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
V1: This rev fixes a bug introduced by flattening the code in the last posting.
*V2: This rev moves the per-protocol bind call above the socket exposure call
and refactors out the unbind procedure.

In particular, the audit subsystem (NETLINK_AUDIT protocol) could benefit by
being able to check specific capabilities for each multicast group before
granting membership to the requesting socket.  Currently, all NETLINK_AUDIT
sockets must have the capability CAP_NET_ADMIN.  No other capabilities are
required to join a multicast group.  This capability is too broad allowing
access to this socket by many applications that must not have access to this
information.  It is proposed to add capability CAP_AUDIT_READ to allow this
access while dropping the exessively broad capability CAP_NET_ADMIN.

There has also been some interest expressed by IETF ForCES folk.
---
 include/linux/netlink.h   |    3 +-
 net/netfilter/nfnetlink.c |    3 +-
 net/netlink/af_netlink.c  |   68 +++++++++++++++++++++++++++++++-------------
 net/netlink/af_netlink.h  |    6 ++-
 4 files changed, 56 insertions(+), 24 deletions(-)

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index aad8eea..5146ce0 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -45,7 +45,8 @@ struct netlink_kernel_cfg {
 	unsigned int	flags;
 	void		(*input)(struct sk_buff *skb);
 	struct mutex	*cb_mutex;
-	void		(*bind)(int group);
+	int		(*bind)(int group);
+	void		(*unbind)(int group);
 	bool		(*compare)(struct net *net, struct sock *sk);
 };
 
diff --git a/net/netfilter/nfnetlink.c b/net/netfilter/nfnetlink.c
index 0df800a..6e42dcf 100644
--- a/net/netfilter/nfnetlink.c
+++ b/net/netfilter/nfnetlink.c
@@ -400,7 +400,7 @@ static void nfnetlink_rcv(struct sk_buff *skb)
 }
 
 #ifdef CONFIG_MODULES
-static void nfnetlink_bind(int group)
+static int nfnetlink_bind(int group)
 {
 	const struct nfnetlink_subsystem *ss;
 	int type = nfnl_group2type[group];
@@ -410,6 +410,7 @@ static void nfnetlink_bind(int group)
 	rcu_read_unlock();
 	if (!ss)
 		request_module("nfnetlink-subsys-%d", type);
+	return 0;
 }
 #endif
 
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 894cda0..7e8d229 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1206,7 +1206,8 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
 	struct module *module = NULL;
 	struct mutex *cb_mutex;
 	struct netlink_sock *nlk;
-	void (*bind)(int group);
+	int (*bind)(int group);
+	void (*unbind)(int group);
 	int err = 0;
 
 	sock->state = SS_UNCONNECTED;
@@ -1232,6 +1233,7 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
 		err = -EPROTONOSUPPORT;
 	cb_mutex = nl_table[protocol].cb_mutex;
 	bind = nl_table[protocol].bind;
+	unbind = nl_table[protocol].unbind;
 	netlink_unlock_table();
 
 	if (err < 0)
@@ -1248,6 +1250,7 @@ static int netlink_create(struct net *net, struct socket *sock, int protocol,
 	nlk = nlk_sk(sock->sk);
 	nlk->module = module;
 	nlk->netlink_bind = bind;
+	nlk->netlink_unbind = unbind;
 out:
 	return err;
 
@@ -1301,6 +1304,7 @@ static int netlink_release(struct socket *sock)
 			kfree_rcu(old, rcu);
 			nl_table[sk->sk_protocol].module = NULL;
 			nl_table[sk->sk_protocol].bind = NULL;
+			nl_table[sk->sk_protocol].unbind = NULL;
 			nl_table[sk->sk_protocol].flags = 0;
 			nl_table[sk->sk_protocol].registered = 0;
 		}
@@ -1411,6 +1415,19 @@ static int netlink_realloc_groups(struct sock *sk)
 	return err;
 }
 
+static void netlink_unbind(int group, long unsigned int groups,
+			   struct netlink_sock *nlk)
+{
+	int undo;
+
+	if (!nlk->netlink_unbind)
+		return;
+
+	for (undo = 0; undo < group; undo++)
+		if (test_bit(group, &groups))
+			nlk->netlink_unbind(undo);
+}
+
 static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 			int addr_len)
 {
@@ -1419,6 +1436,7 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 	struct netlink_sock *nlk = nlk_sk(sk);
 	struct sockaddr_nl *nladdr = (struct sockaddr_nl *)addr;
 	int err;
+	long unsigned int groups = nladdr->nl_groups;
 
 	if (addr_len < sizeof(struct sockaddr_nl))
 		return -EINVAL;
@@ -1427,7 +1445,7 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 		return -EINVAL;
 
 	/* Only superuser is allowed to listen multicasts */
-	if (nladdr->nl_groups) {
+	if (groups) {
 		if (!netlink_capable(sock, NL_CFG_F_NONROOT_RECV))
 			return -EPERM;
 		err = netlink_realloc_groups(sk);
@@ -1435,37 +1453,45 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 			return err;
 	}
 
-	if (nlk->portid) {
+	if (nlk->portid)
 		if (nladdr->nl_pid != nlk->portid)
 			return -EINVAL;
-	} else {
+
+	if (nlk->netlink_bind && groups) {
+		int group;
+
+		for (group = 0; group < nlk->ngroups; group++) {
+			if (!test_bit(group, &groups))
+				continue;
+			err = nlk->netlink_bind(group);
+			if (!err)
+				continue;
+			netlink_unbind(group, groups, nlk);
+			return err;
+		}
+	}
+
+	if (!nlk->portid) {
 		err = nladdr->nl_pid ?
 			netlink_insert(sk, net, nladdr->nl_pid) :
 			netlink_autobind(sock);
-		if (err)
+		if (err) {
+			netlink_unbind(nlk->ngroups - 1, groups, nlk);
 			return err;
+		}
 	}
 
-	if (!nladdr->nl_groups && (nlk->groups == NULL || !(u32)nlk->groups[0]))
+	if (!groups && (nlk->groups == NULL || !(u32)nlk->groups[0]))
 		return 0;
 
 	netlink_table_grab();
 	netlink_update_subscriptions(sk, nlk->subscriptions +
-					 hweight32(nladdr->nl_groups) -
+					 hweight32(groups) -
 					 hweight32(nlk->groups[0]));
-	nlk->groups[0] = (nlk->groups[0] & ~0xffffffffUL) | nladdr->nl_groups;
+	nlk->groups[0] = (nlk->groups[0] & ~0xffffffffUL) | groups;
 	netlink_update_listeners(sk);
 	netlink_table_ungrab();
 
-	if (nlk->netlink_bind && nlk->groups[0]) {
-		int i;
-
-		for (i = 0; i < nlk->ngroups; i++) {
-			if (test_bit(i, nlk->groups))
-				nlk->netlink_bind(i);
-		}
-	}
-
 	return 0;
 }
 
@@ -2103,14 +2129,16 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
 			return err;
 		if (!val || val - 1 >= nlk->ngroups)
 			return -EINVAL;
+		if (nlk->netlink_bind) {
+			err = nlk->netlink_bind(val);
+			if (err)
+				return err;
+		}
 		netlink_table_grab();
 		netlink_update_socket_mc(nlk, val,
 					 optname == NETLINK_ADD_MEMBERSHIP);
 		netlink_table_ungrab();
 
-		if (nlk->netlink_bind)
-			nlk->netlink_bind(val);
-
 		err = 0;
 		break;
 	}
diff --git a/net/netlink/af_netlink.h b/net/netlink/af_netlink.h
index ed13a79..0b59d44 100644
--- a/net/netlink/af_netlink.h
+++ b/net/netlink/af_netlink.h
@@ -38,7 +38,8 @@ struct netlink_sock {
 	struct mutex		*cb_mutex;
 	struct mutex		cb_def_mutex;
 	void			(*netlink_rcv)(struct sk_buff *skb);
-	void			(*netlink_bind)(int group);
+	int			(*netlink_bind)(int group);
+	void			(*netlink_unbind)(int group);
 	struct module		*module;
 #ifdef CONFIG_NETLINK_MMAP
 	struct mutex		pg_vec_lock;
@@ -74,7 +75,8 @@ struct netlink_table {
 	unsigned int		groups;
 	struct mutex		*cb_mutex;
 	struct module		*module;
-	void			(*bind)(int group);
+	int			(*bind)(int group);
+	void			(*unbind)(int group);
 	bool			(*compare)(struct net *net, struct sock *sock);
 	int			registered;
 };
-- 
1.7.1

[PATCH 3/6][v2] netlink: implement unbind to netlink_setsockopt NETLINK_DROP_MEMBERSHIP

[Richard Guy Briggs]

Call the per-protocol unbind function rather than bind function on
NETLINK_DROP_MEMBERSHIP in netlink_setsockopt().

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 net/netlink/af_netlink.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 7e8d229..92f4b69 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2129,7 +2129,7 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
 			return err;
 		if (!val || val - 1 >= nlk->ngroups)
 			return -EINVAL;
-		if (nlk->netlink_bind) {
+		if (optname == NETLINK_ADD_MEMBERSHIP && nlk->netlink_bind) {
 			err = nlk->netlink_bind(val);
 			if (err)
 				return err;
@@ -2138,6 +2138,8 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
 		netlink_update_socket_mc(nlk, val,
 					 optname == NETLINK_ADD_MEMBERSHIP);
 		netlink_table_ungrab();
+		if (optname == NETLINK_DROP_MEMBERSHIP && nlk->netlink_unbind)
+			nlk->netlink_unbind(val);
 
 		err = 0;
 		break;
-- 
1.7.1

[PATCH 4/6][v2] audit: add netlink audit protocol bind to check capabilities on multicast join

[Richard Guy Briggs]

Register a netlink per-protocol bind fuction for audit to check userspace
process capabilities before allowing a multicast group connection.

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 include/uapi/linux/capability.h     |    7 ++++++-
 kernel/audit.c                      |   10 ++++++++++
 security/selinux/include/classmap.h |    2 +-
 3 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
index 154dd6d..12c37a1 100644
--- a/include/uapi/linux/capability.h
+++ b/include/uapi/linux/capability.h
@@ -347,7 +347,12 @@ struct vfs_cap_data {
 
 #define CAP_BLOCK_SUSPEND    36
 
-#define CAP_LAST_CAP         CAP_BLOCK_SUSPEND
+/* Allow reading the audit log via multicast netlink socket */
+
+#define CAP_AUDIT_READ		37
+
+
+#define CAP_LAST_CAP         CAP_AUDIT_READ
 
 #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
 
diff --git a/kernel/audit.c b/kernel/audit.c
index 7c28936..223cb74 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1076,10 +1076,20 @@ static void audit_receive(struct sk_buff  *skb)
 	mutex_unlock(&audit_cmd_mutex);
 }
 
+/* Run custom bind function on netlink socket group connect or bind requests. */
+static int audit_bind(int group)
+{
+	if (!capable(CAP_AUDIT_READ))
+		return -EPERM;
+
+	return 0;
+}
+
 static int __net_init audit_net_init(struct net *net)
 {
 	struct netlink_kernel_cfg cfg = {
 		.input	= audit_receive,
+		.bind	= audit_bind,
 	};
 
 	struct audit_net *aunet = net_generic(net, audit_net_id);
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index 14d04e6..be491a7 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -147,7 +147,7 @@ struct security_class_mapping secclass_map[] = {
 	{ "peer", { "recv", NULL } },
 	{ "capability2",
 	  { "mac_override", "mac_admin", "syslog", "wake_alarm", "block_suspend",
-	    NULL } },
+	    "audit_read", NULL } },
 	{ "kernel_service", { "use_as_override", "create_files_as", NULL } },
 	{ "tun_socket",
 	  { COMMON_SOCK_PERMS, "attach_queue", NULL } },
-- 
1.7.1

[PATCH 5/6][v2] audit: add netlink multicast group for log read

[Richard Guy Briggs]

Add a netlink multicast socket with one group to kaudit for "best-effort"
delivery to read-only userspace clients such as systemd, in addition to the
existing bidirectional unicast auditd userspace client.

Currently, auditd is intended to use the CAP_AUDIT_CONTROL and CAP_AUDIT_WRITE
capabilities, but actually uses CAP_NET_ADMIN.  The CAP_AUDIT_READ capability
is added for use by read-only AUDIT_NLGRP_READLOG netlink multicast group
clients to the kaudit subsystem.

This will safely give access to services such as systemd to consume audit logs
while ensuring write access remains restricted for integrity.

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 include/uapi/linux/audit.h |    8 +++++++
 kernel/audit.c             |   51 ++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 55 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 11917f7..dfa4c86 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -373,6 +373,14 @@ enum {
  */
 #define AUDIT_MESSAGE_TEXT_MAX	8560
 
+/* Multicast Netlink socket groups (default up to 32) */
+enum audit_nlgrps {
+	AUDIT_NLGRP_NONE,	/* Group 0 not used */
+	AUDIT_NLGRP_READLOG,	/* "best effort" read only socket */
+	__AUDIT_NLGRP_MAX
+};
+#define AUDIT_NLGRP_MAX                (__AUDIT_NLGRP_MAX - 1)
+
 struct audit_status {
 	__u32		mask;		/* Bit mask for valid entries */
 	__u32		enabled;	/* 1 = enabled, 0 = disabled */
diff --git a/kernel/audit.c b/kernel/audit.c
index 223cb74..d272cc1 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -424,6 +424,35 @@ static void kauditd_send_skb(struct sk_buff *skb)
 }
 
 /*
+ * kauditd_send_multicast_skb - send the skb to multicast userspace listeners
+ *
+ * This function doesn't consume an skb as might be expected since it has to
+ * copy it anyways.
+ */
+static void kauditd_send_multicast_skb(struct sk_buff *skb)
+{
+	struct sk_buff		*copy;
+	struct audit_net	*aunet = net_generic(&init_net, audit_net_id);
+	struct sock		*sock = aunet->nlsk;
+
+	/*
+	 * The seemingly wasteful skb_copy() rather than bumping the refcount
+	 * using skb_get() is necessary because non-standard mods are made to
+	 * the skb by the original kaudit unicast socket send routine.  The
+	 * existing auditd daemon assumes this breakage.  Fixing this would
+	 * require co-ordinating a change in the established protocol between
+	 * the kaudit kernel subsystem and the auditd userspace code.  There is
+	 * no reason for new multicast clients to continue with this
+	 * non-compliance.
+	 */
+	copy = skb_copy(skb, GFP_KERNEL);
+	if (!copy)
+		return;
+
+	nlmsg_multicast(sock, copy, 0, AUDIT_NLGRP_READLOG, GFP_KERNEL);
+}
+
+/*
  * flush_hold_queue - empty the hold queue if auditd appears
  *
  * If auditd just started, drain the queue of messages already
@@ -1090,6 +1119,8 @@ static int __net_init audit_net_init(struct net *net)
 	struct netlink_kernel_cfg cfg = {
 		.input	= audit_receive,
 		.bind	= audit_bind,
+		.flags	= NL_CFG_F_NONROOT_RECV,
+		.groups	= AUDIT_NLGRP_MAX,
 	};
 
 	struct audit_net *aunet = net_generic(net, audit_net_id);
@@ -1911,10 +1942,10 @@ out:
  * audit_log_end - end one audit record
  * @ab: the audit_buffer
  *
- * The netlink_* functions cannot be called inside an irq context, so
- * the audit buffer is placed on a queue and a tasklet is scheduled to
- * remove them from the queue outside the irq context.  May be called in
- * any context.
+ * netlink_unicast() cannot be called inside an irq context because it blocks
+ * (last arg, flags, is not set to MSG_DONTWAIT), so the audit buffer is placed
+ * on a queue and a tasklet is scheduled to remove them from the queue outside
+ * the irq context.  May be called in any context.
  */
 void audit_log_end(struct audit_buffer *ab)
 {
@@ -1924,6 +1955,18 @@ void audit_log_end(struct audit_buffer *ab)
 		audit_log_lost("rate limit exceeded");
 	} else {
 		struct nlmsghdr *nlh = nlmsg_hdr(ab->skb);
+
+		kauditd_send_multicast_skb(ab->skb);
+
+		/*
+		 * The original kaudit unicast socket sends up messages with
+		 * nlmsg_len set to the payload length rather than the entire
+		 * message length.  This breaks the standard set by netlink.
+		 * The existing auditd daemon assumes this breakage.  Fixing
+		 * this would require co-ordinating a change in the established
+		 * protocol between the kaudit kernel subsystem and the auditd
+		 * userspace code.
+		 */
 		nlh->nlmsg_len = ab->skb->len - NLMSG_HDRLEN;
 
 		if (audit_pid) {
-- 
1.7.1

[PATCH 6/6][v2] audit: send multicast messages only if there are listeners

[Richard Guy Briggs]

Test first to see if there are any userspace multicast listeners bound to the
socket before starting the multicast send work.

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 kernel/audit.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index d272cc1..33531d7 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -435,6 +435,9 @@ static void kauditd_send_multicast_skb(struct sk_buff *skb)
 	struct audit_net	*aunet = net_generic(&init_net, audit_net_id);
 	struct sock		*sock = aunet->nlsk;
 
+	if (!netlink_has_listeners(sock, AUDIT_NLGRP_READLOG))
+		return;
+
 	/*
 	 * The seemingly wasteful skb_copy() rather than bumping the refcount
 	 * using skb_get() is necessary because non-standard mods are made to
-- 
1.7.1

Re: [PATCH 0/6][v2] audit: implement multicast socket for journald

[David Miller]

From: Richard Guy Briggs <rgb@redhat.com>
Date: Tue, 22 Apr 2014 21:31:52 -0400

> This is a patch set Eric Paris and I have been working on to add a restricted
> capability read-only netlink multicast socket to kernel audit to enable
> userspace clients such as systemd/journald to receive audit logs, in addition
> to the bidirectional auditd userspace client.

Series applied, thanks Richard.

Re: [PATCH 0/6][v2] audit: implement multicast socket for journald

[Richard Guy Briggs]

On 14/04/22, David Miller wrote:
> From: Richard Guy Briggs <rgb@redhat.com>
> Date: Tue, 22 Apr 2014 21:31:52 -0400
> 
> > This is a patch set Eric Paris and I have been working on to add a restricted
> > capability read-only netlink multicast socket to kernel audit to enable
> > userspace clients such as systemd/journald to receive audit logs, in addition
> > to the bidirectional auditd userspace client.
> 
> Series applied, thanks Richard.

Thanks for your patience, David.  Can I assume you adopted the 3 audit
patches too, becuase of the dependence?

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
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

Re: [PATCH 0/6][v2] audit: implement multicast socket for journald

[David Miller]

From: Richard Guy Briggs <rgb@redhat.com>
Date: Tue, 22 Apr 2014 21:49:29 -0400

> On 14/04/22, David Miller wrote:
>> From: Richard Guy Briggs <rgb@redhat.com>
>> Date: Tue, 22 Apr 2014 21:31:52 -0400
>> 
>> > This is a patch set Eric Paris and I have been working on to add a restricted
>> > capability read-only netlink multicast socket to kernel audit to enable
>> > userspace clients such as systemd/journald to receive audit logs, in addition
>> > to the bidirectional auditd userspace client.
>> 
>> Series applied, thanks Richard.
> 
> Thanks for your patience, David.  Can I assume you adopted the 3 audit
> patches too, becuase of the dependence?

Yes.

Re: [PATCH 0/6][v2] audit: implement multicast socket for journald

[Steve Grubb]

On Tuesday, April 22, 2014 09:31:52 PM Richard Guy Briggs wrote:
> This is a patch set Eric Paris and I have been working on to add a
> restricted capability read-only netlink multicast socket to kernel audit to
> enable userspace clients such as systemd/journald to receive audit logs, in
> addition to the bidirectional auditd userspace client.

Do have the ability to separate of secadm_r and sysadm_r? By allowing this, we 
will leak to a sysadmin that he is being audited by the security officer. In a 
lot of cases, they are one in the same person. But for others, they are not. I 
have a feeling this will cause problems for MLS systems.

Also, shouldn't we have an audit event for every attempt to connect to this 
socket? We really need to know where this information is getting leaked to.

-Steve


> Currently, auditd has the CAP_AUDIT_CONTROL and CAP_AUDIT_WRITE capabilities
> (but uses CAP_NET_ADMIN).  The CAP_AUDIT_READ capability will be added for
> use by read-only AUDIT_NLGRP_READLOG multicast group clients to the kaudit
> subsystem.  This will remove the dependence on CAP_NET_ADMIN for the
> multicast read-only socket.
> 
> 
> Patches 1-3 provide a way for per-protocol bind functions to
> signal an error and to be able to clean up after themselves.
> 
> The first netfilter cleanup patch has already been accepted by a netfilter
> maintainer, though I don't see it upstream yet, so it is included for
> completeness.
> 
> The second patch adds the per-protocol bind function return code to signal
> to the netlink code that no further processing should be done and to undo
> the work already done.
> V1: This rev fixes a bug introduced by flattening the code in the last
> posting. *V2: This rev moves the per-protocol bind call above the socket
> exposure call and refactors out the unbind procedure.
> 
> The third provides a way per protocol to undo bind actions on DROP.
> 
> 
> Patches 4-6 implement the audit multicast socket with capability checking.
> 
> The fourth patch adds the bind function capability check to multicast join
> requests for audit.
> 
> The fifth patch adds the audit log read multicast group.  An assumption has
> been made that systemd/journald reside in the initial network namespace. 
> This could be changed to check the actual network namespace of
> systemd/journald should this assumption no longer be true since audit now
> supports all network namespaces.  This version of the patch now directly
> sends the broadcast when the packet is ready rather than waiting until it
> passes the queue.
> 
> The sixth checks if any clients actually exist before sending.
> 
> 
> Since the net tree is busier than the audit tree, conflicts are more likely
> and the audit patches depend on the net patches, it is proposed to have the
> net tree carry this entire patchset for 3.16.  Are the net maintainers ok
> with this?
> 
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=887992
> 
> First posted:  
> https://www.redhat.com/archives/linux-audit/2013-January/msg00008.html
> https://lkml.org/lkml/2013/1/27/279
> 
> Please find source for a test program at:
> 	http://people.redhat.com/rbriggs/audit-multicast-listen/
> 
> 
> Richard Guy Briggs (6):
>   netlink: simplify nfnetlink_bind
>   netlink: have netlink per-protocol bind function return an error
>     code.
>   netlink: implement unbind to netlink_setsockopt
>     NETLINK_DROP_MEMBERSHIP
>   audit: add netlink audit protocol bind to check capabilities on
>     multicast join
>   audit: add netlink multicast group for log read
>   audit: send multicast messages only if there are listeners
> 
>  include/linux/netlink.h             |    3 +-
>  include/uapi/linux/audit.h          |    8 ++++
>  include/uapi/linux/capability.h     |    7 +++-
>  kernel/audit.c                      |   64 ++++++++++++++++++++++++++++++--
> net/netfilter/nfnetlink.c           |   10 ++---
>  net/netlink/af_netlink.c            |   70
> +++++++++++++++++++++++++---------- net/netlink/af_netlink.h            |  
>  6 ++-
>  security/selinux/include/classmap.h |    2 +-
>  8 files changed, 135 insertions(+), 35 deletions(-)

Re: [PATCH 0/6][v2] audit: implement multicast socket for journald

[Eric Paris]

On Tue, 2014-04-22 at 22:25 -0400, Steve Grubb wrote:
> On Tuesday, April 22, 2014 09:31:52 PM Richard Guy Briggs wrote:
> > This is a patch set Eric Paris and I have been working on to add a
> > restricted capability read-only netlink multicast socket to kernel audit to
> > enable userspace clients such as systemd/journald to receive audit logs, in
> > addition to the bidirectional auditd userspace client.
> 
> Do have the ability to separate of secadm_r and sysadm_r? By allowing this, we 
> will leak to a sysadmin that he is being audited by the security officer. In a 
> lot of cases, they are one in the same person. But for others, they are not. I 
> have a feeling this will cause problems for MLS systems.

Why?  This requires CAP_AUDIT_READ.  Just don't give CAP_AUDIT_READ to
places you don't want to have read permission.  Exactly the same as you
don't give CAP_AUDIT_CONTROL to sysadm_r.  (If we are giving
CAP_AUDIT_CONTROL to sysadm_r and you think that any file protections
on /var/log/audit/audit.log are adequate we are fooling ourselves!)

> Also, shouldn't we have an audit event for every attempt to connect to this 
> socket? We really need to know where this information is getting leaked to.

We certainly can.  What would you like to see in that event?

-Eric

Re: [PATCH 0/6][v2] audit: implement multicast socket for journald

[Daniel J Walsh]

Here are the capabilities we currently give to sysadm_t with
sysadm_secadm    1.0.0    Disabled

   allow sysadm_t sysadm_t : capability { chown dac_override
dac_read_search fowner fsetid kill setgid setuid setpcap linux_immutable
net_bind_service net_broadcast net_admin net_raw ipc_lock ipc_owner
sys_rawio sys_chroot sys_ptrace sys_pacct sys_admin sys_boot sys_nice
sys_resource sys_time sys_tty_config mknod lease audit_write setfcap } ;
   allow sysadm_t sysadm_t : capability { setgid setuid sys_chroot }

   allow sysadm_t sysadm_t : capability2 { syslog block_suspend } ;

cap_audit_write might be a problem?

On 04/22/2014 11:57 PM, Eric Paris wrote:
> On Tue, 2014-04-22 at 22:25 -0400, Steve Grubb wrote:
>> On Tuesday, April 22, 2014 09:31:52 PM Richard Guy Briggs wrote:
>>> This is a patch set Eric Paris and I have been working on to add a
>>> restricted capability read-only netlink multicast socket to kernel audit to
>>> enable userspace clients such as systemd/journald to receive audit logs, in
>>> addition to the bidirectional auditd userspace client.
>> Do have the ability to separate of secadm_r and sysadm_r? By allowing this, we 
>> will leak to a sysadmin that he is being audited by the security officer. In a 
>> lot of cases, they are one in the same person. But for others, they are not. I 
>> have a feeling this will cause problems for MLS systems.
> Why?  This requires CAP_AUDIT_READ.  Just don't give CAP_AUDIT_READ to
> places you don't want to have read permission.  Exactly the same as you
> don't give CAP_AUDIT_CONTROL to sysadm_r.  (If we are giving
> CAP_AUDIT_CONTROL to sysadm_r and you think that any file protections
> on /var/log/audit/audit.log are adequate we are fooling ourselves!)
>
>> Also, shouldn't we have an audit event for every attempt to connect to this 
>> socket? We really need to know where this information is getting leaked to.
> We certainly can.  What would you like to see in that event?
>
> -Eric
>
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
>
>

Re: [PATCH 0/6][v2] audit: implement multicast socket for journald

[Eric Paris]

On Wed, 2014-04-23 at 09:40 -0400, Daniel J Walsh wrote:
> Here are the capabilities we currently give to sysadm_t with
> sysadm_secadm    1.0.0    Disabled
> 
>    allow sysadm_t sysadm_t : capability { chown dac_override
> dac_read_search fowner fsetid kill setgid setuid setpcap linux_immutable
> net_bind_service net_broadcast net_admin net_raw ipc_lock ipc_owner
> sys_rawio sys_chroot sys_ptrace sys_pacct sys_admin sys_boot sys_nice
> sys_resource sys_time sys_tty_config mknod lease audit_write setfcap } ;
>    allow sysadm_t sysadm_t : capability { setgid setuid sys_chroot }
> 
>    allow sysadm_t sysadm_t : capability2 { syslog block_suspend } ;
> 
> cap_audit_write might be a problem?

cap_audit_write is fine.

syslogd_t (aka journal) is going to need the new permission
cap_audit_read.  Also, as steve pointed out, someone may be likely to
want to be able to disable that permission easily.

-Eric

Re: [PATCH 0/6][v2] audit: implement multicast socket for journald

[Daniel J Walsh]

I guess the problem would be that the sysadm_t would be able to look at
the journal which would now contain the audit content.

On 04/23/2014 10:42 AM, Eric Paris wrote:
> On Wed, 2014-04-23 at 09:40 -0400, Daniel J Walsh wrote:
>> Here are the capabilities we currently give to sysadm_t with
>> sysadm_secadm    1.0.0    Disabled
>>
>>    allow sysadm_t sysadm_t : capability { chown dac_override
>> dac_read_search fowner fsetid kill setgid setuid setpcap linux_immutable
>> net_bind_service net_broadcast net_admin net_raw ipc_lock ipc_owner
>> sys_rawio sys_chroot sys_ptrace sys_pacct sys_admin sys_boot sys_nice
>> sys_resource sys_time sys_tty_config mknod lease audit_write setfcap } ;
>>    allow sysadm_t sysadm_t : capability { setgid setuid sys_chroot }
>>
>>    allow sysadm_t sysadm_t : capability2 { syslog block_suspend } ;
>>
>> cap_audit_write might be a problem?
> cap_audit_write is fine.
>
> syslogd_t (aka journal) is going to need the new permission
> cap_audit_read.  Also, as steve pointed out, someone may be likely to
> want to be able to disable that permission easily.
>
> -Eric
>

Re: [PATCH 0/6][v2] audit: implement multicast socket for journald

[Eric Paris]

On Wed, 2014-04-23 at 11:36 -0400, Daniel J Walsh wrote:
> I guess the problem would be that the sysadm_t would be able to look at
> the journal which would now contain the audit content.

right.  so include it in the sysadm_secadm bool

> 
> On 04/23/2014 10:42 AM, Eric Paris wrote:
> > On Wed, 2014-04-23 at 09:40 -0400, Daniel J Walsh wrote:
> >> Here are the capabilities we currently give to sysadm_t with
> >> sysadm_secadm    1.0.0    Disabled
> >>
> >>    allow sysadm_t sysadm_t : capability { chown dac_override
> >> dac_read_search fowner fsetid kill setgid setuid setpcap linux_immutable
> >> net_bind_service net_broadcast net_admin net_raw ipc_lock ipc_owner
> >> sys_rawio sys_chroot sys_ptrace sys_pacct sys_admin sys_boot sys_nice
> >> sys_resource sys_time sys_tty_config mknod lease audit_write setfcap } ;
> >>    allow sysadm_t sysadm_t : capability { setgid setuid sys_chroot }
> >>
> >>    allow sysadm_t sysadm_t : capability2 { syslog block_suspend } ;
> >>
> >> cap_audit_write might be a problem?
> > cap_audit_write is fine.
> >
> > syslogd_t (aka journal) is going to need the new permission
> > cap_audit_read.  Also, as steve pointed out, someone may be likely to
> > want to be able to disable that permission easily.
> >
> > -Eric
> >
> 

Re: [PATCH 0/6][v2] audit: implement multicast socket for journald

[Daniel J Walsh]

Meaning looking at the journal would be equivalent to looking at
/var/log/audit/audit.log.


On 04/23/2014 11:37 AM, Eric Paris wrote:
> On Wed, 2014-04-23 at 11:36 -0400, Daniel J Walsh wrote:
>> I guess the problem would be that the sysadm_t would be able to look at
>> the journal which would now contain the audit content.
> right.  so include it in the sysadm_secadm bool
>
>> On 04/23/2014 10:42 AM, Eric Paris wrote:
>>> On Wed, 2014-04-23 at 09:40 -0400, Daniel J Walsh wrote:
>>>> Here are the capabilities we currently give to sysadm_t with
>>>> sysadm_secadm    1.0.0    Disabled
>>>>
>>>>    allow sysadm_t sysadm_t : capability { chown dac_override
>>>> dac_read_search fowner fsetid kill setgid setuid setpcap linux_immutable
>>>> net_bind_service net_broadcast net_admin net_raw ipc_lock ipc_owner
>>>> sys_rawio sys_chroot sys_ptrace sys_pacct sys_admin sys_boot sys_nice
>>>> sys_resource sys_time sys_tty_config mknod lease audit_write setfcap } ;
>>>>    allow sysadm_t sysadm_t : capability { setgid setuid sys_chroot }
>>>>
>>>>    allow sysadm_t sysadm_t : capability2 { syslog block_suspend } ;
>>>>
>>>> cap_audit_write might be a problem?
>>> cap_audit_write is fine.
>>>
>>> syslogd_t (aka journal) is going to need the new permission
>>> cap_audit_read.  Also, as steve pointed out, someone may be likely to
>>> want to be able to disable that permission easily.
>>>
>>> -Eric
>>>
>
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
>
>

Re: [PATCH 0/6][v2] audit: implement multicast socket for journald

[Eric Paris]

They would be equivalent if and only if journald had CAP_AUDIT_READ.

I suggest you take CAP_AUDIT_READ away from journald on systems which
need the secadm/sysadmin split (which is a ridiculously stupid split
anyway, but who am I to complain?)

On Wed, Apr 23, 2014 at 11:52 AM, Daniel J Walsh <dwalsh@redhat.com> wrote:
> Meaning looking at the journal would be equivalent to looking at
> /var/log/audit/audit.log.
>
>
> On 04/23/2014 11:37 AM, Eric Paris wrote:
>> On Wed, 2014-04-23 at 11:36 -0400, Daniel J Walsh wrote:
>>> I guess the problem would be that the sysadm_t would be able to look at
>>> the journal which would now contain the audit content.
>> right.  so include it in the sysadm_secadm bool
>>
>>> On 04/23/2014 10:42 AM, Eric Paris wrote:
>>>> On Wed, 2014-04-23 at 09:40 -0400, Daniel J Walsh wrote:
>>>>> Here are the capabilities we currently give to sysadm_t with
>>>>> sysadm_secadm    1.0.0    Disabled
>>>>>
>>>>>    allow sysadm_t sysadm_t : capability { chown dac_override
>>>>> dac_read_search fowner fsetid kill setgid setuid setpcap linux_immutable
>>>>> net_bind_service net_broadcast net_admin net_raw ipc_lock ipc_owner
>>>>> sys_rawio sys_chroot sys_ptrace sys_pacct sys_admin sys_boot sys_nice
>>>>> sys_resource sys_time sys_tty_config mknod lease audit_write setfcap } ;
>>>>>    allow sysadm_t sysadm_t : capability { setgid setuid sys_chroot }
>>>>>
>>>>>    allow sysadm_t sysadm_t : capability2 { syslog block_suspend } ;
>>>>>
>>>>> cap_audit_write might be a problem?
>>>> cap_audit_write is fine.
>>>>
>>>> syslogd_t (aka journal) is going to need the new permission
>>>> cap_audit_read.  Also, as steve pointed out, someone may be likely to
>>>> want to be able to disable that permission easily.
>>>>
>>>> -Eric
>>>>
>>
>> _______________________________________________
>> Selinux mailing list
>> Selinux@tycho.nsa.gov
>> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
>> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
>>
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 0/6][v2] audit: implement multicast socket for journald

[Daniel J Walsh]

I don't disagree.  I would think the real solution to this would be to
not allow sysadm_t to get to SystemHigh, where all of the logging data
will be stored.

On 04/24/2014 09:22 AM, Eric Paris wrote:
> They would be equivalent if and only if journald had CAP_AUDIT_READ.
>
> I suggest you take CAP_AUDIT_READ away from journald on systems which
> need the secadm/sysadmin split (which is a ridiculously stupid split
> anyway, but who am I to complain?)
>
> On Wed, Apr 23, 2014 at 11:52 AM, Daniel J Walsh <dwalsh@redhat.com> wrote:
>> Meaning looking at the journal would be equivalent to looking at
>> /var/log/audit/audit.log.
>>
>>
>> On 04/23/2014 11:37 AM, Eric Paris wrote:
>>> On Wed, 2014-04-23 at 11:36 -0400, Daniel J Walsh wrote:
>>>> I guess the problem would be that the sysadm_t would be able to look at
>>>> the journal which would now contain the audit content.
>>> right.  so include it in the sysadm_secadm bool
>>>
>>>> On 04/23/2014 10:42 AM, Eric Paris wrote:
>>>>> On Wed, 2014-04-23 at 09:40 -0400, Daniel J Walsh wrote:
>>>>>> Here are the capabilities we currently give to sysadm_t with
>>>>>> sysadm_secadm    1.0.0    Disabled
>>>>>>
>>>>>>    allow sysadm_t sysadm_t : capability { chown dac_override
>>>>>> dac_read_search fowner fsetid kill setgid setuid setpcap linux_immutable
>>>>>> net_bind_service net_broadcast net_admin net_raw ipc_lock ipc_owner
>>>>>> sys_rawio sys_chroot sys_ptrace sys_pacct sys_admin sys_boot sys_nice
>>>>>> sys_resource sys_time sys_tty_config mknod lease audit_write setfcap } ;
>>>>>>    allow sysadm_t sysadm_t : capability { setgid setuid sys_chroot }
>>>>>>
>>>>>>    allow sysadm_t sysadm_t : capability2 { syslog block_suspend } ;
>>>>>>
>>>>>> cap_audit_write might be a problem?
>>>>> cap_audit_write is fine.
>>>>>
>>>>> syslogd_t (aka journal) is going to need the new permission
>>>>> cap_audit_read.  Also, as steve pointed out, someone may be likely to
>>>>> want to be able to disable that permission easily.
>>>>>
>>>>> -Eric
>>>>>
>>> _______________________________________________
>>> Selinux mailing list
>>> Selinux@tycho.nsa.gov
>>> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
>>> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
>>>
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
>
>

Re: [PATCH 0/6][v2] audit: implement multicast socket for journald

[Eric Paris]

On Thu, 2014-04-24 at 10:59 -0400, Daniel J Walsh wrote:
> I don't disagree.  I would think the real solution to this would be to
> not allow sysadm_t to get to SystemHigh, where all of the logging data
> will be stored.

make journalctl a userspace object manager and do selinux checks on if
it can see individual records?  so secadm_t running journalctl would see
them and sysadm running journalctl wouldn't see them?

Sounds elegant.  Who is going to code it?  *NOT IT!*

> 
> On 04/24/2014 09:22 AM, Eric Paris wrote:
> > They would be equivalent if and only if journald had CAP_AUDIT_READ.
> >
> > I suggest you take CAP_AUDIT_READ away from journald on systems which
> > need the secadm/sysadmin split (which is a ridiculously stupid split
> > anyway, but who am I to complain?)
> >
> > On Wed, Apr 23, 2014 at 11:52 AM, Daniel J Walsh <dwalsh@redhat.com> wrote:
> >> Meaning looking at the journal would be equivalent to looking at
> >> /var/log/audit/audit.log.
> >>
> >>
> >> On 04/23/2014 11:37 AM, Eric Paris wrote:
> >>> On Wed, 2014-04-23 at 11:36 -0400, Daniel J Walsh wrote:
> >>>> I guess the problem would be that the sysadm_t would be able to look at
> >>>> the journal which would now contain the audit content.
> >>> right.  so include it in the sysadm_secadm bool
> >>>
> >>>> On 04/23/2014 10:42 AM, Eric Paris wrote:
> >>>>> On Wed, 2014-04-23 at 09:40 -0400, Daniel J Walsh wrote:
> >>>>>> Here are the capabilities we currently give to sysadm_t with
> >>>>>> sysadm_secadm    1.0.0    Disabled
> >>>>>>
> >>>>>>    allow sysadm_t sysadm_t : capability { chown dac_override
> >>>>>> dac_read_search fowner fsetid kill setgid setuid setpcap linux_immutable
> >>>>>> net_bind_service net_broadcast net_admin net_raw ipc_lock ipc_owner
> >>>>>> sys_rawio sys_chroot sys_ptrace sys_pacct sys_admin sys_boot sys_nice
> >>>>>> sys_resource sys_time sys_tty_config mknod lease audit_write setfcap } ;
> >>>>>>    allow sysadm_t sysadm_t : capability { setgid setuid sys_chroot }
> >>>>>>
> >>>>>>    allow sysadm_t sysadm_t : capability2 { syslog block_suspend } ;
> >>>>>>
> >>>>>> cap_audit_write might be a problem?
> >>>>> cap_audit_write is fine.
> >>>>>
> >>>>> syslogd_t (aka journal) is going to need the new permission
> >>>>> cap_audit_read.  Also, as steve pointed out, someone may be likely to
> >>>>> want to be able to disable that permission easily.
> >>>>>
> >>>>> -Eric
> >>>>>
> >>> _______________________________________________
> >>> Selinux mailing list
> >>> Selinux@tycho.nsa.gov
> >>> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> >>> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
> >>>
> >>>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >> Please read the FAQ at  http://www.tux.org/lkml/
> > _______________________________________________
> > Selinux mailing list
> > Selinux@tycho.nsa.gov
> > To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> > To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
> >
> >
> 

Re: [PATCH 0/6][v2] audit: implement multicast socket for journald

[Daniel J Walsh]

Yes that would be the long term fix.  But it would involve journal
labelling individual data records.  IE Records from audit.log would be
audit_log_t, while messages from syslog would be var_log_t,  Or some
other kind of crazyness.


On 04/24/2014 11:03 AM, Eric Paris wrote:
> On Thu, 2014-04-24 at 10:59 -0400, Daniel J Walsh wrote:
>> I don't disagree.  I would think the real solution to this would be to
>> not allow sysadm_t to get to SystemHigh, where all of the logging data
>> will be stored.
> make journalctl a userspace object manager and do selinux checks on if
> it can see individual records?  so secadm_t running journalctl would see
> them and sysadm running journalctl wouldn't see them?
>
> Sounds elegant.  Who is going to code it?  *NOT IT!*
>
>> On 04/24/2014 09:22 AM, Eric Paris wrote:
>>> They would be equivalent if and only if journald had CAP_AUDIT_READ.
>>>
>>> I suggest you take CAP_AUDIT_READ away from journald on systems which
>>> need the secadm/sysadmin split (which is a ridiculously stupid split
>>> anyway, but who am I to complain?)
>>>
>>> On Wed, Apr 23, 2014 at 11:52 AM, Daniel J Walsh <dwalsh@redhat.com> wrote:
>>>> Meaning looking at the journal would be equivalent to looking at
>>>> /var/log/audit/audit.log.
>>>>
>>>>
>>>> On 04/23/2014 11:37 AM, Eric Paris wrote:
>>>>> On Wed, 2014-04-23 at 11:36 -0400, Daniel J Walsh wrote:
>>>>>> I guess the problem would be that the sysadm_t would be able to look at
>>>>>> the journal which would now contain the audit content.
>>>>> right.  so include it in the sysadm_secadm bool
>>>>>
>>>>>> On 04/23/2014 10:42 AM, Eric Paris wrote:
>>>>>>> On Wed, 2014-04-23 at 09:40 -0400, Daniel J Walsh wrote:
>>>>>>>> Here are the capabilities we currently give to sysadm_t with
>>>>>>>> sysadm_secadm    1.0.0    Disabled
>>>>>>>>
>>>>>>>>    allow sysadm_t sysadm_t : capability { chown dac_override
>>>>>>>> dac_read_search fowner fsetid kill setgid setuid setpcap linux_immutable
>>>>>>>> net_bind_service net_broadcast net_admin net_raw ipc_lock ipc_owner
>>>>>>>> sys_rawio sys_chroot sys_ptrace sys_pacct sys_admin sys_boot sys_nice
>>>>>>>> sys_resource sys_time sys_tty_config mknod lease audit_write setfcap } ;
>>>>>>>>    allow sysadm_t sysadm_t : capability { setgid setuid sys_chroot }
>>>>>>>>
>>>>>>>>    allow sysadm_t sysadm_t : capability2 { syslog block_suspend } ;
>>>>>>>>
>>>>>>>> cap_audit_write might be a problem?
>>>>>>> cap_audit_write is fine.
>>>>>>>
>>>>>>> syslogd_t (aka journal) is going to need the new permission
>>>>>>> cap_audit_read.  Also, as steve pointed out, someone may be likely to
>>>>>>> want to be able to disable that permission easily.
>>>>>>>
>>>>>>> -Eric
>>>>>>>
>>>>> _______________________________________________
>>>>> Selinux mailing list
>>>>> Selinux@tycho.nsa.gov
>>>>> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
>>>>> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
>>>>>
>>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>> Please read the FAQ at  http://www.tux.org/lkml/
>>> _______________________________________________
>>> Selinux mailing list
>>> Selinux@tycho.nsa.gov
>>> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
>>> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
>>>
>>>
>
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
>
>

[PATCH 3/6] netlink: implement unbind to netlink_setsockopt NETLINK_DROP_MEMBERSHIP

[Richard Guy Briggs]

Call the per-protocol unbind function rather than bind function on
NETLINK_DROP_MEMBERSHIP in netlink_setsockopt().

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 net/netlink/af_netlink.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 3f43e5a..cdb236f 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -2118,7 +2118,7 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
 			return err;
 		if (!val || val - 1 >= nlk->ngroups)
 			return -EINVAL;
-		if (nlk->netlink_bind) {
+		if (optname == NETLINK_ADD_MEMBERSHIP && nlk->netlink_bind) {
 			err = nlk->netlink_bind(val);
 			if (err)
 				return err;
@@ -2127,6 +2127,8 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
 		netlink_update_socket_mc(nlk, val,
 					 optname == NETLINK_ADD_MEMBERSHIP);
 		netlink_table_ungrab();
+		if (optname == NETLINK_DROP_MEMBERSHIP && nlk->netlink_unbind)
+			nlk->netlink_unbind(val);
 
 		err = 0;
 		break;
-- 
1.7.1

[PATCH 4/6] audit: add netlink audit protocol bind to check capabilities on multicast join

[Richard Guy Briggs]

Register a netlink per-protocol bind fuction for audit to check userspace
process capabilities before allowing a multicast group connection.

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 include/uapi/linux/capability.h     |    7 ++++++-
 kernel/audit.c                      |   10 ++++++++++
 security/selinux/include/classmap.h |    2 +-
 3 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
index 154dd6d..12c37a1 100644
--- a/include/uapi/linux/capability.h
+++ b/include/uapi/linux/capability.h
@@ -347,7 +347,12 @@ struct vfs_cap_data {
 
 #define CAP_BLOCK_SUSPEND    36
 
-#define CAP_LAST_CAP         CAP_BLOCK_SUSPEND
+/* Allow reading the audit log via multicast netlink socket */
+
+#define CAP_AUDIT_READ		37
+
+
+#define CAP_LAST_CAP         CAP_AUDIT_READ
 
 #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
 
diff --git a/kernel/audit.c b/kernel/audit.c
index 7c28936..223cb74 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1076,10 +1076,20 @@ static void audit_receive(struct sk_buff  *skb)
 	mutex_unlock(&audit_cmd_mutex);
 }
 
+/* Run custom bind function on netlink socket group connect or bind requests. */
+static int audit_bind(int group)
+{
+	if (!capable(CAP_AUDIT_READ))
+		return -EPERM;
+
+	return 0;
+}
+
 static int __net_init audit_net_init(struct net *net)
 {
 	struct netlink_kernel_cfg cfg = {
 		.input	= audit_receive,
+		.bind	= audit_bind,
 	};
 
 	struct audit_net *aunet = net_generic(net, audit_net_id);
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index 14d04e6..be491a7 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -147,7 +147,7 @@ struct security_class_mapping secclass_map[] = {
 	{ "peer", { "recv", NULL } },
 	{ "capability2",
 	  { "mac_override", "mac_admin", "syslog", "wake_alarm", "block_suspend",
-	    NULL } },
+	    "audit_read", NULL } },
 	{ "kernel_service", { "use_as_override", "create_files_as", NULL } },
 	{ "tun_socket",
 	  { COMMON_SOCK_PERMS, "attach_queue", NULL } },
-- 
1.7.1

[PATCH 5/6] audit: add netlink multicast group for log read

[Richard Guy Briggs]

Add a netlink multicast socket with one group to kaudit for "best-effort"
delivery to read-only userspace clients such as systemd, in addition to the
existing bidirectional unicast auditd userspace client.

Currently, auditd is intended to use the CAP_AUDIT_CONTROL and CAP_AUDIT_WRITE
capabilities, but actually uses CAP_NET_ADMIN.  The CAP_AUDIT_READ capability
is added for use by read-only AUDIT_NLGRP_READLOG netlink multicast group
clients to the kaudit subsystem.

This will safely give access to services such as systemd to consume audit logs
while ensuring write access remains restricted for integrity.

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 include/uapi/linux/audit.h |    8 +++++++
 kernel/audit.c             |   51 ++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 55 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 11917f7..dfa4c86 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -373,6 +373,14 @@ enum {
  */
 #define AUDIT_MESSAGE_TEXT_MAX	8560
 
+/* Multicast Netlink socket groups (default up to 32) */
+enum audit_nlgrps {
+	AUDIT_NLGRP_NONE,	/* Group 0 not used */
+	AUDIT_NLGRP_READLOG,	/* "best effort" read only socket */
+	__AUDIT_NLGRP_MAX
+};
+#define AUDIT_NLGRP_MAX                (__AUDIT_NLGRP_MAX - 1)
+
 struct audit_status {
 	__u32		mask;		/* Bit mask for valid entries */
 	__u32		enabled;	/* 1 = enabled, 0 = disabled */
diff --git a/kernel/audit.c b/kernel/audit.c
index 223cb74..d272cc1 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -424,6 +424,35 @@ static void kauditd_send_skb(struct sk_buff *skb)
 }
 
 /*
+ * kauditd_send_multicast_skb - send the skb to multicast userspace listeners
+ *
+ * This function doesn't consume an skb as might be expected since it has to
+ * copy it anyways.
+ */
+static void kauditd_send_multicast_skb(struct sk_buff *skb)
+{
+	struct sk_buff		*copy;
+	struct audit_net	*aunet = net_generic(&init_net, audit_net_id);
+	struct sock		*sock = aunet->nlsk;
+
+	/*
+	 * The seemingly wasteful skb_copy() rather than bumping the refcount
+	 * using skb_get() is necessary because non-standard mods are made to
+	 * the skb by the original kaudit unicast socket send routine.  The
+	 * existing auditd daemon assumes this breakage.  Fixing this would
+	 * require co-ordinating a change in the established protocol between
+	 * the kaudit kernel subsystem and the auditd userspace code.  There is
+	 * no reason for new multicast clients to continue with this
+	 * non-compliance.
+	 */
+	copy = skb_copy(skb, GFP_KERNEL);
+	if (!copy)
+		return;
+
+	nlmsg_multicast(sock, copy, 0, AUDIT_NLGRP_READLOG, GFP_KERNEL);
+}
+
+/*
  * flush_hold_queue - empty the hold queue if auditd appears
  *
  * If auditd just started, drain the queue of messages already
@@ -1090,6 +1119,8 @@ static int __net_init audit_net_init(struct net *net)
 	struct netlink_kernel_cfg cfg = {
 		.input	= audit_receive,
 		.bind	= audit_bind,
+		.flags	= NL_CFG_F_NONROOT_RECV,
+		.groups	= AUDIT_NLGRP_MAX,
 	};
 
 	struct audit_net *aunet = net_generic(net, audit_net_id);
@@ -1911,10 +1942,10 @@ out:
  * audit_log_end - end one audit record
  * @ab: the audit_buffer
  *
- * The netlink_* functions cannot be called inside an irq context, so
- * the audit buffer is placed on a queue and a tasklet is scheduled to
- * remove them from the queue outside the irq context.  May be called in
- * any context.
+ * netlink_unicast() cannot be called inside an irq context because it blocks
+ * (last arg, flags, is not set to MSG_DONTWAIT), so the audit buffer is placed
+ * on a queue and a tasklet is scheduled to remove them from the queue outside
+ * the irq context.  May be called in any context.
  */
 void audit_log_end(struct audit_buffer *ab)
 {
@@ -1924,6 +1955,18 @@ void audit_log_end(struct audit_buffer *ab)
 		audit_log_lost("rate limit exceeded");
 	} else {
 		struct nlmsghdr *nlh = nlmsg_hdr(ab->skb);
+
+		kauditd_send_multicast_skb(ab->skb);
+
+		/*
+		 * The original kaudit unicast socket sends up messages with
+		 * nlmsg_len set to the payload length rather than the entire
+		 * message length.  This breaks the standard set by netlink.
+		 * The existing auditd daemon assumes this breakage.  Fixing
+		 * this would require co-ordinating a change in the established
+		 * protocol between the kaudit kernel subsystem and the auditd
+		 * userspace code.
+		 */
 		nlh->nlmsg_len = ab->skb->len - NLMSG_HDRLEN;
 
 		if (audit_pid) {
-- 
1.7.1

[PATCH 6/6] audit: send multicast messages only if there are listeners

[Richard Guy Briggs]

Test first to see if there are any userspace multicast listeners bound to the
socket before starting the multicast send work.

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 kernel/audit.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/kernel/audit.c b/kernel/audit.c
index d272cc1..33531d7 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -435,6 +435,9 @@ static void kauditd_send_multicast_skb(struct sk_buff *skb)
 	struct audit_net	*aunet = net_generic(&init_net, audit_net_id);
 	struct sock		*sock = aunet->nlsk;
 
+	if (!netlink_has_listeners(sock, AUDIT_NLGRP_READLOG))
+		return;
+
 	/*
 	 * The seemingly wasteful skb_copy() rather than bumping the refcount
 	 * using skb_get() is necessary because non-standard mods are made to
-- 
1.7.1