Re: [PATCH net v3] net: ti: icssg-prueth: fix XDP_TX from the AF_XDP zero-copy RX path

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Uninitialized descriptor memory used for DMA unmapping in error paths.
- [High] DMA API violation: memory is freed before the DMA mapping is torn down.
- [High] Memory leak and double free when XDP_TX or XDP_REDIRECT fails in AF_XDP zero-copy mode.
- [High] Missing DMA synchronization for transmitted Page Pool XDP_TX frames.
- [High] Use-After-Free reading xdpf->len after handing the frame to the hardware TX queue.
- [Medium] Concurrent non-atomic increments to shared global network statistics.
--

commit dc2f8356525786b4ee8c94beae174920f574ebd4
Author: David Carlier <devnexen@gmail.com>

    net: ti: icssg-prueth: fix XDP_TX from the AF_XDP zero-copy RX path
    
    On XDP_TX from the zero-copy RX path, emac_run_xdp() converts the xsk
    buffer via xdp_convert_zc_to_xdp_frame(), which clones the data into a
    fresh MEM_TYPE_PAGE_ORDER0 page that is not DMA mapped. Transmitting it
    as PRUETH_TX_BUFF_TYPE_XDP_TX derives the DMA address with
    page_pool_get_dma_addr(), reading an uninitialized page->dma_addr, so
    the device DMAs from a bogus address (corrupt TX, or an IOMMU fault).

