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

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3991 bytes --]

> 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.

As already pointed out, this does not seem an issue.

> 
> [ ... ]
> 
> > @@ -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.

ack, I will fix it in v8.

Regards,
Lorenzo

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]