Re: [PATCH net v2 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] 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