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

[PATCH net v1] 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

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 v1] net/ncsi: fix use-after-free of ncsi_channel in ncsi_remove_channel()

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.