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

[netdev-bot+sashiko@kernel.org]

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] Check-then-act race (TOCTOU) between genl_bind() and adding
  the socket to the group.

--

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 change, any user could
join such an ID, and a family that got the ID later would inherit the
membership without any GENL_MCAST_CAP_* check.

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]
Can a socket still end up as a member of an unowned group ID through a
race with genl_unregister_family()?

genl_bind() checks ownership and capabilities under down_read(&cb_lock),
then drops cb_lock and returns. The caller only sets the membership bit
later, under a different lock:

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 the same gap. Its per-group loop calls
nlk->netlink_bind(net, group + 1), and the subscription update happens
afterwards under netlink_table_grab().

If genl_unregister_family() runs in that window, it only clears sockets
that are already members, and then frees the ID:

net/netlink/genetlink.c:genl_unregister_mc_groups() {
    ...
	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);
	}
	...
		if (grp_id != 1)
			clear_bit(grp_id, mc_groups);
    ...
}

The interleaving would look like this:

CPU1 (unprivileged)
netlink_setsockopt(NETLINK_ADD_MEMBERSHIP)
  genl_bind()       /* owner found, no caps needed, returns 0 */

CPU2
genl_unregister_family()
  genl_unregister_mc_groups()
    __netlink_clear_multicast_users()   /* CPU1 is not a member yet */
    clear_bit(grp_id, mc_groups)

CPU1
  netlink_table_grab()
  netlink_update_socket_mc()            /* now a member of a free ID */

A family registered later, for example one with GENL_MCAST_CAP_NET_ADMIN
or GENL_MCAST_CAP_SYS_ADMIN loaded via request_module(), could then get
that ID. The socket would receive its notifications without a capability
check. At this commit that is the same problem the patch is meant to fix,
although the window needs a family to be unregistered concurrently
(module unload, or module init failing after genl_register_family()).

A later patch in this series, "netlink: Check a group again if it was
released while joining it", appears to close this window. It adds a
per-group generation counter, tbl->mc_gen[], which
__netlink_clear_multicast_users() increments. netlink_setsockopt() and
netlink_bind() check the counter again under netlink_table_grab() and
retry genl_bind() if it changed.

Would it make sense to order that patch before this one, or fold the two
together, so that this commit does not leave the window open on its own?

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