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

[PATCH net v2] 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 then frees it with kfree(), after having released
all of its channels.  The generic netlink package info walkers iterate
the same list with the list_for_each_entry_rcu() based
NCSI_FOR_EACH_PACKAGE() and dereference the package in the loop body:

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

None of the NCSI genl handlers enters an RCU read-side critical
section: genl doit and dumpit callbacks run under genl_mutex and
cb_mutex, not under rcu_read_lock(), and ncsi_unregister_dev() takes
neither.  A reader that is preempted mid-walk -- or one that has
resolved the device and then sleeps in the genlmsg_new(GFP_KERNEL)
allocation of ncsi_pkg_info_nl() -- has no protection at all when the
teardown frees the package under it.

The race needs no real NC-SI hardware and is reachable by an
unprivileged user, as NCSI_CMD_PKG_INFO has no GENL_ADMIN_PERM:

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

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

KASAN report (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+0xe3c/0xf84
  Read of size 1 at addr c69e2580 by task poc_dump/69
  The buggy address belongs to the object at c69e2580
   which belongs to the cache kmalloc-64 of size 64
  The buggy address is located 0 bytes inside of
   freed 64-byte region [c69e2580, c69e25c0)

The read is the u8 np->id feeding the nla_put_u32() above;
sizeof(struct ncsi_package) is 60 bytes, so it lands in kmalloc-64,
and with no VLAN churn in this mode the only size-1 read at offset 0
of that cache on this path is np->id.

Free the package with kfree_rcu() so that it survives the grace
period, and give struct ncsi_package an rcu_head.

This is the free side of the fix for the package object.  The read
side -- rcu_read_lock() around the device resolution and the list
walks of all NCSI genl handlers -- is the companion series "net/ncsi:
fix use-after-free of ncsi_channel in ncsi_remove_channel()"; the
parent ncsi_dev_priv is deferred by the companion series "net/ncsi:
fix use-after-free of ncsi_dev_priv in ncsi_unregister_dev()".  The
race is closed by the three series together; each one keeps its own
Fixes: tag so they can be backported independently.

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")
Signed-off-by: Binbin Deng <18983559317@163.com>
---
 net/ncsi/internal.h    | 1 +
 net/ncsi/ncsi-manage.c | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
index adee6dc..cb41953 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 54d0df0..237e041 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -326,7 +326,8 @@ void ncsi_remove_package(struct ncsi_package *np)
 	ndp->package_num--;
 	spin_unlock_irqrestore(&ndp->lock, flags);
 
-	kfree(np);
+	/* Netlink walkers may still hold the package */
+	kfree_rcu(np, rcu);
 }
 
 void ncsi_find_package_and_channel(struct ncsi_dev_priv *ndp,
-- 
2.43.0

Re: [PATCH net v2] 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 3 potential
issue(s) to consider.

Critical: 0 · High: 3 · Medium: 0 · 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] On its own, this patch does not close the ncsi_package use-
  after-free its commit message describes.
- [High] ncsi_unregister_dev() (net/ncsi/ncsi-manage.c:1956-1974) frees
  every package via ncsi_remove_package() at line 1965, and only…
- [High] ncsi_remove_package() now defers only the package free.

--

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

This patch makes ncsi_remove_package() free the package with kfree_rcu()
instead of kfree(), and adds an rcu_head to struct ncsi_package. The
commit message calls this the free side of the fix. The read side and
the deferral of the parent object are left to two companion series.

[ ... ]

> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
> index 54d0df0a9efe6..237e0416e6d93 100644
> --- a/net/ncsi/ncsi-manage.c
> +++ b/net/ncsi/ncsi-manage.c
> @@ -326,7 +326,8 @@ void ncsi_remove_package(struct ncsi_package *np)
>  	ndp->package_num--;
>  	spin_unlock_irqrestore(&ndp->lock, flags);
>  
> -	kfree(np);
> +	/* Netlink walkers may still hold the package */
> +	kfree_rcu(np, rcu);

[Severity: High]

