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

[PATCH net v2] 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.

Found during a static-analysis-assisted audit: candidate sites were
surfaced by a Coccinelle rule, and the reader/writer paths were
confirmed by LLM-assisted (GLM-5.3) code inspection.  The issue was
not observed at runtime; it is identified by code inspection only.

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 adee6dcabdc3..362c3c69d087 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 54d0df0a9efe..05dcd732effa 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 v2] ncsi: Fix use-after-free in the device unregister path

[netdev-bot+sashiko]

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

Critical: 0 · High: 4 · Medium: 1 · Low: 1 (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] After this patch, ftgmac100 removal still uses freed memory
  every time it runs with the interface up.
- [High] Switching to kfree_rcu() does not protect the NCSI netlink
  readers, yet the commit message names a concurrent netlink query as a…
- [High] ncsi_unregister_dev() never cancels the per-request timers
  embedded in ndp->requests[] before kfree(ndp).
- [High] ncsi_unregister_dev() still unlinks ndp from the global
  ncsi_dev_list with list_del_rcu(&ndp->node) (line 1969) and then frees…
- [Medium] ncsi_remove_channel() frees the channel's filter arrays at
  once, before the channel is even unlinked, and only defers freeing the…
- [Low] The commit message says candidate sites came from a Coccinelle
  rule and that the reader/writer paths were confirmed by LLM-assisted…

Pre-existing issues:
- [Low] This problem wasn't introduced by this patch, but
  ncsi_unregister_dev() never frees the vlan_vid entries that…

--

ncsi: Fix use-after-free in the device unregister path

This commit moves disable_work_sync() to the start of
ncsi_unregister_dev(), so the NCSI state machine work stops before the
packages and channels are freed. It also changes ncsi_remove_package()
and ncsi_remove_channel() to free with kfree_rcu() instead of kfree().

> Found during a static-analysis-assisted audit: candidate sites were
> surfaced by a Coccinelle rule, and the reader/writer paths were
> confirmed by LLM-assisted (GLM-5.3) code inspection.  The issue was
> not observed at runtime; it is identified by code inspection only.
>
> Fixes: e6f44ed6d04d ("net/ncsi: Package and channel management")
> Signed-off-by: Binbin Deng <18983559317@163.com>

[Severity: Low]
This isn't a bug, but Documentation/process/coding-assistants.rst and
Documentation/process/submitting-patches.rst ask AI-assisted
contributions to carry a tag of the form "Assisted-by: LLM [TOOL1]
[TOOL2]".

Should the trailers include something like "Assisted-by: GLM-5.3
coccinelle"?

> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
> index 54d0df0a9efe6..05dcd732effa2 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);

[Severity: High]
The commit message gives a concurrent netlink query as one trigger. Do
any of the NCSI netlink readers actually run inside an RCU read-side
critical section?

ndp_from_ifindex() in net/ncsi/ncsi-netlink.c looks the device up
without rcu_read_lock(). It then returns ndp without holding any
reference:

	nd = ncsi_find_dev(dev);
	ndp = nd ? TO_NCSI_DEV_PRIV(nd) : NULL;

	dev_put(dev);
	return ndp;

ncsi_pkg_info_nl(), ncsi_pkg_info_all_nl() and
ncsi_write_package_info() then walk ndp->packages and np->channels with
NCSI_FOR_EACH_PACKAGE() and NCSI_FOR_EACH_CHANNEL() in preemptible
process context. Only genl_mutex serializes them, and
ncsi_unregister_dev() doesn't take it.

A process-context walker outside rcu_read_lock() doesn't hold off a
grace period. Can this sequence still read freed memory?

CPU0 (NCSI_CMD_PKG_INFO)              CPU1
ncsi_write_package_info()
  NCSI_FOR_EACH_PACKAGE(ndp, np)
  <preempted>
                                      ncsi_unregister_dev()
                                        ncsi_remove_package()
                                          list_del_rcu(&np->node)
                                          kfree_rcu(np, rcu_head)
                                      <grace period ends>
  <resumes, reads np->id, nc->...>

NCSI_CMD_PKG_INFO has .flags = 0 in ncsi_ops, so an unprivileged user
can run the reader side.

The same applies to kfree_rcu(np, rcu_head) in ncsi_remove_package().
Do the netlink handlers need rcu_read_lock() plus a lifetime guarantee
for ndp, or a lock they share with teardown?

[Severity: Medium]
Even for a reader under RCU protection, are the channel's filter
arrays still freed too early? Earlier in ncsi_remove_channel():

	spin_lock_irqsave(&nc->lock, flags);

	/* Release filters */
	kfree(nc->mac_filter.addrs);
	kfree(nc->vlan_filter.vids);
	...
	/* Remove and free channel */
	spin_lock_irqsave(&np->lock, flags);
	list_del_rcu(&nc->node);

These arrays are freed at once, before nc is unlinked. The vids
pointer, bitmap and n_vids are left unchanged. kfree_rcu() defers only
the free of nc itself.

