* [PATCH net v3] net/ncsi: fix use-after-free of ncsi_channel in ncsi_remove_channel()
@ 2026-10-19 1:37 Binbin Deng
2026-10-20 1:47 ` netdev-bot+sashiko
0 siblings, 1 reply; 2+ messages in thread
From: Binbin Deng @ 2026-10-19 1:37 UTC (permalink / raw)
To: sam, fercerpav, davem, edumazet, kuba, pabeni, horms, gwshan, joel
Cc: netdev, linux-kernel, Binbin Deng
ncsi_remove_channel() frees the channel filter arrays and the channel
itself immediately after list_del_rcu():
kfree(nc->mac_filter.addrs);
kfree(nc->vlan_filter.vids);
...
list_del_rcu(&nc->node);
...
kfree(nc);
The generic netlink package info walkers iterate np->channels with the
list_for_each_entry_rcu() based NCSI_FOR_EACH_CHANNEL() and
dereference deep into the channel: ncsi_write_channel_info() reads
nc->version.fw_name with nla_put_string() (strlen) and iterates
nc->vlan_filter.vids[]. No NCSI genl handler enters an RCU read-side
critical section -- genl doit and dumpit callbacks run under
genl_mutex and cb_mutex -- so a walk that overlaps the teardown reads
freed memory. NCSI_CMD_PKG_INFO has no GENL_ADMIN_PERM, so an
unprivileged user can race it against rmmod of the driver:
writer: rmmod ftgmac100
-> ftgmac100_remove() -> ncsi_unregister_dev()
-> ncsi_remove_package() -> ncsi_remove_channel()
reader: sendmsg(NETLINK_GENERIC, NCSI_CMD_PKG_INFO)
-> ncsi_pkg_info_nl() -> ncsi_write_package_info()
KASAN reports (arm multi_v7, QEMU ast2600-evb, KASAN + SLUB + PREEMPT,
four threads looping PKG_INFO doit/dump while cycling insmod/rmmod):
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
The buggy address belongs to the object at c6901800
which belongs to the cache kmalloc-1k of size 1024
The buggy address is located 480 bytes inside of
freed 1024-byte region [c6901800, c6901c00)
The 480-byte offset is nc->version.fw_name, read by the strlen() of
the nla_put_string() in ncsi_write_channel_info() inlined into the
walker; kmalloc-1k is the size class of struct ncsi_channel. The
dump variant of the same reader is caught reading the freed filter
array instead:
BUG: KASAN: slab-use-after-free in ncsi_write_package_info+0xb6c/0xf84
Read of size 2 at addr c6b03600 by task poc_dump/72
The buggy address belongs to the object at c6b03600
which belongs to the cache kmalloc-64 of size 64
The buggy address is located 0 bytes inside of
freed 64-byte region [c6b03600, c6b03640)
which is the u16 vlan_filter.vids[] walk of the same handler.
Fix both halves of the contract for the channel:
* The free of the channel and of the filter arrays, which are only
reachable through the channel, moves into an RCU callback.
* All seven genl handlers now resolve the device and walk the lists
inside an RCU read-side critical section. The critical sections
are non-sleeping: the reply skb of the PKG_INFO doit is allocated
before the lock is taken (genlmsg_new(GFP_KERNEL) must not run
under rcu_read_lock()) and its unicast reply is sent after the
walk; NCSI_CMD_SEND_CMD runs ncsi_xmit_cmd() under the lock (it
only uses GFP_ATOMIC command skbs, spinlocks and
dev_queue_xmit()) and defers its error notification, which uses
the net_device captured inside the critical section.
* The handlers used get_net(sock_net(skb->sk)), which takes a
reference on init_net that is never dropped anywhere in
net/ncsi/; plain sock_net() is enough as the socket pins the
netns.
The channel configuration worker is not covered by the RCU callback
on purpose: it is quiesced by the companion series "net/ncsi: fix
use-after-free of ncsi_dev_priv in ncsi_unregister_dev()", which runs
disable_work_sync() before any object is released and stops every
channel monitor before the removal loop, so nothing re-arms them.
The packages and the device itself are deferred by that series and
by "net/ncsi: fix use-after-free of ncsi_package in
ncsi_remove_package()"; the race is closed by the three series
together.
Found by a Coccinelle rule matching kfree() immediately after
list_del_rcu(); the code inspection and the fix were LLM-assisted,
and the race was confirmed at runtime with KASAN as above.
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 | 97 +++++++++++++++++++++++++++++++----------
3 files changed, 90 insertions(+), 28 deletions(-)
diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
index 3630ee4..a077e90 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 70cb31e..dc7f333 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);
+ /* Netlink walkers may still hold the channel or its filter
+ * arrays; the arrays are only reachable through @nc and share
+ * its 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 8cc5383..7e4c77e 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,24 @@ static int ncsi_pkg_info_nl(struct sk_buff *msg, struct genl_info *info)
kfree_skb(skb);
return -EMSGSIZE;
}
+
+ /* Resolve the device and walk its lists inside an RCU read-side
+ * critical section. The channel and its filter arrays are freed
+ * from an RCU callback in this series; the packages and the
+ * device itself get their grace periods from the companion fixes
+ * for ncsi_remove_package() and ncsi_unregister_dev().
+ */
+ 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 +240,25 @@ static int ncsi_pkg_info_all_nl(struct sk_buff *skb,
if (!attrs[NCSI_ATTR_IFINDEX])
return -EINVAL;
- ndp = ndp_from_ifindex(get_net(sock_net(skb->sk)),
- nla_get_u32(attrs[NCSI_ATTR_IFINDEX]));
+ package_id = cb->args[0];
- if (!ndp)
+ rcu_read_lock();
+ ndp = ndp_from_ifindex(sock_net(skb->sk),
+ nla_get_u32(attrs[NCSI_ATTR_IFINDEX]));
+ 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);
@@ -266,8 +283,10 @@ static int ncsi_pkg_info_all_nl(struct sk_buff *skb,
cb->args[0] = package_id + 1;
+ rcu_read_unlock();
return skb->len;
err:
+ rcu_read_unlock();
genlmsg_cancel(skb, hdr);
return rc;
}
@@ -289,10 +308,13 @@ static int ncsi_set_interface_nl(struct sk_buff *msg, struct genl_info *info)
if (!info->attrs[NCSI_ATTR_PACKAGE_ID])
return -EINVAL;
- ndp = ndp_from_ifindex(get_net(sock_net(msg->sk)),
+ rcu_read_lock();
+ ndp = ndp_from_ifindex(sock_net(msg->sk),
nla_get_u32(info->attrs[NCSI_ATTR_IFINDEX]));
- if (!ndp)
+ if (!ndp) {
+ rcu_read_unlock();
return -ENODEV;
+ }
package_id = nla_get_u32(info->attrs[NCSI_ATTR_PACKAGE_ID]);
package = NULL;
@@ -302,6 +324,7 @@ static int ncsi_set_interface_nl(struct sk_buff *msg, struct genl_info *info)
package = np;
if (!package) {
/* The user has set a package that does not exist */
+ rcu_read_unlock();
return -ERANGE;
}
@@ -317,6 +340,7 @@ static int ncsi_set_interface_nl(struct sk_buff *msg, struct genl_info *info)
netdev_info(ndp->ndev.dev,
"NCSI: Channel %u does not exist!\n",
channel_id);
+ rcu_read_unlock();
return -ERANGE;
}
}
@@ -350,6 +374,7 @@ static int ncsi_set_interface_nl(struct sk_buff *msg, struct genl_info *info)
if (!(ndp->flags & NCSI_DEV_RESET))
ncsi_reset_dev(&ndp->ndev);
+ rcu_read_unlock();
return 0;
}
@@ -365,10 +390,13 @@ static int ncsi_clear_interface_nl(struct sk_buff *msg, struct genl_info *info)
if (!info->attrs[NCSI_ATTR_IFINDEX])
return -EINVAL;
- ndp = ndp_from_ifindex(get_net(sock_net(msg->sk)),
+ rcu_read_lock();
+ ndp = ndp_from_ifindex(sock_net(msg->sk),
nla_get_u32(info->attrs[NCSI_ATTR_IFINDEX]));
- if (!ndp)
+ if (!ndp) {
+ rcu_read_unlock();
return -ENODEV;
+ }
/* Reset any whitelists and disable multi mode */
spin_lock_irqsave(&ndp->lock, flags);
@@ -389,12 +417,14 @@ static int ncsi_clear_interface_nl(struct sk_buff *msg, struct genl_info *info)
if (!(ndp->flags & NCSI_DEV_RESET))
ncsi_reset_dev(&ndp->ndev);
+ rcu_read_unlock();
return 0;
}
static int ncsi_send_cmd_nl(struct sk_buff *msg, struct genl_info *info)
{
struct ncsi_dev_priv *ndp;
+ struct net_device *dev;
struct ncsi_pkt_hdr *hdr;
struct ncsi_cmd_arg nca;
unsigned char *data;
@@ -427,17 +457,21 @@ static int ncsi_send_cmd_nl(struct sk_buff *msg, struct genl_info *info)
goto out;
}
- ndp = ndp_from_ifindex(get_net(sock_net(msg->sk)),
+ rcu_read_lock();
+ ndp = ndp_from_ifindex(sock_net(msg->sk),
nla_get_u32(info->attrs[NCSI_ATTR_IFINDEX]));
if (!ndp) {
+ rcu_read_unlock();
ret = -ENODEV;
goto out;
}
+ dev = ndp->ndev.dev;
package_id = nla_get_u32(info->attrs[NCSI_ATTR_PACKAGE_ID]);
channel_id = nla_get_u32(info->attrs[NCSI_ATTR_CHANNEL_ID]);
if (package_id >= NCSI_MAX_PACKAGE || channel_id >= NCSI_MAX_CHANNEL) {
+ rcu_read_unlock();
ret = -ERANGE;
goto out_netlink;
}
@@ -446,6 +480,7 @@ static int ncsi_send_cmd_nl(struct sk_buff *msg, struct genl_info *info)
if (len < sizeof(struct ncsi_pkt_hdr)) {
netdev_info(ndp->ndev.dev, "NCSI: no command to send %u\n",
package_id);
+ rcu_read_unlock();
ret = -EINVAL;
goto out_netlink;
} else {
@@ -462,18 +497,25 @@ static int ncsi_send_cmd_nl(struct sk_buff *msg, struct genl_info *info)
nca.info = info;
nca.payload = ntohs(hdr->length);
if (nca.payload > len - sizeof(*hdr)) {
+ rcu_read_unlock();
ret = -EINVAL;
goto out_netlink;
}
nca.data = data + sizeof(*hdr);
+ /* ncsi_xmit_cmd() is safe to run under the RCU read-side
+ * critical section: it only uses GFP_ATOMIC command skbs,
+ * spinlocks and dev_queue_xmit().
+ */
ret = ncsi_xmit_cmd(&nca);
+ rcu_read_unlock();
+
out_netlink:
if (ret != 0) {
- netdev_err(ndp->ndev.dev,
+ netdev_err(dev,
"NCSI: Error %d sending command\n",
ret);
- ncsi_send_netlink_err(ndp->ndev.dev,
+ ncsi_send_netlink_err(dev,
info->snd_seq,
info->snd_portid,
info->nlhdr,
@@ -608,10 +650,13 @@ static int ncsi_set_package_mask_nl(struct sk_buff *msg,
if (!info->attrs[NCSI_ATTR_PACKAGE_MASK])
return -EINVAL;
- ndp = ndp_from_ifindex(get_net(sock_net(msg->sk)),
+ rcu_read_lock();
+ ndp = ndp_from_ifindex(sock_net(msg->sk),
nla_get_u32(info->attrs[NCSI_ATTR_IFINDEX]));
- if (!ndp)
+ if (!ndp) {
+ rcu_read_unlock();
return -ENODEV;
+ }
spin_lock_irqsave(&ndp->lock, flags);
if (nla_get_flag(info->attrs[NCSI_ATTR_MULTI_FLAG])) {
@@ -639,6 +684,7 @@ static int ncsi_set_package_mask_nl(struct sk_buff *msg,
ncsi_reset_dev(&ndp->ndev);
}
+ rcu_read_unlock();
return rc;
}
@@ -663,10 +709,13 @@ static int ncsi_set_channel_mask_nl(struct sk_buff *msg,
if (!info->attrs[NCSI_ATTR_CHANNEL_MASK])
return -EINVAL;
- ndp = ndp_from_ifindex(get_net(sock_net(msg->sk)),
+ rcu_read_lock();
+ ndp = ndp_from_ifindex(sock_net(msg->sk),
nla_get_u32(info->attrs[NCSI_ATTR_IFINDEX]));
- if (!ndp)
+ if (!ndp) {
+ rcu_read_unlock();
return -ENODEV;
+ }
package_id = nla_get_u32(info->attrs[NCSI_ATTR_PACKAGE_ID]);
package = NULL;
@@ -675,8 +724,10 @@ static int ncsi_set_channel_mask_nl(struct sk_buff *msg,
package = np;
break;
}
- if (!package)
+ if (!package) {
+ rcu_read_unlock();
return -ERANGE;
+ }
spin_lock_irqsave(&package->lock, flags);
@@ -690,6 +741,7 @@ static int ncsi_set_channel_mask_nl(struct sk_buff *msg,
}
if (!channel) {
spin_unlock_irqrestore(&package->lock, flags);
+ rcu_read_unlock();
return -ERANGE;
}
netdev_dbg(ndp->ndev.dev,
@@ -721,6 +773,7 @@ static int ncsi_set_channel_mask_nl(struct sk_buff *msg,
if (!(ndp->flags & NCSI_DEV_RESET))
ncsi_reset_dev(&ndp->ndev);
+ rcu_read_unlock();
return 0;
}
--
2.43.0
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH net v3] net/ncsi: fix use-after-free of ncsi_channel in ncsi_remove_channel()
2026-10-19 1:37 [PATCH net v3] net/ncsi: fix use-after-free of ncsi_channel in ncsi_remove_channel() Binbin Deng
@ 2026-10-20 1:47 ` netdev-bot+sashiko
0 siblings, 0 replies; 2+ messages in thread
From: netdev-bot+sashiko @ 2026-10-20 1:47 UTC (permalink / raw)
To: 18983559317
Cc: sam, fercerpav, davem, edumazet, kuba, pabeni, horms, gwshan,
joel, netdev, linux-kernel
Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider.
Critical: 0 · High: 2 (3) · Medium: 1 · 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] In this tree, only the channel is freed after an RCU grace
period.
- [High] ncsi_send_cmd_nl() (net/ncsi/ncsi-netlink.c) saves `dev =
ndp->ndev.dev` inside the new rcu_read_lock() section.
- [Medium] ncsi_remove_channel() only unlinks the channel from
np->channels (list_del_rcu(&nc->node)) before call_rcu(&nc->rcu,…
- [Low] The new comment in ncsi_pkg_info_nl() (net/ncsi/ncsi-netlink.c)
has two problems.
Pre-existing issues:
- [High] ncsi_vlan_rx_kill_vid() (net/ncsi/ncsi-manage.c:1743-1748) has
the same pattern the commit's Coccinelle rule looks for:…
- [High] ncsi_register_dev() sets up a timer for each of the 256
ndp->requests[i] entries embedded in ndp (timer_setup(...,…
- [High] The patch protects channel reclamation with list_del_rcu() plus
call_rcu(), which only protects readers inside rcu_read_lock().
--
net/ncsi: fix use-after-free of ncsi_channel in ncsi_remove_channel()
This patch moves the free of struct ncsi_channel and its filter arrays
into an RCU callback. It also wraps the device lookup and list walks of
the seven NCSI generic netlink handlers in rcu_read_lock(), and drops the
unbalanced get_net() calls. The commit message says the race is only
fully closed together with two companion series for ncsi_remove_package()
and ncsi_unregister_dev().
> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
> index 54d0df0a9efe6..2b117cfd0d6ab 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);
> + /* Netlink walkers may still hold the channel or its filter
> + * arrays; the arrays are only reachable through @nc and share
> + * its grace period.
> + */
> + call_rcu(&nc->rcu, ncsi_channel_release);
[Severity: Medium]
Is np->channels the only place where this channel is still published when
call_rcu() is queued?
ncsi_remove_channel() only does list_del_rcu(&nc->node). It does not
take nc->link off ndp->channel_queue, which is added to with
list_add_tail_rcu() and read with list_first_or_null_rcu(). It also does
not clear ndp->active_channel or np->preferred_channel when they point
at nc.
If the channel is queued for multi-channel configuration, it stays on
channel_queue after the grace period ends. Later walkers would then touch
the freed channel, for example:
ncsi_reset_dev()
list_for_each_entry_safe(nc, tmp, &ndp->channel_queue, link)
list_del_init(&nc->link);
That walk is reached from the SET_INTERFACE, CLEAR_INTERFACE,
SET_PACKAGE_MASK and SET_CHANNEL_MASK handlers.
ncsi_process_next_channel() also reads the queue.
There is also a window where a genl handler finds nc during its RCU walk
of np->channels in ncsi_reset_dev()->ncsi_choose_active_channel(), just
before the concurrent list_del_rcu(). ncsi_remove_channel() has already
set nc->state = NCSI_CHANNEL_INACTIVE, so the channel passes the filter
and can be queued again:
ncsi_choose_active_channel()
list_add_tail_rcu(&nc->link, &ndp->channel_queue);
After that, ncsi_process_next_channel() may store it in
ndp->active_channel. The channel is then still reachable after the
reader's rcu_read_unlock(), and it gets freed while still linked.
Should ncsi_remove_channel() also unlink nc->link and clear these
pointers before queueing the callback?
[Severity: High]
This isn't a bug introduced by this patch, but call_rcu() only waits for
readers inside rcu_read_lock(). Several process-context walkers of
np->channels use NCSI_FOR_EACH_CHANNEL() without taking it. One example
is ncsi_stop_dev(), which is reached from ndo_stop under RTNL:
ncsi_stop_dev()
NCSI_FOR_EACH_PACKAGE(ndp, np) {
NCSI_FOR_EACH_CHANNEL(np, nc) {
ncsi_stop_channel_monitor(nc);
These walkers have the same problem:
- the ncsi_start_dev()->ncsi_reset_dev() walk;
- ncsi_kick_channels() and ncsi_choose_active_channel() when they are
called outside the genl handlers.
ftgmac100_remove() calls ncsi_unregister_dev() without RTNL and before
unregister_netdev(), so the netdev can still be brought up or down at
the same time. With PREEMPT_RCU, holding RTNL does not make the task an
RCU reader:
CPU0 CPU1
ip link set ethX down rmmod ftgmac100
ftgmac100_stop() ftgmac100_remove()
ncsi_stop_dev() ncsi_unregister_dev()
walking np->channels ncsi_remove_package()
ncsi_remove_channel()
list_del_rcu()
call_rcu()
grace period ends, nc freed
ncsi_stop_channel_monitor(nc)
Can CPU0 then read nc->node.next, nc->lock and nc->monitor.timer from the
freed channel?
[Severity: High]
This is a pre-existing issue, but the Coccinelle rule described in the
commit message (kfree() right after list_del_rcu()) would also match
ncsi_vlan_rx_kill_vid():
net/ncsi/ncsi-manage.c:ncsi_vlan_rx_kill_vid() {
...
list_for_each_entry_safe(vlan, tmp, &ndp->vlan_vids, list)
if (vlan->vid == vid) {
netdev_dbg(dev, "NCSI: vid %u found, removing\n", vid);
list_del_rcu(&vlan->list);
found = true;
kfree(vlan);
}
...
}
set_one_vid() walks ndp->vlan_vids with list_for_each_entry_rcu() under
rcu_read_lock(). It runs from ncsi_configure_channel(), driven by the
NCSI worker and the response path, not under RTNL. The writer runs from
ndo_vlan_rx_kill_vid under RTNL and shares no lock with the reader.
Can set_one_vid() read vlan->vid and vlan->list.next from a freed
struct vlan_vid here? Neither this patch nor the companion series named
in the commit message seems to cover this site.
[Severity: High]
This is also a pre-existing issue, but on the same rmmod path: what
happens to the request timers embedded in ndp?
ncsi_register_dev() sets up a timer for each of the 256 ndp->requests[]
entries:
timer_setup(&ndp->requests[i].timer, ncsi_request_timeout, 0);
ncsi_xmit_cmd() arms one for every command, including NCSI_CMD_SEND_CMD
and the periodic channel monitor commands:
nr->enabled = true;
mod_timer(&nr->timer, jiffies + 1 * HZ);
The only timer_delete_sync() on these timers is in ncsi_free_request().
ncsi_unregister_dev() stops response processing and then frees ndp
without cancelling them:
ncsi_unregister_dev()
dev_remove_pack(&ndp->ptype);
...
disable_work_sync(&ndp->work);
kfree(ndp);
If a request is still outstanding at teardown, could
ncsi_request_timeout() fire up to a second later on the freed ndp? It
would take ndp->lock, write nr->enabled = false and call
ncsi_free_request(). The outstanding nr->cmd and nr->rsp skbs also look
like they would be leaked.
> }
[ ... ]
> diff --git a/net/ncsi/ncsi-netlink.c b/net/ncsi/ncsi-netlink.c
> index 8cc538358f6a3..7e4c77eef2d8f 100644
> --- a/net/ncsi/ncsi-netlink.c
> +++ b/net/ncsi/ncsi-netlink.c
[ ... ]
> @@ -192,7 +187,24 @@ static int ncsi_pkg_info_nl(struct sk_buff *msg, struct genl_info *info)
> kfree_skb(skb);
> return -EMSGSIZE;
> }
> +
> + /* Resolve the device and walk its lists inside an RCU read-side
> + * critical section. The channel and its filter arrays are freed
> + * from an RCU callback in this series; the packages and the
> + * device itself get their grace periods from the companion fixes
> + * for ncsi_remove_package() and ncsi_unregister_dev().
> + */
[Severity: Low]
This isn't a bug, but this comment refers to "this series" and to
"companion fixes". Neither will mean anything once the patch is merged.
The comment also doesn't match the code at this revision.
ncsi_remove_package() still calls kfree(np) right after list_del_rcu().
ncsi_unregister_dev() calls kfree(ndp) right after list_del_rcu() and
disable_work_sync().
Could the comment describe the lifetime rules that actually apply in
the tree?
> + 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: High]
Does this rcu_read_lock() protect anything other than the channel at
this revision?
ncsi_remove_package() and ncsi_unregister_dev() still free the package
and the device without a grace period:
ncsi_remove_package()
list_del_rcu(&np->node);
ndp->package_num--;
spin_unlock_irqrestore(&ndp->lock, flags);
kfree(np);
ncsi_unregister_dev()
list_for_each_entry_safe(np, tmp, &ndp->packages, node)
ncsi_remove_package(np);
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);
All packages are removed while ndp is still on ncsi_dev_list. Also,
ftgmac100_remove() calls ncsi_unregister_dev() before
unregister_netdev(). So ndp_from_ifindex() can still find ndp for the
whole teardown.
An unprivileged NCSI_CMD_PKG_INFO request racing rmmod could then read
these fields from freed memory in ncsi_write_package_info():
- ndp->package_num, ndp->package_whitelist and ndp->packages;
- np->id and np->channels.
ncsi_write_channel_info() also uses a channel that RCU keeps alive to
reach its parent package, which may already be freed:
if (nc == nc->package->preferred_channel)
The same applies to ncsi_pkg_info_all_nl().
The commit message says the race is closed by the three series
together. Neither companion change is in this tree, though. The only
call_rcu() in net/ncsi/ is the new one in ncsi_remove_channel(). With
the Fixes: tags, this patch could be backported or bisected on its own.
Would it make sense to send it together with, or after, the package and
device lifetime changes?
[ ... ]
> @@ -427,17 +457,21 @@ static int ncsi_send_cmd_nl(struct sk_buff *msg, struct genl_info *info)
> goto out;
> }
>
> - ndp = ndp_from_ifindex(get_net(sock_net(msg->sk)),
> + rcu_read_lock();
> + ndp = ndp_from_ifindex(sock_net(msg->sk),
> nla_get_u32(info->attrs[NCSI_ATTR_IFINDEX]));
> if (!ndp) {
> + rcu_read_unlock();
> ret = -ENODEV;
> goto out;
> }
> + dev = ndp->ndev.dev;
>
> package_id = nla_get_u32(info->attrs[NCSI_ATTR_PACKAGE_ID]);
> channel_id = nla_get_u32(info->attrs[NCSI_ATTR_CHANNEL_ID]);
>
> if (package_id >= NCSI_MAX_PACKAGE || channel_id >= NCSI_MAX_CHANNEL) {
> + rcu_read_unlock();
> ret = -ERANGE;
> goto out_netlink;
> }
[ ... ]
> @@ -462,18 +497,25 @@ static int ncsi_send_cmd_nl(struct sk_buff *msg, struct genl_info *info)
> nca.info = info;
> nca.payload = ntohs(hdr->length);
> if (nca.payload > len - sizeof(*hdr)) {
> + rcu_read_unlock();
> ret = -EINVAL;
> goto out_netlink;
> }
> nca.data = data + sizeof(*hdr);
>
> + /* ncsi_xmit_cmd() is safe to run under the RCU read-side
> + * critical section: it only uses GFP_ATOMIC command skbs,
> + * spinlocks and dev_queue_xmit().
> + */
> ret = ncsi_xmit_cmd(&nca);
> + rcu_read_unlock();
> +
> out_netlink:
> if (ret != 0) {
> - netdev_err(ndp->ndev.dev,
> + netdev_err(dev,
> "NCSI: Error %d sending command\n",
> ret);
> - ncsi_send_netlink_err(ndp->ndev.dev,
> + ncsi_send_netlink_err(dev,
> info->snd_seq,
> info->snd_portid,
> info->nlhdr,
[Severity: High]
dev is saved inside the RCU section, but every path that reaches
out_netlink calls rcu_read_unlock() first. That includes the -ERANGE and
-EINVAL branches above and a failing ncsi_xmit_cmd().
Is anything keeping the net_device alive at this point?
ndp_from_ifindex() drops its reference before it returns:
nd = ncsi_find_dev(dev);
ndp = nd ? TO_NCSI_DEV_PRIV(nd) : NULL;
dev_put(dev);
return ndp;
The handler does not take its own dev_hold() or netdev_hold() either.
After rcu_read_unlock() the task is neither an RCU reader nor a
reference holder. On PREEMPT it could be preempted while
ftgmac100_remove() finishes three steps:
- ncsi_unregister_dev();
- unregister_netdev();
- the devres free of the devm_alloc_etherdev() netdev.
netdev_err(dev, ...) and ncsi_send_netlink_err(dev, ...) would then
read the freed net_device. ncsi_send_netlink_err() also uses
dev_net(dev) to find net->genl_sock for nlmsg_unicast().
The commit message describes the deferred error notification as safe
because it uses the net_device saved under the RCU lock. That pointer is
only valid until rcu_read_unlock(), though.
Should the handler hold a reference on dev, or send the error before
dropping the RCU lock?
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261019013757.450036-1-18983559317%40163.com
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2026-10-20 1:47 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-10-19 1:37 [PATCH net v3] net/ncsi: fix use-after-free of ncsi_channel in ncsi_remove_channel() Binbin Deng
2026-10-20 1:47 ` netdev-bot+sashiko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®