[PATCH net v2] net/ncsi: fix use-after-free of ncsi_channel in ncsi_remove_channel()

[PATCH net v2] net/ncsi: fix use-after-free of ncsi_channel in ncsi_remove_channel()

[Binbin Deng]

The NCSI generic netlink package information paths walk the global
ncsi_dev_list (ndp_from_ifindex() -> ncsi_find_dev()) and the
ndp->packages / np->channels lists through the list_for_each_entry_rcu()
based NCSI_FOR_EACH_* macros, but they never enter an RCU read-side
critical section.  Everything these walkers touch is only unlinked with
list_del_rcu(), so the lifetime contract of the objects is "alive for an
RCU grace period after unlink"; a walker that does not hold
rcu_read_lock() has no protection at all once it is preempted mid-walk,
no matter what the free side does.

The teardown side breaks the other half of that contract for the
channels: ncsi_remove_channel() unlinks the channel from its package's
channel list with list_del_rcu() and then frees the channel's mac/vlan
filter arrays and the channel object itself immediately.  The netlink
walkers dereference deep into the channel --
ncsi_write_channel_info() reads nc->version.fw_name with nla_put_string()
(strlen) and iterates the nc->vlan_filter.vids[] entries -- so a reader
mid-walk reads freed memory when a teardown overlaps it.

Either half alone leaves the race, so fix both sides:

  * Enter the read-side critical section in the NCSI_CMD_PKG_INFO doit
    (ncsi_pkg_info_nl()) and dump (ncsi_pkg_info_all_nl()) handlers and
    keep it held across every dereference of ndp, np and nc.  The doit
    response skb is allocated before the lock is taken so that the
    GFP_KERNEL allocation does not run under rcu_read_lock(); the dump
    handler only appends to the callback skb it is given and can hold
    the lock across its whole walk.

  * Move the frees of nc->mac_filter.addrs, nc->vlan_filter.vids and nc
    into an RCU callback.  The two arrays are only reachable through
    the channel object, so they have to share its grace period.

The race needs no real NC hardware:

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

(freeing happens before unregister_netdev(), so the netdev, and with it
the genl entry point, stays reachable during the whole window).

Tested on an arm multi_v7 kernel (KASAN + SLUB + PREEMPT) running as a
QEMU ast2600-evb guest with a host-side NC-SI responder; the PoC hammers
NCSI_CMD_PKG_INFO from four threads while cycling insmod/rmmod
ftgmac100 (doit and dump variants in parallel).  KASAN fires at the
first teardown of every boot:

  BUG: KASAN: slab-use-after-free in ncsi_write_package_info+0xa5c/0xf84
  Read of size 1 at addr c69019e0 by task poc_dump/68
  CPU: 0 UID: 0 PID: 68 Comm: poc_dump Not tainted 7.2.6 #1 PREEMPT
  Hardware name: Generic DT based system
  Call trace:
   ...
   kasan_report from ncsi_write_package_info+0xa5c/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

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")
Fixes: 955dc68cb9b2 ("net/ncsi: Add generic netlink family")
Signed-off-by: Binbin Deng <18983559317@163.com>
---
 net/ncsi/internal.h     |  1 +
 net/ncsi/ncsi-manage.c  | 20 ++++++++++++++------
 net/ncsi/ncsi-netlink.c | 36 ++++++++++++++++++++++++++----------
 3 files changed, 41 insertions(+), 16 deletions(-)

diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
index adee6dcabdc3..530834d8caa7 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;
 };
 
 struct ncsi_package {
diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index 54d0df0a9efe..c16047dee3f8 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -242,17 +242,21 @@ struct ncsi_channel *ncsi_add_channel(struct ncsi_package *np, unsigned char id)
 	return nc;
 }
 
