| Message ID | 1422525113-5698-1-git-send-email-pablo@netfilter.org |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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);
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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(-)