Re: [PATCH net] net: xps: re-validate the queue index after switching to the subordinate device

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 1 · Low: 2

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 applies the wrong bound and leaves the faulty access in
  place.
- [Medium] The new check also runs when the queue has no sb_dev and `dev`
  is still the parent.
- [Low] The commit message describes the defect inaccurately.
- [Low] The submission notes ('Triggering conditions', below the ---
  line) say the in-tree subordinate binding happens on 'the i40e and…

--

net: xps: re-validate the queue index after switching to the subordinate device

This patch adds a check to __netif_set_xps_queue(). After dev has been
switched to the subordinate device, index is compared against that
device's real_num_tx_queues and -EINVAL is returned if it is too large.
The goal is to stop the KASAN slab-out-of-bounds write reported from
xps_cpus_store().

> A subordinate with fewer TX queues than the parent's index then
> overflows sb_dev->_tx[] / the XPS map:

[Severity: Low]
Is the "/ the XPS map" part accurate? In __netif_set_xps_queue() the XPS
maps are indexed by tci, not by index:

    tci = j * num_tc + tc;
    map = copy ? xmap_dereference(dev_maps->attr_map[tci]) : NULL;

and index is only stored as a value:

    map->queues[map->len++] = index;

The only out-of-bounds subscript seems to be netdev_get_tx_queue(dev,
index) in the NUMA node write at out_no_new_maps.

Also, netdev_set_sb_channel() rejects multiqueue devices, so every in-tree
subordinate has exactly one TX queue. That means "fewer TX queues than the
parent's index" is not a corner case. It covers every XPS write to any
offloaded queue.

Could the commit message describe this more precisely?

> CAP_NET_ADMIN to set the XPS mask.  An sb_dev is bound in-tree by
> netdev_bind_sb_channel_queue(), which the i40e and ixgbe SR-IOV paths call
> when a VF's channel is bound to the PF.

[Severity: Low]
Is this description of the trigger correct? The only callers of
netdev_bind_sb_channel_queue() are ixgbe_fwd_ring_up() and
i40e_fwd_ring_up(). Both bind queues to a macvlan netdevice for L2
forwarding offload, via ndo_dfwd_add_station (ixgbe_fwd_add() and
i40e_fwd_add()):

drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:ixgbe_fwd_ring_up() {
    ...
	/* record configuration for macvlan interface in vdev */
	for (i = 0; i < num_tc; i++)
		netdev_bind_sb_channel_queue(adapter->netdev, vdev,
					     i, rss_i, baseq + (rss_i * i));
    ...
}

Enabling VFs alone does not seem to set up this binding. The trigger
appears to be a macvlan with l2-fwd-offload on an ixgbe or i40e parent.

