From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id EEBE0386C3B for ; Thu, 3 Sep 2026 03:46:55 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1788407220; cv=none; b=LUGYmENO2IVsymvJocTYFix1b/LFAE1+EYqhvfzr4qqhnsthfXHfA9VCh5rX3lRPWzZbafHQ+Mq6LpS3AGqcrp7Eu7LUFKDOdoDJBDbiQeKKqP3KbbUB7bvofn3Adzka5Hkk73oYVqre8zeTOeyCBVki4eF/AKx6EFXqTzU6aQg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1788407220; c=relaxed/simple; bh=ty7L/Gclxc5Nq++dVrW9pRlTNZEIHoV56D5YyvNsNTg=; h=Subject:From:To:Cc:Date:Message-ID:In-Reply-To:References: Content-Type:MIME-Version; b=ewiWwsf7pDZ+awqhw2G6xSJAwsnojj/MfAbD9tb8jbH5Tf159fX17gWRevujV1pUKS8p4t7BTu7vGJc5tuqfzjEVXEs4GdRNtYf/h4kHRDh5leJ9+vb7KSWcgwgfeAfRyg5sBHeAH+zZtTl05ZDe6MHHvqnKDCoGdA6C/ZN3Edc= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=VORVjIrg; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="VORVjIrg" Received: by smtp.kernel.org (Postfix) with ESMTPSA id E00AD1F000E9; Thu, 3 Sep 2026 03:46:54 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1788407215; bh=uGxHSCyD/Z+nnwalCu+jrhKkfSyofWawOHG4iw7AhRA=; h=Subject:From:To:Cc:Date:In-Reply-To:References; b=VORVjIrggbVp/mrhmGcs8HekSm7SM/om2JGBPoEkhvhkgOPI+/UKr8x9sH57HNp1y Uxgxg00PpRe0Ko3z5R/DIBUK3KIK8D7Ea55dLPHVFrHC4/DdTB/452/bnwN8/VZNos /f8ZaUosbdQ3JkOWDXlgY7L3IKWrbmD7Yp+5lQkFi+AgvgfxkLaX1YOPPgyaJ1Y7/m Ch0zRiMAalvLETCQpMBn5RMS4Rd6/YThP6aXpLrJ2I48ny5sQ5fqRj1ULMPBHLwDqz zPWSdOffaKR4da74IwISPGHDBKBLGTG08vL/jM9/nXO6sovZChSBhZYu610M+AFfm7 +hZVKGUF6T8iA== Subject: Re: [v3] net/ncsi: Fix Use-After-Free in NCSI channel and package removal From: netdev-bot+sashiko@kernel.org To: wongboonjhee52@gmail.com Cc: netdev@vger.kernel.org, sam@mendozajonas.com, kuba@kernel.org Date: Thu, 03 Sep 2026 03:46:54 +0000 Message-ID: <178840721439.3394541.3495751154654451960@kernel.org> In-Reply-To: <20260830064337.327128-2-wongboonjhee52@gmail.com> References: <20260830064337.327128-2-wongboonjhee52@gmail.com> X-sashiko-severity: High Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit Precedence: bulk X-Mailing-List: netdev@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: Critical: 0 · High: 4 · Medium: 1 (3) · Low: 0 - [High] Two code paths newly created by this patch return with the RCU read-side critical section still held. - [High] The patch defers package/channel frees with call_rcu()/kfree_rcu() but leaves the main consumers outside any RCU… - [High] The new kref pins struct ncsi_dev_priv, but every netlink handler actually dereferences ndp->ndev.dev (the struct net_device),… - [High] The new pending-request sweep in ncsi_unregister_dev() runs before its producers are stopped. - [Medium] The commit message describes only (a) kfree_rcu()/call_rcu() deferral, (b) a kref on ncsi_dev_priv, and (c) rcu_read_lock() around… Pre-existing issues: - [Medium] The patch adds ncsi_dev_get()/ncsi_dev_put() and documents, in code, that the lookup must run 'while under RCU lock', but converts… - [Medium] ndp->channel_queue (linked through nc->link) is read with list_for_each_entry_rcu() in ncsi_channel_is_tx() (net/ncsi/ncsi-… - [Medium] Six NCSI netlink handlers call ndp_from_ifindex(get_net(sock_net(...)), ifindex) — net/ncsi/ncsi-… -- net/ncsi: Fix Use-After-Free in NCSI channel and package removal This converts the NCSI package and channel frees from kfree() to kfree_rcu()/call_rcu(), adds a kref to struct ncsi_dev_priv with new ncsi_dev_get()/ncsi_dev_put() helpers used by the netlink handlers, and wraps several lockless package/channel traversals in rcu_read_lock(). A few questions about the result below. The commit message says: This patch fixes the issue by implementing a proper kref and RCU lifetime model for NCSI objects. It replaces kfree() with kfree_rcu() / call_rcu() [...] Concurrent lockless readers are now explicitly protected with rcu_read_lock(). [Severity: Medium] Could the message also describe the other changes folded into this patch? The ones I could not find mentioned anywhere are: - ncsi_unregister_dev() is reordered so that dev_remove_pack() (and its synchronize_net()) and disable_work_sync() now run after a new pending-request sweep, and the package removal loop now runs after disable_work_sync(). - a new request-draining loop, "if (nr->enabled) ncsi_free_request(nr)". - a new bool work_cancelled field which gates exactly one of the nine schedule_work(&ndp->work) call sites in ncsi-manage.c. - ncsi_remove_channel() now unlinks the channel from ndp->channel_queue with list_del_rcu(&nc->link), which looks like the most concrete use-after-free actually fixed here. Also, on this sentence: Because there is no call to synchronize_rcu() or kfree_rcu(), concurrent readers traversing these lists under rcu_read_lock() (such as Netlink dump handlers) can still hold a valid pointer to the object. Were there any such readers before this patch? ncsi_pkg_info_all_nl() only gains its rcu_read_lock() in this same patch, so the described readers did not exist yet. > diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c > index 54d0df0a9efe..afc34c2b3613 100644 > --- a/net/ncsi/ncsi-manage.c > +++ b/net/ncsi/ncsi-manage.c [ ... ] > @@ -242,28 +252,37 @@ struct ncsi_channel *ncsi_add_channel(struct ncsi_package *np, unsigned char id) > return nc; > } > > +static void ncsi_channel_rcu_free(struct rcu_head *head) > +{ > + struct ncsi_channel *nc = container_of(head, 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; > + struct ncsi_dev_priv *ndp = np->ndp; > unsigned long flags; > > - spin_lock_irqsave(&nc->lock, flags); > - > - /* Release filters */ > - kfree(nc->mac_filter.addrs); > - kfree(nc->vlan_filter.vids); > + ncsi_stop_channel_monitor(nc); > > + spin_lock_irqsave(&nc->lock, flags); > nc->state = NCSI_CHANNEL_INACTIVE; > spin_unlock_irqrestore(&nc->lock, flags); > - ncsi_stop_channel_monitor(nc); > > - /* Remove and free channel */ > + spin_lock_irqsave(&ndp->lock, flags); > + list_del_rcu(&nc->link); > + spin_unlock_irqrestore(&ndp->lock, flags); > + [Severity: Medium] This isn't a bug introduced by this patch, but ndp->channel_queue now has three different removal primitives applied to it. ncsi_process_next_channel() and ncsi_reset_dev() both remove entries with list_del_init(): list_del_init(&nc->link); list_for_each_entry_safe(nc, tmp, &ndp->channel_queue, link) list_del_init(&nc->link); while the reader in ncsi_channel_is_tx() treats it as an RCU list: list_for_each_entry_rcu(channel, &ndp->channel_queue, link) { list_del_init() makes the removed node point at itself, so a reader positioned on it under list_for_each_entry_rcu() never reaches the head. Should whichever semantics is intended for this list be applied at all three removal sites, rather than only adding list_del_rcu() here? > spin_lock_irqsave(&np->lock, flags); > list_del_rcu(&nc->node); > np->channel_num--; > spin_unlock_irqrestore(&np->lock, flags); > > - kfree(nc); > + call_rcu(&nc->rcu, ncsi_channel_rcu_free); > } > > struct ncsi_package *ncsi_find_package(struct ncsi_dev_priv *ndp, [ ... ] > @@ -326,7 +345,7 @@ void ncsi_remove_package(struct ncsi_package *np) > ndp->package_num--; > spin_unlock_irqrestore(&ndp->lock, flags); > > - kfree(np); > + kfree_rcu(np, rcu); > } [ ... ] > @@ -409,7 +428,8 @@ void ncsi_free_request(struct ncsi_request *nr) > spin_unlock_irqrestore(&ndp->lock, flags); > > if (driven && cmd && --ndp->pending_req_num == 0) > - schedule_work(&ndp->work); > + if (!READ_ONCE(ndp->work_cancelled)) > + schedule_work(&ndp->work); > > /* Release command and response */ > consume_skb(cmd); [ ... ] > @@ -912,6 +932,7 @@ int ncsi_update_tx_channel(struct ncsi_dev_priv *ndp, > nca.req_flags = 0; > > /* Find current channel with Tx enabled */ > + rcu_read_lock(); > NCSI_FOR_EACH_PACKAGE(ndp, np) { > if (disable) > break; [ ... ] > @@ -951,6 +974,7 @@ int ncsi_update_tx_channel(struct ncsi_dev_priv *ndp, > } > } > } > + rcu_read_unlock(); > > if (disable == enable) > return -1; [Severity: High] Are the channel pointers found inside these read sections still safe to use after rcu_read_unlock()? Here disable and enable are looked up under RCU, and then dereferenced after the section ends: nca.channel = disable->id; nca.package = disable->package->id; ... ret = ncsi_xmit_cmd(&nca); ncsi_choose_active_channel() has the same shape - found is picked up inside the section and then linked into a live list afterwards: rcu_read_unlock(); if (list_empty(&ndp->channel_queue) && found) { ... list_add_tail_rcu(&found->link, &ndp->channel_queue); If found has been removed and call_rcu()'d in the meantime, does this splice a freed list node into ndp->channel_queue? Related: ncsi_reset_dev() walks the same lists with no RCU read section and no ndp->lock at all: active = NULL; NCSI_FOR_EACH_PACKAGE(ndp, np) { NCSI_FOR_EACH_CHANNEL(np, nc) { spin_lock_irqsave(&nc->lock, flags); Both NCSI_FOR_EACH_PACKAGE() and NCSI_FOR_EACH_CHANNEL() expand to list_for_each_entry_rcu(), and ncsi_reset_dev() is called by ncsi_set_interface_nl(), ncsi_clear_interface_nl(), ncsi_set_package_mask_nl() and ncsi_set_channel_mask_nl() after they drop rcu_read_lock(), holding only the new ndp kref - which pins ncsi_dev_priv, not the packages or channels. With the frees now deferred by a grace period rather than immediate, can a concurrent ncsi_unregister_dev() let ncsi_channel_rcu_free() run while ncsi_reset_dev() is touching nc->state and nc->lock? ncsi_kick_channels(), ncsi_channel_is_tx(), ncsi_stop_dev() and ncsi_find_package()/ncsi_find_package_and_channel() (the latter reached from the ncsi_request_timeout() timer) also still traverse these RCU lists without a read section, so CONFIG_PROVE_RCU_LIST should complain about them too. [ ... ] > @@ -1332,6 +1358,7 @@ static bool ncsi_check_hwa(struct ncsi_dev_priv *ndp) > /* The hardware arbitration is disabled if any one channel > * doesn't support explicitly. > */ > + rcu_read_lock(); > NCSI_FOR_EACH_PACKAGE(ndp, np) { > NCSI_FOR_EACH_CHANNEL(np, nc) { > has_channel = true; > @@ -1345,6 +1372,7 @@ static bool ncsi_check_hwa(struct ncsi_dev_priv *ndp) > } > } > } > + rcu_read_unlock(); > > if (has_channel) { > ndp->flags |= NCSI_DEV_HWA; [Severity: High] Does ncsi_check_hwa() return with the read section still held? The early exit inside the nested loop was not converted: cap = nc->caps[NCSI_CAP_GENERIC].cap; if (!(cap & NCSI_CAP_GENERIC_HWA) || (cap & NCSI_CAP_GENERIC_HWA_MASK) != NCSI_CAP_GENERIC_HWA_SUPPORT) { ndp->flags &= ~NCSI_DEV_HWA; return false; } That return is taken for any channel that does not advertise hardware arbitration, which is the common case during probing, and it skips the new rcu_read_unlock() after the loop. ncsi_channel_is_last() in this same patch was converted with "goto out" for exactly this reason. There is a second one in net/ncsi/ncsi-netlink.c, ncsi_set_channel_mask_nl(), which takes rcu_read_lock() before the package lookup but returns without unlocking when the channel id does not resolve: if (!channel) { spin_unlock_irqrestore(&package->lock, flags); ncsi_dev_put(ndp); return -ERANGE; } The sibling "if (!package)" path a few lines above does call rcu_read_unlock(). That one is reachable at will from user space by passing an NCSI_ATTR_CHANNEL_ID that does not exist. Returning to user space with rcu_read_lock_nesting (or the preempt count) left elevated should produce scheduling-while-atomic or lockdep splats, and since the task never leaves the read section, do grace periods stall so that the kfree_rcu()/call_rcu() frees added by this patch never run? [ ... ] > @@ -1952,23 +1981,65 @@ int ncsi_reset_dev(struct ncsi_dev *nd) > return 0; > } > > +static void ncsi_dev_release(struct kref *ref) > +{ > + struct ncsi_dev_priv *ndp; > + > + ndp = container_of(ref, struct ncsi_dev_priv, ref); > + kfree_rcu(ndp, rcu); > +} > + > +struct ncsi_dev_priv *ncsi_dev_get(struct net_device *dev) > +{ > + struct ncsi_dev_priv *ndp = NULL; > + struct ncsi_dev *nd; > + > + rcu_read_lock(); > + nd = ncsi_find_dev(dev); > + if (nd) { > + ndp = TO_NCSI_DEV_PRIV(nd); > + /* Safely grab a reference while under RCU lock */ > + if (!kref_get_unless_zero(&ndp->ref)) > + ndp = NULL; > + } > + rcu_read_unlock(); > + > + return ndp; > +} [Severity: Medium] This is a pre-existing pattern, but should the other ncsi_find_dev() callers be converted too? The comment here documents that the lookup must run "while under RCU lock", yet ncsi_vlan_rx_add_vid(), ncsi_vlan_rx_kill_vid(), ncsi_register_dev() and ncsi_rcv_rsp() still do: nd = ncsi_find_dev(dev); ... ndp = TO_NCSI_DEV_PRIV(nd); and ncsi_find_dev() iterates ncsi_dev_list with: #define NCSI_FOR_EACH_DEV(ndp) \ list_for_each_entry_rcu(ndp, &ncsi_dev_list, node) with neither a reference nor (for the vlan and register paths) an rcu_read_lock(). Since ncsi_dev_release() now frees ndp with kfree_rcu(), the deferral does not help those callers, and internal.h now advertises two lookup APIs with different safety contracts. > + > +void ncsi_dev_put(struct ncsi_dev_priv *ndp) > +{ > + if (ndp) > + kref_put(&ndp->ref, ncsi_dev_release); > +} > + > void ncsi_unregister_dev(struct ncsi_dev *nd) > { > struct ncsi_dev_priv *ndp = TO_NCSI_DEV_PRIV(nd); > struct ncsi_package *np, *tmp; > unsigned long flags; > > - dev_remove_pack(&ndp->ptype); > - > - 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); > > + spin_lock_irqsave(&ndp->lock, flags); > + ndp->work_cancelled = true; > + spin_unlock_irqrestore(&ndp->lock, flags); > + > + for (int i = 0; i < ARRAY_SIZE(ndp->requests); i++) { > + struct ncsi_request *nr = &ndp->requests[i]; > + > + if (nr->enabled) > + ncsi_free_request(nr); > + } > + > + dev_remove_pack(&ndp->ptype); > disable_work_sync(&ndp->work); > > - kfree(ndp); > + list_for_each_entry_safe(np, tmp, &ndp->packages, node) > + ncsi_remove_package(np); > + > + kref_put(&ndp->ref, ncsi_dev_release); > } > EXPORT_SYMBOL_GPL(ncsi_unregister_dev); [Severity: High] Does the new request sweep run before its producers are stopped? The order is now: unlink ndp from ncsi_dev_list, set work_cancelled, free every enabled request, dev_remove_pack(), disable_work_sync(), remove packages (the only path reaching ncsi_stop_channel_monitor() and timer_delete_sync(&nc->monitor.timer)), then kfree_rcu(ndp). dev_remove_pack() used to be first, and it is the only synchronize_net() barrier here. With it moved after the sweep, ndp->ptype is still registered while the loop runs, so ncsi_rcv_rsp() can be in progress: net/ncsi/ncsi-rsp.c:ncsi_rcv_rsp() { ... nr->rsp = skb; ... spin_unlock_irqrestore(&ndp->lock, flags); payload = nrh->payload; ... ret = ncsi_validate_rsp_pkt(nr, payload); ... } Can the sweep's ncsi_free_request() clear and consume_skb() nr->rsp and nr->cmd while ncsi_rcv_rsp() is using them after dropping ndp->lock? Note also that both contexts then execute "--ndp->pending_req_num" outside ndp->lock. Second, the channel monitor timers are only stopped in the package removal loop, which now happens after the sweep. A monitor tick landing in that window calls ncsi_xmit_cmd(): net/ncsi/ncsi-cmd.c:ncsi_xmit_cmd() { ... nr->enabled = true; mod_timer(&nr->timer, jiffies + 1 * HZ); ... } Nothing deletes that timer afterwards, and kfree_rcu(ndp) only waits one grace period, so does ncsi_request_timeout() then dereference nr->ndp and ndp->lock in freed memory about a second later? Third, the sweep only frees requests with nr->enabled set. Do requests that are nr->used with nr->cmd or nr->rsp set, but not enabled, still leak their skbs? > diff --git a/net/ncsi/ncsi-netlink.c b/net/ncsi/ncsi-netlink.c > index 8cc538358f6a..59fd735c367a 100644 > --- a/net/ncsi/ncsi-netlink.c > +++ b/net/ncsi/ncsi-netlink.c > @@ -35,8 +35,6 @@ static struct ncsi_dev_priv *ndp_from_ifindex(struct net *net, u32 ifindex) > { > struct ncsi_dev_priv *ndp; > struct net_device *dev; > - struct ncsi_dev *nd; > - struct ncsi_dev; > > if (!net) > return NULL; > @@ -47,9 +45,7 @@ static struct ncsi_dev_priv *ndp_from_ifindex(struct net *net, u32 ifindex) > return NULL; > } > > - nd = ncsi_find_dev(dev); > - ndp = nd ? TO_NCSI_DEV_PRIV(nd) : NULL; > - > + ndp = ncsi_dev_get(dev); > dev_put(dev); > return ndp; > } [Severity: High] The commit message says the kref exists "to prevent the device structure from being freed while netlink handlers are still operating on it". Which device structure does the kref protect? It pins struct ncsi_dev_priv, but the handlers dereference ndp->ndev.dev, the struct net_device, and this helper drops the netdev reference with dev_put() before returning. After ncsi_unregister_dev() returns, the driver continues: drivers/net/ethernet/faraday/ftgmac100.c:ftgmac100_remove() { ... if (priv->ndev) ncsi_unregister_dev(priv->ndev); unregister_netdev(netdev); ... } ncsi_unregister_dev() now only does kref_put(), so ndp stays alive as long as a handler holds a reference, and nothing marks ndp dead or re-checks ndp->ndev.dev. Can a handler still holding the kref then touch the freed netdev, for example via: ncsi_send_netlink_err(ndp->ndev.dev, ... ); or netdev_info()/netdev_err()/netdev_dbg() in ncsi_set_interface_nl(), ncsi_clear_interface_nl() and ncsi_set_channel_mask_nl(), or ncsi_reset_dev(&ndp->ndev) reaching ncsi_xmit_cmd() and dev_queue_xmit() on that netdev? The NCSI genl ops do not take RTNL, so they are not serialised against netdev teardown. [Severity: Medium] This is a pre-existing issue, but while ndp_from_ifindex() is being touched: does every NCSI netlink command leak a struct net reference? All six callers pass a reference in: ndp = ndp_from_ifindex(get_net(sock_net(msg->sk)), nla_get_u32(info->attrs[NCSI_ATTR_IFINDEX])); and neither ndp_from_ifindex() nor any caller calls put_net(). A grep over net/ncsi/ shows six get_net() calls and no put_net(), so NCSI_CMD_PKG_INFO, SET_INTERFACE, CLEAR_INTERFACE, SEND_CMD, SET_PACKAGE_MASK and SET_CHANNEL_MASK each pin the namespace permanently, whether or not the device lookup succeeds. [ ... ] -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260830064337.327128-2-wongboonjhee52%40gmail.com