[PATCH net v2 0/2] genetlink: fix joining multicast groups no family owns

[PATCH net v2 0/2] genetlink: fix joining multicast groups no family owns

[Anil Kaushik]

genl_bind() checks the GENL_MCAST_CAP_* flags of a group only when a
family owns it. A socket can join a free group ID and stay a member when
a family with those flags gets the ID later.

Patch 1 rejects joins to group IDs that no family owns. Patch 2 clears
the members of the group IDs when a family is given them. This covers
the race Sashiko found on v1, where the family is unregistered between
genl_bind() and the point where the membership bit is set.

One case is left. If the family is unregistered and a family with
GENL_MCAST_CAP_* flags gets the ID, both inside that window, the bit is
set after the clear. Closing it needs the check and the update under one
lock in af_netlink.c. I will send that separately.

Tested as an unprivileged user, with an msleep() added after the bind
callback in netlink_setsockopt() and a test family that registers again
with GENL_MCAST_CAP_NET_ADMIN:

                                         v1      v2
  unregister in the window,
  register later                         member  not member
  unregister and register in the window  member  member

The test from v1 (join all free IDs, then load gtp and drop_monitor)
gives the same result as before, and patch 2 alone also fixes it.

Changes in v2:
- New patch 2 (Sashiko).
- Patch 1: no change, added Reviewed-by tags.

v1: https://lore.kernel.org/netdev/20261029062214.3407223-1-anilkaushikwireless@gmail.com/

Anil Kaushik (2):
  genetlink: Reject joining multicast groups no family owns
  genetlink: Clear stale members when assigning multicast group IDs

 net/netlink/genetlink.c | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

-- 
2.25.1

[PATCH net v2 1/2] genetlink: Reject joining multicast groups no family owns

[Anil Kaushik]

genl_bind() checks the GENL_MCAST_CAP_* flags of a group only when a
family owns it. For a group ID that no family owns, it returns 0, so any
user can join it.

A family registered later can get that ID, and the socket stays a member:
nothing checks it again. An unprivileged user can join the free IDs, load
a family module with CTRL_CMD_GETFAMILY, which calls request_module(),
and then receive the notifications of a group with
GENL_MCAST_CAP_NET_ADMIN or GENL_MCAST_CAP_SYS_ADMIN. For example the
drop_monitor "events" group always gets ID 1, and is free until the
drop_monitor module is loaded.

Fix by returning -ENOENT from genl_bind() when no family owns the group.
Nothing is ever sent to such a group, so joining it has no use.

Found by the Sashiko review of the gtp multicast group fix. Tested as an
unprivileged user who joins all free IDs, then loads gtp and
drop_monitor:

Before:
  joins: ok=62 EPERM=2
  gtp/gtp (ID 15): member
  NET_DM/events (ID 1): member

After:
  joins: ok=19 ENOENT=43 EPERM=2
  gtp/gtp (ID 15): not member
  NET_DM/events (ID 1): not member

Link: https://lore.kernel.org/netdev/179324692107.3134.14443689892607921337@kernel.org/
Fixes: e03781879a0d ("drop_monitor: Require 'CAP_SYS_ADMIN' when joining "events" group")
Reviewed-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
Reviewed-by: Ido Schimmel <idosch@nvidia.com>
Signed-off-by: Anil Kaushik <anilkaushikwireless@gmail.com>
---
 net/netlink/genetlink.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 5cc1037d4917..f038f04ef4ea 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -1825,7 +1825,7 @@ static int genl_bind(struct net *net, int group)
 {
 	const struct genl_family *family;
 	unsigned int id;
-	int ret = 0;
+	int ret = -ENOENT;
 
 	down_read(&cb_lock);
 
@@ -1841,6 +1841,7 @@ static int genl_bind(struct net *net, int group)
 			continue;
 
 		grp = &family->mcgrps[i];
+		ret = 0;
 		if ((grp->flags & GENL_MCAST_CAP_NET_ADMIN) &&
 		    !ns_capable(net->user_ns, CAP_NET_ADMIN))
 			ret = -EPERM;
-- 
2.25.1

[PATCH net v2 2/2] genetlink: Clear stale members when assigning multicast group IDs

[Anil Kaushik]

genl_bind() checks a group under cb_lock, but netlink_setsockopt() and
netlink_bind() set the membership bit only after it returns and the lock
is dropped. If the family is unregistered in between, its members are
cleared before this socket is one, and the socket stays a member of a
group ID that no family owns. A family registered later can get that ID
and its GENL_MCAST_CAP_* flags are never checked for the socket.

Clear the members of the group IDs when a family is given them, the
same way genl_unregister_mc_groups() does when they are freed. No socket
can be a valid member at that point: cb_lock is held for writing, so
genl_bind() cannot see the new family yet.

Link: https://lore.kernel.org/netdev/179340003139.3131.11274717768928642724@kernel.org/
Fixes: e03781879a0d ("drop_monitor: Require 'CAP_SYS_ADMIN' when joining "events" group")
Signed-off-by: Anil Kaushik <anilkaushikwireless@gmail.com>
---
 net/netlink/genetlink.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index f038f04ef4ea..0ddebd828585 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -452,6 +452,22 @@ static int genl_allocate_reserve_groups(int n_groups, int *first_id)
 
 static struct genl_family genl_ctrl;
 
+static void genl_clear_mc_users(const struct genl_family *family)
+{
+	struct net *net;
+	int i;
+
+	netlink_table_grab();
+	rcu_read_lock();
+	for_each_net_rcu(net) {
+		for (i = 0; i < family->n_mcgrps; i++)
+			__netlink_clear_multicast_users(net->genl_sock,
+							family->mcgrp_offset + i);
+	}
+	rcu_read_unlock();
+	netlink_table_ungrab();
+}
+
 static int genl_validate_assign_mc_groups(struct genl_family *family)
 {
 	int first_id;
@@ -527,23 +543,18 @@ static int genl_validate_assign_mc_groups(struct genl_family *family)
 			clear_bit(family->mcgrp_offset + i, mc_groups);
 	}
 
+	/* drop sockets that joined the IDs while no family owned them */
+	if (!err)
+		genl_clear_mc_users(family);
+
 	return err;
 }
 
 static void genl_unregister_mc_groups(const struct genl_family *family)
 {
-	struct net *net;
 	int i;
 
-	netlink_table_grab();
-	rcu_read_lock();
-	for_each_net_rcu(net) {
-		for (i = 0; i < family->n_mcgrps; i++)
-			__netlink_clear_multicast_users(
-				net->genl_sock, family->mcgrp_offset + i);
-	}
-	rcu_read_unlock();
-	netlink_table_ungrab();
+	genl_clear_mc_users(family);
 
 	for (i = 0; i < family->n_mcgrps; i++) {
 		int grp_id = family->mcgrp_offset + i;
-- 
2.25.1