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

[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@163.com>
---
 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

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.