[PATCH net v1] net/ncsi: fix use-after-free of ncsi_dev_priv in ncsi_unregister_dev()

[PATCH net v1] net/ncsi: fix use-after-free of ncsi_dev_priv in ncsi_unregister_dev()

[Binbin Deng]

ncsi_unregister_dev() releases every package (which releases every
channel), unlinks the ncsi_dev_priv from the global ncsi_dev_list with
list_del_rcu(), disables the configuration work and frees the device
itself -- in that order.  Two defects:

  * kfree(ndp) runs immediately after list_del_rcu().  The netlink
    reporting paths resolve the device through ncsi_dev_list
    (ndp_from_ifindex()) and keep dereferencing it, starting with the
    ndp->package_num check at the entry of ncsi_write_package_info(),
    so a reader can be inside the freed allocation.

  * The packages and channels are freed before
    disable_work_sync(&ndp->work); the configuration worker
    (ncsi_dev_work, including set_one_vid()/clear_one_vid()) can be
    walking the very objects being freed, and the teardown has to
    race it instead of quiescing it first.

  writer: rmmod ftgmac100 -> ftgmac100_remove() -> ncsi_unregister_dev()
            -> ncsi_remove_package() ... -> kfree(ndp)
  reader: sendmsg(NETLINK_GENERIC, NCSI_CMD_PKG_INFO)
            -> ncsi_pkg_info_nl() -> ncsi_write_package_info()

Quiesce the work first, then release the packages, and defer the free
of the ncsi_dev_priv with kfree_rcu().  A corollary of the old
ordering: the ftgmac100 driver calls ncsi_stop_dev() right after
ncsi_unregister_dev() and used to read the already freed device.

Commit a0ffa68c70b3 ("net/ncsi: Disable the ncsi work before freeing
the associated structure") already added a disable_work_sync() before
the kfree(ndp), but its placement still releases the packages and
channels first, so the worker can walk those while they are being
freed, and it does nothing for the netlink readers.

Both defects are visible in KASAN.  The netlink reader, on the
ndp->package_num entry check of ncsi_write_package_info() -- a read of
size 4 at ndp+160; struct ncsi_dev_priv is 17700 bytes on arm32, hence
kmalloc-32k:

  BUG: KASAN: use-after-free in ncsi_write_package_info+0xf50/0xf84
  Read of size 4 at addr c9f100a0 by task poc_dump/62
  Call trace:
   ...
   kasan_report from ncsi_write_package_info+0xf50/0xf84
   ncsi_write_package_info from ncsi_pkg_info_nl+0x320/0x63c
   ncsi_pkg_info_nl from genl_family_rcv_msg_doit+0x218/0x2d8
   genl_family_rcv_msg_doit from genl_rcv_msg+0x578/0xb24
   genl_rcv_msg from netlink_rcv_skb+0x134/0x3c4
   netlink_rcv_skb from genl_rcv+0x24/0x34
   genl_rcv from netlink_unicast+0x884/0xac0
   netlink_unicast from netlink_sendmsg+0x62c/0xb98
   netlink_sendmsg from __sys_sendto+0x290/0x3e0
   __sys_sendto from sys_send+0x18/0x20
   sys_send from ret_fast_syscall+0x0/0x5c
  The buggy address belongs to the physical page:

(a page-level report: the freed pages had already been returned to the
buddy allocator, so no cache or object lines are printed).  The same
reader site also surfaces once the page has been recycled into another
cache -- the same free at a different stage of the page lifecycle:

  BUG: KASAN: slab-out-of-bounds in ncsi_write_package_info+0xf50/0xf84
  Read of size 4 at addr c9d700a0 by task poc_dump/70
  Call trace:
   ...
   kasan_report from ncsi_write_package_info+0xf50/0xf84
   ncsi_write_package_info from ncsi_pkg_info_nl+0x320/0x63c
   ...
   netlink_unicast from netlink_sendmsg+0x62c/0xb98
  The buggy address belongs to the object at c9d70000
   which belongs to the cache kmalloc-8k of size 8192
  The buggy address is located 160 bytes inside of
   allocated 8192-byte region [c9d70000, c9d72000)

And the corollary, in the rmmod task itself -- its stack documents the
very ordering this patch changes (ncsi_unregister_dev() has already
freed the device when unregister_netdev() closes it):

  BUG: KASAN: use-after-free in ncsi_stop_dev+0x398/0x3a0
  Read of size 4 at addr c9d480a8 by task poc_dump/55
  Call trace:
   ...
   kasan_report from ncsi_stop_dev+0x398/0x3a0
   ncsi_stop_dev from ftgmac100_stop+0x2dc/0x364 [ftgmac100]
   ftgmac100_stop [ftgmac100] from __dev_close_many+0x2d8/0x57c
   __dev_close_many from netif_close_many+0x264/0x5a4
   netif_close_many from unregister_netdevice_many_notify+0x87c/0x1f0c
   unregister_netdevice_many_notify from
    unregister_netdevice_queue+0x290/0x328
   unregister_netdevice_queue from unregister_netdev+0x18/0x68
   unregister_netdev from ftgmac100_remove+0x74/0x9c [ftgmac100]
   ftgmac100_remove [ftgmac100] from
    device_release_driver_internal+0x41c/0x5e0
   ...
   bus_remove_driver from sys_delete_module+0x308/0x438
   sys_delete_module from ret_fast_syscall+0x0/0x5c

Tested on an arm multi_v7 kernel (KASAN + SLUB + PREEMPT) running as a
QEMU ast2600-evb guest with a host-side NC-SI responder; with the
series applied the same rmmod hammering completes without KASAN
reports.

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.

Fixes: 2d283bdd079c ("net/ncsi: Resource management")
Signed-off-by: Binbin Deng <18983559317@163.com>
---
 net/ncsi/internal.h    | 1 +
 net/ncsi/ncsi-manage.c | 9 ++++++---
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
index adee6dcabdc3..5d315768409a 100644
--- a/net/ncsi/internal.h
+++ b/net/ncsi/internal.h
@@ -316,6 +316,7 @@ struct vlan_vid {
 
 struct ncsi_dev_priv {
 	struct ncsi_dev     ndev;            /* Associated NCSI device     */
+	struct rcu_head     rcu;             /* Deferred free (kfree_rcu)  */
 	unsigned int        flags;           /* NCSI device flags          */
 #define NCSI_DEV_PROBED		1            /* Finalized NCSI topology    */
 #define NCSI_DEV_HWA		2            /* Enabled HW arbitration     */
diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index 54d0df0a9efe..35e9dd85e044 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -1960,6 +1960,11 @@ void ncsi_unregister_dev(struct ncsi_dev *nd)
 
 	dev_remove_pack(&ndp->ptype);
 
+	/* Quiesce the configuration worker before releasing the packages
+	 * and channels it walks.
+	 */
+	disable_work_sync(&ndp->work);
+
 	list_for_each_entry_safe(np, tmp, &ndp->packages, node)
 		ncsi_remove_package(np);
 
@@ -1967,8 +1972,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);
+	kfree_rcu(ndp, rcu);
 }
 EXPORT_SYMBOL_GPL(ncsi_unregister_dev);
-- 
2.43.0

Re: [PATCH net v1] net/ncsi: fix use-after-free of ncsi_dev_priv in ncsi_unregister_dev()

[netdev-bot+sashiko]

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

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

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] Switching to kfree_rcu(ndp, rcu) in ncsi_unregister_dev() does
  not fix the netlink reader use-after-free that the commit message…
