Re: [PATCH net] net: airoha: Do not read uninitialized fragment address in airoha_dev_xmit()

[Lorenzo Bianconi <lorenzo@kernel.org>]

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

> The transmit loop in airoha_dev_xmit() reads fragment address and length
> during its final iteration, when the loop index equals
> skb_shinfo(skb)->nr_frags, at which point the fragment data is
> uninitialized. While these values are never consumed, the read itself is
> unsafe and may trigger a page fault. Fix this by avoiding the fragment
> read on the last iteration.
> Additionally, move the skb pointer from the first to the last used packet
> descriptor, so that airoha_qdma_tx_napi_poll() defers freeing the skb
> until the final descriptor is processed.
> 
> Fixes: 23020f0493270 ("net: airoha: Introduce ethernet support for EN7581 SoC")
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>  drivers/net/ethernet/airoha/airoha_eth.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 2bb0a3ff9810..d3a841908c82 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
> @@ -1997,8 +1997,8 @@ static netdev_tx_t airoha_dev_xmit(struct sk_buff *skb,
>  	struct netdev_queue *txq;
>  	struct airoha_queue *q;
>  	LIST_HEAD(tx_list);
> +	int i = 0, qid;
>  	void *data;
> -	int i, qid;
>  	u16 index;
>  	u8 fport;
>  
> @@ -2057,7 +2057,7 @@ static netdev_tx_t airoha_dev_xmit(struct sk_buff *skb,
>  			     list);
>  	index = e - q->entry;
>  
> -	for (i = 0; i < nr_frags; i++) {
> +	while (true) {
>  		struct airoha_qdma_desc *desc = &q->desc[index];
>  		skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
>  		dma_addr_t addr;
> @@ -2069,7 +2069,7 @@ static netdev_tx_t airoha_dev_xmit(struct sk_buff *skb,
>  			goto error_unmap;
>  
>  		list_move_tail(&e->list, &tx_list);
> -		e->skb = i ? NULL : skb;
> +		e->skb = i == nr_frags - 1 ? skb : NULL;
>  		e->dma_addr = addr;
>  		e->dma_len = len;
>  
> @@ -2088,6 +2088,9 @@ static netdev_tx_t airoha_dev_xmit(struct sk_buff *skb,
>  		WRITE_ONCE(desc->msg1, cpu_to_le32(msg1));
>  		WRITE_ONCE(desc->msg2, cpu_to_le32(0xffff));
>  
> +		if (++i == nr_frags)
> +			break;
> +
>  		data = skb_frag_address(frag);
>  		len = skb_frag_size(frag);
>  	}
> 
> ---
> base-commit: e728258debd553c95d2e70f9cd97c9fde27c7130
> change-id: 20260423-airoha-xmit-fix-read-frag-dc6aa001ca4b
> 
> Best regards,
> -- 
> Lorenzo Bianconi <lorenzo@kernel.org>
> 

Commenting on Sashiko reported issues:
https://sashiko.dev/#/patchset/20260424-airoha-xmit-fix-read-frag-v1-1-fdc0a83c79e8%40kernel.org

- Does the TSO checksum calculation earlier in this function ensure the
  TCP header is in the linear portion of the SKB?
  This issue is not related to the current patch. Moreover, can we have a TSO
  packet where the tcp header is not in the linear area of the skb?

- If dma_map_single() fails partway through a multi-fragment packet and
  triggers this goto error_unmap, will it break the Tx ring contiguity?
  This issue is not related to the current patch. Moreover, I guess the hw is
  capable of managing out-of-order descriptors.

- Is it safe to map fragment data using dma_map_single() instead of
  skb_frag_dma_map()?
  This issue is not related to the current patch. I will post a dedicated patch
  for it.

Regards,
Lorenzo

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