Re: [RFC PATCH net-next 5/7] net: airoha: add QDMA support for SOE packets

[sashiko-bot@kernel.org]

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

New issues:
- [High] Accessing `skb` after handing it off to the QDMA hardware TX ring causes a Use-After-Free vulnerability.

Pre-existing issues:
- [High] Using `dma_map_single()` with `skb_frag_address()` instead of `skb_frag_dma_map()` causes mapping failures or kernel panics on systems with high memory.
- [High] Unnecessarily re-evaluating `dev` and `netdev` from scattered RX descriptors causes invalid metadata extraction, resulting in misrouted or dropped packets.
--

commit a3fa3b0477739d5fa5e7176d3210e1a5b7b0f8df
Author: Jihong Min <hurryman2212@gmail.com>

net: airoha: add QDMA support for SOE packets

Add QDMA RX/TX plumbing for SOE packets, including the SOE RX ring,
coherent RX slots, SOE completion decoding, and the private QDMA submit
helper used by the SOE provider. Wire the Ethernet netdev feature and
lifetime hooks through the SOE stubs.

> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
[ ... ]
> @@ -822,92 +822,210 @@ static int airoha_qdma_rx_process(struct airoha_queue *q, int budget)
[ ... ]
>  		len = FIELD_GET(QDMA_DESC_LEN_MASK, desc_ctrl);
> -		data_len = q->skb ? AIROHA_RX_LEN(q->buf_size) : e->dma_len;
> +		data_len = q->skb && !q->rx_coherent ?
> +				   AIROHA_RX_LEN(q->buf_size) :
> +				   e->dma_len;
>  		if (!len || data_len < len)
>  			goto free_frag;
>  
> -		dev = airoha_qdma_get_gdm_dev(eth, desc);
> -		if (IS_ERR(dev))
> -			goto free_frag;
> +		msg0 = le32_to_cpu(READ_ONCE(desc->msg0));
> +		msg1 = le32_to_cpu(READ_ONCE(desc->msg1));
> +		msg2 = le32_to_cpu(READ_ONCE(desc->msg2));
> +#if IS_REACHABLE(CONFIG_NET_AIROHA_SOE)
> +		soe_pkt = partial_soe || airoha_qdma_rx_is_soe(msg0);
> +#endif
> +		if (!soe_pkt) {
> +			dev = airoha_qdma_get_gdm_dev(eth, desc);
> +			if (IS_ERR(dev))
> +				goto free_frag;
> +			netdev = netdev_from_priv(dev);
> +			rx_dev = netdev;
> +		}

[Severity: High]
This is a pre-existing issue, but does re-evaluating dev and netdev on every
descriptor in a scattered chain overwrite the valid metadata from the first
descriptor?

If the hardware only writes msg0 and msg1 to the first descriptor and
zeroes them out in subsequent ones, re-evaluating here might corrupt DSA
routing and flow hashing by using zeroed data.

[ ... ]
> @@ -2176,6 +2552,110 @@ int airoha_get_fe_port(struct airoha_gdm_dev *dev)
[ ... ]
> +int airoha_qdma_xmit_skb(struct airoha_gdm_dev *dev, struct sk_buff *skb,
> +			 u32 msg0, u32 msg1, u32 msg2)
> +{
[ ... ]
> +	while (true) {
> +		struct airoha_qdma_desc *desc = &q->desc[index];
> +		skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
> +		dma_addr_t addr;
> +		u32 val;
> +
> +		addr = dma_map_single(netdev->dev.parent, data, len,
> +				      DMA_TO_DEVICE);

[Severity: High]
This is a pre-existing issue copied into this new function (and also present
in airoha_dev_xmit), but does using dma_map_single() with skb_frag_address()
risk mapping failures or kernel panics on systems with high memory?

On architectures with HIGHMEM, page_address() returns NULL for pages not
mapped into the kernel's virtual address space. Calling dma_map_single() on
a NULL address could map garbage or trigger a kernel oops. Should this use
skb_frag_dma_map() instead?

[ ... ]
> @@ -2293,13 +2779,24 @@ static netdev_tx_t airoha_dev_xmit(struct sk_buff *skb,
>  		q->txq_stopped = true;
>  	}
>  
> -	if (netif_xmit_stopped(txq) || !netdev_xmit_more())
> +	if (netif_xmit_stopped(txq) || !netdev_xmit_more() ||
> +	    soe_decrypt_skb)
>  		airoha_qdma_rmw(qdma, REG_TX_CPU_IDX(qid),
>  				TX_RING_CPU_IDX_MASK,
>  				FIELD_PREP(TX_RING_CPU_IDX_MASK, index));
>  
>  	spin_unlock_bh(&q->lock);
>  
> +	/* SOE decrypt flow binding needs the final egress netdev and QDMA
> +	 * descriptor context. The SOE RX path marks only candidate packets; bind
> +	 * only after the current plaintext packet has been queued and kicked so
> +	 * the newly bound decrypt entry cannot race the CPU-transmitted packet.
> +	 */
> +#if IS_REACHABLE(CONFIG_NET_AIROHA_SOE)
> +	if (soe_decrypt_skb)
> +		airoha_ppe_soe_xmit_skb(&dev->eth->ppe->dev, skb, netdev);
> +#endif
> +
>  	return NETDEV_TX_OK;

[Severity: High]
Could this cause a use-after-free by accessing the skb after it has been
handed off to the hardware?

Once the transmission queue spinlock is released and the hardware doorbell
is kicked via airoha_qdma_rmw(), the skb ownership is fully transferred.
The transmission could complete concurrently on another CPU, which would
invoke dev_kfree_skb_any(skb) in the TX completion handler before
airoha_ppe_soe_xmit_skb() is able to safely access skb->mark.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260614040032.1567994-1-hurryman2212@gmail.com?part=5