- [High] Only the outermost ncsi_dev_priv gets an RCU-deferred free.
- [High] The ftgmac100 teardown use-after-free from the third KASAN trace
  (ncsi_stop_dev called from ftgmac100_stop during unregister_netdev) is…
- [High] net/ncsi: active request and monitor timers not quiesced in
  ncsi_unregister_dev()

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  ncsi_unregister_dev() never cancels the 256 request timers embedded in…
- [High] This problem wasn't introduced by this patch, but
  ncsi_vlan_rx_kill_vid() (net/ncsi/ncsi-manage.c) unlinks a vlan_vid…
- [Medium] This problem wasn't introduced by this patch, but
  ncsi_unregister_dev() never frees the struct vlan_vid entries on…

--

net/ncsi: fix use-after-free of ncsi_dev_priv in ncsi_unregister_dev()

This patch moves disable_work_sync(&ndp->work) in ncsi_unregister_dev()
so it runs before the packages and channels are torn down. It also
replaces the final kfree(ndp) with kfree_rcu(ndp, rcu). The goal is to
fix the use-after-free reports from the NCSI netlink readers and from
ftgmac100_stop() -> ncsi_stop_dev() during driver removal.

> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
> index 54d0df0a9efe6..35e9dd85e0443 100644
> --- a/net/ncsi/ncsi-manage.c
> +++ b/net/ncsi/ncsi-manage.c
> @@ -1960,6 +1960,11 @@ void ncsi_unregister_dev(struct ncsi_dev *nd)
>  
>  	dev_remove_pack(&ndp->ptype);
>  
> +	/* Quiesce the configuration worker before releasing the packages
> +	 * and channels it walks.
> +	 */
> +	disable_work_sync(&ndp->work);
> +
>  	list_for_each_entry_safe(np, tmp, &ndp->packages, node)
>  		ncsi_remove_package(np);

[Severity: High]
Does disable_work_sync() quiesce everything that walks the packages
and channels here? The per-channel monitor timers still seem to be
running when the removal loop starts.

