Re: [PATCH net-next v7 02/10] net: airoha: Introduce airoha_gdm_dev struct

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The patch introduces a race condition exposing a partially initialized port, leading to a NULL pointer dereference in NAPI polling handlers.
- [High] The `airoha_get_dsa_tag` function lacks packet length validation before stripping the MTK DSA tag, leading to an out-of-bounds memory access and `skb->len` integer underflow.
--

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

net: airoha: Introduce airoha_gdm_dev struct

This commit introduces the airoha_gdm_dev structure to collect net_device
related info, preparing the driver for a 1:n relationship between GDM ports
and net_devices.

> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 5a027cc7ffcb1..3ca0dbd19275a 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c

[ ... ]

> @@ -2007,7 +2020,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);

While looking at this patch, I noticed an issue in airoha_get_dsa_tag() that
pre-exists in the code. Does this function lack packet length validation
before pulling the MTK DSA tag?

airoha_get_dsa_tag() unconditionally performs memmove() and __skb_pull():

        memmove(skb->data + MTK_HDR_LEN, skb->data, 2 * ETH_ALEN);
        __skb_pull(skb, MTK_HDR_LEN);

If an unprivileged user sends an undersized packet via a raw socket
(AF_PACKET), could this result in an out-of-bounds memory access and cause
skb->len to integer-underflow?

This could lead to dma_map_single() attempting to map a massive amount of
memory, potentially causing an IOMMU fault and a local denial-of-service.

[ ... ]

> @@ -2975,53 +3060,22 @@ static int airoha_alloc_gdm_port(struct airoha_eth *eth,
>  		return -EINVAL;
>  	}
>  
> -	dev = devm_alloc_etherdev_mqs(eth->dev, sizeof(*port),
> -				      AIROHA_NUM_NETDEV_TX_RINGS,
> -				      AIROHA_NUM_RX_RING);
> -	if (!dev) {
> -		dev_err(eth->dev, "alloc_etherdev failed\n");
> +	port = devm_kzalloc(eth->dev, sizeof(*port), GFP_KERNEL);
> +	if (!port)
>  		return -ENOMEM;

[ ... ]

> -	port = netdev_priv(dev);
>  	u64_stats_init(&port->stats.syncp);
>  	spin_lock_init(&port->stats.lock);
> -	port->eth = eth;
> -	port->dev = dev;
>  	port->id = id;
>  	/* XXX: Read nbq from DTS */
>  	port->nbq = id == AIROHA_GDM3_IDX && airoha_is_7581(eth) ? 4 : 0;
>  	eth->ports[p] = port;
>  
> -	return airoha_metadata_dst_alloc(port);
> +	err = airoha_metadata_dst_alloc(port);
> +	if (err)
> +		return err;
> +
> +	return airoha_alloc_gdm_device(eth, port, np);
>  }

Could an interrupt cause a NULL pointer dereference in the NAPI handlers?

Since NAPI is enabled earlier in airoha_probe() via airoha_qdma_start_napi(),
and eth->ports[p] is assigned here before airoha_alloc_gdm_device() populates
port->dev, NAPI handlers like airoha_qdma_rx_process() could see a NULL
port->dev:

airoha_qdma_rx_process()
        ...
        port = eth->ports[p];
        netdev = port->dev->dev;

I note that this issue is fixed later in the series by commit 68e8fa8fe0224
("net: airoha: Support multiple net_devices for a single FE GDM port"), which
changes port->dev to an array and adds proper NULL checks.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260516-airoha-eth-multi-serdes-v7-0-99e0093303e2@kernel.org?part=2