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

[Binbin Deng]

Thank you for the review. All four findings are legitimate. This patch is
one of three companion fixes, and none of the Highs is closed by this patch
alone -- v2 will make the dependency explicit in the commit message and fix
the [Low] inaccuracy.

1. [High] "kfree_rcu() only delays the free ... none of the NCSI netlink
   walkers enter an RCU read-side critical section"

   Agreed. The read side enters rcu_read_lock() in the companion series
   "net/ncsi: fix use-after-free of ncsi_channel in ncsi_remove_channel()"
   (msgid 20261017024947.420683-1-18983559317@xxxxxxx), which wraps the
   genl handlers around the device resolution and the list walks, keeping
   the genlmsg_new(GFP_KERNEL) of the PKG_INFO doit outside the critical
   section. I split free side and read side this way so each object keeps
   its own Fixes: tag for -stable backporting; the race is closed by the
   union of the series. The vlan fix that was just merged,
   9c7e89fc1b68 ("ncsi: Fix use-after-free in the VLAN id list removal"),
   shows the free-side-only half: there it was sufficient because the
   set_one_vid() reader already held rcu_read_lock(); here no reader does,
   so the read-side series is required alongside, and v2 of this patch
   will stop implying that a grace period is already relied upon.

2. [High] "The child channels of the package this patch keeps alive are
   still freed with no grace period"

   Agreed; covered by the same companion series: ncsi_remove_channel()
   releases nc->mac_filter.addrs, nc->vlan_filter.vids and nc itself from
   an RCU callback, since the readers reach the filter arrays through the
   channel (the KASAN report on the dump reader is a "Read of size 2" on
   the freed u16 vids[] entries).

3. [High] "The netlink readers hold no reference to the parent
   struct ncsi_dev_priv, and the same teardown frees it immediately"

   Agreed; covered by the third companion series, "net/ncsi: fix
   use-after-free of ncsi_dev_priv in ncsi_unregister_dev()" (msgid
   20261017015315.415794-1-18983559317@xxxxxxx), whose v2 also quiesces
   the channel monitor timers and the 256 request timers before the
   removal loop, which is your pre-existing High on that series.

4. [Low] commit message describes the reader inaccurately

   Conceded, my mistake: the walker writes the package id with

       nla_put_u32(skb, NCSI_PKG_ATTR_ID, np->id);

   and never reads np->multi_channel; I had dragged in fields from
   ncsi_set_interface_nl()/ncsi_clear_interface_nl(), which are not the
   readers raced here. The KASAN report the patch is built on matches
   your reading: the "Read of size 1 ... 0 bytes inside of freed 64-byte
   region" in ncsi_write_package_info+0xe3c is the u8 np->id load feeding
   that nla_put_u32() (sizeof(struct ncsi_package) is 60 bytes, so it
   lands in kmalloc-64). v2 corrects the message.

5. Pre-existing High: ncsi_unregister_dev() frees every package and
   channel before it stops the work item

   Agreed. The worker race is real and kfree_rcu() does not help against
   it; it is addressed by disable_work_sync() being moved ahead of the
   removal loop in v2 of the ndp companion series, together with the
   monitor and request-timer quiescing.

pw-bot: cr