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

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Incorrect teardown order causes Use-After-Free and DMA memory corruption.
- [Critical] Flawed DMA synchronization logic leads to data corruption and complete pointer desynchronization.
- [High] Uninitialized variable `ret` returned on probe failure leads to random probe status.
- [High] Memory leak of DMA buffer and unterminated cyclic transfer on probe failure.
- [Medium] Probe incorrectly aborted on DMA initialization failure instead of falling back to PIO.
- [Medium] Artificial Kconfig dependency arbitrarily breaks driver functionality on systems without the Loongson DMA driver.

Pre-existing issues:
- [High] Memory allocation failure causes a permanent NAPI hang.
- [High] Missing bounds check on frame payload length allows heap buffer overflow.
--

commit 33782565e156b12b3812d0b21ca4a945e50adf11
Author: Bingxiong Li <libingxiong@loongson.cn>

Add optional DMA support for RX path using the Loongson APB CMC DMA engine

This patch adds optional DMA support for the RX path in the Loongson CAN-FD
driver. It requests a DMA channel and uses cyclic transfers to directly write
incoming frames to a coherent buffer, falling back to PIO mode if unavailable.

> diff --git a/drivers/net/can/Kconfig b/drivers/net/can/Kconfig
> --- a/drivers/net/can/Kconfig
> +++ b/drivers/net/can/Kconfig
> @@ -191,7 +191,7 @@ config CAN_KVASER_PCIEFD
>  
>  config CAN_LOONGSON_CANFD
>  	tristate "Loongson CAN-FD controller"
> -	depends on HAS_IOMEM || COMPILE_TEST
> +	depends on HAS_IOMEM && (LOONGSON2_APB_CMC_DMA || COMPILE_TEST)

[Severity: Medium]
Does this introduce a hard dependency that unnecessarily breaks compilation
for systems that only want to use PIO mode? Because the driver uses the
generic dmaengine framework, there shouldn't be a strict compile-time
requirement on the specific LOONGSON2_APB_CMC_DMA module.

[ ... ]

> diff --git a/drivers/net/can/loongson_canfd.c b/drivers/net/can/loongson_canfd.c
> --- a/drivers/net/can/loongson_canfd.c
> +++ b/drivers/net/can/loongson_canfd.c
> @@ -614,6 +632,125 @@ static int loongson_canfd_reset(struct net_device *ndev)
[ ... ]
> +static bool loongson_canfd_rxfrc_in_dma(struct loongson_canfd_priv *priv)
> +{
> +	struct dma_tx_state state;
> +	enum dma_status status;
> +
> +	status = dmaengine_tx_status(priv->rx_ch, priv->rx_ch->cookie, &state);
> +
> +	return priv->rx_frc != (state.residue / DMA_SLAVE_BUSWIDTH_4_BYTES) &&
> +	       (status == DMA_IN_PROGRESS);
> +}

[Severity: Critical]
Does this cause data corruption by triggering reads for partial DMA transfers?

If the DMA engine writes just a single word, this condition evaluates to true,
but loongson_canfd_rx() will unconditionally read an entire frame consisting
of multiple words. If the CPU outpaces the DMA transfer, could it read stale
unwritten memory and permanently desynchronize the pointers?

Also, when residue reaches 0, does rx_frc != 0 evaluate to true indefinitely
because rx_frc wraps from 1 to LOONGSON_CANFD_RXDMA_NUM, potentially causing
an infinite NAPI loop reading garbage?

[ ... ]