ftgmac100_remove() calls ncsi_unregister_dev() before
unregister_netdev(). So ncsi_stop_dev() has not run yet, and every
active channel still has nc->monitor.timer armed.
ncsi_remove_channel() only calls ncsi_stop_channel_monitor() on the
channel it is about to free. While the loop frees the first package,
the ncsi_channel_monitor() callback of a channel on a later package
can still fire.

That callback does one of two things:

- It sends a GLS with ncsi_xmit_cmd().
- In its timeout path, it calls ncsi_report_link(ndp, true), sets
  NCSI_DEV_RESHUFFLE, queues nc on ndp->channel_queue and calls
  ncsi_process_next_channel(ndp).

Either way it runs concurrently with ncsi_remove_package() and
touches package and channel state that is being torn down.

The GLS case also arms a timer. ncsi_xmit_cmd() sets nr->enabled and
calls mod_timer() with a one second timeout. That timer lives in one
of the ndp->requests[] entries, which are embedded in ndp.

dev_remove_pack() has already run, so ncsi_rcv_rsp() can never
complete that request and the timer is certain to fire. Commands
left in flight by ncsi_dev_work or by NCSI_CMD_SEND_CMD have the same
problem.

Nothing in ncsi_unregister_dev() deletes these timers.
kfree_rcu(ndp, rcu) frees ndp after one grace period, normally well
before the one second timeout. ncsi_request_timeout() then runs in
softirq context on freed memory. It reads nr->ndp, takes ndp->lock
and calls ncsi_free_request(). The timer_list is also still linked on
the timer wheel when its memory is freed, so the timer base can be
corrupted as well. Any outstanding nr->cmd and nr->rsp skbs leak.

Should every channel monitor be stopped before any package is freed?
The teardown would then call timer_delete_sync() on each
ndp->requests[i].timer and release outstanding requests before the
final kfree_rcu(). Or would you rather require callers to run
ncsi_stop_dev() first, and reorder ftgmac100_remove() to match?

[Severity: High]
Only ndp gets a deferred free here. Can the netlink readers still reach
packages and channels that have already been freed?

ncsi_remove_package() frees each package right after unlinking it:

net/ncsi/ncsi-manage.c:ncsi_remove_package() {
    ...
	spin_lock_irqsave(&ndp->lock, flags);
	list_del_rcu(&np->node);
	ndp->package_num--;
	spin_unlock_irqrestore(&ndp->lock, flags);

	kfree(np);
}

ncsi_remove_channel() does the same. It frees nc->mac_filter.addrs and
nc->vlan_filter.vids while nc is still on np->channels. It then calls
kfree(nc) right after list_del_rcu(&nc->node).

While this runs, ndp is still on ncsi_dev_list, so ndp_from_ifindex()
can still find it. ncsi_write_package_info() walks NCSI_FOR_EACH_PACKAGE
and NCSI_FOR_EACH_CHANNEL (both list_for_each_entry_rcu()), and
ncsi_write_channel_info() reads np->id, nc->package->preferred_channel,
nc->version.fw_name and ncf->vids[i] into the reply skb.

A reader that loaded np or nc just before the kfree() would then
dereference freed memory. NCSI_CMD_PKG_INFO has no GENL_ADMIN_PERM, so an
unprivileged user can race this against driver removal.

Even with the ndp lifetime fixed, don't the packages and channels also
need an RCU-deferred or refcounted free?

[ ... ]

> @@ -1967,8 +1972,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);
> +	kfree_rcu(ndp, rcu);

[Severity: High]
Does kfree_rcu() actually protect the netlink reader named in the commit
message? None of the NCSI genetlink handlers seem to be RCU readers.

ndp_from_ifindex() finds ndp through ncsi_find_dev(), which walks
ncsi_dev_list with list_for_each_entry_rcu() but without rcu_read_lock().
It then drops the netdev reference and returns a bare pointer:

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_pkg_info_nl() can sleep before it uses that pointer:

net/ncsi/ncsi-netlink.c:ncsi_pkg_info_nl() {
    ...
	ndp = ndp_from_ifindex(genl_info_net(info),
			       nla_get_u32(info->attrs[NCSI_ATTR_IFINDEX]));
	...
	skb = genlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
	...
	rc = ncsi_write_package_info(skb, ndp, package_id);
    ...
}

ncsi_write_package_info() then reads ndp->package_num. That is the
access in the first KASAN trace.

The genl doit and dumpit callbacks run in process context under
genl_mutex, not inside an RCU read-side critical section.
ncsi_unregister_dev() never takes genl_mutex. So the grace period can
end, and the kfree_rcu() callback can free ndp, while ncsi_pkg_info_nl()
still holds the pointer.