+static void ncsi_channel_release(struct rcu_head *rcu)
+{
+	struct ncsi_channel *nc = container_of(rcu, struct ncsi_channel, rcu);
+
+	kfree(nc->mac_filter.addrs);
+	kfree(nc->vlan_filter.vids);
+	kfree(nc);
+}
+
 static void ncsi_remove_channel(struct ncsi_channel *nc)
 {
 	struct ncsi_package *np = nc->package;
 	unsigned long flags;
 
 	spin_lock_irqsave(&nc->lock, flags);
-
-	/* Release filters */
-	kfree(nc->mac_filter.addrs);
-	kfree(nc->vlan_filter.vids);
-
 	nc->state = NCSI_CHANNEL_INACTIVE;
 	spin_unlock_irqrestore(&nc->lock, flags);
 	ncsi_stop_channel_monitor(nc);
@@ -263,7 +267,11 @@ static void ncsi_remove_channel(struct ncsi_channel *nc)
 	np->channel_num--;
 	spin_unlock_irqrestore(&np->lock, flags);
 
-	kfree(nc);
+	/* Readers (netlink package info walkers, channel configuration
+	 * worker) may still hold the channel or its filter arrays;
+	 * release everything after a grace period.
+	 */
+	call_rcu(&nc->rcu, ncsi_channel_release);
 }
 
 struct ncsi_package *ncsi_find_package(struct ncsi_dev_priv *ndp,
diff --git a/net/ncsi/ncsi-netlink.c b/net/ncsi/ncsi-netlink.c
index 8cc538358f6a..3de607f6982c 100644
--- a/net/ncsi/ncsi-netlink.c
+++ b/net/ncsi/ncsi-netlink.c
@@ -169,11 +169,6 @@ static int ncsi_pkg_info_nl(struct sk_buff *msg, struct genl_info *info)
 	if (!info->attrs[NCSI_ATTR_PACKAGE_ID])
 		return -EINVAL;
 
-	ndp = ndp_from_ifindex(genl_info_net(info),
-			       nla_get_u32(info->attrs[NCSI_ATTR_IFINDEX]));
-	if (!ndp)
-		return -ENODEV;
-
 	skb = genlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
 	if (!skb)
 		return -ENOMEM;
@@ -192,7 +187,21 @@ static int ncsi_pkg_info_nl(struct sk_buff *msg, struct genl_info *info)
 		kfree_skb(skb);
 		return -EMSGSIZE;
 	}
+	/* The device is resolved and the lists are walked inside an RCU
+	 * read-side critical section: the teardown path only unlinks these
+	 * objects and frees them after a grace period.
+	 */
+	rcu_read_lock();
+	ndp = ndp_from_ifindex(genl_info_net(info),
+			       nla_get_u32(info->attrs[NCSI_ATTR_IFINDEX]));
+	if (!ndp) {
+		rcu_read_unlock();
+		kfree_skb(skb);
+		return -ENODEV;
+	}
+
 	rc = ncsi_write_package_info(skb, ndp, package_id);
