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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 30 Jan 2015 20:26:37 +0100
From:	Pablo Neira Ayuso <pablo@...filter.org>
To:	Ivan Delalande <colona@...sta.com>
Cc:	netdev@...r.kernel.org, davem@...emloft.net, andre@...t.net
Subject: Re: [PATCH net] netlink: fix wrong subscription bitmask to group
 mapping in

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@...t.net>
> > Reported-by: Ivan Delalande <colona@...sta.com>
> > Signed-off-by: Pablo Neira Ayuso <pablo@...filter.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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html