[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

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] genl_bind() checks ownership and GENL_MCAST_CAP_* under
  down_read(&cb_lock) and releases the lock before the caller adds the…

--

genetlink: Reject joining multicast groups no family owns

This commit makes genl_bind() return -ENOENT when no registered family
owns the requested multicast group ID. Before this, such joins succeeded.
The socket then stayed a member if a privileged family was later assigned
that ID.

Link: https://lore.kernel.org/netdev/179324692107.3134.14443689892607921337@kernel.org/

> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> index 5cc1037d4917e..f038f04ef4ead 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;

[Severity: Medium]
genl_bind() checks ownership and capabilities under down_read(&cb_lock),
but it drops cb_lock before the caller installs the membership. Can the
owner of the group change in between?

In netlink_setsockopt() the membership update is a separate critical
section:

net/netlink/af_netlink.c:netlink_setsockopt() {
    ...
		if (optname == NETLINK_ADD_MEMBERSHIP && nlk->netlink_bind) {
			err = nlk->netlink_bind(sock_net(sk), val);
			if (err)
				return err;
		}
		netlink_table_grab();
		netlink_update_socket_mc(nlk, val,
					 optname == NETLINK_ADD_MEMBERSHIP);
		netlink_table_ungrab();
    ...
}

The window in netlink_bind() is wider. netlink_lock_table() and the
insert/autobind step both run between the per-group genl_bind() calls
and the netlink_table_grab() that updates nlk->groups.

Consider this sequence:

CPU0
setsockopt(NETLINK_ADD_MEMBERSHIP, X)
  genl_bind()
    family A owns X with no GENL_MCAST_CAP_* flags, ret = 0
    up_read(&cb_lock)

CPU1
genl_unregister_family(A)
  genl_unregister_mc_groups(A)
    clears members of X (CPU0 is not a member yet)
    clear_bit(X, mc_groups)
  idr_remove(&genl_fam_idr, A->id)

CPU2
genl_register_family(B)   /* B has a GENL_MCAST_CAP_NET_ADMIN group */
  genl_allocate_reserve_groups()
    find_first_zero_bit() returns X

CPU0
netlink_table_grab()
netlink_update_socket_mc(nlk, X, 1)

Would the socket then belong to B's privileged group without passing a
capability check? Nothing seems to check that membership again later.

The follow-up commit "genetlink: Clear stale members when assigning
multicast group IDs" adds a genl_clear_mc_users() call to
genl_validate_assign_mc_groups(). That handles a stale membership that is
already installed when B registers. In the sequence above, though, the
clear runs before CPU0 installs the membership, so it finds no member.

The follow-up's commit message says:

  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

Does that reasoning miss a socket whose genl_bind() already passed for
the previous owner, with its membership update still pending?

To trigger this, A has to be unregistered inside the window, for example
by rmmod, or by a module init that registers a family, then fails and
unregisters it. B also has to get the freed ID. An unprivileged user can
trigger B's registration through CTRL_CMD_GETFAMILY and request_module().

Would serializing the check with the membership update close this
completely? That could mean holding cb_lock across both, or re-checking
under netlink_table_grab().

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261031054632.3697295-1-anilkaushikwireless%40gmail.com

