Re: [PATCH net-next v2 6/8] net: xilinx: tsn: add the endpoint RX data path

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The return value of dmaengine_submit() is ignored in the RX path, leading to permanent RX ring desynchronization and data corruption if submission fails.
- [Medium] Out-of-bounds read of DMA descriptor metadata due to missing length validation.
--

commit 5fd8b30c57440a41e4ad90bda57e202d89a1aa7b
Author: Srinivas Neeli <srinivas.neeli@xxxxxxx>
Subject: net: xilinx: tsn: add the endpoint RX data path

This commit adds the RX data path for the Xilinx TSN endpoint, allowing it to
receive frames. It posts RX buffers to each channel upon opening and reaps
them via a completion callback, serializing the refill against teardown.

> diff --git a/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c b/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c
> index 23a15df71baad..9ed1170794e03 100644
> --- a/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c
> +++ b/drivers/net/ethernet/xilinx/tsn/xilinx_tsn_ep.c

[ ... ]

> @@ -113,6 +132,174 @@ ep_get_desc(struct xlnx_tsn_ep_dma_chan *xchan, int idx)
>  	return xchan->skb_ring[idx];
>  }
>  
> +static void ep_dma_rx_cb(void *data, const struct dmaengine_result *result);
> +
> +static int ep_rx_submit_desc(struct xlnx_tsn_ep_dma_chan *xchan)
> +{

[ ... ]

> +	skbuf_dma->skb = skb;
> +	skbuf_dma->dma_address = sg_dma_address(skbuf_dma->sgl);
> +	skbuf_dma->desc = dma_rx_desc;
> +	dma_rx_desc->callback_param = xchan;
> +	dma_rx_desc->callback_result = ep_dma_rx_cb;
> +	xchan->ring_head++;
> +	dmaengine_submit(dma_rx_desc);
> +
> +	return 0;

[Severity: High]
Does this code ignore the return value of dmaengine_submit() in the RX path?

If dmaengine_submit() fails and returns an error cookie (e.g., due to an
exhausted DMA descriptor pool), the hardware will not receive the descriptor.
However, because the software pre-increments xchan->ring_head and ignores
the error, it assumes the descriptor is safely armed.

When the next successful descriptor completes, the software will read from
xchan->ring_tail, mistakenly unmapping and passing the SKB of the failed
descriptor to the network stack. Could this lead to passing uninitialized
kernel memory up the network stack and shifting all subsequent packet payloads
by one descriptor?

[ ... ]

> +static void ep_dma_rx_cb(void *data, const struct dmaengine_result *result)
> +{

[ ... ]

> +	metadata = dmaengine_desc_get_metadata_ptr(skbuf_dma->desc,
> +						   &meta_len,
> +						   &meta_max_len);
> +	if (IS_ERR_OR_NULL(metadata)) {
> +		if (net_ratelimit())
> +			dev_warn(ep->dev, "Failed to get RX metadata pointer\n");
> +
> +		dev_kfree_skb_any(skb);
> +		DEV_STATS_INC(ndev, rx_dropped);
> +		DEV_STATS_INC(ndev, rx_errors);
> +		goto submit_new;
> +	}
> +
> +	/* MCDMA metadata: [0] = status, [1] = sideband (TID/TDEST/TUSER), [2..] = app */
> +	tuser = metadata[1] & TSN_TUSER_MASK;

[Severity: Medium]
Is it possible for the DMA driver to return a valid metadata pointer with a
short payload where meta_len is less than 8 bytes (2 words)?

If meta_len does not cover metadata[1], this access could result in an
out-of-bounds memory read. Does this code need to verify that meta_len is
large enough before accessing metadata[1]?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909-patches_v2_external-v2-0-3a40babaff4c@xxxxxxx?part=6