[PATCH net] netlink: fix wrong subscription bitmask to group mapping in

[PATCH net] netlink: fix wrong subscription bitmask to group mapping in

[Pablo Neira Ayuso]

The subscription bitmask passed via struct sockaddr_nl is converted to
the group number when calling the netlink_bind() and netlink_unbind()
callbacks.

The conversion is however incorrect since bitmask (1 << 0) needs to be
mapped to group number 1. Note that you cannot specify the group number 0
(usually known as _NONE) from setsockopt() using NETLINK_ADD_MEMBERSHIP
since this is rejected through -EINVAL.

This problem became noticeable since 97840cb ("netfilter: nfnetlink:
fix insufficient validation in nfnetlink_bind") when binding to bitmask
(1 << 0) in ctnetlink.

Reported-by: Andre Tomt <andre@tomt.net>
Reported-by: Ivan Delalande <colona@arista.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
v2: Rebased upon current net tree. Previous patch:

http://patchwork.ozlabs.org/patch/426205/

did not apply cleanly.

 net/netlink/af_netlink.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 02fdde2..75532ef 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1438,7 +1438,7 @@ static void netlink_undo_bind(int group, long unsigned int groups,
 
 	for (undo = 0; undo < group; undo++)
 		if (test_bit(undo, &groups))
-			nlk->netlink_unbind(sock_net(sk), undo);
+			nlk->netlink_unbind(sock_net(sk), undo + 1);
 }
 
 static int netlink_bind(struct socket *sock, struct sockaddr *addr,
@@ -1476,7 +1476,7 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
 		for (group = 0; group < nlk->ngroups; group++) {
 			if (!test_bit(group, &groups))
 				continue;
-			err = nlk->netlink_bind(net, group);
+			err = nlk->netlink_bind(net, group + 1);
 			if (!err)
 				continue;
 			netlink_undo_bind(group, groups, sk);
-- 
1.7.10.4

Re: [PATCH net] netlink: fix wrong subscription bitmask to group mapping in

[Ivan Delalande]

On Thu, Jan 29, 2015 at 10:51:53AM +0100, Pablo Neira Ayuso wrote:
> The subscription bitmask passed via struct sockaddr_nl is converted to
> the group number when calling the netlink_bind() and netlink_unbind()
> callbacks.
> 
> The conversion is however incorrect since bitmask (1 << 0) needs to be
> mapped to group number 1. Note that you cannot specify the group number 0
> (usually known as _NONE) from setsockopt() using NETLINK_ADD_MEMBERSHIP
> since this is rejected through -EINVAL.
> 
> This problem became noticeable since 97840cb ("netfilter: nfnetlink:
> fix insufficient validation in nfnetlink_bind") when binding to bitmask
> (1 << 0) in ctnetlink.
> 
> Reported-by: Andre Tomt <andre@tomt.net>
> Reported-by: Ivan Delalande <colona@arista.com>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Thanks a lot for this fix!

> ---
> v2: Rebased upon current net tree. Previous patch:
> 
> http://patchwork.ozlabs.org/patch/426205/
> 
> did not apply cleanly.
> 
>  net/netlink/af_netlink.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index 02fdde2..75532ef 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -1438,7 +1438,7 @@ static void netlink_undo_bind(int group, long unsigned int groups,
>  
>  	for (undo = 0; undo < group; undo++)
>  		if (test_bit(undo, &groups))
> -			nlk->netlink_unbind(sock_net(sk), undo);
> +			nlk->netlink_unbind(sock_net(sk), undo + 1);
>  }
>  
>  static int netlink_bind(struct socket *sock, struct sockaddr *addr,
> @@ -1476,7 +1476,7 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
>  		for (group = 0; group < nlk->ngroups; group++) {
>  			if (!test_bit(group, &groups))
>  				continue;
> -			err = nlk->netlink_bind(net, group);
> +			err = nlk->netlink_bind(net, group + 1);
>  			if (!err)
>  				continue;
>  			netlink_undo_bind(group, groups, sk);

I guess this should also be group + 1 there:

	netlink_undo_bind(group + 1, groups, sk);

-- 
Ivan "Colona" Delalande
Arista Networks

Re: [PATCH net] netlink: fix wrong subscription bitmask to group mapping in

[Pablo Neira Ayuso]

On Thu, Jan 29, 2015 at 11:27:01PM +0100, Ivan Delalande wrote:
> On Thu, Jan 29, 2015 at 10:51:53AM +0100, Pablo Neira Ayuso wrote:
> > The subscription bitmask passed via struct sockaddr_nl is converted to
> > the group number when calling the netlink_bind() and netlink_unbind()
> > callbacks.
> >
> > The conversion is however incorrect since bitmask (1 << 0) needs to be
> > mapped to group number 1. Note that you cannot specify the group number 0
> > (usually known as _NONE) from setsockopt() using NETLINK_ADD_MEMBERSHIP
> > since this is rejected through -EINVAL.
> >
> > This problem became noticeable since 97840cb ("netfilter: nfnetlink:
> > fix insufficient validation in nfnetlink_bind") when binding to bitmask
> > (1 << 0) in ctnetlink.
> >
> > Reported-by: Andre Tomt <andre@tomt.net>
> > Reported-by: Ivan Delalande <colona@arista.com>
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
>
> Thanks a lot for this fix!
>
> > ---
> > v2: Rebased upon current net tree. Previous patch:
> >
> > http://patchwork.ozlabs.org/patch/426205/
> >
> > did not apply cleanly.
> >
> >  net/netlink/af_netlink.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> > index 02fdde2..75532ef 100644
> > --- a/net/netlink/af_netlink.c
> > +++ b/net/netlink/af_netlink.c
> > @@ -1438,7 +1438,7 @@ static void netlink_undo_bind(int group, long unsigned int groups,
> >
> >  	for (undo = 0; undo < group; undo++)
> >  		if (test_bit(undo, &groups))
> > -			nlk->netlink_unbind(sock_net(sk), undo);
> > +			nlk->netlink_unbind(sock_net(sk), undo + 1);
> >  }
> >
> >  static int netlink_bind(struct socket *sock, struct sockaddr *addr,
> > @@ -1476,7 +1476,7 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr,
> >  		for (group = 0; group < nlk->ngroups; group++) {
> >  			if (!test_bit(group, &groups))
> >  				continue;
> > -			err = nlk->netlink_bind(net, group);
> > +			err = nlk->netlink_bind(net, group + 1);
> >  			if (!err)
> >  				continue;
> >  			netlink_undo_bind(group, groups, sk);
>
> I guess this should also be group + 1 there:
>
> 	netlink_undo_bind(group + 1, groups, sk);

We don't need that change you suggest.

Asumming nlk->netlink_bind(...) fails when 'group' is 0, then we have
nothing to undo.

See netlink_undo_bind():

     for (undo = 0; undo < group; undo++)
             if (test_bit(undo, &groups))
                   nlk->netlink_unbind(sock_net(sk), undo + 1);

'undo' and 'group' will be both 0, so nothing is undone as expected.

So my patch seems good to me after second look. Let me know if you
have more concerns, thanks.

Re: [PATCH net] netlink: fix wrong subscription bitmask to group mapping in

[David Miller]

From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Thu, 29 Jan 2015 10:51:53 +0100

> The subscription bitmask passed via struct sockaddr_nl is converted to
> the group number when calling the netlink_bind() and netlink_unbind()
> callbacks.
> 
> The conversion is however incorrect since bitmask (1 << 0) needs to be
> mapped to group number 1. Note that you cannot specify the group number 0
> (usually known as _NONE) from setsockopt() using NETLINK_ADD_MEMBERSHIP
> since this is rejected through -EINVAL.
> 
> This problem became noticeable since 97840cb ("netfilter: nfnetlink:
> fix insufficient validation in nfnetlink_bind") when binding to bitmask
> (1 << 0) in ctnetlink.
> 
> Reported-by: Andre Tomt <andre@tomt.net>
> Reported-by: Ivan Delalande <colona@arista.com>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Applied, thanks Pablo.