Re: [PATCH v4] net/ncsi: Fix Use-After-Free in NCSI channel and package removal

[Simon Horman <horms@kernel.org>]

On Sat, Sep 05, 2026 at 10:59:02PM +0800, Wong Boon Jhee wrote:
> In net/ncsi/ncsi-manage.c, ncsi_remove_channel() and
> ncsi_remove_package() remove objects from an RCU-protected linked list
> using list_del_rcu() and immediately free them using kfree().
> 
> 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. When kfree()
> executes, the reader is left holding a dangling pointer to freed memory,
> resulting in a slab-use-after-free.
> 
> 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()
> to defer memory freeing until all pre-existing RCU readers have finished
> their critical sections. It also introduces a kref for ncsi_dev_priv to
> prevent the device structure from being freed while netlink handlers are
> still operating on it. Concurrent lockless readers are now explicitly
> protected with rcu_read_lock().
> 
> RCU read-side critical sections are added around lockless package and
> channel traversals that may race with object removal. The KASAN
> reproducer no longer reports the slab-use-after-free.
> 
> Fixes: 2d283bdd079c ("net/ncsi: Resource management")
> Signed-off-by: Wong Boon Jhee <wongboonjhee52@gmail.com>
> ---
> v3 -> v4:
> - Reworked NCSI object lifetime to pin the underlying net_device using
>   dev_hold()/dev_put() inside ncsi_dev_get()/ncsi_dev_put(), preventing
>   UAF if the driver unbinds while netlink handlers are active.
> - Moved package/channel destruction and request sweeping into the
>   ncsi_dev_release() kref callback to guarantee all asynchronous
>   producers are fully stopped before teardown begins.
> - Fixed pre-existing get_net() namespace leaks in all netlink handlers.
> - Fixed RCU lock leaks on early returns in ncsi_check_hwa() and
>   ncsi_set_channel_mask_nl().
> - Unified channel_queue semantics by removing incorrect RCU usage and
>   protecting traversals with ndp->lock.
> - Extracted channel IDs inside the RCU read section in
>   ncsi_update_tx_channel() to prevent pointer escapes.
> - Fixed memory corruption in ncsi_vlan_rx_kill_vid() by using a
>   temporary pointer for the freed VLAN object.
> 
>  net/ncsi/internal.h     |   8 +-
>  net/ncsi/ncsi-aen.c     |   2 +
>  net/ncsi/ncsi-manage.c  | 260 +++++++++++++++++++++++++++++++---------
>  net/ncsi/ncsi-netlink.c |  69 ++++++++---
>  net/ncsi/ncsi-rsp.c     |  17 ++-

Hi Wong Boon,

I am concerned that as a fix this patch is quite large and complex.  And on
the current course likely to be come more so in order to address concerns
such as those raised by AI-generated review here:

https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260830064337.327128-2-wongboonjhee52%40gmail.com

It does seem to me that while the architectural direction this patch is
taking is the right long term solution. But, given it's complexity, I
wonder if it would be best to implement a simpler fix for the bug, and then
follow-up with the architectural changes.

Reviewing the problem described in the patch description, I see that
ncsi_remove_channel() and ncsi_remove_package() are only called during
device tear-down. So I wonder if focussing on tear-down would lead us to a
simpler bug-fix.

For example in ncsi_unregister_dev: stop the producers; flush the readers
using synchronize_rcu() and then flush all the callbacks using rcu_barrier().

I'm no fan of synchronize_rcu(), but this may lead to a far more compact
and thus less risky bug-fix.

LMKWYT