Re: [PATCH v8 2/2] can: loongson_canfd: Add RXDMA support

[sashiko-bot@kernel.org]

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

New issues:
- [High] The cyclic DMA buffer wrap detection logic incorrectly interprets a normal buffer wrap as an overflow, silently discarding valid unread CAN frames.
- [High] The lockless NAPI DMA ring buffer logic reads variable-length CAN frames without verifying that the DMA engine has transferred the entire frame, leading to CPU over-reads of uninitialized or stale memory.
- [High] The cyclic DMA transfer is started during driver probe and runs continuously, leading to stream corruption across interface resets.

Pre-existing issues:
- [Critical] Missing payload length constraint for classic CAN frames leads to a heap buffer overflow in the SKB linear data area.
- [High] Malformed or invalid CAN frames cause permanent FIFO/DMA buffer desynchronization and an infinite NAPI polling loop by returning early without draining the invalid frame payload.
--

commit 27376fa7494d4080cdce1ee73b4800fd7a987b82
Author: Binbin Zhou <zhoubinbin@loongson.cn>

can: loongson_canfd: Add RXDMA support

Extend the driver with an optional DMA engine support for the receive path.
When a DMA channel is available, the RX buffer is mapped to a cyclic DMA
transfer that continuously fills a pre-allocated ring buffer. The driver
then reads incoming CAN frames directly from the DMA buffer, improving
performance under high traffic.