In ncsi_ops, NCSI_CMD_PKG_INFO (ncsi_pkg_info_nl() and
ncsi_pkg_info_all_nl()) has .flags = 0, so any local user can loop on it
during removal. If the memory has been reused, the reply would copy it to
user space. The admin-only SET_* and SEND_CMD handlers look up ndp the
same way and then write to it.

Does this need real lifetime handling instead? Some options are a
refcount taken under ncsi_dev_lock, readers that hold rcu_read_lock() and
don't sleep, or serializing unregister against genl.

[Severity: High]
Is the ftgmac100 teardown use-after-free from the third KASAN trace fixed
here, or only made less likely?

ftgmac100_remove() still unregisters the NCSI device before the netdev:

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

priv->ndev and priv->use_ncsi are never cleared. unregister_netdev()
takes rtnl_lock(), which can sleep, and under PREEMPT it can also be
preempted. It then reaches:

unregister_netdev()
  __dev_close_many()
    ftgmac100_stop()
      ncsi_stop_dev(priv->ndev)

ncsi_stop_dev() walks ndp->packages, reads ndp->ndev.dev, and calls
ncsi_report_link(ndp, true). That call writes nd->state and nd->link_up
and calls nd->handler. Nothing holds RCU or a reference across this
sequence, so the kfree_rcu() callback could free ndp first.

Would it be better to reorder ftgmac100_remove() so ncsi_unregister_dev()
runs after unregister_netdev()? Clearing priv->ndev and priv->use_ncsi
would also work. Either way, the fix would not depend on how long a grace
period takes.

[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but
ncsi_unregister_dev() never cancels the request timers in
ndp->requests[].

ncsi_register_dev() sets up all of them:

	timer_setup(&ndp->requests[i].timer, ncsi_request_timeout, 0);

and ncsi_xmit_cmd() arms one for each command:

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

After dev_remove_pack() no response can arrive, so ncsi_rcv_rsp() ->
ncsi_free_request() can no longer delete an outstanding timer.

Requests can still be outstanding at this point:

- ncsi_channel_monitor() can send a GLS between dev_remove_pack() and
  ncsi_stop_channel_monitor() in ncsi_remove_channel(). With the new
  ordering the monitors keep running through disable_work_sync().
- ncsi_dev_work may have commands in flight.
- A netlink NCSI_CMD_SEND_CMD request may be in flight.

ndp is freed after one grace period, usually well before the 1 second
timeout, while the timer_list is still on the timer wheel. When the timer
fires, ncsi_request_timeout() dereferences nr and nr->ndp, takes
ndp->lock and calls ncsi_free_request(), all on freed memory. The pending
nr->cmd and nr->rsp skbs also leak.

Should ncsi_unregister_dev() call timer_delete_sync() on each
ndp->requests[i].timer and release outstanding requests before freeing
ndp?

[Severity: Medium]
This is also a pre-existing issue, but are the struct vlan_vid entries
on ndp->vlan_vids leaked here?

ncsi_vlan_rx_add_vid() allocates each entry with kzalloc_obj() and links
it onto ndp->vlan_vids with list_add_rcu(). The only free path is
ncsi_vlan_rx_kill_vid(), which first looks up ndp:

net/ncsi/ncsi-manage.c:ncsi_vlan_rx_kill_vid() {
    ...
	nd = ncsi_find_dev(dev);
	if (!nd) {
		netdev_warn(dev, "NCSI: no net_device?\n");
		return 0;
	}
    ...
}

ftgmac100_remove() calls ncsi_unregister_dev(), which removes ndp from
ncsi_dev_list, before it calls unregister_netdev(). When the 8021q
NETDEV_UNREGISTER handling later calls ncsi_vlan_rx_kill_vid(),
ncsi_find_dev() returns NULL, so every registered vlan_vid leaks when
ndp is freed.

[Severity: High]
This is another pre-existing issue, unrelated to the ordering changed
here. ncsi_vlan_rx_kill_vid() frees a vlan_vid as soon as it is
unlinked, with no grace period:

net/ncsi/ncsi-manage.c:ncsi_vlan_rx_kill_vid() {
    ...
			list_del_rcu(&vlan->list);
			found = true;
			kfree(vlan);
    ...
}

set_one_vid() reads the same list as an RCU reader from ncsi_dev_work:

	rcu_read_lock();
	list_for_each_entry_rcu(vlan, &ndp->vlan_vids, list) {
		vid = vlan->vid;

ncsi_vlan_rx_kill_vid() runs under rtnl from vlan_vid_del().
set_one_vid() runs from the workqueue under nc->lock and does not hold
rtnl.

With no shared lock, can set_one_vid() read vlan->vid or vlan->list.next
after the entry has been freed? Should this use kfree_rcu() instead?

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