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

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 2 (1) · 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] The fix is incomplete, and the race the commit message targets
  (rmmod ftgmac100 vs. NCSI_CMD_PKG_INFO) can still cause a use-after-…
- [Medium] The patch adds the RCU reader protocol only to the
  NCSI_CMD_PKG_INFO handlers.
- [Medium] The comment the patch adds in ncsi_remove_channel() (ncsi-
  manage.c:270-273) says the call_rcu() deferral protects the 'channel…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  ncsi_pkg_info_all_nl() passes get_net(sock_net(skb->sk)) to…

--

net/ncsi: fix use-after-free of ncsi_channel in ncsi_remove_channel()

This patch wraps the NCSI_CMD_PKG_INFO doit and dump handlers in
rcu_read_lock(). It also moves the freeing of ncsi_channel and its filter
arrays into an RCU callback. The goal is to close a use-after-free between
netlink package info readers and ftgmac100 teardown through
ncsi_unregister_dev().

> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
> index 54d0df0a9efe6..c16047dee3f8a 100644
> --- a/net/ncsi/ncsi-manage.c
> +++ b/net/ncsi/ncsi-manage.c

[ ... ]

> @@ -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);

[Severity: Medium]

Does this grace period really cover the channel configuration worker?
ncsi_dev_work() -> ncsi_process_next_channel() / ncsi_configure_channel() /
ncsi_suspend_channel() does not run under rcu_read_lock(), so call_rcu()
does not wait for it.

ncsi_unregister_dev() stops the worker only after every channel has already
been passed to call_rcu():

ncsi_unregister_dev() {
	...
	list_for_each_entry_safe(np, tmp, &ndp->packages, node)
		ncsi_remove_package(np);
	...
	disable_work_sync(&ndp->work);
	...
}

ncsi_remove_channel() does not unlink nc->link from ndp->channel_queue. It
also does not clear ndp->active_channel, ndp->hot_channel or
np->preferred_channel.

A worker that is running or preempted could still dereference nc after
ncsi_channel_release() has freed it. It could also list_del_init() a stale
nc->link in ncsi_process_next_channel().

It could also re-arm nc->monitor.timer through ncsi_start_channel_monitor()
in ncsi_configure_channel() after ncsi_stop_channel_monitor() has already
run timer_delete_sync(). The timer would then fire on freed memory.

The worker race itself predates this patch, because the old code called
kfree() right away.

Should the comment leave out the worker? Or should teardown call
disable_work_sync() before releasing the channels, use timer_shutdown_sync()
on the monitor timer, and unlink nc->link from channel_queue?

>  }
>  
>  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 8cc538358f6a3..8d1fe60beb22f 100644
> --- a/net/ncsi/ncsi-netlink.c
> +++ b/net/ncsi/ncsi-netlink.c

[ ... ]

> @@ -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.
> +	 */

[Severity: High]

Is this comment accurate for ndp and np? Only the channel free moved to
call_rcu(). The package and the device are still freed right after
list_del_rcu():

ncsi_remove_package() {
	...
	spin_lock_irqsave(&ndp->lock, flags);
	list_del_rcu(&np->node);
	ndp->package_num--;
	spin_unlock_irqrestore(&ndp->lock, flags);

	kfree(np);
}

ncsi_unregister_dev() {
	...
	spin_lock_irqsave(&ncsi_dev_lock, flags);
	list_del_rcu(&ndp->node);
	spin_unlock_irqrestore(&ncsi_dev_lock, flags);

	disable_work_sync(&ndp->work);

	kfree(ndp);
}

Inside the new rcu_read_lock() section, ncsi_write_package_info() still
reads ndp->package_num, ndp->package_whitelist and np->id, and it follows
the np->node and np->channels links.

ncsi_write_channel_info() reaches the parent package through a channel
that call_rcu() keeps alive:

	if (nc == nc->package->preferred_channel)

ncsi_pkg_info_all_nl() also holds on to package and reads package->id
after the walk.

ftgmac100_remove() calls ncsi_unregister_dev() before unregister_netdev().
So it looks like the same rmmod vs NCSI_CMD_PKG_INFO race from the commit
message can still cause a use-after-free, now on np or ndp instead of nc.

Do np and ndp also need kfree_rcu() or call_rcu(), or a synchronize_rcu()
before the kfree()?

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

[Severity: Medium]

Do the other genl handlers in ncsi-netlink.c need the same change? These
handlers still call ndp_from_ifindex() and walk NCSI_FOR_EACH_PACKAGE() /
NCSI_FOR_EACH_CHANNEL() without rcu_read_lock():

ncsi_set_interface_nl()
ncsi_clear_interface_nl()
ncsi_send_cmd_nl()
ncsi_set_package_mask_nl()
ncsi_set_channel_mask_nl()

The commit message says "Either half alone leaves the race". On a
preemptible kernel, one of these walkers can be preempted mid-walk during
ncsi_unregister_dev(). The grace period can then end and
ncsi_channel_release() can free nc while the walker still uses it.

Several of these handlers also write through the pointers they find.
ncsi_set_interface_nl() stores the channel into package->preferred_channel
and updates package->channel_whitelist. ncsi_clear_interface_nl() takes
np->lock and writes np fields for each package.

Can these turn into use-after-free writes during teardown?

These ops are GENL_ADMIN_PERM and the family only works in init_net, so a
trigger needs CAP_NET_ADMIN. With CONFIG_PROVE_RCU_LIST these walks would
also cause suspicious RCU usage warnings.

>  
>  	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]));

[Severity: Medium]

This is a pre-existing issue and was not introduced by this patch, but does
ncsi_pkg_info_all_nl() leak a netns reference here?

get_net(sock_net(skb->sk)) takes a reference that ndp_from_ifindex() never
drops, and there is no put_net() anywhere in net/ncsi/. get_net() runs
before the lookup, so the reference leaks even when no NCSI device is
found.

The same pattern is in ncsi_set_interface_nl(), ncsi_clear_interface_nl(),
ncsi_send_cmd_nl(), ncsi_set_package_mask_nl() and
ncsi_set_channel_mask_nl().

The family does not set netnsok, so only init_net is affected, and its
refcount grows with every dump call.

Would plain sock_net() be enough here?

[ ... ]

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