Re: [PATCH v6 2/2] dmaengine: atcdmac300: Add driver for Andes ATCDMAC300 DMA controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
- [Critical] Double-callback and Use-After-Free race in cyclic transfer completion during channel termination.
- [High] Masking pending hardware interrupts with software state (`used_chan`) causes an unacknowledged IRQ storm.
- [High] The DMA device is exposed to clients before its IRQ handler and cache coherency (IOCP) are fully initialized.
- [High] Polling for up to 100ms with interrupts disabled in `atcdmac_terminate_all` causes soft lockups.
- [High] The channel-wide `cyclic` flag is sticky, and unconditional resume restarts cause arbitrary DMA execution or kernel panics.
- [High] Suspend/resume race with the threaded IRQ causes transfer desynchronization and hardware corruption.
- [High] dmaengine API violation in `atcdmac_terminate_all`: invoking completion callbacks for aborted transfers.
- [High] System suspend times out and fails if a slave DMA channel is actively waiting for peripheral data.
- [High] Sleep-in-atomic bug on PREEMPT_RT due to using `spinlock_t` in hardirq context.
- [High] Kernel panic due to list_del_init() on an uninitialized list_head in error paths.
- [Medium] Writing a blanket clear mask to `REG_INT_STA` clears unhandled interrupts, permanently losing them.
--

commit e06d7bd262ebaa03646e2e3ac2876918f25083d4
Author: CL Wang <cl634@andestech.com>

dmaengine: atcdmac300: Add driver for Andes ATCDMAC300 DMA controller

This patch adds support for the Andes ATCDMAC300 DMA controller.
The ATCDMAC300 is a memory-to-memory and peripheral DMA controller
that provides scatter-gather, cyclic, and slave transfer capabilities.