+	rcu_read_unlock();
 
 	if (rc) {
 		nla_nest_cancel(skb, attr);
@@ -228,20 +237,26 @@ static int ncsi_pkg_info_all_nl(struct sk_buff *skb,
 	if (!attrs[NCSI_ATTR_IFINDEX])
 		return -EINVAL;
 
+	package_id = cb->args[0];
+
+	rcu_read_lock();
 	ndp = ndp_from_ifindex(get_net(sock_net(skb->sk)),
 			       nla_get_u32(attrs[NCSI_ATTR_IFINDEX]));
 
-	if (!ndp)
+	if (!ndp) {
+		rcu_read_unlock();
 		return -ENODEV;
+	}
 
-	package_id = cb->args[0];
 	package = NULL;
 	NCSI_FOR_EACH_PACKAGE(ndp, np)
 		if (np->id == package_id)
 			package = np;
 
-	if (!package)
+	if (!package) {
+		rcu_read_unlock();
 		return 0; /* done */
+	}
 
 	hdr = genlmsg_put(skb, NETLINK_CB(cb->skb).portid, cb->nlh->nlmsg_seq,
 			  &ncsi_genl_family, NLM_F_MULTI,  NCSI_CMD_PKG_INFO);
@@ -265,9 +280,10 @@ static int ncsi_pkg_info_all_nl(struct sk_buff *skb,
 	genlmsg_end(skb, hdr);
 
 	cb->args[0] = package_id + 1;
-
+	rcu_read_unlock();
 	return skb->len;
 err:
+	rcu_read_unlock();
 	genlmsg_cancel(skb, hdr);
 	return rc;
 }
-- 
2.43.0

Re: [PATCH net v2] net/ncsi: fix use-after-free of ncsi_channel in ncsi_remove_channel()

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 2 (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] The fix is incomplete, and the race the commit message targets
  (rmmod ftgmac100 vs. NCSI_CMD_PKG_INFO) can still cause a use-after-…
- [Medium] The patch adds the RCU reader protocol only to the
  NCSI_CMD_PKG_INFO handlers.
- [Medium] The comment the patch adds in ncsi_remove_channel() (ncsi-
  manage.c:270-273) says the call_rcu() deferral protects the 'channel…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  ncsi_pkg_info_all_nl() passes get_net(sock_net(skb->sk)) to…

--

net/ncsi: fix use-after-free of ncsi_channel in ncsi_remove_channel()

This patch wraps the NCSI_CMD_PKG_INFO doit and dump handlers in
rcu_read_lock(). It also moves the freeing of ncsi_channel and its filter
arrays into an RCU callback. The goal is to close a use-after-free between
netlink package info readers and ftgmac100 teardown through
ncsi_unregister_dev().

> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
> index 54d0df0a9efe6..c16047dee3f8a 100644
> --- a/net/ncsi/ncsi-manage.c
> +++ b/net/ncsi/ncsi-manage.c

[ ... ]

> @@ -263,7 +267,11 @@ static void ncsi_remove_channel(struct ncsi_channel *nc)
>  	np->channel_num--;
>  	spin_unlock_irqrestore(&np->lock, flags);
>  
> -	kfree(nc);
> +	/* Readers (netlink package info walkers, channel configuration
> +	 * worker) may still hold the channel or its filter arrays;
> +	 * release everything after a grace period.
> +	 */
> +	call_rcu(&nc->rcu, ncsi_channel_release);

[Severity: Medium]

Does this grace period really cover the channel configuration worker?
ncsi_dev_work() -> ncsi_process_next_channel() / ncsi_configure_channel() /
ncsi_suspend_channel() does not run under rcu_read_lock(), so call_rcu()
does not wait for it.

ncsi_unregister_dev() stops the worker only after every channel has already
been passed to call_rcu():

ncsi_unregister_dev() {
	...
	list_for_each_entry_safe(np, tmp, &ndp->packages, node)
		ncsi_remove_package(np);
	...
	disable_work_sync(&ndp->work);
	...
}

ncsi_remove_channel() does not unlink nc->link from ndp->channel_queue. It
also does not clear ndp->active_channel, ndp->hot_channel or
np->preferred_channel.

A worker that is running or preempted could still dereference nc after
ncsi_channel_release() has freed it. It could also list_del_init() a stale
nc->link in ncsi_process_next_channel().

It could also re-arm nc->monitor.timer through ncsi_start_channel_monitor()
in ncsi_configure_channel() after ncsi_stop_channel_monitor() has already
run timer_delete_sync(). The timer would then fire on freed memory.

The worker race itself predates this patch, because the old code called
kfree() right away.

Should the comment leave out the worker? Or should teardown call
disable_work_sync() before releasing the channels, use timer_shutdown_sync()
on the monitor timer, and unlink nc->link from channel_queue?

>  }
>  
>  struct ncsi_package *ncsi_find_package(struct ncsi_dev_priv *ndp,
> diff --git a/net/ncsi/ncsi-netlink.c b/net/ncsi/ncsi-netlink.c
> index 8cc538358f6a3..8d1fe60beb22f 100644
> --- a/net/ncsi/ncsi-netlink.c
> +++ b/net/ncsi/ncsi-netlink.c

[ ... ]

> @@ -192,7 +187,21 @@ static int ncsi_pkg_info_nl(struct sk_buff *msg, struct genl_info *info)
>  		kfree_skb(skb);
>  		return -EMSGSIZE;
>  	}
> +	/* The device is resolved and the lists are walked inside an RCU
> +	 * read-side critical section: the teardown path only unlinks these
> +	 * objects and frees them after a grace period.
> +	 */

[Severity: High]

Is this comment accurate for ndp and np? Only the channel free moved to
call_rcu(). The package and the device are still freed right after
list_del_rcu():

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_unregister_dev() {
	...
	spin_lock_irqsave(&ncsi_dev_lock, flags);
	list_del_rcu(&ndp->node);
	spin_unlock_irqrestore(&ncsi_dev_lock, flags);

	disable_work_sync(&ndp->work);

	kfree(ndp);
}

Inside the new rcu_read_lock() section, ncsi_write_package_info() still
reads ndp->package_num, ndp->package_whitelist and np->id, and it follows
the np->node and np->channels links.

ncsi_write_channel_info() reaches the parent package through a channel
that call_rcu() keeps alive:

	if (nc == nc->package->preferred_channel)

ncsi_pkg_info_all_nl() also holds on to package and reads package->id
after the walk.

ftgmac100_remove() calls ncsi_unregister_dev() before unregister_netdev().
So it looks like the same rmmod vs NCSI_CMD_PKG_INFO race from the commit
message can still cause a use-after-free, now on np or ndp instead of nc.

Do np and ndp also need kfree_rcu() or call_rcu(), or a synchronize_rcu()
before the kfree()?

> +	rcu_read_lock();
> +	ndp = ndp_from_ifindex(genl_info_net(info),
> +			       nla_get_u32(info->attrs[NCSI_ATTR_IFINDEX]));
> +	if (!ndp) {
> +		rcu_read_unlock();
> +		kfree_skb(skb);
> +		return -ENODEV;
> +	}
> +
>  	rc = ncsi_write_package_info(skb, ndp, package_id);
> +	rcu_read_unlock();

[Severity: Medium]

Do the other genl handlers in ncsi-netlink.c need the same change? These
handlers still call ndp_from_ifindex() and walk NCSI_FOR_EACH_PACKAGE() /
NCSI_FOR_EACH_CHANNEL() without rcu_read_lock():

ncsi_set_interface_nl()
ncsi_clear_interface_nl()
ncsi_send_cmd_nl()
ncsi_set_package_mask_nl()
ncsi_set_channel_mask_nl()

The commit message says "Either half alone leaves the race". On a
preemptible kernel, one of these walkers can be preempted mid-walk during
ncsi_unregister_dev(). The grace period can then end and
ncsi_channel_release() can free nc while the walker still uses it.

Several of these handlers also write through the pointers they find.
ncsi_set_interface_nl() stores the channel into package->preferred_channel
and updates package->channel_whitelist. ncsi_clear_interface_nl() takes
np->lock and writes np fields for each package.

Can these turn into use-after-free writes during teardown?

These ops are GENL_ADMIN_PERM and the family only works in init_net, so a
trigger needs CAP_NET_ADMIN. With CONFIG_PROVE_RCU_LIST these walks would
also cause suspicious RCU usage warnings.

>  
>  	if (rc) {
>  		nla_nest_cancel(skb, attr);
> @@ -228,20 +237,26 @@ static int ncsi_pkg_info_all_nl(struct sk_buff *skb,
>  	if (!attrs[NCSI_ATTR_IFINDEX])
>  		return -EINVAL;
>  
> +	package_id = cb->args[0];
> +
> +	rcu_read_lock();
>  	ndp = ndp_from_ifindex(get_net(sock_net(skb->sk)),
>  			       nla_get_u32(attrs[NCSI_ATTR_IFINDEX]));

[Severity: Medium]

This is a pre-existing issue and was not introduced by this patch, but does
ncsi_pkg_info_all_nl() leak a netns reference here?

get_net(sock_net(skb->sk)) takes a reference that ndp_from_ifindex() never
drops, and there is no put_net() anywhere in net/ncsi/. get_net() runs
before the lookup, so the reference leaks even when no NCSI device is
found.

The same pattern is in ncsi_set_interface_nl(), ncsi_clear_interface_nl(),
ncsi_send_cmd_nl(), ncsi_set_package_mask_nl() and
ncsi_set_channel_mask_nl().

The family does not set netnsok, so only init_net is affected, and its
refcount grows with every dump call.

Would plain sock_net() be enough here?

[ ... ]

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