Re: [PATCH v10] net/ncsi: Fix use-after-free in NCSI teardown

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 3 (1) · Medium: 2 · Low: 1

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] ncsi_unregister_dev_prepare() holds the global ncsi_dev_mutex
  while it calls disable_work_sync(&ndp->work), so it waits for…
- [High] The prepare/unregister split leaves a use-after-free window on
  request timers.
- [High] The commit says it guards channel_queue additions against
  duplicates and keeps state and queue transitions under the ndp->lock-…
- [Medium] In the single-channel link-loss branch of
  ncsi_aen_handler_lsc(), the patch re-checks the channel and queues it…
- [Medium] ncsi_unregister_dev_prepare() calls
  disable_work_sync(&ndp->work), removes the ptype (so NC responses are…
- [Low] The patch is titled "Fix use-after-free in NCSI teardown" and has
  one Fixes: tag (2d283bdd079c), but it bundles several independent…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but an NCSI Get
  Capabilities response can corrupt kernel memory.

--

net/ncsi: Fix use-after-free in NCSI teardown

This patch splits NCSI unregistration into two steps. The prepare step
unlinks the device, removes the packet handler and drains the work and
timers, and a final step frees everything. It also adds a global
ncsi_dev_mutex that serializes netlink and VLAN operations with teardown,
reworks channel state and queue locking to take ndp->lock before
nc->lock, fixes several netlink handler issues, and converts ftgmac100 to
the two-step teardown.

> net/ncsi: Fix use-after-free in NCSI teardown

[ ... ]

> Fixes: 2d283bdd079c ("net/ncsi: Resource management")

[Severity: Low]
This isn't a bug, but could this be split into a series? Besides the
teardown use-after-free, the patch also carries:

  - the get_net() reference leak fix in five netlink handlers
  - the genlmsg_cancel(skb, NULL) fix in ncsi_pkg_info_all_nl()
  - the nr->cmd->data vs skb_network_header() offset fix in
    ncsi_send_netlink_timeout()
  - the PKG_INFO behaviour change for sparse package IDs
  - the uninitialized vid in set_one_vid()
  - a lock order and state machine rework across the AEN handlers,
    ncsi_report_link(), ncsi_channel_monitor(),
    ncsi_choose_active_channel() and ncsi_process_next_channel()
  - a batch/cursor rewrite of ncsi_kick_channels()
  - new netlink timeout notifications sent from ncsi_unregister_dev()

Several of these fix code from the later NCSI netlink commits rather than
2d283bdd079c. The single Fixes: tag doesn't match those, and backports to
trees without the netlink code would not apply cleanly.

It also means a regression in the state machine rework couldn't be
reverted without also reverting the use-after-free fix.

> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
> index 6d2fe5c2f3903..f45388810570d 100644
> --- a/drivers/net/ethernet/faraday/ftgmac100.c
> +++ b/drivers/net/ethernet/faraday/ftgmac100.c

[ ... ]

> @@ -2108,8 +2110,12 @@ static void ftgmac100_remove(struct platform_device *pdev)
>  	priv = netdev_priv(netdev);
>  
>  	if (priv->ndev)
> -		ncsi_unregister_dev(priv->ndev);
> +		ncsi_unregister_dev_prepare(priv->ndev);
>  	unregister_netdev(netdev);
> +	if (priv->ndev) {
> +		ncsi_unregister_dev(priv->ndev);
> +		priv->ndev = NULL;
> +	}

[Severity: Medium]
ncsi_unregister_dev_prepare() disables ndp->work, removes the ptype (so NC
responses are dropped), stops the monitors and deletes the request timers,
all before unregister_netdev().

If a channel is mid-configuration or mid-suspend at that point, it stays
in NCSI_CHANNEL_INVISIBLE. Only the config_done and suspend_done steps in
the work move it out of that state.

The monitors are also stopped after the work is disabled. In between, a
monitor callback in its timeout branch can still call
ncsi_process_next_channel().

unregister_netdev() then reaches:

unregister_netdev()
  ... -> ftgmac100_stop()
    ncsi_stop_dev()

and ncsi_stop_dev() checks:

			WARN_ON_ONCE(chained ||
				     old_state == NCSI_CHANNEL_INVISIBLE);

Will removing the device during a configure or suspend transition always
hit this WARN, and panic on panic_on_warn systems? Nothing in the stop
path checks unregister_prepared, even though the header comment says the
prepare step quiesces NCSI before the netdev is stopped.

> diff --git a/net/ncsi/ncsi-aen.c b/net/ncsi/ncsi-aen.c
> index 040a31557201b..c3614f0648649 100644
> --- a/net/ncsi/ncsi-aen.c
> +++ b/net/ncsi/ncsi-aen.c

[ ... ]

> @@ -94,7 +96,11 @@ static int ncsi_aen_handler_lsc(struct ncsi_dev_priv *ndp,
>  			ndp->flags |= NCSI_DEV_RESHUFFLE;
>  			ncsi_stop_channel_monitor(nc);
>  			spin_lock_irqsave(&ndp->lock, flags);
> -			list_add_tail_rcu(&nc->link, &ndp->channel_queue);
> +			spin_lock(&nc->lock);
> +			if (nc->state == NCSI_CHANNEL_ACTIVE &&
> +			    list_empty(&nc->link))
> +				list_add_tail_rcu(&nc->link, &ndp->channel_queue);
> +			spin_unlock(&nc->lock);
>  			spin_unlock_irqrestore(&ndp->lock, flags);
>  			return ncsi_process_next_channel(ndp);

[Severity: Medium]
What happens here when the new re-check fails? For example, nc may be
INVISIBLE because it is being configured or suspended, or it may already
be INACTIVE.

Nothing is queued in that case, but ncsi_process_next_channel() is still
called with NCSI_DEV_RESHUFFLE just set. With an empty channel_queue it
goes to:

out:
	ndp->active_channel = NULL;
	ndp->active_package = NULL;
	if (ndp->flags & NCSI_DEV_RESHUFFLE) {
		ndp->flags &= ~NCSI_DEV_RESHUFFLE;
		return ncsi_choose_active_channel(ndp);
	}

Doesn't this abandon the transition that is in progress on nc?

ncsi_choose_active_channel() can start configuring another INACTIVE
channel while nc's requests are still outstanding. Or it can call
ncsi_report_link(ndp, true), which sets nd->state to functional. Either
way nc stays INVISIBLE until a reset.

ncsi_aen_handler_cr() handles the same failed re-check by returning 0:

	if (nc->state != NCSI_CHANNEL_ACTIVE || !list_empty(&nc->link)) {
		spin_unlock(&nc->lock);
		spin_unlock_irqrestore(&ndp->lock, flags);
		return 0;
	}

Should the lsc path do the same?

>  		}

[ ... ]

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

[ ... ]

> @@ -640,7 +649,7 @@ static int set_one_vid(struct ncsi_dev_priv *ndp, struct ncsi_channel *nc,
>  	unsigned long flags;
>  	int i, index;
>  	void *bitmap;
> -	u16 vid;
> +	u16 vid = 0;

[Severity: High]
This isn't a bug introduced by this patch, but while looking at the VLAN
filter code: can a Get Capabilities response with vlan_cnt > 64 corrupt
memory?

ncsi_rsp_handler_gc() takes the count straight from the response:

	nc->vlan_filter.vids = kcalloc(rsp->vlan_cnt,
				       sizeof(*nc->vlan_filter.vids),
				       GFP_ATOMIC);
	...
	nc->vlan_filter.bitmap = U64_MAX;
	nc->vlan_filter.n_vids = rsp->vlan_cnt;

but the bitmap is a single u64 placed directly before the vids pointer:

struct ncsi_channel_vlan_filter {
	u8	n_vids;
	u64	bitmap;
	u16	*vids;
};

In clear_one_vid(), once the 64 real bits are cleared,
find_first_bit(bitmap, ncf->n_vids) scans into the vids pointer. Then:

	clear_bit(index, bitmap);
	ncf->vids[index] = 0;

This flips a bit of the vids pointer and then writes through the
corrupted pointer.

set_one_vid() (find_first_zero_bit()/set_bit()) and
ncsi_rsp_handler_gp() also write past the bitmap. The later kfree() of
vids then gets the corrupted pointer.

Should n_vids be limited to the bitmap width?

>  
>  	if (list_empty(&ndp->vlan_vids))
>  		return -1;

[ ... ]

> @@ -1285,10 +1296,8 @@ static int ncsi_choose_active_channel(struct ncsi_dev_priv *ndp)
>  			 * so they will have AENs enabled.
>  			 */
>  			if (with_link || np->multi_channel) {
> -				spin_lock_irqsave(&ndp->lock, flags);
>  				list_add_tail_rcu(&nc->link,
>  						  &ndp->channel_queue);
> -				spin_unlock_irqrestore(&ndp->lock, flags);

[Severity: High]
The commit message says queue additions are guarded against duplicates
and kept under the ndp-before-channel lock order. The reconfigure branch
of the config_done state in ncsi_configure_channel() doesn't seem to have
been converted:

net/ncsi/ncsi-manage.c:ncsi_configure_channel() {
    ...
		if (nc->reconfigure_needed) {
			...
			nc->reconfigure_needed = false;
			nc->state = NCSI_CHANNEL_INACTIVE;
			spin_unlock_irqrestore(&nc->lock, flags);

			spin_lock_irqsave(&ndp->lock, flags);
			list_add_tail_rcu(&nc->link, &ndp->channel_queue);
			spin_unlock_irqrestore(&ndp->lock, flags);
    ...
}

Between dropping nc->lock and taking ndp->lock, nc is INACTIVE and not
on any list.

ncsi_choose_active_channel() can run on another CPU in that window. One
way in is ncsi_process_next_channel() with an empty queue and
NCSI_DEV_RESHUFFLE set, which the lsc handler now reaches after its guard
fails. It would pass the INACTIVE/list_empty() check and queue nc here.

Can the work then add the same nc->link a second time and corrupt
channel_queue? Should that path also change the state and add to the
queue under ndp->lock and nc->lock, with a list_empty() check?

>  
>  				netdev_dbg(ndp->ndev.dev,
>  					   "NCSI: Channel %u added to queue (link %s)\n",

[ ... ]

> @@ -1952,22 +2045,74 @@ int ncsi_reset_dev(struct ncsi_dev *nd)
>  	return 0;
>  }
>  
> +void ncsi_unregister_dev_prepare(struct ncsi_dev *nd)
> +{
> +	struct ncsi_dev_priv *ndp = TO_NCSI_DEV_PRIV(nd);
> +	struct ncsi_package *np;
> +	struct ncsi_channel *nc;
> +	int i;
> +	unsigned long flags;
> +
> +	mutex_lock(&ncsi_dev_mutex);
> +	if (!ndp->unregister_prepared) {
> +		ndp->unregister_prepared = true;
> +		spin_lock_irqsave(&ncsi_dev_lock, flags);
> +		list_del_rcu(&ndp->node);
> +		spin_unlock_irqrestore(&ncsi_dev_lock, flags);
> +
> +		dev_remove_pack(&ndp->ptype);
> +
> +		/* Wait for lookups that found this device before it was unlinked. */
> +		synchronize_rcu();
> +
> +		/* Quiesce NCSI activity before the netdev is stopped. */
> +		disable_work_sync(&ndp->work);

[Severity: High]
This waits for ndp->work while holding the global ncsi_dev_mutex. The
work can take RTNL in the apply_mac state:

ncsi_dev_work()
  ncsi_configure_channel()
    case ncsi_dev_state_config_apply_mac:
      rtnl_lock();

This patch also makes ncsi_vlan_rx_add_vid() and ncsi_vlan_rx_kill_vid()
take ncsi_dev_mutex. Those ndo callbacks are called from vlan_vid_add()
and vlan_vid_del() with RTNL held.

Can this deadlock?

CPU0:
ftgmac100_remove()
  ncsi_unregister_dev_prepare()
    mutex_lock(&ncsi_dev_mutex);
    disable_work_sync(&ndp->work);   <- waits for the work

kworker:
ncsi_dev_work()
  ncsi_configure_channel()
    rtnl_lock();                     <- waits for CPU1

CPU1 (RTNL held, e.g. adding a VLAN):
vlan_vid_add()
  ncsi_vlan_rx_add_vid()
    mutex_lock(&ncsi_dev_mutex);     <- waits for CPU0

The mutex is global, so CPU1 can be working on a different NCSI netdev
from the one being removed.

> +		list_for_each_entry(np, &ndp->packages, node)
> +			list_for_each_entry(nc, &np->channels, node)
> +				ncsi_stop_channel_monitor(nc);
> +		for (i = 0; i < ARRAY_SIZE(ndp->requests); i++)
> +			timer_delete_sync(&ndp->requests[i].timer);
> +	}
> +	mutex_unlock(&ncsi_dev_mutex);
> +}
> +EXPORT_SYMBOL_GPL(ncsi_unregister_dev_prepare);
> +
>  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;
> +	struct ncsi_channel *nc;
> +	struct vlan_vid *vlan, *vlan_tmp;
> +	int i;
>  
> -	dev_remove_pack(&ndp->ptype);
> +	ncsi_unregister_dev_prepare(nd);
> +	for (i = 0; i < ARRAY_SIZE(ndp->requests); i++) {
> +		struct ncsi_request *nr = &ndp->requests[i];
>  
> -	list_for_each_entry_safe(np, tmp, &ndp->packages, node)
> -		ncsi_remove_package(np);
> +		if (!nr->used)
> +			continue;
>  
> -	spin_lock_irqsave(&ncsi_dev_lock, flags);
> -	list_del_rcu(&ndp->node);
> -	spin_unlock_irqrestore(&ncsi_dev_lock, flags);
> +		if (nr->flags == NCSI_REQ_FLAG_NETLINK_DRIVEN && nr->cmd) {
> +			struct ncsi_cmd_pkt *cmd = (struct ncsi_cmd_pkt *)
> +				skb_network_header(nr->cmd);
> +
> +			rcu_read_lock();
> +			ncsi_find_package_and_channel(ndp,
> +						      cmd->cmd.common.channel,
> +						      &np, &nc);
> +			rcu_read_unlock();
> +			ncsi_send_netlink_timeout(nr, np, nc);
> +		}
> +		ncsi_free_request(nr);
> +	}

[ ... ]

> +	list_for_each_entry_safe(np, tmp, &ndp->packages, node)
> +		ncsi_remove_package(np);
>  
>  	kfree(ndp);
>  }

[Severity: High]
When ftgmac100_remove() has already called ncsi_unregister_dev_prepare(),
the second call here does nothing. So the request timers are drained only
once, before unregister_netdev().

Between the two calls the netdev is still registered and RTNL isn't held,
so the interface can be brought up:

ftgmac100_open()
  ncsi_start_dev()
    ncsi_reset_dev()
      ncsi_choose_active_channel()
        ncsi_process_next_channel()
          ncsi_configure_channel()
            ncsi_xmit_cmd()   <- sets nr->enabled, arms nr->timer

ncsi_start_dev() doesn't check unregister_prepared. This path can run
when the device is probed and no channel is ACTIVE.

When this loop runs later, ncsi_free_request() only syncs the timer if
nr->enabled is set:

	if (nr->enabled) {
		nr->enabled = false;
		timer_delete_sync(&nr->timer);
	}

ncsi_request_timeout() clears enabled as its first step and then keeps
using nr and ndp, including its own call to ncsi_free_request():

	spin_lock_irqsave(&ndp->lock, flags);
	nr->enabled = false;

If the timeout fires while this loop is running, can the loop skip
timer_delete_sync() and go on to ncsi_remove_package() and kfree(ndp)
while ncsi_request_timeout() is still using ndp->lock, ndp->requests[]
and ndp->work on another CPU?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012060434.1974740-1-wongboonjhee52%40gmail.com