> diff --git a/drivers/dma/atcdmac300.c b/drivers/dma/atcdmac300.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/dma/atcdmac300.c
[ ... ]
> +static struct atcdmac_desc *atcdmac_alloc_desc(struct dma_chan *chan,
> +					       gfp_t gfp_flags)
> +{
> +	struct atcdmac_dmac *dmac = atcdmac_dev_to_dmac(chan->device);
> +	struct atcdmac_desc *desc;
> +	dma_addr_t phys;
> +
> +	desc = dma_pool_zalloc(dmac->dma_desc_pool, gfp_flags, &phys);
> +	if (desc) {
> +		INIT_LIST_HEAD(&desc->tx_list);
> +		dma_async_tx_descriptor_init(&desc->txd, chan);

[Severity: High]
Does this code leave desc_node uninitialized?

dma_pool_zalloc zeroes out the descriptor, leaving the pointers for desc_node
as NULL since INIT_LIST_HEAD is not called for it. If a subsequent descriptor
allocation fails (for instance, in atcdmac_prep_device_sg), the error path
calls atcdmac_put_desc on the first descriptor.

This descends into atcdmac_put_desc_nolock, which performs list_del_init on
desc_node, potentially causing a NULL pointer dereference on the
uninitialized list head.

[ ... ]
> +static void atcdmac_put_desc_nolock(struct atcdmac_chan *dmac_chan,
> +				    struct atcdmac_desc *desc)
> +{
[ ... ]
> +		list_del_init(&desc->desc_node);

[Severity: High]
Can this trigger a NULL pointer dereference as described above?

[ ... ]
> +static void atcdmac_start_next_trans(struct atcdmac_chan *dmac_chan)
> +{
> +	struct atcdmac_desc *next_tx = NULL;
> +	struct atcdmac_desc *dma_desc;
> +
> +	if (dmac_chan->cyclic) {
> +		/* Get the next DMA descriptor from tx_list. */
> +		dma_desc = atcdmac_get_active_head(dmac_chan);
> +		dma_desc->at = dma_desc->at->next;

[Severity: High]
Could this dereference an invalid pointer if the active_list is empty?

The channel-wide cyclic flag is sticky and is never cleared when a transfer
is terminated. If atcdmac_start_next_trans is called (for example, during
system resume) on a previously cyclic channel that is now idle, active_list
will be empty.

Using list_first_entry via atcdmac_get_active_head on an empty list returns
a bogus pointer, leading to a fault when dma_desc->at is accessed.

[ ... ]
> +static void atcdmac_advance_work(struct atcdmac_chan *dmac_chan)
> +{
[ ... ]
> +	if (dmac_chan->cyclic) {
> +		if (!stop)
> +			atcdmac_start_next_trans(dmac_chan);
> +
> +		spin_unlock_irqrestore(&dmac_chan->lock, flags);
> +		res.result = DMA_TRANS_NOERROR;
> +		dmaengine_desc_get_callback_invoke(&dma_desc->txd, &res);

[Severity: Critical]
Is there a use-after-free race here if atcdmac_terminate_all runs
concurrently?

dma_desc remains in the unlocked active_list while the channel lock is dropped
to invoke the client callback. If another thread calls atcdmac_terminate_all,
it will splice active_list, abort the channel, manually run the callback on
dma_desc again, and return it to the free_list.

Another CPU can then immediately reallocate the descriptor and overwrite the
callback pointers while the first CPU is still reading them to execute, which
could lead to arbitrary code execution.

[Severity: High]
Can this delayed threaded IRQ cause a double-start race with system resume?

If a transfer finishes right before suspend, the hardware interrupt fires
but the threaded IRQ handler may not execute before the system freezes. On
resume, stop_mask is cleared and the transfer is restarted. When normal
operations resume, this delayed threaded IRQ runs, finds stop cleared, and
erroneously double-starts the transfer while it is already actively running.

[ ... ]
> +static irqreturn_t atcdmac_interrupt(int irq, void *dev_id)
> +{
> +	struct atcdmac_dmac *dmac = dev_id;
> +	struct atcdmac_chan *dmac_chan;
> +	unsigned int status;
> +	unsigned int int_ch;
> +	int ret = IRQ_NONE;
> +	int i;
> +
> +	regmap_read(dmac->regmap, REG_INT_STA, &status);
> +	int_ch = READ_ONCE(dmac->used_chan) & DMA_INT_ALL(status);

[Severity: High]
Does masking hardware interrupts with the software used_chan state risk an
unacknowledged IRQ storm?

If an interrupt fires for a channel where the software used_chan bit is 0
(such as an abort interrupt triggered by atcdmac_terminate_all on an idle
channel), int_ch evaluates to 0.

The handler bypasses the clear routine and returns without acknowledging the
hardware interrupt, potentially causing an infinite loop of interrupts.

> +
> +	while (int_ch) {
> +		spin_lock(&dmac->lock);

[Severity: High]
Is it safe to use a standard spinlock_t in hardirq context here?

On PREEMPT_RT kernels, spinlock_t is implemented as an RT mutex which can
sleep. Locks acquired in genuine hardware interrupt context typically need
to be typed as raw_spinlock_t to avoid scheduling while atomic.

> +		dmac->used_chan = READ_ONCE(dmac->used_chan) & ~int_ch;
> +		spin_unlock(&dmac->lock);
> +		regmap_write(dmac->regmap, REG_INT_STA, DMA_INT_CLR(int_ch));

[Severity: Medium]
Does writing a blanket clear mask erase unhandled interrupts?

The DMA_INT_CLR macro writes 1s to the TC, ABT, and ERR bits for the given
channel, regardless of which interrupts were actually set in the status read.
If a new interrupt (like ERR) asserts after the status read but before this
clear write, it gets cleared but never handled.

[ ... ]
> +static int atcdmac_terminate_all(struct dma_chan *chan)
> +{
[ ... ]
> +	spin_lock_irqsave(&dmac_chan->lock, flags);
> +	atcdmac_abort_chan(dmac_chan);
> +	atcdmac_enable_chan(dmac_chan, 0);
> +	ret = regmap_read_poll_timeout_atomic(dmac_chan->dma_dev->regmap,
> +					      REG_CH_EN,
> +					      val,
> +					      !(val & BIT(dmac_chan->chan_id)),
> +					      10,
> +					      ATCDMAC_CHAN_TIMEOUT_US);

[Severity: High]
Can this cause a soft lockup by polling for up to 100ms with interrupts
disabled?

Calling regmap_read_poll_timeout_atomic with a 100,000us timeout while
holding a spinlock in hardirq-disabled context can trigger the NMI watchdog
and create extreme system latency spikes.

[ ... ]
> +	list_for_each_entry_safe(desc_cur, desc_next, &list, desc_node) {
> +		atcdmac_run_tx_complete_actions(desc_cur, DMA_TRANS_ABORTED);

[Severity: High]
Is it permissible to invoke completion callbacks for aborted transfers?

The dmaengine API explicitly prohibits executing callbacks for aborted
descriptors because clients (such as ALSA) typically call terminate_all with
their own stream locks held. Invoking the callback here can cause a deadlock
when the client attempts to acquire the exact same lock.

[ ... ]
> +static int atcdmac_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	ret = dma_async_device_register(&dmac->dma_device);
> +	if (ret)
> +		return ret;
> +
> +	ret = atcdmac_init_irq(pdev, dmac);
> +	if (ret)
> +		goto err_dma_async_register;
> +
> +	ret = atcdmac_init_iocp(pdev, dmac);

[Severity: High]
Are the DMA channels exposed to clients before the IRQ handler and cache
coherency (IOCP) are fully initialized?

dma_async_device_register makes the DMA channels immediately available to
kernel clients. If a client immediately requests a channel and starts a
transfer before atcdmac_init_irq and atcdmac_init_iocp execute, the transfer
will run without cache snooping configured, which could lead to memory
corruption.

[ ... ]
> +static int atcdmac_resume(struct device *dev)
> +{
[ ... ]
> +	list_for_each_entry_safe(chan,
> +				 chan_next,
> +				 &dmac->dma_device.channels,
> +				 device_node) {
> +		dmac_chan = atcdmac_chan_to_dmac_chan(chan);
> +		spin_lock_irqsave(&dmac_chan->lock, flags);
> +		atcdmac_start_next_trans(dmac_chan);

[Severity: High]
Could this improperly restart idle cyclic channels?

This unconditionally calls atcdmac_start_next_trans for every channel
regardless of its active software state. If a channel was previously cyclic
but is currently idle, it will cause a fault as described earlier when
dereferencing the empty active_list.

[ ... ]
> +static int atcdmac_suspend(struct device *dev)
> +{
> +	struct atcdmac_dmac *dmac = dev_get_drvdata(dev);
> +	int ret;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&dmac->lock, flags);
> +	dmac->stop_mask = BIT(dmac->num_ch) - 1;
> +	spin_unlock_irqrestore(&dmac->lock, flags);
> +	ret = atcdmac_wait_chan_idle(dmac,
> +				     dmac->stop_mask,
> +				     ATCDMAC_CHAN_TIMEOUT_US * dmac->num_ch);

[Severity: High]
Will system suspend time out and fail if a slave DMA channel is actively
waiting for peripheral data?

The suspend sequence sets a stop_mask that prevents software from chaining the
next descriptor, but does not pause actively running transfers. It then blocks
waiting for all channels to report idle.

If a DEV_TO_MEM slave channel is waiting for hardware flow control or
incoming data, it will never complete. The polling loop will eventually time
out, aborting the entire system sleep transition.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911072524.1297406-1-cl634@andestech.com?part=2