[PATCH net] net: netlink: cap max groups which will be considered in netlink_bind()

[PATCH net] net: netlink: cap max groups which will be considered in netlink_bind()

[Nikolay Aleksandrov]

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(-)

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;
 		}
 	}
-- 
2.24.1

Re: [PATCH net] net: netlink: cap max groups which will be considered in netlink_bind()

[David Miller]

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.

Re: [PATCH net] net: netlink: cap max groups which will be considered in netlink_bind()

[David Ahern]

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)

Re: [PATCH net] net: netlink: cap max groups which will be considered in netlink_bind()

[Nikolay Aleksandrov]

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.