ncsi_write_channel_info() reads the VLAN array through nc without
nc->lock and copies it into the reply:

	while ((i = find_next_bit((void *)&ncf->bitmap, ncf->n_vids,
				  i + 1)) < ncf->n_vids) {
		if (ncf->vids[i])
			nla_put_u16(skb, NCSI_CHANNEL_ATTR_VLAN_ID,
				    ncf->vids[i]);
	}

Can this read freed vids[] when a reader is at or past nc in
NCSI_FOR_EACH_CHANNEL()? Should the arrays be freed together with nc
from an RCU callback?

>  }

[ ... ]

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

[Severity: High]
disable_work_sync() waits for ndp->work to return. Does anything cancel
the per-request timers in ndp->requests[] before kfree(ndp)?

ncsi_xmit_cmd() arms one timer for each outstanding command:

	nr->enabled = true;
	mod_timer(&nr->timer, jiffies + 1 * HZ);

The only timer_delete_sync(&nr->timer) is in ncsi_free_request(). That
runs when a response arrives or when the timer fires. After
dev_remove_pack(), no more responses arrive to release these requests.

The channel monitor timer is stopped only later, in
ncsi_remove_channel(). Until then, ncsi_channel_monitor() can still
send a GLS command through ncsi_xmit_cmd(), which arms a new request
timer.

If one of these timers fires after kfree(ndp), ncsi_request_timeout()
does this:

	struct ncsi_dev_priv *ndp = nr->ndp;
	...
	spin_lock_irqsave(&ndp->lock, flags);

For netlink-driven requests it also walks ndp->packages through
ncsi_find_package_and_channel() before it calls ncsi_free_request().

Can this cause a use-after-free in timer softirq, with a timer_list
still pending inside the freed ndp?

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

[Severity: High]
The commit message describes the bug like this:

    list_del_rcu() removes the entry for subsequent readers, but the
    following bare kfree() is not covered by any grace period

Isn't the same pattern still here for ndp? ndp is removed from
ncsi_dev_list with list_del_rcu() and then freed right away.

ncsi_dev_list has RCU readers. ncsi_rcv_rsp() runs in RX softirq and
calls ncsi_find_dev(), which walks the whole global list:

	NCSI_FOR_EACH_DEV(ndp) {
		if (ndp->ndev.dev == dev)
			return &ndp->ndev;
	}

With two NCSI-enabled MACs, RX on device B can walk past device A's
ndp while A is being torn down. The synchronize_net() in
dev_remove_pack(&ndp->ptype) only covers A's own handler. It also runs
before list_del_rcu(&ndp->node).

ndp_from_ifindex() also passes the unreferenced ndp to the netlink
handlers.

ndp itself is freed with no grace period. Does the kfree_rcu() on the
packages and channels then help any reader that reached them through
ndp->packages?

[Severity: High]
The commit message says:

    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

With that ordering, is ndp still used after this kfree()?
ftgmac100_remove() in drivers/net/ethernet/faraday/ftgmac100.c does:

	if (priv->ndev)
		ncsi_unregister_dev(priv->ndev);
	unregister_netdev(netdev);

It doesn't clear priv->ndev. If the interface is up,
unregister_netdev() calls ftgmac100_stop() as .ndo_stop:

	if (priv->use_ncsi)
		ncsi_stop_dev(priv->ndev);

ncsi_stop_dev() then:

  - walks the freed ndp->packages;
  - calls ncsi_stop_channel_monitor(), which runs timer_delete_sync()
    on timers in freed channels;
  - calls ncsi_report_link(ndp, true), which makes an indirect call
    through nd->handler in the freed object.

The call chain is:

ftgmac100_remove()->ncsi_unregister_dev()->kfree(ndp)
ftgmac100_remove()->unregister_netdev()->...->ftgmac100_stop()
    ->ncsi_stop_dev()

The error path in ftgmac100_probe() sets priv->ndev = NULL after
ncsi_unregister_dev(), but ftgmac100_remove() does not.

This path needs no race. Does every removal with the interface up
still hit a use-after-free here?

[Severity: Low]
This isn't a bug introduced by this patch, but ncsi_unregister_dev()
never frees the entries on ndp->vlan_vids. ncsi_vlan_rx_add_vid() adds
them with list_add_rcu(), and only ncsi_vlan_rx_kill_vid() frees them.

With the ftgmac100_remove() ordering, ndp is already off ncsi_dev_list
when unregister_netdev() tears down the VLANs. So
ncsi_vlan_rx_kill_vid() returns early:

	nd = ncsi_find_dev(dev);
	if (!nd) {
		netdev_warn(dev, "NCSI: no net_device?\n");
		return 0;
	}

Does this leak the vlan_vid entry for each configured VLAN ID on
removal?

>  }
>  EXPORT_SYMBOL_GPL(ncsi_unregister_dev);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261010075828.5724-1-18983559317%40163.com