[PATCH net v1] ncsi: Fix use-after-free in the device unregister path

[Binbin Deng]

ncsi_unregister_dev() tears the NCSI device down in this order:

	dev_remove_pack(&ndp->ptype);

	list_for_each_entry_safe(np, tmp, &ndp->packages, node)
		ncsi_remove_package(np);
	...
	disable_work_sync(&ndp->work);

	kfree(ndp);

ncsi_remove_package() and ncsi_remove_channel() remove the objects
with list_del_rcu() and free them with an immediate kfree().  Two
concurrent users are not covered by this sequence:

  1. The NCSI state machine work (ndp->work) iterates the package and
     channel lists (NCSI_FOR_EACH_PACKAGE/NCSI_FOR_EACH_CHANNEL, which
     are list_for_each_entry_rcu) while configuring channels, holding
     neither ndp->lock nor np->lock across the iteration.
     disable_work_sync() runs only after all packages have been freed,
     so it does not prevent the work from walking the lists over freed
     objects.

  2. RCU readers of the published lists.  list_del_rcu() removes the
     entry for subsequent readers, but the following bare kfree() is
     not covered by any grace period, so a reader that has already
     obtained the node pointer (e.g. a list_for_each_entry_rcu
     iteration in flight) dereferences freed memory when it resumes.

The ordering is reachable on BMC systems: ftgmac100_remove() calls
ncsi_unregister_dev() before unregister_netdev(), i.e. before
ncsi_stop_dev() has stopped the state machine, so the work is still
active while the packages are freed.  Device removal concurrent with
the NCSI configuration cycle or a netlink query triggers the race.

Fix this in two parts:

  - stop the state machine work before freeing the packages and
    channels instead of after;
  - free the package and channel objects with kfree_rcu() so that
    readers covered by an RCU read-side critical section do not
    access freed memory.

Fixes: e6f44ed6d04d ("net/ncsi: Package and channel management")
Signed-off-by: Binbin Deng <18983559317@xxxxxxx>
---
 net/ncsi/internal.h    | 2 ++
 net/ncsi/ncsi-manage.c | 8 ++++----
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
index 2c9d1f22c16a..461469cc2600 100644
--- a/net/ncsi/internal.h
+++ b/net/ncsi/internal.h
@@ -239,6 +239,7 @@ struct ncsi_channel {
 	} monitor;
 	struct list_head            node;
 	struct list_head            link;
+	struct rcu_head             rcu_head;
 };
 
 struct ncsi_package {
@@ -253,6 +254,7 @@ struct ncsi_package {
 	bool                 multi_channel; /* Enable multiple channels  */
 	u32                  channel_whitelist; /* Channels to configure */
 	struct ncsi_channel  *preferred_channel; /* Primary channel      */
+	struct rcu_head      rcu_head;
 };
 
 struct ncsi_request {
diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index 1d63958c4429..39ac7075cd1b 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -263,7 +263,7 @@ static void ncsi_remove_channel(struct ncsi_channel *nc)
 	np->channel_num--;
 	spin_unlock_irqrestore(&np->lock, flags);
 
-	kfree(nc);
+	kfree_rcu(nc, rcu_head);
 }
 
 struct ncsi_package *ncsi_find_package(struct ncsi_dev_priv *ndp,
@@ -326,7 +326,7 @@ void ncsi_remove_package(struct ncsi_package *np)
 	ndp->package_num--;
 	spin_unlock_irqrestore(&ndp->lock, flags);
 
-	kfree(np);
+	kfree_rcu(np, rcu_head);
 }
 
 void ncsi_find_package_and_channel(struct ncsi_dev_priv *ndp,
@@ -1958,6 +1958,8 @@ void ncsi_unregister_dev(struct ncsi_dev *nd)
 	struct ncsi_package *np, *tmp;
 	unsigned long flags;
 
+	disable_work_sync(&ndp->work);
+
 	dev_remove_pack(&ndp->ptype);
 
 	list_for_each_entry_safe(np, tmp, &ndp->packages, node)
@@ -1967,8 +1969,6 @@ 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);
 }
 EXPORT_SYMBOL_GPL(ncsi_unregister_dev);
-- 
2.43.0