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

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

[Binbin Deng]

ncsi_remove_package() unlinks the package from ndp->packages with
list_del_rcu() and immediately kfree()s it, after having released all
of its channels the same way.  As with the channels, the generic
netlink package info walkers iterate ndp->packages with
list_for_each_entry_rcu() and dereference the package in the loop body
(nla_put_u8(NCSI_PKG_ATTR_ID, np->id), np->multi_channel,
np->channels, ...), so an immediate kfree() turns a mid-walk reader
into a use-after-free the moment a teardown overlaps it.

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

Free the package with kfree_rcu() so that it survives the grace period
the readers rely on.

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 while cycling insmod/rmmod ftgmac100:

  BUG: KASAN: slab-use-after-free in ncsi_write_package_info+0xe3c/0xf84
  Read of size 1 at addr c69e2580 by task poc_dump/69
   ncsi_write_package_info from ncsi_pkg_info_nl+0x320/0x63c
   netlink_unicast from netlink_sendmsg+0x62c/0xb98
  Freed by task 65:
   which belongs to the cache kmalloc-64 of size 64
  The buggy address is located 0 bytes inside of

struct ncsi_package is 60 bytes and lands in kmalloc-64; a size-1 read
at offset 0 of that cache can only be np->id in this walker (the
u16 vids[] array reads show up as "Read of size 2"), pinning the report
to the kfree() fixed here.

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 | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
index adee6dcabdc3..082862581ea6 100644
--- a/net/ncsi/internal.h
+++ b/net/ncsi/internal.h
@@ -253,6 +253,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;
 };
 
 struct ncsi_request {
diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index 54d0df0a9efe..76b2cb9efe4f 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -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);
 }
 
 void ncsi_find_package_and_channel(struct ncsi_dev_priv *ndp,
-- 
2.43.0

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 3 (1) · 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] kfree_rcu() only delays the free of np until existing RCU read-
  side critical sections end, and none of the NCSI netlink walkers of…
- [High] The child channels of the package this patch keeps alive are
  still freed with no grace period.
- [High] The netlink readers hold no reference to the parent struct
  ncsi_dev_priv, and the same teardown frees it immediately.
- [Low] The commit message describes the reader code inaccurately: - It
  says the walker does 'nla_put_u8(NCSI_PKG_ATTR_ID, np->id)'.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  ncsi_unregister_dev() frees every package and channel (net/ncsi/ncsi-…

--

net/ncsi: fix use-after-free of ncsi_package in ncsi_remove_package()

This patch adds an rcu_head to struct ncsi_package. It also changes
ncsi_remove_package() to free the package with kfree_rcu() instead of
kfree(). The aim is to stop the generic netlink package info walkers
from reading a freed package when they race with device teardown.

> ncsi_remove_package() unlinks the package from ndp->packages with
> list_del_rcu() and immediately kfree()s it, after having released all
> of its channels the same way.  As with the channels, the generic
> netlink package info walkers iterate ndp->packages with
> list_for_each_entry_rcu() and dereference the package in the loop body
> (nla_put_u8(NCSI_PKG_ATTR_ID, np->id), np->multi_channel,
> np->channels, ...), so an immediate kfree() turns a mid-walk reader
> into a use-after-free the moment a teardown overlaps it.

[Severity: Low]
Is this description of the reader accurate? ncsi_write_package_info() in
net/ncsi/ncsi-netlink.c writes the package ID with nla_put_u32():

	rc = nla_put_u32(skb, NCSI_PKG_ATTR_ID, np->id);

It also never reads np->multi_channel. Only the set/clear interface
handlers write that field.

> Free the package with kfree_rcu() so that it survives the grace period
> the readers rely on.

None of the NCSI netlink readers hold rcu_read_lock(), so do they rely on
a grace period at all? Could the commit message be updated to match the
code?

> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
> index 54d0df0a9efe6..76b2cb9efe4f7 100644
> --- a/net/ncsi/ncsi-manage.c
> +++ b/net/ncsi/ncsi-manage.c
> @@ -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);

[Severity: High]
Does kfree_rcu() actually protect the netlink readers here? It only waits
for RCU read-side critical sections that already exist. The walkers in
net/ncsi/ncsi-netlink.c don't seem to enter one:

