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

[netdev-bot+sashiko@kernel.org]

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