[PATCH net] net/ncsi: disable the ncsi work before freeing channels and packages

[PATCH net] net/ncsi: disable the ncsi work before freeing channels and packages

[hyunjungg]

From: Hyunjung Ko <hj351016@gmail.com>

ncsi_unregister_dev() frees all channels and packages via
ncsi_remove_package() and only afterwards calls
disable_work_sync(&ndp->work):

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

The state-machine work, ncsi_dev_work(), dereferences channels and
packages (it walks ndp->channel_queue with list_for_each_entry_rcu()
and operates on the per-package channel lists).  ncsi_remove_channel()
and ncsi_remove_package() free those objects with a bare kfree() right
after list_del_rcu(), without waiting for a grace period or for the
work to stop.  If ncsi_dev_work() is pending or running when
ncsi_unregister_dev() executes, it can therefore dereference a channel
or package that has already been freed -> use-after-free.

Commit a0ffa68c70b3 ("net/ncsi: Disable the ncsi work before freeing
the associated structure") added disable_work_sync() to fix the
use-after-free of the ncsi_dev_priv itself, but placed it immediately
before kfree(ndp) -- that is, after the channels and packages have
already been freed -- so the channel/package use-after-free window
remains.

Move disable_work_sync() ahead of the ncsi_remove_package() loop, so
the work is quiesced before any channel or package it may still be
touching is freed.

Found by code review; not runtime-reproduced (requires NC-SI capable
hardware and the state-machine work racing device unregister).

Fixes: a0ffa68c70b3 ("net/ncsi: Disable the ncsi work before freeing the associated structure")
Cc: stable@vger.kernel.org
Signed-off-by: Hyunjung Ko <hj351016@gmail.com>
---
 net/ncsi/ncsi-manage.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index 1d63958..1c2dadf 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -1960,6 +1960,15 @@ void ncsi_unregister_dev(struct ncsi_dev *nd)
 
 	dev_remove_pack(&ndp->ptype);
 
+	/*
+	 * Quiesce the state-machine work before freeing any channel or
+	 * package it may still be dereferencing (via the channel_queue /
+	 * package lists).  Commit a0ffa68c70b3 added this disable only
+	 * ahead of kfree(ndp), i.e. after the channels and packages had
+	 * already been freed below, leaving that use-after-free window.
+	 */
+	disable_work_sync(&ndp->work);
+
 	list_for_each_entry_safe(np, tmp, &ndp->packages, node)
 		ncsi_remove_package(np);
 
@@ -1967,8 +1976,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