> diff --git a/drivers/net/ethernet/ti/icssg/icssg_common.c b/drivers/net/ethernet/ti/icssg/icssg_common.c
> index 4a7d1a6f470b1..1d9ae764ce88f 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_common.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_common.c
[ ... ]
> @@ -259,6 +260,7 @@ int emac_tx_complete_packets(struct prueth_emac *emac, int chn,
>  			napi_consume_skb(skb, budget);
>  			break;
>  		case PRUETH_SWDATA_XDPF:
> +		case PRUETH_SWDATA_XDPF_TX:
>  			xdpf = swdata->data.xdpf;
>  			dev_sw_netstats_tx_add(ndev, 1, xdpf->len);
>  			total_bytes += xdpf->len;

[Severity: High]
This is a pre-existing issue, but does this violate the DMA API by freeing the
memory before tearing down the DMA mapping?

In both emac_tx_complete_packets() and prueth_tx_cleanup(), the driver frees
the XDP frame via xdp_return_frame() before calling prueth_xmit_free(), which
performs the actual dma_unmap_single().

The physical memory is returned to the allocator and can be immediately reused
by another subsystem while the device or IOMMU still has an active mapping for
it, creating a use-after-free window.

[ ... ]
> @@ -768,7 +770,8 @@ u32 emac_xmit_xdp_frame(struct prueth_emac *emac,
>  	k3_udma_glue_tx_dma_to_cppi5_addr(tx_chn->tx_chn, &buf_dma);
>  	cppi5_hdesc_attach_buf(first_desc, buf_dma, xdpf->len, buf_dma, xdpf->len);
>  	swdata = cppi5_hdesc_get_swdata(first_desc);
> -	swdata->type = PRUETH_SWDATA_XDPF;
> +	swdata->type = (buff_type == PRUETH_TX_BUFF_TYPE_XDP_TX ?
> +		PRUETH_SWDATA_XDPF_TX : PRUETH_SWDATA_XDPF);
>  	swdata->data.xdpf = xdpf;

[Severity: Critical]
This is a pre-existing issue, but does the error path pass an uninitialized
first_desc to the cleanup function?

If dma_map_single() or virt_to_head_page() fails in emac_xmit_xdp_frame(), the
driver jumps to the drop_free_descs error path and passes the newly allocated
first_desc to prueth_xmit_free().

Because the descriptor is allocated via k3_cppi_desc_pool_alloc() without
zeroing the memory, and it is passed before swdata->type and obuf are
initialized, prueth_xmit_free() will read stale swdata->type and garbage
buf_dma/buf_dma_len from a previous packet.

It then calls dma_unmap_single() with this garbage DMA address, potentially
unmapping memory actively used by other subsystems.

[Severity: High]
This isn't a bug introduced by this patch, but does the page pool frame
transmission miss a DMA synchronization?

When buff_type == PRUETH_TX_BUFF_TYPE_XDP_TX, emac_xmit_xdp_frame()
retrieves the DMA address via page_pool_get_dma_addr() and passes it to the
TX hardware without calling dma_sync_single_for_device().

Because the hardware is not cache-coherent, the device may transmit stale data
from RAM instead of the modified cache lines from the BPF program.

[ ... ]
> @@ -825,11 +829,21 @@ static u32 emac_run_xdp(struct prueth_emac *emac, struct xdp_buff *xdp, u32 *len
>  			goto drop;
>  		}
>  
> +		/* In AF_XDP zero-copy mode xdp_convert_buff_to_frame()
> +		 * clones the xsk buffer into a fresh MEM_TYPE_PAGE_ORDER0
> +		 * page that is not DMA mapped. Such a frame must be mapped
> +		 * via the NDO path; only a page pool-backed frame already
> +		 * carries a usable page_pool DMA address.
> +		 */
> +		tx_buff_type = (xdpf->mem_type == MEM_TYPE_PAGE_POOL ?
> +				PRUETH_TX_BUFF_TYPE_XDP_TX :
> +				PRUETH_TX_BUFF_TYPE_XDP_NDO);
> +
>  		q_idx = cpu % emac->tx_ch_num;
>  		netif_txq = netdev_get_tx_queue(ndev, q_idx);
>  		__netif_tx_lock(netif_txq, cpu);
>  		result = emac_xmit_xdp_frame(emac, xdpf, q_idx,
> -					     PRUETH_TX_BUFF_TYPE_XDP_TX);
> +					     tx_buff_type);
>  		__netif_tx_unlock(netif_txq);
>  		if (result == ICSSG_XDP_CONSUMED) {
>  			ndev->stats.tx_dropped++;

[Severity: High]
This is a pre-existing issue, but does this leak memory and cause a
double-free when XDP_TX fails in AF_XDP zero-copy mode?

When an AF_XDP zero-copy buffer is handled, xdp_convert_buff_to_frame()
allocates a new page for the XDP frame and frees the original xsk_buff via
xsk_buff_free().

If the transmission fails and emac_xmit_xdp_frame() returns
ICSSG_XDP_CONSUMED, emac_run_xdp() propagates ICSSG_XDP_CONSUMED to the
caller but never frees the newly allocated xdpf page.

Furthermore, the caller emac_rx_packet_zc() unconditionally calls
xsk_buff_free(xdp) when receiving ICSSG_XDP_CONSUMED, double-freeing the
original xdp buffer.

[Severity: Medium]
This isn't a bug introduced by this patch, but does this perform an unprotected
increment to a shared global statistic?

The driver performs unprotected increments like ndev->stats.tx_dropped++
in multiple contexts, including emac_run_xdp() and icssg_ndo_start_xmit().
Because multiple CPUs can execute these paths concurrently, these
read-modify-write operations on the shared net_device struct can result in
torn reads and lost updates.

[Severity: High]
This is a pre-existing issue, but is there a use-after-free when reading
xdpf->len after handing the frame to the hardware TX queue?

In emac_run_xdp(), the driver passes xdpf to emac_xmit_xdp_frame(), which
enqueues it to the hardware DMA ring. Once pushed, the hardware could transmit
it immediately, allowing the TX completion interrupt on another CPU to pop the
descriptor and free xdpf.

Following this handoff, emac_run_xdp() accesses xdpf->len for statistics
accounting (dev_sw_netstats_rx_add), resulting in a use-after-free.

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