Re: [PATCH net v2 07/10] net: systemport: Fix out-of-bounds array accesses in DSA queue mapping

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 1 (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).

- [Medium] In bcm_sysport_map_queues()
  (drivers/net/ethernet/broadcom/bcmsysport.c), the new bounds check…
- [Low] The commit message understates the trigger.

Pre-existing issues:
- [Low] bcm_sysport_select_queue() runs in the TX datapath without RTNL.

--

net: systemport: Fix out-of-bounds array accesses in DSA queue mapping

This patch adds bounds checks against ARRAY_SIZE(priv->ring_map) in
bcm_sysport_select_queue(), bcm_sysport_map_queues() and
bcm_sysport_unmap_queues(). It also clamps the argument that SYSTEMPORT
Lite passes to netif_set_real_num_tx_queues() so it is never below 1.

> The priv->ring_map array has a fixed size of (DSA_MAX_PORTS * 8). In
> bcm_sysport_select_queue(), bcm_sysport_map_queues(), and
> bcm_sysport_unmap_queues(), indices calculated as
> (qp + port * num_tx_queues) were accessed without checking against
> ARRAY_SIZE(priv->ring_map). If unusual port or queue configurations are
> encountered, this could lead to out-of-bounds array accesses.

[Severity: Low]

Is "unusual port or queue configurations" the right trigger to describe
here?

With the in-tree switch drivers, ds->num_tx_queues is at most 8 and
bcm_sf2 user ports are below 9. So the map/unmap index stays under 96.

The read in bcm_sysport_select_queue() looks reachable with ordinary
traffic, though. It decodes queue_mapping for every skb once
netdev_uses_dsa(dev) is true. But only brcm_tag_xmit_ll() in
net/dsa/tag_brcm.c writes the port/queue encoding:

    skb_set_queue_mapping(skb, BRCM_TAG_SET_PORT_QUEUE(dp->index, queue));

Other skbs sent on the conduit carry unrelated values. Two examples:

  - forwarded skbs that still have the ingress queue from
    skb_record_rx_queue() (rxq + 1), e.g. from multiqueue tun/veth or
    NICs with many RX queues
  - tc-BPF writes to __sk_buff->queue_mapping

If the low byte of queue_mapping is 96 or more, the lookup reads past the
end of ring_map[], which is the last member of priv. The loaded value is
then dereferenced as tx_ring->index.

The code change covers this case. Could the commit message say that
ordinary traffic can hit it, so stable backporters can judge the urgency?

> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c b/drivers/net/ethernet/broadcom/bcmsysport.c
> index ed4337af58da1..4efcefd33b781 100644
> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c

[ ... ]

> @@ -2281,8 +2281,11 @@ static u16 bcm_sysport_select_queue(struct net_device *dev, struct sk_buff *skb,
>  	/* DSA tagging layer will have configured the correct queue */
>  	q = BRCM_TAG_GET_QUEUE(queue);
>  	port = BRCM_TAG_GET_PORT(queue);
> -	tx_ring = priv->ring_map[q + port * priv->per_port_num_tx_queues];
> +	index = q + port * priv->per_port_num_tx_queues;
> +	if (unlikely(index >= ARRAY_SIZE(priv->ring_map)))
> +		return netdev_pick_tx(dev, skb, NULL);
>  
> +	tx_ring = priv->ring_map[index];
>  	if (unlikely(!tx_ring))
>  		return netdev_pick_tx(dev, skb, NULL);
>  

[Severity: Low]

This is a pre-existing issue and was not introduced by this patch.
bcm_sysport_select_queue() runs in the TX path without RTNL. It reads
priv->per_port_num_tx_queues and priv->ring_map[index] with plain loads.

At the same time, bcm_sysport_map_queues() and bcm_sysport_unmap_queues()
write those fields with plain stores. They run from the NETDEV_CHANGEUPPER
notifier and hold only RTNL. For example, dsa_user_destroy() unlinks
without first draining in-flight transmits:

    netif_carrier_off(user_dev);
    rtnl_lock();
    netdev_upper_dev_unlink(conduit, user_dev);

The new index >= ARRAY_SIZE() check and the existing !tx_ring check both
assume each value is read only once. Without READ_ONCE()/WRITE_ONCE(), can
the compiler reload per_port_num_tx_queues or ring_map[index] after the
check?

Would a follow-up be worthwhile that adds READ_ONCE() in
bcm_sysport_select_queue() and WRITE_ONCE() in the map/unmap paths?

[ ... ]

> @@ -2352,7 +2356,8 @@ static int bcm_sysport_map_queues(struct net_device *dev,
>  		ring->switch_queue = qp;
>  		ring->switch_port = port;
>  		ring->inspect = true;
> -		priv->ring_map[qp + port * num_tx_queues] = ring;
> +		if (qp + port * num_tx_queues < ARRAY_SIZE(priv->ring_map))
> +			priv->ring_map[qp + port * num_tx_queues] = ring;
>  		qp++;
>  	}
>  

[Severity: Medium]

When qp + port * num_tx_queues is out of range, does this leave the ring
half claimed? By this point ring->switch_queue, ring->switch_port and
ring->inspect are already set, and qp++ still runs. Only the ring_map[]
store is skipped.

The index goes out of range when num_tx_queues is above 8, or when the
port index is 12 or more with 8 queues.

On every open, bcm_sysport_init_tx_ring() programs the hardware from
those fields:

	if (ring->inspect) {
		reg |= ring->switch_queue & RING_QID_MASK;
		reg |= ring->switch_port << RING_PORT_ID_SHIFT;
	} else {
		reg |= RING_IGNORE_STATUS;
	}

So the switch port/queue still flow-controls the ring. The later commit
"net: systemport: Update TDMA queue mapping dynamically on changeupper"
also has bcm_sysport_map_queues() program this right away, with
RING_IGNORE_STATUS cleared.

However, no ring_map[] entry points at the ring. For that (port, queue),
bcm_sysport_select_queue() hits the new bounds check and falls back to
netdev_pick_tx(). That can hash onto any ring, including rings that
inspect other ports/queues.

Could this bring back the flow-control cross-talk and head-of-line
blocking that ring_map[] exists to prevent?

While the port stays linked, later bcm_sysport_map_queues() calls for
other ports also skip the claimed ring because of this check:

		if (ring->inspect)
			continue;

Would it be better to check the index before claiming the ring, and then
skip or reject the mapping?

The commit message also doesn't mention this half-claimed state. It is
still present at the end of the series.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922232440.598918-1-florian.fainelli%40broadcom.com