Re: [PATCH net v4 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 (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] In net/netlink/af_netlink.c, netlink_bind() lets one failing
  group abort the whole bind().

Pre-existing issues:
- [Medium] Kept only because requires_validation=true.

--

genetlink: Reject joining multicast groups no family owns

genl_bind() now returns -ENOENT when no registered family owns the
requested multicast group ID. Before this change such joins succeeded.
The socket also stayed a member if a family with privileged-only groups
later got the same 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;

[Severity: Medium]
Can a bind() with an nl_groups mask now fail as a whole when only one
bit in the mask has no owner?

netlink_bind() in net/netlink/af_netlink.c calls the bind hook once for
each set bit. On the first error it undoes the groups it already added
and returns:

net/netlink/af_netlink.c:netlink_bind() {
    ...
		for (group = 0; group < BITS_PER_TYPE(u32); group++) {
			if (!test_bit(group, &groups))
				continue;
			err = nlk->netlink_bind(net, group + 1);
			if (!err)
				continue;
			netlink_undo_bind(group, groups, sk);
			return err;
		}
    ...
}

Generic netlink group IDs are sparse. mc_group_start reserves only a few
bits:

static unsigned long mc_group_start = 0x3 | BIT(GENL_ID_CTRL) |
				      BIT(GENL_ID_VFS_DQUOT) |
				      BIT(GENL_ID_PMCRAID);

Other IDs are given out only when a family registers. So on a typical
system most IDs in 1..32 have no owner. genl's nlk->ngroups is at least
32, so the masking at the top of netlink_bind() does not remove these
bits.

Before this patch, a bind() on a NETLINK_GENERIC socket with a mask that
mixed owned and unowned IDs (for example nl_groups = ~0) returned 0 and
joined the owned groups.

With this patch, will that call return -ENOENT and join none of the
requested groups, including the valid owned ones?

The netlink core also still treats every ID below ngroups as valid in
netlink_setsockopt():

		if (!val || val - 1 >= nlk->ngroups)
			return -EINVAL;

genl_bind() now rejects many of those IDs, so the two layers disagree.

The commit message argues that joining an unowned group has no use, and
its test counts single-group joins. It does not mention the bind() mask
case.

The later commit "netlink: Check a group again if it was released while
joining it" keeps both the -ENOENT return and the all-or-nothing loop.
So this behaviour change is still there at the end of the series.

Should the bind() mask path skip unowned IDs instead of failing? If not,
should the commit message mention this uAPI change?

[ ... ]

> @@ -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]
This isn't a bug introduced by this patch, but is there still a window
where a socket can become a member of a group ID that was just freed?

genl_bind() checks ownership and the CAP flags under down_read(&cb_lock).
It drops cb_lock before returning. The caller adds the membership later,
under netlink_table_grab():

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() splits the check and the update in the same way for the
nl_groups mask.

Suppose genl_unregister_family() runs in that gap:

CPU1                                  CPU2
setsockopt(NETLINK_ADD_MEMBERSHIP)
  genl_bind() returns 0
  (cb_lock dropped)
                                      genl_unregister_family()
                                        genl_unregister_mc_groups()
                                          __netlink_clear_multicast_users()
                                          clear_bit(grp_id, mc_groups)
  netlink_table_grab()
  netlink_update_socket_mc()

__netlink_clear_multicast_users() clears only the sockets that are
members at that moment.

Would the socket on CPU1 then be added to the now-free ID?

A family registered later could get that ID, including one whose groups
have GENL_MCAST_CAP_NET_ADMIN or GENL_MCAST_CAP_SYS_ADMIN. It would then
deliver to the socket through do_one_broadcast(), and nothing checks the
membership again.

The commit message says:

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

This patch alone does not close that window. The window is narrow,
because a family has to be unregistered during it, for example by rmmod
or by a module init that fails after registration.

The next commit in the series, "netlink: Check a group again if it was
released while joining it", appears to fix this:

- It adds a per-group generation counter, nl_table[].mc_gen, which
  __netlink_clear_multicast_users() increments.
- netlink_setsockopt() and netlink_bind() compare the counter again
  under netlink_table_grab() and retry if the group was released.
- On retry, genl_bind() returns -ENOENT for the freed ID.

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