Re: [PATCH net] 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: 1 · Medium: 0 · 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).

- [High] The fix is incomplete because of a check-then-use race.

--

genetlink: Reject joining multicast groups no family owns

This patch makes genl_bind() return -ENOENT when no registered family
owns the requested multicast group ID. Before the patch such joins
succeeded. If a family with GENL_MCAST_CAP_* flags was later given that
ID, the socket stayed a member.

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

> 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: High]
Does this fully close the case described in the commit message?

genl_bind() checks ownership and the GENL_MCAST_CAP_* flags under
down_read(&cb_lock). It drops the lock before it returns. The caller
sets the membership bit only afterwards:

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();
    ...
}

netlink_bind() follows the same pattern. It updates nlk->groups[0] under
netlink_table_grab() after genl_bind() has returned.

The owning family can be unregistered inside that window, for example by
a module unload or by a module init that fails after
genl_register_family(). Can the following sequence happen?

CPU1 (unprivileged)                  CPU2
netlink_setsockopt(ADD_MEMBERSHIP, G)
  genl_bind()
    down_read(&cb_lock)
    family F owns G, no caps, ret = 0
    up_read(&cb_lock)
                                     genl_unregister_family(F)
                                       down_write(&cb_lock)
                                       genl_unregister_mc_groups()
                                         __netlink_clear_multicast_users()
                                           (CPU1 not a member yet)
                                         clear_bit(G, mc_groups)
                                       idr_remove()
  netlink_table_grab()
  netlink_update_socket_mc(nlk, G, 1)

After this sequence the socket is a member of a group ID that no family
owns.

Later, genl_register_family()->genl_validate_assign_mc_groups()->
genl_allocate_reserve_groups() can give G to a family whose group has
GENL_MCAST_CAP_NET_ADMIN or GENL_MCAST_CAP_SYS_ADMIN, such as psample,
mptcp pm, ioam6 or cifs. Nothing clears the existing members of G at
that point. do_one_broadcast() delivers based only on the membership
bit. The CTRL_CMD_GETFAMILY->request_module() autoload path from the
commit message could still trigger that registration.

That is the end state the commit message says is fixed:

  "A family registered later can get that ID, and the socket stays a
  member: nothing checks it again."

Would it be more robust to clear existing members of newly assigned
group IDs in genl_validate_assign_mc_groups()? That could run under
netlink_table_grab(), the same way genl_unregister_mc_groups() clears
them on removal. It would cover both the original case and this race,
instead of relying on the check in genl_bind().

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