> @@ -1074,12 +1211,12 @@ static void loongson_canfd_read_rx_frame(struct loongson_canfd_priv *priv, struc
[ ... ]
>  	/* Data */
>  	for (i = 0; i < len; i += LOONGSON_CANFD_DW_BYTE) {
> -		regmap_read(priv->regmap, LOONGSON_CANFD_RX_DATA, &data);
> +		data = priv->get_rx_data(priv);
>  		*(__le32 *)(cf->data + i) = cpu_to_le32(data);
>  	}

[Severity: High]
This is a pre-existing issue, but does the lack of bounds checking on len
allow a heap buffer overflow?

The len is calculated from the word count in the frame metadata. Without
validation ensuring it is bounded by CANFD_MAX_DLEN, could a corrupted word
count cause this loop to write out-of-bounds into the skb data buffer?

[ ... ]

> @@ -1328,16 +1465,16 @@ static int loongson_canfd_rx_napi(struct napi_struct *napi, int quota)
[ ... ]
>  	while (rx_frc && work_done < quota && ret > 0) {
>  		ret = loongson_canfd_rx(ndev);
>  		work_done++;
> -		regmap_read(priv->regmap, LOONGSON_CANFD_RX_STAT, &rx_sts);
> -		rx_frc = FIELD_GET(REG_RX_STAT_RXFRC, rx_sts);
> +		rx_frc = priv->get_rx_frc(priv);
>  	}
[ ... ]
> @@ -1367,13 +1504,11 @@ static int loongson_canfd_rx_napi(struct napi_struct *napi, int quota)
[ ... ]
>  	if (!rx_frc && ret != 0) {
>  		if (napi_complete_done(napi, work_done)) {

[Severity: High]
This is a pre-existing issue, but does a memory allocation failure cause a
permanent NAPI hang here?

If loongson_canfd_rx() fails to allocate an skb, it returns -ENOMEM. The
loop breaks while rx_frc remains true, which makes the check
"if (!rx_frc && ret != 0)" evaluate to false. Doesn't this bypass
napi_complete_done() and leave NAPI permanently in the SCHED state without
re-enabling interrupts?

[ ... ]

> @@ -1663,6 +1799,7 @@ static int loongson_canfd_probe(struct platform_device *pdev)
[ ... ]
>  	/* Create a CAN device instance */
>  	ndev = alloc_candev(sizeof(*priv), LOONGSON_CANFD_TXBUF_NUM);
>  	if (!ndev)
> -		return -ENOMEM;
> +		goto err_dma_rx;

[Severity: High]
Does this return an uninitialized variable on probe failure?

The variable ret is declared at the top of the function but not initialized.
If alloc_candev() fails, jumping to err_dma_rx will return the uninitialized
ret, potentially signaling a false success to the device core.

[ ... ]

> @@ -1720,6 +1867,19 @@ static int loongson_canfd_probe(struct platform_device *pdev)
[ ... ]
> +	if (priv->rx_ch) {
[ ... ]
> +		ret = loongson_canfd_rxdma_init(ndev);
> +		if (ret) {
> +			dev_err(dev, "interrupt mode used for rx (no dma)\n");
> +			goto err_candev_free;
> +		}

[Severity: Medium]
Is it intentional to abort the probe entirely here?

The error message indicates an intent to fall back to interrupt (PIO) mode
if DMA setup fails, but jumping to err_candev_free will cause the probe to
fail and leave the driver unusable instead of gracefully falling back.

[ ... ]

> @@ -1732,6 +1892,9 @@ static int loongson_canfd_probe(struct platform_device *pdev)
>  
>  err_candev_free:
>  	free_candev(ndev);
> +err_dma_rx:
> +	if (rx_ch)
> +		dma_release_channel(rx_ch);
>  	return ret;
>  }

[Severity: High]
Does this error path leak memory and leave a cyclic DMA transfer active?

If register_candev() fails, this path frees the device and releases the
channel, but it does not call loongson_canfd_rxdma_remove() to free the
coherent DMA memory.

Also, isn't a dmaengine_terminate_sync() call needed to stop the hardware
from actively writing to the leaked memory?

[ ... ]

> @@ -1740,6 +1903,11 @@ static void loongson_canfd_remove(struct platform_device *pdev)
>  	struct net_device *ndev = platform_get_drvdata(pdev);
>  	struct loongson_canfd_priv *priv = netdev_priv(ndev);
>  
> +	if (priv->rx_ch) {
> +		loongson_canfd_rxdma_remove(priv, &pdev->dev);
> +		dma_release_channel(priv->rx_ch);
> +	}
> +
>  	unregister_candev(ndev);
>  	netif_napi_del(&priv->napi);
>  	free_candev(ndev);

[Severity: Critical]
Does freeing the DMA memory before unregistering the device cause a
use-after-free and memory corruption?

While unregister_candev() is executing, the network device and NAPI are
still active. If a CAN interrupt fires, couldn't the NAPI poll loop
(loongson_canfd_rx_napi) read from the freed priv->rx_buf?

Additionally, since the cyclic DMA transfer is never stopped via
dmaengine_terminate_sync() before releasing the channel, could the hardware
continue writing network frames into freed system memory?

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