Re: [PATCH net v3] netlink: fix out-of-bounds bitmap access clearing stale mc groups

[Jakub Kicinski <kuba@kernel.org>]

On Tue, 20 Oct 2026 11:21:19 +0800 Henry Martin wrote:
> netlink_realloc_groups() sizes nlk->groups to the number of groups
> that exist at bind/ADD_MEMBERSHIP time.  When more multicast groups
> are registered later (e.g. a new genl family), existing sockets keep
> their smaller bitmap.
> 
> When clearing a multicast group of a departing family,
> __netlink_clear_multicast_users() runs netlink_update_socket_mc() on
> each socket on mc_list, which does test_bit()/__assign_bit() on
> group - 1 with no regard to nlk->ngroups.  Skip groups that lie
> beyond the socket's bitmap: such a socket could never have joined
> them.  This is the same guard do_one_broadcast() already applies.
> 
> The overflow windows differ per architecture, both needing privileged
> module (un)loading.  On 32-bit kernels the ~54 dynamically allocated
> genl groups already exceed the initial 32-bit bitmap, so the table
> can grow there directly.  On 64-bit kernels the same in-tree set
> fits the initial 64-bit bitmap only while allocation stays
> contiguous: genl_allocate_reserve_groups() takes the first free
> contiguous range and unregister does not compact, so an
> unload/reload sequence can fragment the ~59 free IDs until no
> fitting range remains, forcing the table past 64 bits and reaching
> the same stale-bitmap overflow on the next family teardown.
> 
> This issue was discovered by Tencent CodeBuddy Security.
> 
> Fixes: 84659eb529b3 ("[NETLIKN]: Allow removing multicast groups.")
> Signed-off-by: Henry Martin <bsdhenrymartin@gmail.com>
> ---
> 
> v3: Correct the reachability again (sashiko review): 64-bit is also
>     reachable - group IDs are allocated in contiguous ranges without
>     compaction, so fragmentation from an unload/reload sequence can
>     force the table past the initial 64-bit bitmap too.
> 
> v2: Correct the Fixes tag (the clear path was added by 84659eb529b3,
>     not by the dynamic bitmap commit), drop Cc: stable and describe
>     the actual reachability (32-bit kernels with in-tree families)
>     instead of implying generic heap corruption.
> 
>  net/netlink/af_netlink.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
> index 9fdf964224ab4..8cc655aa927f1 100644
> --- a/net/netlink/af_netlink.c
> +++ b/net/netlink/af_netlink.c
> @@ -1657,6 +1657,12 @@ static void netlink_update_socket_mc(struct netlink_sock *nlk,
>  {
>  	int old, new = !!is_new, subscriptions;
>  
> +	/* A socket whose bitmap predates this group can never be a member;
> +	 * don't touch bits beyond its allocation.
> +	 */
> +	if (group - 1 >= nlk->ngroups)
> +		return;
> +
>  	old = test_bit(group - 1, nlk->groups);
>  	subscriptions = nlk->subscriptions - old + new;
>  	__assign_bit(group - 1, nlk->groups, new);

Dunno :/ I would prefer the following fix, otherwise it looks like we
may silently ignore a netlink_update_socket_mc(..., 1)

@@ -2163,8 +2157,12 @@ void __netlink_clear_multicast_users(struct sock *ksk, unsigned int group)
        struct netlink_table *tbl = &nl_table[ksk->sk_protocol];
        struct hlist_node *tmp;
 
-       sk_for_each_bound_safe(sk, tmp, &tbl->mc_list)
+       sk_for_each_bound_safe(sk, tmp, &tbl->mc_list) {
+               /* bitmap predates the group, can't be a member */
+               if (group - 1 >= nlk_sk(sk)->ngroups)
+                       continue;
                netlink_update_socket_mc(nlk_sk(sk), group, 0);
+       }
 }
-- 
pw-bot: cr