[PATCH net v2 1/2] net/ncsi: fix use-after-free of ncsi_dev_priv in ncsi_unregister_dev()

[Binbin Deng]

ncsi_unregister_dev() releases every package and channel, unlinks the
device from the global ncsi_dev_list with list_del_rcu() and
immediately frees it with kfree().  The generic netlink handlers
resolve the device through that list (ndp_from_ifindex() ->
ncsi_find_dev()) and walk ndp->packages with no lock and no RCU
protection, so a reader that overlaps the teardown dereferences freed
memory.  NCSI_CMD_PKG_INFO has no GENL_ADMIN_PERM, so an
unprivileged user can race it against rmmod of the driver:

  writer: rmmod ftgmac100
            -> ftgmac100_remove() -> ncsi_unregister_dev() -> kfree(ndp)
  reader: sendmsg(NETLINK_GENERIC, NCSI_CMD_PKG_INFO)
            -> ncsi_pkg_info_nl() -> ncsi_write_package_info()

The genlmsg_new(GFP_KERNEL) allocation between the resolution and the
walk makes the window wide enough to hit even without preemption.

KASAN report (arm multi_v7, QEMU ast2600-evb, KASAN + SLUB + PREEMPT):

  BUG: KASAN: use-after-free in ncsi_write_package_info+0xf50/0xf84
  Read of size 4 at addr c9f100a0 by task poc_dump/62

which is ndp->package_num at offset 160, the bounds check at the
entry of ncsi_write_package_info(); the page had already been
returned to the buddy allocator.  A second capture of the same access
shows the object as an 8K kmalloc region still in the slab.

Two kernel-side users keep running into the objects as well:

  * The per-channel monitor timers keep running while the removal
    loop frees the packages: ncsi_remove_channel() stops the monitor
    only of the channel it is about to free, so the monitor of a
    channel on a later package can fire concurrently with the loop.

  * Every outstanding request has a one second timeout timer in
    ndp->requests[].  dev_remove_pack() has already cut off the
    responses, so these timers are all certain to fire, and
    ncsi_request_timeout() would run on an ndp that the deferred
    free has already released, with the timer_list still linked into
    the timer wheel.  Their command and response skbs leak.

Quiesce everything that walks the objects before releasing them, and
defer the final free:

    dev_remove_pack(&ndp->ptype);   no more responses
    disable_work_sync(&ndp->work);  worker cannot re-arm monitors
    stop every channel monitor;
    ncsi_free_request() every used request
                                     (deletes its timer, frees skbs);
    ncsi_remove_package() loop;
    list_del_rcu(&ndp->node);
    kfree_rcu(ndp, rcu);

The rcu_head sits at the head of struct ncsi_dev_priv because
kfree_rcu() requires its offset to be below 4096 and the structure is
about 17 KB on arm32 (requests[256]).

This is the free side of the fix for the device object: the packages
and channels are deferred by the companion series "net/ncsi: fix
use-after-free of ncsi_package in ncsi_remove_package()" and
"net/ncsi: fix use-after-free of ncsi_channel in ncsi_remove_channel()",
which also adds the rcu_read_lock() to the genl readers.  The driver
side that walks the released objects from ftgmac100_stop() during
unregister_netdev() is fixed by the second patch of this series.

Found by a Coccinelle rule matching kfree() immediately after
list_del_rcu(); the code inspection and the fix were LLM-assisted,
and the race was confirmed at runtime with KASAN as above.

Fixes: 2d283bdd079c ("net/ncsi: Resource management")
Signed-off-by: Binbin Deng <18983559317@xxxxxxx>
---
 net/ncsi/internal.h    |  4 ++++
 net/ncsi/ncsi-manage.c | 36 +++++++++++++++++++++++++++++++++---
 2 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
index cb41953..3630ee4 100644
--- a/net/ncsi/internal.h
+++ b/net/ncsi/internal.h
@@ -317,6 +317,10 @@ struct vlan_vid {
 
 struct ncsi_dev_priv {
 	struct ncsi_dev     ndev;            /* Associated NCSI device     */
+	/* RCU deferred free; must sit near the head of the struct as
+	 * kfree_rcu() requires the offset to be below 4096
+	 */
+	struct rcu_head     rcu;
 	unsigned int        flags;           /* NCSI device flags          */
 #define NCSI_DEV_PROBED		1            /* Finalized NCSI topology    */
 #define NCSI_DEV_HWA		2            /* Enabled HW arbitration     */
diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index 237e041..70cb31e 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -1956,11 +1956,42 @@ int ncsi_reset_dev(struct ncsi_dev *nd)
 void ncsi_unregister_dev(struct ncsi_dev *nd)
 {
 	struct ncsi_dev_priv *ndp = TO_NCSI_DEV_PRIV(nd);
+	struct ncsi_request *nr;
 	struct ncsi_package *np, *tmp;
+	struct ncsi_channel *nc;
 	unsigned long flags;
+	bool used;
+	int i;
 
 	dev_remove_pack(&ndp->ptype);
 
+	/* Quiesce everything that walks the packages and channels
+	 * before releasing them.  The monitors are stopped after the
+	 * worker so that ncsi_configure_channel() cannot re-arm them.
+	 */
+	disable_work_sync(&ndp->work);
+	NCSI_FOR_EACH_PACKAGE(ndp, np)
+		NCSI_FOR_EACH_CHANNEL(np, nc)
+			ncsi_stop_channel_monitor(nc);
+
+	/* With the Rx handler gone no response can complete an
+	 * outstanding request any more.  Free every request so that
+	 * its timer is deleted and its command and response skbs are
+	 * released; otherwise ncsi_request_timeout() would fire on
+	 * @ndp after the deferred free below, with the timer still
+	 * linked into the timer wheel.
+	 */
+	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);
 
@@ -1968,8 +1999,7 @@ void ncsi_unregister_dev(struct ncsi_dev *nd)
 	list_del_rcu(&ndp->node);
 	spin_unlock_irqrestore(&ncsi_dev_lock, flags);
 
-	disable_work_sync(&ndp->work);
-
-	kfree(ndp);
+	/* Netlink readers may still hold the device */
+	kfree_rcu(ndp, rcu);
 }
 EXPORT_SYMBOL_GPL(ncsi_unregister_dev);
-- 
2.43.0