[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

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 (2) · Medium: 0 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch only quiesces the workqueue entry point of the NC-SI
  state machine.
- [Low] The comment added in ncsi_unregister_dev() mostly narrates
  history ('Commit a0ffa68c70b3 added this disable only ahead of…

Pre-existing issues:
- [High] ncsi_unregister_dev() never cancels the per-request timers
  ndp->requests[i].timer, and never releases in-flight requests, before…
- [High] Packages, channels and ndp are freed with a bare kfree() right
  after list_del_rcu().

--

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

This patch moves disable_work_sync(&ndp->work) in ncsi_unregister_dev()
ahead of the ncsi_remove_package() loop. The state-machine work is then
quiesced before channels and packages are freed. It changes the ordering
added by commit a0ffa68c70b3, which disabled the work only right before
kfree(ndp).

> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
> index 1d63958c4429b..1c2dadfc0a2ad 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.
> +	 */

[Severity: Low]

This isn't a bug, but most of this comment is history ("Commit
a0ffa68c70b3 added this disable only ahead of kfree(ndp) ..."). The
changelog and the Fixes: tag already cover that. Could the comment
describe the current invariant instead?

Also, "before freeing any channel or package it may still be
dereferencing" claims more than disable_work_sync() covers. See the
question below about ncsi_channel_monitor().

> +	disable_work_sync(&ndp->work);

[Severity: High]

Does disabling ndp->work here stop the whole state machine?

ncsi_channel_monitor() also runs the state machine, directly from timer
context. The monitor timers of channels the removal loop hasn't reached
yet are still armed.

ftgmac100_remove() calls ncsi_unregister_dev() before unregister_netdev(),
so ncsi_stop_dev() has not run:

drivers/net/ethernet/faraday/ftgmac100.c:ftgmac100_remove() {
    ...
	if (priv->ndev)
		ncsi_unregister_dev(priv->ndev);
	unregister_netdev(netdev);
    ...
}

After dev_remove_pack(), no GLS responses can arrive. The monitors
therefore move toward the timeout branch:

net/ncsi/ncsi-manage.c:ncsi_channel_monitor() {
    ...
	default:
		...
		ncsi_report_link(ndp, true);
		...
		spin_lock_irqsave(&ndp->lock, flags);
		nc->state = NCSI_CHANNEL_ACTIVE;
		list_add_tail_rcu(&nc->link, &ndp->channel_queue);
		spin_unlock_irqrestore(&ndp->lock, flags);
		ncsi_process_next_channel(ndp);
		return;
    ...
}

ncsi_report_link() walks the package and channel lists.
ncsi_process_next_channel() then does the following with the first
channel on channel_queue:

  - pops it with list_first_or_null_rcu() and list_del_init()
  - takes nc->lock and writes nc->state
  - reads nc->package
  - calls ncsi_configure_channel() or ncsi_suspend_channel()

ncsi_remove_channel() unlinks only nc->node. It never removes nc->link
from ndp->channel_queue.

In multi-package or multi-channel mode, ncsi_choose_active_channel() can
queue several channels, and the work normally drains that queue. If the
work is disabled in the middle of configuration, can freed channels stay
on channel_queue?

If so, a later monitor timeout could write into a freed nc->link through
list_add_tail_rcu(). ncsi_process_next_channel() would then dereference
the freed channel.

There is a related race on the channel being removed. The timeout branch
clears nc->monitor.enabled under nc->lock and keeps using nc after that.
ncsi_stop_channel_monitor() skips timer_delete_sync() when enabled is
already false:

net/ncsi/ncsi-manage.c:ncsi_stop_channel_monitor() {
	spin_lock_irqsave(&nc->lock, flags);
	if (!nc->monitor.enabled) {
		spin_unlock_irqrestore(&nc->lock, flags);
		return;
	}
    ...
}

Could ncsi_remove_channel() then kfree(nc) while that channel's own
monitor callback is still running on another CPU?

A possible fix is to stop every monitor timer unconditionally, using
timer_shutdown_sync() without the enabled early return. Then drain or
unlink ndp->channel_queue under ndp->lock before freeing any channel.

> +
>  	list_for_each_entry_safe(np, tmp, &ndp->packages, node)
>  		ncsi_remove_package(np);

[Severity: High]

This isn't a bug introduced by this patch, but the commit message names
the bare kfree() right after list_del_rcu() in ncsi_remove_channel() and
ncsi_remove_package() as part of the problem, and that part is not
fixed. Can the generic netlink handlers still read freed packages and
channels here?

ndp stays on ncsi_dev_list until this loop has freed every package and
channel. ndp itself is then freed with no grace period after
list_del_rcu(&ndp->node).

The netlink side looks up ndp without taking a reference or the RCU read
lock:

net/ncsi/ncsi-netlink.c:ndp_from_ifindex() {
    ...
	nd = ncsi_find_dev(dev);
	ndp = nd ? TO_NCSI_DEV_PRIV(nd) : NULL;

	dev_put(dev);
	return ndp;
}

ncsi_write_package_info() then walks NCSI_FOR_EACH_PACKAGE() and
NCSI_FOR_EACH_CHANNEL() without rcu_read_lock().

ftgmac100_remove() calls ncsi_unregister_dev() before unregister_netdev(),
so the ifindex still resolves during teardown.

NCSI_CMD_PKG_INFO is registered with .flags = 0, so an unprivileged
caller can race a driver unbind. Could ncsi_pkg_info_nl() or
ncsi_pkg_info_all_nl() copy freed memory into the netlink reply? Examples
are nc->version.fw_name, nc->modes and nc->vlan_filter.vids, which
ncsi_remove_channel() frees even earlier.

The admin-only commands could also write through the same freed
pointers: set_interface, set_package_mask, set_channel_mask and
send_cmd.

>  
> @@ -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);

[Severity: High]

This is a pre-existing issue, but what happens here to in-flight
requests in ndp->requests[]?

ncsi_xmit_cmd() arms a one second timer for each request:

net/ncsi/ncsi-cmd.c:ncsi_xmit_cmd() {
    ...
	nr->enabled = true;
	mod_timer(&nr->timer, jiffies + 1 * HZ);
    ...
}

ncsi_free_request() is the only place that deletes this timer, on a
response or a timeout. After dev_remove_pack() no response can arrive,
so an outstanding request can only finish through
ncsi_request_timeout().

Requests can be outstanding when ncsi_unregister_dev() is called. New
ones can also be sent during the removal loop:

  - by the monitors of channels not yet removed (GLS in START/RETRY)
  - by netlink, since ndp is still on ncsi_dev_list

Suppose the timer fires during the loop for a request flagged
NCSI_REQ_FLAG_NETLINK_DRIVEN. ncsi_request_timeout() then calls
ncsi_find_package_and_channel() on lists that are being freed.

If it fires after kfree(ndp), it reads nr->ndp and takes ndp->lock from
freed memory:

net/ncsi/ncsi-manage.c:ncsi_request_timeout() {
    ...
	spin_lock_irqsave(&ndp->lock, flags);
	nr->enabled = false;
	if (nr->rsp || !nr->cmd) {
    ...
}

Is the timer_list in requests[] also left queued on the timer wheel
while it sits in freed slab memory? If the timers never fire, the
nr->cmd and nr->rsp skbs would leak.

Also, with the work disabled, the schedule_work() call in
ncsi_free_request() now silently does nothing on this path.

Should the unregister path call timer_shutdown_sync() on each request
timer and release the in-flight requests before freeing ndp?

>  }
>  EXPORT_SYMBOL_GPL(ncsi_unregister_dev);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261107051825.3313019-1-hj351016%40gmail.com