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

[henry martin]

Hi Eric,

Thanks for the careful review, you're right on all points.

This came from static analysis; I don't have a KASAN report.  For
what it's worth, 32-bit x86 cannot provide one either, since
HAVE_ARCH_KASAN there is X86_64-only, and on 64-bit a reproducer
would need an out-of-tree family to push the table past the initial
bitmap.

v2 corrects the Fixes tag to 84659eb529b3, scopes the changelog to
the actual reachability (32-bit kernels with in-tree families, at
module unload) instead of implying generic corruption, and drops
Cc: stable.

Regards,
Henry

Eric Dumazet <edumazet@xxxxxxxxxx> 于2026年10月10日周六 16:26写道：
>
> Le sam. 10 oct. 2026 à 09:26, Henry Martin <bsdhenrymartin@xxxxxxxxx> a écrit :
> >
> > 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.
> >
> > __netlink_clear_multicast_users() however iterates every group of
> > the departing family and calls netlink_update_socket_mc() for each
> > socket on mc_list, which does test_bit()/__assign_bit() on group - 1
> > with no regard to nlk->ngroups.  A stale socket therefore gets bits
> > read and cleared past its bitmap allocation, corrupting whichever
> > heap object follows it; the corruption repeats on every family
> > unregister.
>
> Do you have a KASAN report, or is this from static analysis only ?
>
> nl_table[NETLINK_GENERIC].groups only grows when group ids no longer
> fit in mc_groups_longs * BITS_PER_LONG.
>
> On 64bit, 59 ids are free in the initial bitmap. I count 58 genl
> multicast groups in the whole tree, 4 of them using reserved ids
> (ctrl, quota, pmcraid, NET_DM), so 54 dynamically allocated ones
> if every family was loaded at the same time.
>
> I do not see how a 64bit kernel with in-tree families can get there.
> 32bit kernels could, and then only from genl_unregister_family(),
> i.e. at module unload.
>
> The check itself looks fine and matches what do_one_broadcast()
> does, but the changelog should describe the real conditions instead
> of suggesting a generic heap corruption.
>
> Unless you have a reproducer, I think this is net-next material,
> without the Cc: stable.
>
> >
> > Skip groups that lie beyond the socket's bitmap: such a socket could
> > never have joined them.
> >
> > This issue was discovered by Tencent CodeBuddy Security.
> >
> > Cc: stable@xxxxxxxxxxxxxxx
> > Fixes: b4ff4f0419ae ("[NETLINK]: allocate group bitmaps dynamically")
>
> This commit did not add netlink_update_socket_mc() or
> netlink_clear_multicast_users(). This should be
>
> Fixes: 84659eb529b3 ("[NETLIKN]: Allow removing multicast groups.")
>
> > Signed-off-by: Henry Martin <bsdhenrymartin@xxxxxxxxx>
> > ---
> >  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);
> > --
> > 2.43.7
> >