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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [High] Unmanaged `cyclic` flag leads to NULL pointer dereferences, infinite loops, and resume panics.
- [High] Interrupt handler masks hardware interrupt status with `used_chan`, leaving unmasked interrupts unacknowledged.
- [High] Premature execution of unissued DMA descriptors violates the DMA engine API.
- [High] Missing `device_synchronize` callback prevents safe client synchronization with the threaded IRQ handler.
- [High] Fetching the client callback outside the lock in `atcdmac_advance_work` creates a Use-After-Free race with channel termination.
- [High] Reusing descriptors from `free_list` does not clear previously set callbacks and parameters.
- [Medium] `atcdmac_set_device_config` strictly validates unused configuration fields, breaking unidirectional slave configurations.
- [Medium] Asymmetric registration and teardown order creates Use-After-Free vulnerabilities and Race Conditions.
- [Medium] Prepared but unsubmitted DMA descriptors are permanently leaked due to missing `desc_free` callback.
- [Medium] Truncation of `size_t len` to `unsigned int trans_size` could lead to incomplete DMA transfers on 64-bit platforms.
--

commit 892196b95d4c6cbcb6477d01fb69f6b5e13d4612
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.
It provides memory-to-memory and peripheral DMA controller
support with 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
> @@ -0,0 +1,1518 @@
[ ... ]
> +static struct atcdmac_desc *atcdmac_alloc_desc(struct dma_chan *chan,
> +					       gfp_t gfp_flags)
> +{
[ ... ]
> +	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);
> +		desc->txd.flags = DMA_CTRL_ACK;

[Severity: Medium]
Is there a missing assignment for the desc_free callback here?

When a client prepares a transaction, descriptors are removed from
dmac_chan->free_list. If the client aborts and calls dmaengine_desc_free(),
the dmaengine core attempts to invoke desc->txd.desc_free. Because
atcdmac_alloc_desc() never assigns this callback, the core drops the
descriptor.

Since atcdmac_free_chan_resources() only reclaims descriptors currently
present in free_list, any orphaned, unsubmitted descriptors seem permanently
leaked from dmac->dma_desc_pool. Can we add the appropriate callback to
prevent pool exhaustion?

> +		desc->txd.tx_submit = atcdmac_tx_submit;
> +		desc->txd.phys = phys;
> +	}
> +
> +	return desc;
> +}
[ ... ]
> +static struct atcdmac_desc *atcdmac_get_desc(struct atcdmac_chan *dmac_chan)
> +{
[ ... ]
> +	spin_lock_irqsave(&dmac_chan->lock, flags);
> +	list_for_each_entry_safe(desc, desc_next,
> +				 &dmac_chan->free_list,
> +				 desc_node) {
> +		if (async_tx_test_ack(&desc->txd)) {
> +			list_del_init(&desc->desc_node);
> +			ret = desc;
> +			break;
> +		}

[Severity: High]
Should the descriptor callback and parameter fields be reset before reusing
it from the free_list?

When atcdmac_get_desc() retrieves a descriptor for a new transfer, it does
not appear to zero out desc->txd.callback and desc->txd.callback_param. If a
client previously used this descriptor with a callback, freed its context,
and reuses it without setting a new callback, the DMA engine might
inadvertently execute the stale callback.

Could this lead to use-after-free issues if the context is already freed?

> +	}
> +	spin_unlock_irqrestore(&dmac_chan->lock, flags);
[ ... ]
> +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]
Does this sequence safely handle channels that have transitioned from cyclic
to non-cyclic transfers, or handle resume states properly?

The driver sets dmac_chan->cyclic to true in atcdmac_prep_dma_cyclic(), but
this flag is never reset to false during atcdmac_terminate_all() or when
preparing subsequent non-cyclic transfers like memcpy.

If a channel is used for a cyclic transfer, terminated, and then reused for a
non-cyclic transfer, the stale cyclic state causes atcdmac_advance_work() to
enter the cyclic path here. For memcpy descriptors, this leads to a null
pointer dereference on dma_desc->at->next.