> diff --git a/drivers/net/can/loongson_canfd.c b/drivers/net/can/loongson_canfd.c
> index 6c983987a1208..7fb28bd00c14c 100644
> --- a/drivers/net/can/loongson_canfd.c
> +++ b/drivers/net/can/loongson_canfd.c
> @@ -523,12 +543,166 @@ static void loongson_canfd_set_txbuf_cmd(struct net_device *ndev,

[ ... ]

> +static bool loongson_canfd_rxbuf_empty_dma(struct loongson_canfd_priv *priv)
> +{
> +	struct dma_tx_state state;
> +	enum dma_status status;
> +	u32 wptr_dist;
> +
> +	status = dmaengine_tx_status(priv->rx_ch, priv->rx_cookie, &state);
> +	if (status != DMA_IN_PROGRESS)
> +		return true;
> +
> +	/* Convert residue from bytes to number of 32-bit words */
> +	wptr_dist = state.residue / DMA_SLAVE_BUSWIDTH_4_BYTES;
> +
> +	/*
> +	 * wptr_dist is the write pointer's distance from the buffer end.
> +	 * It starts at BUFFER_SIZE (when write pointer is at start) and
> +	 * decreases to 0 (when write pointer reaches end), then wraps back.
> +	 * priv->rx_rd_ptr_dist is the read pointer's distance from the end.
> +	 * If write pointer wraps past read pointer, wptr_dist becomes larger,
> +	 * indicating data loss, so we advance read pointer to write position.
> +	 */
> +	if (priv->rx_rd_ptr_dist < wptr_dist)
> +		priv->rx_rd_ptr_dist = wptr_dist;

[Severity: High]
Does this logic mistakenly trigger on every normal buffer wrap-around?

Both wptr_dist and rx_rd_ptr_dist track the pointer's distance from the end
of the buffer, decreasing towards 0. When the DMA write pointer completes a
cycle and wraps back to the beginning, wptr_dist resets to its maximum value.

If the read pointer has not yet wrapped, the condition
priv->rx_rd_ptr_dist < wptr_dist unexpectedly evaluates to true during a
normal wrap-around.

The driver misinterprets this as the write pointer overtaking the read
pointer and forcefully sets priv->rx_rd_ptr_dist = wptr_dist, which will
erroneously skip all unread valid data remaining at the end of the buffer.

Conversely, if a true overflow occurs, wptr_dist is smaller, evaluating to
false and failing to detect the overflow.

> +
> +	return priv->rx_rd_ptr_dist == wptr_dist;

[Severity: High]
Could this cause the CPU to over-read uninitialized or stale memory?

This function returns false as soon as the DMA engine writes a single 32-bit
word. Because the DMA engine transfers data asynchronously and word-by-word
(or in small bursts), it may not have finished writing the full variable-length
CAN frame when this function signals that data is ready.

The CPU will read ahead of the DMA write pointer in loongson_canfd_rx(),
fetching stale memory, reading garbage payload or frame lengths, and
permanently corrupting the stream pointer.

> +}

[ ... ]

> +static int loongson_canfd_rxdma_init(struct net_device *ndev)
> +{
> +	struct loongson_canfd_priv *priv = netdev_priv(ndev);
> +	struct dma_async_tx_descriptor *desc = NULL;
> +	struct device *dev = ndev->dev.parent;
> +	struct dma_slave_config config;
> +	int ret;
> +
> +	if (!priv->rx_ch)
> +		return -EINVAL;
> +
> +	priv->rx_buf = dma_alloc_coherent(dev, LOONGSON_CANFD_RXBUF_SZ,
> +					  &priv->rx_dma_buf, GFP_KERNEL);
> +	if (!priv->rx_buf) {
> +		dma_release_channel(priv->rx_ch);
> +		priv->rx_ch = NULL;
> +		return -ENOMEM;
> +	}
> +
> +	memset(&config, 0, sizeof(config));
> +	config.src_addr = priv->res->start + LOONGSON_CANFD_RX_DATA;
> +	config.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> +
> +	ret = dmaengine_slave_config(priv->rx_ch, &config);
> +	if (ret < 0) {
> +		netdev_err(ndev, "RX DMA channel config failed\n");
> +		goto err_config;
> +	}
> +
> +	desc = dmaengine_prep_dma_cyclic(priv->rx_ch, priv->rx_dma_buf,
> +					 LOONGSON_CANFD_RXBUF_SZ, LOONGSON_CANFD_RXBUF_SZ,
> +					 DMA_DEV_TO_MEM, DMA_PREP_INTERRUPT);
> +	if (!desc) {
> +		netdev_err(ndev, "RX DMA cyclic transaction preparation failed\n");
> +		ret = -EBUSY;
> +		goto err_config;
> +	}
> +
> +	priv->rx_cookie = dmaengine_submit(desc);

[Severity: High]
Could starting the DMA engine here during probe and running it continuously
lead to stream corruption across interface resets?

When the interface is brought down via loongson_canfd_chip_stop(), the CAN
controller stops and resets, but the DMA engine remains active with its
current pointers.

When brought back up, the CAN controller is reset, but the DMA pointers and
CPU read pointers are not reset. Because the hardware FIFO was flushed but
the DMA buffer was not, the newly received CAN frames are appended to
whatever partial frame was left in the DMA buffer before the reset.

Will this splice frames together and cause permanent stream desynchronization
the first time the interface recovers from a bus-off condition or is manually
reset?

> +	ret = dma_submit_error(priv->rx_cookie);

[ ... ]

> @@ -929,14 +1106,14 @@ static int loongson_canfd_rx(struct net_device *ndev)
>  {
>  	struct loongson_canfd_priv *priv = netdev_priv(ndev);
>  	struct net_device_stats *stats = &ndev->stats;
> -	u32 frame_hdr0, frame_hdr1, dlc, rwcnt, dbcnt, i, data;
> +	u32 frame_hdr0, frame_hdr1, dlc, rwcnt, dbcnt, i;
>  	struct canfd_frame *cfd;
>  	struct can_frame *ccf;
>  	struct sk_buff *skb;
>  	bool is_can_fd;
>  
> -	regmap_read(priv->regmap, LOONGSON_CANFD_RX_DATA, &frame_hdr0);
> -	regmap_read(priv->regmap, LOONGSON_CANFD_RX_DATA, &frame_hdr1);
> +	frame_hdr0 = priv->get_rx_data(priv);
> +	frame_hdr1 = priv->get_rx_data(priv);
>  
>  	rwcnt = FIELD_GET(REG_FRAME_META1_RWCNT, frame_hdr1);
>  	if (rwcnt < 2)
>  		return -EAGAIN;

[Severity: High]
This is a pre-existing issue, but does this early return cause permanent
FIFO or DMA buffer desynchronization?

If the parsed header yields rwcnt < 2, the function returns -EAGAIN early.
This aborts processing but leaves the read pointer (either the hardware FIFO
pop count or the DMA read index) partially advanced by only 2 words.

The remaining payload words of the invalid frame are left in the buffer or
FIFO. On the next NAPI poll, the driver will mistakenly read these leftover
payload words as if they were a new header, resulting in permanent stream
desynchronization.

Because the unread payload remains in the buffer, the empty check
continuously returns true, triggering an interrupt storm and an infinite NAPI
loop that hangs the CPU.

>  
>  	dbcnt = (rwcnt - 2) * LOONGSON_CANFD_DW_BYTE;
>  	if (dbcnt > CANFD_MAX_DLEN)
>  		return -EAGAIN;

[Severity: High]
This is a pre-existing issue, but could this early return also cause permanent
buffer desynchronization and NAPI loops, just like the rwcnt < 2 case above,
because it fails to drain the remainder of the malformed frame?

>  
>  	is_can_fd = !!(frame_hdr1 & REG_FRAME_META1_FDF);
>  	if (is_can_fd) {

[ ... ]

> @@ -983,13 +1160,11 @@ static int loongson_canfd_rx(struct net_device *ndev)
>  	else
>  		can_frame_set_cc_len(ccf, dlc, priv->can.ctrlmode);
>  
> -	for (i = 0; i < dbcnt; i += LOONGSON_CANFD_DW_BYTE) {
> -		regmap_read(priv->regmap, LOONGSON_CANFD_RX_DATA, &data);
> -		put_unaligned_le32(data, cfd->data + i);
> -	}
> +	for (i = 0; i < dbcnt; i += LOONGSON_CANFD_DW_BYTE)
> +		put_unaligned_le32(priv->get_rx_data(priv), cfd->data + i);

[Severity: Critical]
This is a pre-existing issue, but does this loop cause a heap buffer overflow
in cfd->data for classic CAN frames?

The code checks if dbcnt > CANFD_MAX_DLEN (64 bytes), but fails to ensure
dbcnt <= CAN_MAX_DLEN (8 bytes) when processing a classic CAN frame.

For classic CAN frames, alloc_can_skb() allocates an SKB containing exactly
a 16-byte struct can_frame.

If dbcnt exceeds 8 (which happens when the parser is desynchronized by the
DMA race conditions or corrupted hardware state), this loop copies up to 64
bytes into cfd->data. This writes up to 56 bytes past the end of the allocated
16-byte buffer, triggering an overflow in the SKB's linear data area.

[Severity: High]
Could this unconditional read of the payload read ahead of the DMA write
pointer?

Because loongson_canfd_rxbuf_empty_dma() signals data ready when the first
word is written, the DMA engine may not have transferred these payload words
yet. This will read garbage payload data and corrupt the stream state.

>  
>  	while (unlikely(i < dbcnt)) {
> -		regmap_read(priv->regmap, LOONGSON_CANFD_RX_DATA, &data);
> +		priv->get_rx_data(priv);
>  		i += LOONGSON_CANFD_DW_BYTE;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788945497.git.zhoubinbin@loongson.cn?part=2