| Message ID | 20200220144213.860206-1-nikolay@cumulusnetworks.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] net: netlink: cap max groups which will be considered in netlink_bind() | expand |
From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> Date: Thu, 20 Feb 2020 16:42:13 +0200 > Since nl_groups is a u32 we can't bind more groups via ->bind > (netlink_bind) call, but netlink has supported more groups via > setsockopt() for a long time and thus nlk->ngroups could be over 32. > Recently I added support for per-vlan notifications and increased the > groups to 33 for NETLINK_ROUTE which exposed an old bug in the > netlink_bind() code causing out-of-bounds access on archs where unsigned > long is 32 bits via test_bit() on a local variable. Fix this by capping the > maximum groups in netlink_bind() to BITS_PER_TYPE(u32), effectively > capping them at 32 which is the minimum of allocated groups and the > maximum groups which can be bound via netlink_bind(). > > CC: Christophe Leroy <christophe.leroy@c-s.fr> > CC: Richard Guy Briggs <rgb@redhat.com> > Fixes: 4f520900522f ("netlink: have netlink per-protocol bind function return an error code.") > Reported-by: Erhard F. <erhard_f@mailbox.org> > Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> Applied. > Dave it is not necessary to queue this fix for stable releases since > NETLINK_ROUTE is the first to reach more groups after I added the vlan > notification changes and I don't think we'll ever backport new groups. :) > Up to you of course. > > In fact looking at netlink_kernel_create nlk->groups can't be less than 32 > so we can add a NETLINK_MIN_GROUPS == NETLINK_MAX_LEGACY_BIND_GRPS == 32 > in net-next to replace the raw value. Ok, thanks for letting me know. Let's skip the -stable backport.
On 2/20/20 5:02 PM, David Miller wrote: > >> Dave it is not necessary to queue this fix for stable releases since >> NETLINK_ROUTE is the first to reach more groups after I added the vlan >> notification changes and I don't think we'll ever backport new groups. :) >> Up to you of course. RTNLGRP_NEXTHOP was the first to overflow; RTNLGRP_NEXTHOP = 32. Apparently my comment about overflowing the groups was only in the iproute2 commit (e7cd93e7afe1a0407ccb94b9124bbd56b87b8660)
On 21/02/2020 04:55, David Ahern wrote: > On 2/20/20 5:02 PM, David Miller wrote: >> >>> Dave it is not necessary to queue this fix for stable releases since >>> NETLINK_ROUTE is the first to reach more groups after I added the vlan >>> notification changes and I don't think we'll ever backport new groups. :) >>> Up to you of course. > > RTNLGRP_NEXTHOP was the first to overflow; RTNLGRP_NEXTHOP = 32. > > Apparently my comment about overflowing the groups was only in the > iproute2 commit (e7cd93e7afe1a0407ccb94b9124bbd56b87b8660) > While that is true, RTNLGRP_MAX == (__RTNLGRP_MAX - 1) == RTNLGRP_NEXTHOP == 32 which is fine. You just can't bind to the group via netlink_bind() but the loop is capped at 32 again and the test_bit() shouldn't access anything beyond because it's a strict less than.
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c index 4e31721e7293..edf3e285e242 100644 --- a/net/netlink/af_netlink.c +++ b/net/netlink/af_netlink.c @@ -1014,7 +1014,8 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr, if (nlk->netlink_bind && groups) { int group; - for (group = 0; group < nlk->ngroups; group++) { + /* nl_groups is a u32, so cap the maximum groups we can bind */ + for (group = 0; group < BITS_PER_TYPE(u32); group++) { if (!test_bit(group, &groups)) continue; err = nlk->netlink_bind(net, group + 1); @@ -1033,7 +1034,7 @@ static int netlink_bind(struct socket *sock, struct sockaddr *addr, netlink_insert(sk, nladdr->nl_pid) : netlink_autobind(sock); if (err) { - netlink_undo_bind(nlk->ngroups, groups, sk); + netlink_undo_bind(BITS_PER_TYPE(u32), groups, sk); goto unlock; } }
Since nl_groups is a u32 we can't bind more groups via ->bind (netlink_bind) call, but netlink has supported more groups via setsockopt() for a long time and thus nlk->ngroups could be over 32. Recently I added support for per-vlan notifications and increased the groups to 33 for NETLINK_ROUTE which exposed an old bug in the netlink_bind() code causing out-of-bounds access on archs where unsigned long is 32 bits via test_bit() on a local variable. Fix this by capping the maximum groups in netlink_bind() to BITS_PER_TYPE(u32), effectively capping them at 32 which is the minimum of allocated groups and the maximum groups which can be bound via netlink_bind(). CC: Christophe Leroy <christophe.leroy@c-s.fr> CC: Richard Guy Briggs <rgb@redhat.com> Fixes: 4f520900522f ("netlink: have netlink per-protocol bind function return an error code.") Reported-by: Erhard F. <erhard_f@mailbox.org> Signed-off-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com> --- Dave it is not necessary to queue this fix for stable releases since NETLINK_ROUTE is the first to reach more groups after I added the vlan notification changes and I don't think we'll ever backport new groups. :) Up to you of course. In fact looking at netlink_kernel_create nlk->groups can't be less than 32 so we can add a NETLINK_MIN_GROUPS == NETLINK_MAX_LEGACY_BIND_GRPS == 32 in net-next to replace the raw value. net/netlink/af_netlink.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)