Does kfree_rcu() protect anything here if this patch is applied by
itself? It only waits for RCU read-side critical sections that already
exist. At this revision, none of the NCSI genl handlers enters one. The
only rcu_read_lock() in net/ncsi/ is the vlan_vids walk in
ncsi-manage.c.

For example, ncsi_pkg_info_nl() resolves ndp, sleeps in an allocation,
and then walks the list without rcu_read_lock():

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

ncsi_write_package_info() then does:

	NCSI_FOR_EACH_PACKAGE(ndp, np) {
		if (np->id != id)
			continue;

The dumpit handler ncsi_pkg_info_all_nl() keeps package after an
unprotected walk and later passes package->id to
ncsi_write_package_info().

ncsi_set_interface_nl() also keeps package. It then takes package->lock
and writes multi_channel, channel_whitelist and preferred_channel through
that pointer.

Under CONFIG_PREEMPT_RCU, a reader that is preempted or blocked outside
an RCU section does not hold up the grace period. So the kfree_rcu()
callback can run before the reader touches np. That matches the splat in
the commit message, which came from a PREEMPT kernel. NCSI_CMD_PKG_INFO
has no GENL_ADMIN_PERM, so unprivileged users can reach these readers.

ncsi_unregister_dev() also still frees ndp itself with a plain kfree().

The commit message says:

  "The race is closed by the three series together; each one keeps its
  own Fixes: tag so they can be backported independently."

If this patch is backported alone, the ncsi_package use-after-free is
still reachable. Should the dependency on the read-side series be stated
explicitly, or should the series be merged and backported as one unit?

Also, the read side can't just wrap ncsi_pkg_info_nl() in one
rcu_read_lock(). ndp is resolved before the GFP_KERNEL genlmsg_new()
call, and that call can sleep.

[Severity: High]

Is there also a reader of the package outside netlink that RCU deferral
can't cover? ncsi_unregister_dev() frees every package before it
disables the work item:

net/ncsi/ncsi-manage.c:ncsi_unregister_dev() {
	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);
}

A request timer can still expire after dev_remove_pack(). The timeout
path then queues the work again:

ncsi_request_timeout()
  ncsi_free_request()
	if (driven && cmd && --ndp->pending_req_num == 0)
		schedule_work(&ndp->work);

ncsi_dev_work() then runs ncsi_configure_channel(), which caches the
active package and channel outside any RCU section:

	struct ncsi_package *np = ndp->active_package;
	struct ncsi_channel *nc = ndp->active_channel;
	...
		nca.package = np->id;

ncsi_probe_channel() reads ndp->active_package->id in the same way.

ncsi_remove_package() never clears ndp->active_package or active_channel.
The active_package pointer also lives across separate work runs, so no
single RCU section could cover it.

Channels are still freed right away, and later config states write
channel state under nc->lock.

Would it work to disable ndp->work and stop the request timers before
the package loop in ncsi_unregister_dev()?

[Severity: High]

Only the package free is deferred here. Earlier in ncsi_remove_package(),
the child channels are still freed synchronously:

	list_for_each_entry_safe(nc, tmp, &np->channels, node)
		ncsi_remove_channel(nc);

ncsi_remove_channel() frees the filter arrays while the channel is still
linked on np->channels. It then frees the channel right after
list_del_rcu():

net/ncsi/ncsi-manage.c:ncsi_remove_channel() {
    ...
	/* 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);
}

Even if the package survives, can ncsi_write_package_info() still walk
into freed channels?

ncsi_write_package_info()
  NCSI_FOR_EACH_CHANNEL(np, nc)
    ncsi_write_channel_info()
      reads nc->version.fw_name and ncf->vids[i]

If so, freed heap contents would be copied into a netlink reply sent to
an unprivileged caller.

The commit message leaves the channel to the companion series. But the
filter arrays are freed before the channel is unlinked, so deferring nc
alone would not be enough. The filter frees would also need to move after
list_del_rcu() and be deferred.

Does the companion series handle that ordering?

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