Re: [PATCH net v2] net/ncsi: fix use-after-free of ncsi_channel in ncsi_remove_channel()

[Binbin Deng]

Thank you for the review. The High is conceded with a companion-series
answer, the two Mediums on code scope are conceded for v3, and on the
get_net() question: yes, plain sock_net() is enough. Details:

1. [High] the comment claims teardown "only unlinks these objects and frees
   them after a grace period", but np and ndp are still freed right after
   their list_del_rcu()

   Conceded, the comment overclaims. Only the channel and its filter
   arrays get a grace period from this series; np and ndp are deferred by
   the two companion series:

     - "net/ncsi: fix use-after-free of ncsi_package in
        ncsi_remove_package()" (msgid
        20261017012431.413648-1-18983559317@xxxxxxx)
     - "net/ncsi: fix use-after-free of ncsi_dev_priv in
        ncsi_unregister_dev()" (msgid
        20261017015315.415794-1-18983559317@xxxxxxx)

   v3 rewords the comment to the channel scope and names the companions,
   so the reader-side statement stays true independent of their
   application state.

2. [Medium] the RCU reader protocol is only added to the PKG_INFO handlers

   Conceded, v3 wraps all seven handlers (ncsi_set_interface_nl(),
   ncsi_clear_interface_nl(), ncsi_send_cmd_nl(),
   ncsi_set_package_mask_nl(), ncsi_set_channel_mask_nl(), and the two
   PKG_INFO ones) around the ndp_from_ifindex() resolution and the walks.
   Two sleeping constraints are handled:

     - The PKG_INFO doit reply skb keeps its genlmsg_new(GFP_KERNEL)
       outside the critical section: allocated before the lock, unicast
       after the unlock, so the walk itself never sleeps.
     - NCSI_CMD_SEND_CMD's error path (ncsi_send_netlink_err() ->
       nlmsg_unicast(), which may sleep) is deferred to after the unlock,
       using the net_device captured inside the critical section.
       ncsi_xmit_cmd() itself is atomic -- GFP_ATOMIC command skbs,
       spinlocks and dev_queue_xmit() -- and runs under the lock.

   You are right that the SET_*/SEND_CMD handlers can turn the race into
   use-after-free writes; they are GENL_ADMIN_PERM, but that only limits
   who can trigger it, not whether it exists. The CONFIG_PROVE_RCU_LIST
   angle is another reason the lock belongs around every walk, not just
   the info ones.

3. [Medium] the ncsi_remove_channel() comment says the grace period covers
   the "channel configuration worker"

   Conceded as written; v3 drops the worker from the comment. The worker
   is safe, but not because of call_rcu() -- it is quiesced by v2 of the
   ndp companion series, which runs disable_work_sync(&ndp->work) before
   any package or channel is released and stops every channel monitor
   before the removal loop, so nothing can re-arm nc->monitor.timer
   through ncsi_start_channel_monitor() afterwards. Given that, the stale
   pointers you list are not walked after the quiesce point:

     - nc->link sits on ndp->channel_queue, which only the worker walks
       (ncsi_process_next_channel()); no netlink handler traverses it.
     - ndp->active_channel and ndp->hot_channel are worker-only state.
     - The info walkers' only use of np->preferred_channel is a pointer
       comparison,

           net/ncsi/ncsi-netlink.c:71:
               if (nc == nc->package->preferred_channel)

       which loads through np (kept alive by the package companion
       series) and never dereferences the stale channel pointer.

   The pre-teardown worker race itself (concurrent with the old immediate
   kfree) predates this patch, as you note; the disable_work_sync()
   ordering in the ndp companion is what closes it.

4. [Medium] pre-existing: get_net(sock_net(skb->sk)) leaks a netns
   reference in six handlers

   Yes. Nothing in net/ncsi/ stores the struct net beyond the handler,
   and the netlink socket pins the netns for the duration of the call, so
   plain sock_net() is enough. v3 switches all six occurrences
   (ncsi_pkg_info_all_nl(), ncsi_set_interface_nl(),
   ncsi_clear_interface_nl(), ncsi_send_cmd_nl(),
   ncsi_set_package_mask_nl(), ncsi_set_channel_mask_nl()).

pw-bot: cr