[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

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.

Critical: 0 · High: 2 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The fix is incomplete: the check-then-set race the commit
  message describes can still be reached.
- [High] The patch puts the expensive multicast-clear walk on every
  family registration after boot, and unprivileged users can trigger…
- [Medium] The new registration-time genl_clear_mc_users() assumes nobody
  can legitimately be a member of the IDs being assigned.
- [Low] The Fixes: tag points at e03781879a0d ("drop_monitor: Require
  'CAP_SYS_ADMIN' when joining "events" group", Dec 2023), but the race…

--

genetlink: Clear stale members when assigning multicast group IDs

This patch moves the multicast member clearing loop out of
genl_unregister_mc_groups() into a new genl_clear_mc_users() helper. It
also calls the helper from genl_validate_assign_mc_groups() once a family
has been given its group IDs. The goal is to drop sockets that became
members of a group ID while no family owned it, so a later owner's
GENL_MCAST_CAP_* flags can't be bypassed.

Link: https://lore.kernel.org/netdev/179340003139.3131.11274717768928642724@kernel.org/

> Fixes: e03781879a0d ("drop_monitor: Require 'CAP_SYS_ADMIN' when joining "events" group")

[Severity: Low]
Is e03781879a0d the right commit for this tag?

git blame of genl_bind() attributes the function, the idr walk under
cb_lock and the ns_capable(CAP_NET_ADMIN) check to 4d54cc32112d ("mptcp:
avoid lock_fast usage in accept path"), from v5.12. That commit added
genl_bind() as the genl socket's .bind callback. It already had the same
structure: check under the lock, set the bit later.

e03781879a0d only added the second check, for CAP_SYS_ADMIN:

	if ((grp->flags & GENL_MCAST_CAP_SYS_ADMIN) &&
	    !ns_capable(net->user_ns, CAP_SYS_ADMIN))
		ret = -EPERM;

The race in the commit message applies just as much to CAP_NET_ADMIN
groups, such as the MPTCP PM events group.

With the current tag, could stable trees that have 4d54cc32112d but not
e03781879a0d (or its backport) miss this fix? That depends on stable
backport history, which wasn't checked here.

> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> index f038f04ef4ead..0ddebd8285853 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);
> +	}

[Severity: High]
With this patch, can an unprivileged user drive this walk while IRQs are
disabled?

netlink_table_grab() is write_lock_irq(&nl_table_lock).
__netlink_clear_multicast_users() only uses ksk->sk_protocol, so each
call walks the global list, which holds sockets from every namespace:

net/netlink/af_netlink.c:__netlink_clear_multicast_users() {
	struct netlink_table *tbl = &nl_table[ksk->sk_protocol];
	...
	sk_for_each_bound_safe(sk, tmp, &tbl->mc_list) {
		...
		netlink_update_socket_mc(nlk_sk(sk), group, 0);
	}
}

So the for_each_net_rcu() loop repeats the same full mc_list walk once
per netns.

netlink_update_socket_mc() is called even when the bit isn't set. It
always calls netlink_update_listeners(), which walks mc_list again:

net/netlink/af_netlink.c:netlink_update_listeners() {
	for (i = 0; i < NLGRPLONGS(tbl->groups); i++) {
		mask = 0;
		sk_for_each_bound(sk, &tbl->mc_list) {
	...
}

That makes each call roughly O(netns * n_mcgrps * sockets^2), with IRQs
off and cb_lock held for writing.

Before this patch, the walk only ran when a family was unregistered
(module unload). The new call in genl_validate_assign_mc_groups() runs it
on every registration after boot, and an unprivileged user can trigger a
registration:

ctrl_getfamily()
  request_module("net-pf-%d-proto-%d-family-%s", ...)
    genl_register_family()
      genl_validate_assign_mc_groups()
        genl_clear_mc_users()

The same user can put many NETLINK_GENERIC sockets on mc_list by joining
the nlctrl "notify" group. They can also create many namespaces through
user namespaces.

Could one autoloaded registration then hold nl_table_lock with IRQs off
long enough to fire hardlockup or RCU stall warnings and stall netlink
across the whole system?

> +	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);

[Severity: High]
Does this fully close the race described in the commit message?

genl_bind() checks the GENL_MCAST_CAP_* flags of whichever family owns the
group ID at that moment, then drops cb_lock and returns. The bit is set
later, in a separate critical section, and nothing checks the owner
again:

net/netlink/af_netlink.c:netlink_setsockopt() {
	...
		if (optname == NETLINK_ADD_MEMBERSHIP && nlk->netlink_bind) {
			err = nlk->netlink_bind(sock_net(sk), val);
			if (err)
				return err;
		}
		netlink_table_grab();
		netlink_update_socket_mc(nlk, val,
					 optname == NETLINK_ADD_MEMBERSHIP);
	...
}

netlink_bind() has a wider window. It also goes through netlink_insert()
(lock_sock) and a netlink_table_grab() that can sleep before it writes
nlk->groups[0].

The new clear runs once, at registration, and can only drop bits that are
already set. Is this sequence still possible?

CPU1 (unprivileged)              CPU2
netlink_setsockopt()
  genl_bind(X)
    family A owns X, no caps
    up_read(&cb_lock)
                                 genl_unregister_family(A)
                                   genl_clear_mc_users()
                                     /* CPU1 not a member yet */
                                   clear_bit(X, mc_groups)
                                 genl_register_family(B)
                                   /* B gets X, CAP_NET_ADMIN group */
                                   genl_clear_mc_users()
                                     /* nothing to drop */
  netlink_table_grab()
  netlink_update_socket_mc(nlk, X, 1)

CPU1 is then a member of B's capability-protected group, and B's flags
were never checked. do_one_broadcast() delivers B's multicasts to it.

The unprivileged user can trigger B's registration through
ctrl_getfamily()->request_module(). Families with such groups include
gtp, mptcp_pm, psample, ioam6 and the smb client.

This also has a side effect. B->bind() never ran for this socket, but
genl_unbind() will call B->unbind() on NETLINK_DROP_MEMBERSHIP or from
netlink_release(). Could that unbalance families that count listeners in
bind/unbind, such as drivers/thermal/thermal_netlink.c?

The commit message says:

    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.

That covers sockets that haven't called genl_bind() yet. It doesn't cover
a socket that already passed the check and released cb_lock.

The window is smaller now, because both the unregister and the
re-register have to land inside it. It still doesn't look closed.

Would it work better to make the check and the bit set atomic with
respect to group reassignment? Some options:

- set the bit before calling ->netlink_bind() and roll back on error
- check the owner again under netlink_table_grab()
- record a generation counter in genl_bind() and check it again before
  setting the bit

[Severity: Medium]
When pmcraid is reloaded, can this call unsubscribe the legitimate
listeners of another family that is still registered?

The fixed-ID branch for pmcraid doesn't check or reserve mc_groups:

	} else if (family->id == GENL_ID_PMCRAID) {
		first_id = GENL_ID_PMCRAID;
		BUG_ON(n_groups != 1);

mc_group_start reserves BIT(GENL_ID_PMCRAID) statically, but
genl_unregister_mc_groups() frees the ID on unload:

		if (grp_id != 1)
			clear_bit(grp_id, mc_groups);

So this sequence looks possible:

rmmod pmcraid
  pmcraid_exit()->pmcraid_netlink_release()->genl_unregister_family()
    clear_bit(GENL_ID_PMCRAID, mc_groups)
another family with mcgrps registers
  genl_allocate_reserve_groups() hands out GENL_ID_PMCRAID
  users join that group through genl_bind()
modprobe pmcraid
  genl_register_family() forces family->id = GENL_ID_PMCRAID
  genl_validate_assign_mc_groups()
    first_id = GENL_ID_PMCRAID, no ownership check
    genl_clear_mc_users()
      clears every member of the other family's group

The ID overlap itself predates this patch, but registration never touched
other sockets' membership before. With this call, the other family's
listeners would silently stop getting its notifications.

> +
>  	return err;
>  }

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261031054632.3697295-1-anilkaushikwireless%40gmail.com