Re: [PATCH v3 30/34] dmaengine: altera-msgdma: use channel BH helpers

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

Pre-existing issues:
- [High] The failure path in `msgdma_probe()` does not call `dma_async_device_unregister()` if `of_dma_controller_register()` fails, leading to a Use-After-Free.
- [High] The IRQ handler is requested before crucial data structures, lists, spinlocks, and the bottom-half worker are initialized, which can cause a kernel panic on an early interrupt.
- [High] Batching multiple DMA transactions causes dropped transfers due to incorrect list splicing in `msgdma_start_transfer()`.
- [High] Dropping the spinlock while iterating a list via `list_for_each_entry_safe` enables arbitrary concurrent list destruction, leading to a Use-After-Free.
- [High] Zero-length transfers bypass descriptor limit checking, breaking accounting metrics and eventually leading to a NULL pointer dereference.
- [High] A hardware-induced soft lockup where the driver spins infinitely with interrupts disabled if the hardware descriptor FIFO remains full.
- [High] Missing bitmask application when reading the response fill-level register allows undefined upper bits to impose massive computational loops, freezing the kernel.
--

--- Patch [30]: [PATCH v3 30/34] dmaengine: altera-msgdma: use channel BH helpers ---
commit 728ca1a7dab7d5ba74ab85afa0d21836b794d564
Author: Allen Pais <[email protected]>

dmaengine: altera-msgdma: use channel BH helpers

Replace the device tasklet with the per-channel BH helper so completion
handling runs via dmaengine_*_bh.

