Re: [PATCH net-next v6 03/12] net: airoha: Introduce airoha_gdm_dev struct

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Use-After-Free (UAF) of Device Tree Node
- [High] Incorrect `real_num_tx_queues` bounds accounting in HTB offload completely breaks QoS
- [High] Missing linear packet bounds check in `airoha_get_dsa_tag()` leads to out-of-bounds memory accesses and kernel panics
- [High] Off-by-one Error in HTB Queue Rate Limit Teardown leaves rate limit active
--

commit f41fe0c0ac52b8a3d4e79ff54502343df0bc05cd
Author: Lorenzo Bianconi <lorenzo@kernel.org>

net: airoha: Introduce airoha_gdm_dev struct

EN7581 and AN7583 SoCs support connecting multiple external SerDes to GDM3
or GDM4 ports via a hw arbiter that manages the traffic in a TDM manner.
As a result multiple net_devices can connect to the same GDM{3,4} port
and there is a theoretical "1:n" relation between GDM port and
net_devices.
Introduce airoha_gdm_dev struct to collect net_device related info (e.g.
net_device and external phy pointer). Please note this is just a
preliminary patch and we are still supporting a single net_device for
each GDM port. Subsequent patches will add support for multiple net_devices
connected to the same GDM port.

> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 3fe2561c85f17..18a89de4d58ab 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
[ ... ]
> @@ -2009,7 +2022,7 @@ static netdev_tx_t airoha_dev_xmit(struct sk_buff *skb,
>  	u8 fport;
>  
>  	qid = airoha_qdma_get_txq(qdma, skb_get_queue_mapping(skb));
> -	tag = airoha_get_dsa_tag(skb, dev);
> +	tag = airoha_get_dsa_tag(skb, netdev);

This problem wasn't introduced by this patch, but does this function call lead
to out-of-bounds memory accesses?

In airoha_get_dsa_tag(), the code blindly executes
memmove(skb->data + MTK_HDR_LEN, skb->data, 2 * ETH_ALEN) and
__skb_pull(skb, MTK_HDR_LEN) under the assumption that the linear area
contains at least 16 bytes.

Calling skb_cow_head(skb, 0) only ensures headroom, it does not linearize
the packet. A local attacker using a raw socket can inject heavily fragmented
packets where the linear region is extremely small. In such cases, the memmove
will overwrite and corrupt adjacent kernel memory, and __skb_pull() will
trigger a bug because it drops skb->len below skb->data_len.

Could a pskb_may_pull() check be added before accessing or modifying the
headers?

[ ... ]
> @@ -2605,9 +2627,10 @@ static int airoha_tc_htb_alloc_leaf_queue(struct net_device *dev,
>  	if (opt->command == TC_HTB_NODE_MODIFY)
>  		return 0;
>  
> -	err = netif_set_real_num_tx_queues(dev, num_tx_queues + 1);
> +	err = netif_set_real_num_tx_queues(netdev, num_tx_queues + 1);

This problem wasn't introduced by this patch, but does this incorrect bounds
accounting in HTB offload break QoS?

The mapped queue index is calculated as channel = TC_H_MIN(opt->classid) %
AIROHA_NUM_QOS_CHANNELS, and opt->qid is set to AIROHA_NUM_TX_RING + channel
(which can be up to 11).

However, the driver tracks active queues by simply incrementing the limit by 1
via netif_set_real_num_tx_queues(netdev, num_tx_queues + 1). If a user
specifies a non-sequential or sparse class ID that resolves to a high channel
index, the generated qid will exceed real_num_tx_queues.

When traffic hits this class, the core networking stack detects an
out-of-bounds queue mapping, emits a warning, and forces the traffic to queue
0, thereby completely breaking the hardware QoS offload.

[ ... ]
> @@ -2794,47 +2819,51 @@ static int airoha_dev_setup_tc_block(struct net_device *dev,
>  	}
>  }
>  
> -static void airoha_tc_remove_htb_queue(struct net_device *dev, int queue)
> +static void airoha_tc_remove_htb_queue(struct net_device *netdev, int queue)
>  {
> -	struct airoha_gdm_port *port = netdev_priv(dev);
> +	struct airoha_gdm_dev *dev = netdev_priv(netdev);
> +	struct airoha_gdm_port *port = dev->port;
>  
> -	netif_set_real_num_tx_queues(dev, dev->real_num_tx_queues - 1);
> -	airoha_qdma_set_tx_rate_limit(dev, queue + 1, 0, 0);
> +	netif_set_real_num_tx_queues(netdev, netdev->real_num_tx_queues - 1);
> +	airoha_qdma_set_tx_rate_limit(netdev, queue + 1, 0, 0);

This problem wasn't introduced by this patch, but does this leave the rate
limit active due to an off-by-one error?

The driver attempts to clear the hardware rate limit for a removed QoS queue by
calling airoha_qdma_set_tx_rate_limit(netdev, queue + 1, 0, 0). However,
during creation in airoha_tc_htb_alloc_leaf_queue(), the rate limit was
properly configured on channel (which has the exact same value as queue).

Clearing queue + 1 instead of queue appears to leave the deleted queue's rate
limit fully active while silently breaking the rate limit of the adjacent
queue.

[ ... ]
> +static int airoha_alloc_gdm_device(struct airoha_eth *eth,
> +				   struct airoha_gdm_port *port,
> +				   struct device_node *np)
> +{
> +	struct airoha_gdm_dev *dev;
> +	struct net_device *netdev;
> +	int err;
> +
> +	netdev = devm_alloc_etherdev_mqs(eth->dev, sizeof(*dev),
> +					 AIROHA_NUM_NETDEV_TX_RINGS,
> +					 AIROHA_NUM_RX_RING);
> +	if (!netdev) {
> +		dev_err(eth->dev, "alloc_etherdev failed\n");
> +		return -ENOMEM;
> +	}
> +
> +	netdev->netdev_ops = &airoha_netdev_ops;
> +	netdev->ethtool_ops = &airoha_ethtool_ops;
> +	netdev->max_mtu = AIROHA_MAX_MTU;
> +	netdev->watchdog_timeo = 5 * HZ;
> +	netdev->hw_features = NETIF_F_IP_CSUM | NETIF_F_RXCSUM | NETIF_F_TSO6 |
> +			      NETIF_F_IPV6_CSUM | NETIF_F_SG | NETIF_F_TSO |
> +			      NETIF_F_HW_TC;
> +	netdev->features |= netdev->hw_features;
> +	netdev->vlan_features = netdev->hw_features;
> +	netdev->dev.of_node = np;

This problem wasn't introduced by this patch, but does this lead to a
use-after-free of the device tree node?

In airoha_probe(), the code iterates over child nodes using the
for_each_child_of_node() macro, which automatically acquires and releases
reference counts on the child nodes during iteration:

airoha_probe()
    for_each_child_of_node(pdev->dev.of_node, np)
        ...
        airoha_alloc_gdm_port(eth, np)
            airoha_alloc_gdm_device(eth, port, np)

Inside airoha_alloc_gdm_device(), np is directly assigned to
netdev->dev.of_node without calling of_node_get(np).

Once the loop advances or terminates, the reference to the node is implicitly
dropped. If device tree overlays are dynamically removed, netdev->dev.of_node
becomes a dangling pointer.

Could of_node_get() be called before assignment to ensure the reference
is held?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511-airoha-eth-multi-serdes-v6-0-c899462c4f75@kernel.org?part=3