Additionally, during system resume, atcdmac_resume() unconditionally calls
atcdmac_start_next_trans() on all channels. For a terminated cyclic channel,
active_list is empty, causing atcdmac_get_active_head() to return an invalid
pointer and crashing the kernel upon dereference.

> +		if ((uintptr_t)dma_desc->at == (uintptr_t)&dma_desc->tx_list)
> +			next_tx = list_entry(dma_desc->at,
> +					     struct atcdmac_desc,
> +					     tx_list);
> +		else
> +			next_tx = list_entry(dma_desc->at,
> +					     struct atcdmac_desc,
> +					     desc_node);
> +	} else {
> +		if (list_empty(&dmac_chan->active_list)) {
> +			if (!list_empty(&dmac_chan->queue_list)) {
> +				list_splice_init(&dmac_chan->queue_list,
> +						 &dmac_chan->active_list);

[Severity: High]
Is it safe to splice the entire queue_list into active_list without checking
if the client has explicitly issued the pending descriptors?

The DMA engine API contract requires that descriptors submitted via tx_submit
remain pending and are not started until the client explicitly calls
dma_async_issue_pending().

However, atcdmac_advance_work() unconditionally splices unissued descriptors
from queue_list into active_list when a prior transaction completes.
Similarly, atcdmac_resume() pulls unissued descriptors from queue_list. If a
client submits descriptors but intentionally delays issuing them (e.g. waiting
for a peripheral), the driver will incorrectly execute them prematurely.

> +				next_tx = atcdmac_get_active_head(dmac_chan);
> +			}
[ ... ]
> +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: High]
Does fetching the client callback outside the channel lock create a
use-after-free race condition?

For cyclic transfers, dma_desc remains on active_list. The lock is dropped
via spin_unlock_irqrestore() before accessing dma_desc->txd to invoke the
callback.

While the lock is dropped, a concurrent dmaengine_terminate_all() can acquire
the lock, splice the active_list away, and return dma_desc to the free_list.
The client might then release the channel, freeing the descriptor back to the
pool. The IRQ thread will then use the freed descriptor to invoke the
callback. Should the callback information be extracted while the lock is
still held?

> +	} else {
[ ... ]
> +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);
> +
> +	while (int_ch) {

[Severity: High]
Can this loop leave hardware interrupts unacknowledged and cause an interrupt
storm?

The handler masks the hardware interrupt status with the software-tracked
used_chan mask. It only clears the interrupts identified by int_ch. If the
hardware asserts an interrupt for a channel not currently in used_chan
(for instance, an ABT interrupt generated by atcdmac_terminate_all() on a
previously idle channel, or a spurious interrupt), int_ch evaluates to 0.

Because the while loop is skipped, the handler will not clear REG_INT_STA
and returns IRQ_NONE. Since the interrupt is level-triggered, the uncleared
interrupt will immediately re-fire, causing an infinite loop until the
kernel disables the IRQ line.

> +		spin_lock(&dmac->lock);
> +		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));
[ ... ]
> +static struct dma_async_tx_descriptor *
> +atcdmac_prep_dma_memcpy(struct dma_chan *chan,
> +			dma_addr_t dst,
> +			dma_addr_t src,
> +			size_t len,
> +			unsigned long flags)
> +{
[ ... ]
> +	ctrl = SRC_BURST_SIZE(src_max_burst) |
> +	       SRC_ADDR_MODE_INCR |
> +	       DST_ADDR_MODE_INCR |
> +	       DST_WIDTH(dst_width) |
> +	       SRC_WIDTH(src_width);
> +
> +	desc = atcdmac_build_desc(dmac_chan, src, dst, ctrl,
> +				  len >> src_width, 1);

[Severity: Medium]
Could the truncation of size_t len lead to incomplete DMA transfers on 64-bit
platforms?

Here, the 64-bit size_t len is shifted and passed as an unsigned int into
atcdmac_build_desc(). If a user requests a transfer length such that
len >> src_width exceeds 32 bits, the value will be silently truncated. The
DMA hardware would perform a shorter transfer but report success, leading to
silent data corruption. Is there a way to safely bound or reject overly large
sizes?

> +	if (!desc)
> +		goto err_desc_get;
[ ... ]
> +static int atcdmac_set_device_config(struct dma_chan *chan,
> +				     struct dma_slave_config *sconfig)
> +{
> +	struct atcdmac_chan *dmac_chan = atcdmac_chan_to_dmac_chan(chan);
> +
> +	/* Check if this chan is configured for device transfers */
> +	if (!dmac_chan->dev_chan)
> +		return -EINVAL;
> +
> +	/* Must be powers of two according to ATCDMAC300 spec */
> +	if (!is_power_of_2(sconfig->src_maxburst) ||
> +	    !is_power_of_2(sconfig->dst_maxburst) ||
> +	    !is_power_of_2(sconfig->src_addr_width) ||
> +	    !is_power_of_2(sconfig->dst_addr_width))
> +		return -EINVAL;

[Severity: Medium]
Does this check incorrectly reject valid, standard unidirectional
configurations?

Standard DMA engine consumers populate only the dma_slave_config fields
corresponding to their active transfer direction, leaving fields for the
unused direction as 0.

Because is_power_of_2(0) returns false, this strict global validation rejects
perfectly valid setups with -EINVAL, forcing clients to provide dummy
parameters for unused directions. Could the validation be limited to the
active transfer direction?

> +
> +	memcpy(&dmac_chan->dma_sconfig, sconfig, sizeof(*sconfig));
[ ... ]
> +static void atcdmac_init_dma_device(struct platform_device *pdev,
> +				    struct atcdmac_dmac *dmac)
> +{
> +	struct dma_device *device = &dmac->dma_device;
> +
> +	device->device_alloc_chan_resources = atcdmac_alloc_chan_resources;
> +	device->device_free_chan_resources = atcdmac_free_chan_resources;
> +	device->device_tx_status = atcdmac_get_tx_status;
> +	device->device_issue_pending = atcdmac_issue_pending;
> +	device->device_prep_dma_memcpy = atcdmac_prep_dma_memcpy;
> +	device->device_prep_slave_sg = atcdmac_prep_device_sg;
> +	device->device_config = atcdmac_set_device_config;
> +	device->device_terminate_all = atcdmac_terminate_all;
> +	device->device_prep_dma_cyclic = atcdmac_prep_dma_cyclic;

[Severity: High]
Is the device_synchronize callback missing from this list?

The driver processes completions and invokes client callbacks in a threaded
IRQ handler (atcdmac_advance_work). Clients rely on
dmaengine_terminate_sync() to ensure all transfers are aborted and no
callbacks are executing before freeing resources.

Without a device_synchronize callback, dmaengine_terminate_sync() fails to
wait for the threaded IRQ. If a client unloads immediately after calling it,
the IRQ thread may still be executing the client's callback, triggering a
use-after-free or executing unloaded module code.

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

[Severity: Medium]
Does registering the device before fully initializing it expose the driver
to use-after-free or race conditions?

Here, dma_async_device_register() is called before atcdmac_init_irq() and
atcdmac_init_iocp(). Once registered, the device is globally visible and
clients can acquire channels and submit transfers before the IRQ handler or
cache consistency parameters are fully set up.

If the subsequent initialization steps fail, dmac is freed while a client may
be holding a channel reference. Symmetrically, in atcdmac_remove(), the
driver resets the hardware before calling dma_async_device_unregister(),
meaning active clients may submit new transfers that the now-reset hardware
will attempt to execute. Should registration happen at the very end of probe?

> +	if (ret)
> +		goto err_dma_async_register;
> +
> +	ret = atcdmac_init_iocp(pdev, dmac);

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