[PATCH net v5 0/2] genetlink: fix joining multicast groups no family owns

[Anil Kaushik]

genl_bind() checks the GENL_MCAST_CAP_* flags of a group, but the
membership bit is set later, after the lock is dropped. A socket can end
up a member of a group whose flags were never checked for it: when it
joins a group ID that no family owns, or when the family of the group is
unregistered between the check and the point where the bit is set.

Patch 1 rejects joins to group IDs that no family owns: setsockopt()
returns -ENOENT, and a bind() mask skips them and joins the other groups.
Patch 2 counts,
per group, how many times its members were cleared, which genetlink does
when a family is unregistered. If the count changed after the check, the
group is checked again against its current owner, after the bind
callback is undone if that owner is the one it reached. The core cannot
always tell, see patch 2.

Tested as an unprivileged user, with an msleep() added after the bind
callback in netlink_setsockopt() and netlink_bind(), and a test family
that registers again with GENL_MCAST_CAP_NET_ADMIN:

                                         patch 1 only   v5
  setsockopt:
  unregister in the window,
  register later                         member         not member
  unregister and register in the window  member         not member
  another family unregistered
  (must still join)                      member         member
  bind():
  unregister and register in the window  member         not member

With the msleep() before the bind callback instead, and a test family
that registers again without the flag, the family is bound once, and not
twice as with v3:

                                         v3             v5
  binds while a member, setsockopt/bind  2              1
  binds once the socket is closed        1              0

The test family counts its bind and unbind callbacks: they stay
balanced in all these cases. If the family is replaced just after the
bind callback returned, and before the second read, with an msleep()
there, it gets one unbind too many. The socket is still rejected or
checked against the new family. Normal joins and rtnetlink bind() show
no change.

The test from v1 (join all free IDs, then load gtp and drop_monitor)
gives the same result as before.

bind() with a mask, as an unprivileged user and as root:

                                         v4             v5
  an owned group and free IDs            -ENOENT        ok, owned group
  all IDs, as root                       -ENOENT        ok, owned groups
  only free IDs                          -ENOENT        ok, no group
  a group the user may not join          -ENOENT        -EPERM, no group

With a test module that releases its kernel socket while a socket joins,
v4 gets a NULL pointer dereference in netlink_setsockopt(), and v5 returns
-ENOENT, with one unbind for the one bind. I did not test this with a real
nfnetlink rollback.

I used an LLM to help write and review the code, the tests and the
commit messages of this series. I checked and tested every change
myself.

Changes in v5:
- Patch 1: a bind() mask skips the IDs no family owns, instead of failing
  as a whole, so that such a mask keeps working (Jacob Keller, netdev
  Sashiko). This changes the patch, so I dropped the Reviewed-by tags.
- Patch 2: return -ENOENT if the last kernel socket of the table was
  released and the counts are gone, which was a NULL pointer dereference,
  and call ->netlink_unbind() if ->netlink_bind() already ran (Sashiko).
- Not changed, from the netdev Sashiko review of v4: the counts of a table
  whose group count is reduced, and NETLINK_USERSOCK having no counts.
  Nothing in the tree reduces the group count, or uses the helpers with
  those counts on NETLINK_USERSOCK.

Changes in v4:
- Patch 2: read the count after ->netlink_bind() too, and call
  ->netlink_unbind() before checking again if the count had already
  changed: ->netlink_bind() can have reached the new owner, which was
  then bound twice (netdev Sashiko). The commit message says what the
  core still cannot tell.
- Patch 1: no change. It does not close the window on its own, patch 2
  does, and it keeps its tags (netdev Sashiko asked about the order).
- The deadlock netdev Sashiko found on the insert failure path of
  netlink_bind() exists without this series, it has its own patch:
  https://lore.kernel.org/netdev/20261102120929.4077030-1-anilkaushikwireless@xxxxxxxxx/

Changes in v3:
- Patch 2: check the group again in af_netlink.c instead of clearing
  the members when a family registers. This also covers the case where
  the family is unregistered and another one registers, both in the
  window, does not walk the sockets on every registration, and does not
  touch the members of a group another family still owns (Sashiko).
- Fixes: tag of both patches: the race exists since genl_bind() first
  checked a capability, in 4d54cc32112d (Sashiko).

Changes in v2:
- New patch 2 (Sashiko).
- Patch 1: no change, added Reviewed-by tags.

v4: https://lore.kernel.org/netdev/20261102155819.4100930-1-anilkaushikwireless@xxxxxxxxx/
v3: https://lore.kernel.org/netdev/20261101101347.3910654-1-anilkaushikwireless@xxxxxxxxx/
v2: https://lore.kernel.org/netdev/20261031054632.3697295-1-anilkaushikwireless@xxxxxxxxx/
v1: https://lore.kernel.org/netdev/20261029062214.3407223-1-anilkaushikwireless@xxxxxxxxx/

Anil Kaushik (2):
  genetlink: Reject joining multicast groups no family owns
  netlink: Check a group again if it was released while joining it

 net/netlink/af_netlink.c | 116 ++++++++++++++++++++++++++++++++++++++-
 net/netlink/af_netlink.h |   1 +
 net/netlink/genetlink.c  |   3 +-
 3 files changed, 118 insertions(+), 2 deletions(-)

-- 
2.25.1