> diff --git a/net/core/dev.c b/net/core/dev.c
> index e76762e29360e..d5ce251448062 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2900,6 +2900,14 @@ int __netif_set_xps_queue(struct net_device *dev, const unsigned long *mask,
>  		/* If queue belongs to subordinate dev use its map */
>  		dev = netdev_get_tx_queue(dev, index)->sb_dev ? : dev;
>  
> +		/* dev may now be the subordinate device, whose queue
> +		 * count can be smaller than the parent's.  index was only
> +		 * validated against the parent above, so re-check it before
> +		 * it is used to index the (possibly different) dev->_tx[].
> +		 */
> +		if (index >= dev->real_num_tx_queues)
> +			return -EINVAL;

[Severity: High]
Does this compare a parent-space index against the subordinate's queue
count?

netdev_bind_sb_channel_queue() checks offset + count against the parent's
real_num_tx_queues and stores the parent-space offset in
sb_dev->tc_to_txq[]. So index here is a parent queue number:

net/core/dev.c:netdev_bind_sb_channel_queue() {
    ...
	/* We cannot hand out queues we don't have */
	if ((offset + count) > dev->real_num_tx_queues)
		return -EINVAL;
    ...
	WRITE_ONCE(sb_dev->tc_to_txq[tc].combined, res.combined);
    ...
}

netdev_set_sb_channel() refuses multiqueue devices, and ixgbe_fwd_add()
rejects multiqueue macvlans. So sb_dev->real_num_tx_queues is always 1:

net/core/dev.c:netdev_set_sb_channel() {
	/* Do not use a multiqueue device to represent a subordinate channel */
	if (netif_is_multiqueue(dev))
		return -ENODEV;
    ...
}

ixgbe_fwd_ring_up() binds at baseq + (rss_i * i), where baseq is
pool * queues_per_pool. Pool 0 is reserved for the PF, so pool is at least
1. i40e_fwd_ring_up() binds at iter->base_queue, which comes after the
PF's own queues. In both cases every bound index is >= 1.

With that, won't every xps_cpus and xps_rxqs write to a parent TX queue
bound to an offloaded macvlan now fail with -EINVAL? Before this patch the
map update itself worked. Only the trailing NUMA hint write was out of
bounds.

Driver-internal callers are hit too. ixgbe_configure_tx_ring() ignores the
return value of netif_set_xps_queue() after test_and_set_bit() has already
set __IXGBE_TX_XPS_INIT_DONE. XPS setup for those rings is then never
retried.

XPS_RXQS writes through xps_rxqs_store() are now rejected as well, even
though that path never indexes _tx[index]. Meanwhile xps_cpus_show() still
accepts the same parent index on the same queue, so show and store now
disagree.

The faulty access also seems to be left unchanged:

net/core/dev.c:__netif_set_xps_queue() {
    ...
out_no_new_maps:
	if (type == XPS_CPUS)
		/* update Tx queue numa node */
		netdev_queue_numa_node_write(netdev_get_tx_queue(dev, index),
					     (numa_node_id >= 0) ?
					     numa_node_id : NUMA_NO_NODE);
    ...
}

If an index ever gets past the new check, won't the NUMA hint go to
sb_dev->_tx[index] instead of the parent's queue? The parent queue would
then keep a stale value.

Would it be better to save the original parent device (or its txq) before
the rebind and use it for the NUMA write, rather than bounding index by the
subordinate's queue count?

[Severity: Medium]
This check is inside if (dev_num_tc) and runs whether or not a rebind
happened. When the queue has no sb_dev, dev is still the parent.

On any device with num_tc != 0, the accepted range therefore shrinks from
num_tx_queues to real_num_tx_queues. num_tx_queues is the existing
contract, per the WARN_ON_ONCE() at the top of the function. Devices with
num_tc == 0 get no such check.

Can this break drivers that configure XPS before raising
real_num_tx_queues? ixgbe_open() does things in this order:

drivers/net/ethernet/intel/ixgbe/ixgbe_main.c:ixgbe_open() {
    ...
	ixgbe_configure(adapter);
    ...
	/* Notify the stack of the actual queue counts. */
	queues = adapter->num_tx_queues;
	err = netif_set_real_num_tx_queues(netdev, queues);
    ...
}

ixgbe_configure() reaches netif_set_xps_queue() through
ixgbe_configure_tx_ring(). When DCB is enabled through ixgbe_setup_tc(),
the flow is netdev_set_num_tc() -> new rings -> ixgbe_open(). If the new
ring count is larger than the previous real_num_tx_queues, XPS for the
extra queues would now fail with -EINVAL.

ixgbe_configure_tx_ring() ignores that error, and __IXGBE_TX_XPS_INIT_DONE
is already set. XPS would then be silently and permanently missing for
those queues. i40e_vsi_open() has the same ordering:
i40e_vsi_configure() runs before i40e_netif_set_realnum_tx_rx_queues().

>  		tc = netdev_txq_to_tc(dev, index);
>  		if (tc < 0 || tc >= num_tc)
>  			return -EINVAL;

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