> diff --git a/drivers/dma/altera-msgdma.c b/drivers/dma/altera-msgdma.c
> index e23e5b441a24e..c8e145cea6d25 100644
> --- a/drivers/dma/altera-msgdma.c
> +++ b/drivers/dma/altera-msgdma.c
[ ... ]
> @@ -684,11 +683,12 @@ static int msgdma_alloc_chan_resources(struct dma_chan *dchan)
>  
>  /**
>   * msgdma_tasklet - Schedule completion tasklet
> - * @t: Pointer to the Altera sSGDMA channel structure
> + * @chan: Pointer to the Altera sSGDMA channel structure
>   */
> -static void msgdma_tasklet(struct tasklet_struct *t)
> +static void msgdma_tasklet(struct dma_chan *chan)
>  {
> -	struct msgdma_device *mdev = from_tasklet(mdev, t, irq_tasklet);
> +	struct msgdma_device *mdev = container_of(chan, struct msgdma_device,
> +						  dmachan);
>  	u32 count;
>  	u32 __maybe_unused size;
>  	u32 __maybe_unused status;

[Severity: High]
This isn't a bug introduced by this patch, but there appears to be a missing
bitmask on the response fill-level read further down in this function:

	if (mdev->resp) {
		/* Read number of responses that are available */
		count = ioread32(mdev->csr + MSGDMA_CSR_RESP_FILL_LEVEL);

The macro MSGDMA_CSR_RESP_FILL_LEVEL_GET() exists but isn't used here. If the
upper 16 bits contain non-zero data, count will be massively inflated. Could
this cause the subsequent while (count--) loop to iterate excessively and
cause a CPU stall?

[ ... ]
> @@ -766,7 +766,7 @@ static void msgdma_dev_remove(struct msgdma_device *mdev)
>  		return;
>  
>  	devm_free_irq(mdev->dev, mdev->irq, mdev);
> -	tasklet_kill(&mdev->irq_tasklet);
> +	dmaengine_kill_bh(&mdev->dmachan);
>  	list_del(&mdev->dmachan.device_node);
>  }

[Severity: High]
This is a pre-existing issue, but I have a question about the cleanup path.

If dma_async_device_register() succeeds in msgdma_probe(), but
of_dma_controller_register() fails, the code jumps to the fail label and calls
msgdma_dev_remove().

However, msgdma_dev_remove() lacks a dma_async_device_unregister() call. Since
mdev was allocated with devm_kzalloc(), it is automatically freed on probe
failure. Does this leave the dmaengine core with dangling pointers and cause
a Use-After-Free?

[ ... ]
> @@ -852,7 +852,7 @@ static int msgdma_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	tasklet_setup(&mdev->irq_tasklet, msgdma_tasklet);
> +	dmaengine_init_bh(&mdev->dmachan, msgdma_tasklet);
>  
>  	dma_cookie_init(&mdev->dmachan);

[Severity: High]
This isn't a bug introduced by this patch, but I have a question about the
initialization order here.

Higher up in msgdma_probe(), devm_request_irq() is called before the BH worker,
spinlocks, and lists are initialized.

If an interrupt fires immediately upon registration, could msgdma_irq_handler()
attempt to lock the uninitialized spinlock, access uninitialized lists in
msgdma_start_transfer(), and schedule an uninitialized BH worker?

I also noticed several other pre-existing issues in this file:

[Severity: High]
This is a pre-existing issue, but I noticed something concerning in how batched
transfers are handled.

In msgdma_start_transfer():

	desc = list_first_entry_or_null(&mdev->pending_list,
					struct msgdma_sw_desc, node);
	if (!desc)
		return;

	list_splice_tail_init(&mdev->pending_list, &mdev->active_list);
	msgdma_copy_desc_to_fifo(mdev, desc);

When multiple transactions are queued, this takes the first descriptor but moves
the entire pending_list to active_list using list_splice_tail_init(). However,
it only copies the first descriptor to the hardware FIFO.

When this transfer completes, subsequent transactions remain trapped in
active_list because msgdma_start_transfer() only checks pending_list. Does this
result in dropped transfers when a client driver batches multiple requests?

[Severity: High]
This is a pre-existing issue, but the lock dropping pattern seems dangerous.

In msgdma_chan_desc_cleanup(), the code iterates over mdev->done_list using
list_for_each_entry_safe and temporarily drops mdev->lock:

		if (dmaengine_desc_callback_valid(&cb)) {
			spin_unlock_irqrestore(&mdev->lock, irqflags);
			dmaengine_desc_callback_invoke(&cb, NULL);
			spin_lock_irqsave(&mdev->lock, irqflags);
		}

If dma_release_channel() is called concurrently, msgdma_free_chan_resources()
can acquire the lock, reset all lists, and free the backing array with
kfree(mdev->sw_desq). When msgdma_chan_desc_cleanup() reacquires the lock, its
next pointer will reference freed memory. Can this race lead to a
Use-After-Free?

[Severity: High]
This isn't a bug introduced by this patch, but there seems to be an accounting
flaw for zero-length transfers.

In msgdma_prep_memcpy():

	desc_cnt = DIV_ROUND_UP(len, MSGDMA_MAX_TRANS_LEN);
	...
	mdev->desc_free_cnt -= desc_cnt;
	...
	do {
		/* Allocate and populate the descriptor */
		new = msgdma_get_descriptor(mdev);
		...
	} while (len);

If len is 0, desc_cnt is 0. The free count check passes, and desc_free_cnt is
decremented by 0.

However, the do-while loop executes exactly once, withdrawing a physical
descriptor from mdev->free_list via msgdma_get_descriptor(). If multiple
zero-length requests are submitted, won't this drain free_list entirely without
decrementing desc_free_cnt, eventually causing a NULL pointer dereference in
msgdma_get_descriptor()?

[Severity: High]
This is a pre-existing issue, but I am worried about the polling loop in
msgdma_copy_one():

	while (ioread32(mdev->csr + MSGDMA_CSR_STATUS) &
	       MSGDMA_CSR_STAT_DESC_BUF_FULL)
		mdelay(1);

Since this function is invoked under a spinlock with local interrupts disabled,
could a malfunctioning hardware device permanently stall the CPU core and trigger
the NMI watchdog?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=30