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

[henry martin]

Agreed, that's the better place.

Thanks,
Henry

Jakub Kicinski <kuba@xxxxxxxxxx> 于2026年10月24日周六 06:15写道：
>
> 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@xxxxxxxxx>
> > ---
> >
> > 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