Re: [PATCH net v1] net/ncsi: fix use-after-free of ncsi_dev_priv in ncsi_unregister_dev()

[Binbin Deng]

Thank you for the review. All four Highs are legitimate and will be
addressed in v2. One note first: of the two remedies you suggest for the
ftgmac100 teardown, the reorder regresses a BUG_ON; the clearing variant is
the one to take (item 4).

1. [High] monitors still running during the removal loop; request timers
   never cancelled

   Agreed, this is the real hole in v1: dev_remove_pack() has already cut
   off responses, so every in-flight request's one-second timer is certain
   to fire, and a monitor GLS armed between dev_remove_pack() and the
   removal loop is queued behind kfree_rcu(ndp). v2 quiesces everything
   before the first package is released, in this order:

       dev_remove_pack(&ndp->ptype);
       disable_work_sync(&ndp->work);
       NCSI_FOR_EACH_PACKAGE(ndp, np)
               NCSI_FOR_EACH_CHANNEL(np, nc)
                       ncsi_stop_channel_monitor(nc);
       for (i = 0; i < ARRAY_SIZE(ndp->requests); i++) {
               nr = &ndp->requests[i];
               spin_lock_irqsave(&ndp->lock, flags);
               used = nr->used;
               spin_unlock_irqrestore(&ndp->lock, flags);
               if (used)
                       ncsi_free_request(nr);
       }
       list_for_each_entry_safe(np, tmp, &ndp->packages, node)
               ncsi_remove_package(np);

   disable_work_sync() runs first so ncsi_configure_channel() cannot
   re-arm a monitor afterwards; stopping all monitors before any free
   closes the GLS window; ncsi_free_request() then takes each outstanding
   request down, deleting nr->timer synchronously and releasing the cmd
   and rsp skbs (nr->used is read under ndp->lock).

2. [High] only the outermost ncsi_dev_priv gets an RCU-deferred free

   Agreed; the channels and their filter arrays are deferred in the
   companion series "net/ncsi: fix use-after-free of ncsi_channel in
   ncsi_remove_channel()" (msgid
   20261017024947.420683-1-18983559317@xxxxxxx) and the packages in
   "net/ncsi: fix use-after-free of ncsi_package in ncsi_remove_package()"
   (msgid 20261017012431.413648-1-18983559317@xxxxxxx).

3. [High] kfree_rcu() does not protect the netlink reader, which resolves
   ndp through ndp_from_ifindex() without RCU

   Agreed. The readers enter rcu_read_lock() in the channel companion
   series, which also keeps the genlmsg_new(GFP_KERNEL) allocation of the
   PKG_INFO doit outside the critical section -- that sleep window between
   ndp_from_ifindex() and ncsi_write_package_info() is exactly the
   "ndp->package_num at offset 160" access of the first KASAN trace. The
   residual you sketch at the end (a CAP_NET_ADMIN NCSI_CMD_SEND_CMD
   slipping a request in between the request sweep here and the deferred
   free) cannot be closed by RCU lifetime alone; it needs serialization
   between the genl handlers and unregistration, and I would like to leave
   that for follow-up work rather than widen this series.

4. [High] the ftgmac100 teardown UAF is only made less likely

   Agreed that the fix must not depend on grace period length, but the
   first suggested remedy is not viable: registering the NCSI Rx handler
   sets ndp->ptype.dev = dev, so it sits on the netdev's
   dev->ptype_specific, and netdev unregistration runs
   BUG_ON(!list_empty(&dev->ptype_specific)) -- unregistering the netdev
   before ncsi_unregister_dev() hits that BUG_ON. This is exactly the
   crash that 3d5179458d22 ("net: ftgmac100: Fix crash when removing
   driver", 2020) fixed by putting ncsi_unregister_dev() first. I will
   take your second option instead: v2 adds a second patch to the series
   that clears priv->ndev and priv->use_ncsi in ftgmac100_remove() right
   after ncsi_unregister_dev(), so the unregister_netdev() ->
   __dev_close_many() -> ftgmac100_stop() -> ncsi_stop_dev() path no
   longer walks the released objects (it becomes a no-op), with its own
   Fixes: 3d5179458d22.

5. Pre-existing issues

   - The ncsi_vlan_rx_kill_vid() use-after-free against set_one_vid() is
     fixed by 9c7e89fc1b68 ("ncsi: Fix use-after-free in the VLAN id
     list removal"), which is in net-next already.

   - The ndp->vlan_vids leak on unregister is real: after ncsi_unregister_
     dev() has removed ndp from ncsi_dev_list, the later
     ncsi_vlan_rx_kill_vid() from the 8021q NETDEV_UNREGISTER path finds
     no device and the entries leak. Freeing them from
     ncsi_unregister_dev() with kfree_rcu() (same release discipline as
     9c7e89fc1b68) is a natural separate follow-up patch.

pw-bot: cr