ncsi_pkg_info_nl()
  ncsi_write_package_info()
    NCSI_FOR_EACH_PACKAGE(ndp, np) {
        if (np->id != id)
        ...
        NCSI_FOR_EACH_CHANNEL(np, nc) {

NCSI_FOR_EACH_PACKAGE() expands to list_for_each_entry_rcu(), but nothing
calls rcu_read_lock() around it. Genl doit and dumpit handlers run under
genl_mutex or cb_mutex, not RCU, and ncsi_unregister_dev() takes neither.

The same pattern shows up in other handlers:

  - ncsi_pkg_info_all_nl() keeps package after the walk and dereferences
    package->id later.
  - ncsi_set_interface_nl(), ncsi_clear_interface_nl() and
    ncsi_set_channel_mask_nl() write multi_channel, channel_whitelist and
    preferred_channel on the package they find.

The testing used a CONFIG_PREEMPT kernel. On such a kernel, a reader that
is preempted mid-walk outside rcu_read_lock() reports a quiescent state.
Can the grace period then finish and kfree(np) run while the reader is
still using np->id and np->channels?

The NCSI_CMD_PKG_INFO op has no GENL_ADMIN_PERM. An unprivileged user can
therefore run the reader while root runs rmmod ftgmac100.

Do the walkers need rcu_read_lock(), or some other serialization against
ncsi_unregister_dev(), to close this race?

[Severity: High]
The package now gets a grace period, but what about its channels? Before
it gets here, ncsi_remove_package() calls ncsi_remove_channel() on each
channel, and that still frees everything immediately:

net/ncsi/ncsi-manage.c:ncsi_remove_channel() {
	spin_lock_irqsave(&nc->lock, flags);

	/* Release filters */
	kfree(nc->mac_filter.addrs);
	kfree(nc->vlan_filter.vids);
	...
	list_del_rcu(&nc->node);
	np->channel_num--;
	spin_unlock_irqrestore(&np->lock, flags);

	kfree(nc);
}

The filter arrays are freed while nc is still linked on np->channels.
nc itself is then freed with no grace period.

The walker named in the commit message reads both without nc->lock or
RCU:

net/ncsi/ncsi-netlink.c:ncsi_write_channel_info() {
	...
	nla_put_u32(skb, NCSI_CHANNEL_ATTR_ID, nc->id);
	...
	if (nc == nc->package->preferred_channel)
	...
		if (ncf->vids[i])
			nla_put_u16(skb, NCSI_CHANNEL_ATTR_VLAN_ID,
				    ncf->vids[i]);
	...
}

Say a PKG_INFO reader on one CPU is inside NCSI_FOR_EACH_CHANNEL(np, nc).
At the same time another CPU runs
ncsi_unregister_dev()->ncsi_remove_package()->ncsi_remove_channel().

Can the reader then access the freed vids[] array, or the freed nc and
nc->node.next? This doesn't depend on preemption, because channels get no
grace period at all.

struct ncsi_channel has no rcu_head. Also, switching nc to kfree_rcu()
alone would still leave the vids and addrs arrays freed immediately.

[Severity: High]
Is the parent struct ncsi_dev_priv open to the same race? The readers get
ndp from ndp_from_ifindex(), which drops the netdev reference and returns
ndp without pinning it:

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

The same teardown then frees ndp with no grace period:

net/ncsi/ncsi-manage.c:ncsi_unregister_dev() {
	...
	list_del_rcu(&ndp->node);
	spin_unlock_irqrestore(&ncsi_dev_lock, flags);

	disable_work_sync(&ndp->work);

	kfree(ndp);
}

ftgmac100_remove() calls ncsi_unregister_dev() before unregister_netdev(),
so dev_get_by_index() keeps succeeding during teardown.

In ncsi_pkg_info_nl(), the call to genlmsg_new(NLMSG_DEFAULT_SIZE,
GFP_KERNEL) sits between ndp_from_ifindex() and ncsi_write_package_info(),
and it can sleep.

Could ndp be freed during that window? ncsi_write_package_info() would
then read ndp->package_num, walk ndp->packages and read
ndp->package_whitelist from freed memory. This window exists even on
non-preemptible kernels.

[Severity: High]
This isn't a bug introduced by this patch, but ncsi_unregister_dev()
frees all packages and channels before it stops the work item:

net/ncsi/ncsi-manage.c:ncsi_unregister_dev() {
	...
	list_for_each_entry_safe(np, tmp, &ndp->packages, node)
		ncsi_remove_package(np);
	...
	disable_work_sync(&ndp->work);
	...
}

ncsi_dev_work() can still be queued or running at that point. Response
handlers, request timeout timers and ncsi_reset_dev() all schedule it.
ftgmac100_remove() can also call ncsi_unregister_dev() while the interface
is still up.

The work dispatches to ncsi_configure_channel() and ncsi_suspend_channel().
Both load the active package and channel:

	np = ndp->active_package;
	nc = ndp->active_channel;

They then use np->id, nc->id and other channel state.

The work item isn't an RCU reader, and nothing serializes it against
ncsi_remove_package() or ncsi_remove_channel(). Can it use a package or
channel that was just freed? kfree_rcu(np) wouldn't help here.

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