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

[Frank Li <Frank.li@oss.nxp.com>]

On Wed, Sep 16, 2026 at 02:01:19PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
> - [High] dmaengine: atcdmac300: UAF on driver unbind due to devres lifecycle mismatch
> - [High] dmaengine: atcdmac300: permanent controller halt if suspend times out
> - [High] dmaengine: atcdmac300: NULL pointer dereference mixing cyclic and non-cyclic transfers
> - [High] dmaengine: atcdmac300: cross-transfer corruption on simultaneous TC and ERR interrupts
> - [Medium] dmaengine: atcdmac300: memory leak of unsubmitted descriptors
> - [Medium] dmaengine: atcdmac300: silent truncation of DT req_num exceeding 15
> - [Medium] dmaengine: atcdmac300: transfers queued during suspend are stranded on resume
> - [Low] dmaengine: atcdmac300: missing endianness conversions for DMA descriptors
> --

Please fix these problem or provide explain.

Frank

>
> commit 94fd62b6d9e6afc2cf50f8dff89eedcdc57f24c9
> Author: CL Wang <cl634@andestech.com>
> Subject: dmaengine: atcdmac300: Add driver for Andes ATCDMAC300 DMA controller
>
> This patch adds support for the Andes ATCDMAC300 DMA controller. It provides
> scatter-gather, cyclic, and slave transfer capabilities.
>
> > diff --git a/drivers/dma/atcdmac300.c b/drivers/dma/atcdmac300.c
> > index 0000000000000..e0a8a980e2fe8
> > --- /dev/null
> > +++ b/drivers/dma/atcdmac300.c
> [ ... ]
> > @@ -82,9 +82,6 @@ static struct atcdmac_desc *atcdmac_alloc_desc(struct dma_chan *chan,
> >  		INIT_LIST_HEAD(&desc->desc_node);
> >  		INIT_LIST_HEAD(&desc->tx_list);
> >  		dma_async_tx_descriptor_init(&desc->txd, chan);
> >  		desc->txd.flags = DMA_CTRL_ACK;
> >  		desc->txd.tx_submit = atcdmac_tx_submit;
> >  		desc->txd.phys = phys;
> >  	}
>
> [Severity: Medium]
> The desc_free callback is omitted from the transaction descriptor here.
> If a client prepares a descriptor but aborts and frees it before
> submission, the DMA engine core won't be able to free it.
>
> [ ... ]
> > @@ -494,11 +494,11 @@ static irqreturn_t atcdmac_irq_thread(int irq, void *dev_id)
> >  	for (i = 0; i < dmac->num_ch; i++) {
> >  		dmac_chan = &dmac->chan[i];
> >
> >  		if (test_and_clear_bit(ATCDMAC_STA_TC, &dmac_chan->status)) {
> >  			atcdmac_advance_work(dmac_chan);
> >  			handled = true;
> >  		}
> >
> >  		if (test_and_clear_bit(ATCDMAC_STA_ERR, &dmac_chan->status)) {
> >  			atcdmac_handle_error(dmac_chan);
> >  			handled = true;
> >  		}
>
> [Severity: High]
> The lack of mutual exclusivity allows both conditions to execute if both
> status bits are asserted simultaneously by the hardware. In this case,
> atcdmac_advance_work() would complete the current descriptor and start the
> next one, followed immediately by atcdmac_handle_error() wrongly aborting
> the newly started, innocent descriptor. Should these checks be mutually
> exclusive?
>
> [ ... ]
> > @@ -911,10 +911,8 @@ atcdmac_prep_dma_cyclic(struct dma_chan *chan,
> >  		if (!desc)
> >  			goto err_desc_get;
> >  		atcdmac_chain_desc(&first, &prev, desc, true);
> >  	}
> >
> >  	first->txd.flags = flags;
> >  	dmac_chan->cyclic = true;
>
> [Severity: High]
> This modifies the channel-wide state during descriptor preparation.
> If a client prepares a cyclic transfer, does not submit it, and
> subsequently prepares and submits a non-cyclic transfer, this global
> flag remains set. When the non-cyclic transfer completes, could this
> cause atcdmac_advance_work() to treat it as cyclic and attempt to
> dereference dma_desc->at->next in atcdmac_start_next_trans(), leading
> to a kernel panic?
>
> [ ... ]
> > @@ -1144,10 +1144,8 @@ static void atcdmac_free_chan_resources(struct dma_chan *chan)
> >  	spin_lock_irqsave(&dmac_chan->lock, flags);
> >  	list_for_each_entry_safe(desc,
> >  				 desc_next,
> >  				 &dmac_chan->free_list,
> >  				 desc_node) {
> >  		list_del(&desc->desc_node);
> >  		dma_pool_free(dmac->dma_desc_pool, desc, desc->txd.phys);
> >  	}
>
> [Severity: Medium]
> This loop only frees descriptors currently sitting in the free_list.
> Combined with the missing desc_free callback above, does this mean any
> prepared but unsubmitted descriptors are permanently leaked and never
> returned to the dma_desc_pool?
>
> [ ... ]
> > @@ -1189,8 +1189,8 @@ static struct dma_chan *atcdmac_dma_xlate_handler(struct of_phandle_args *dmac,
> >  	if (!chan)
> >  		return NULL;
> >
> >  	dmac_chan = atcdmac_chan_to_dmac_chan(chan);
> >  	dmac_chan->dev_chan = true;
> >  	dmac_chan->req_num = dmac->args[0] & 0xff;
>
> [Severity: Medium]
> The request number is extracted and masked to 8 bits. However, the hardware
> request selection registers (SRC_REQ_SEL_MASK, DST_REQ_SEL_MASK) are only
> 4 bits wide. Since FIELD_PREP silently truncates values, a device tree
> req_num of 16 will be programmed into hardware as 0 without warning.
> Should this value be explicitly validated against the 4-bit maximum?
>
> [ ... ]
> > @@ -1305,9 +1305,9 @@ static int atcdmac_init_ioremap_and_regmap(struct platform_device *pdev,
> >  	regmap_read(regmap, REG_CFG, &val);
> >  	num_ch = val & CH_NUM;
> >  	size = sizeof(*dmac) + num_ch * sizeof(struct atcdmac_chan);
> >  	dmac = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
> >  	if (!dmac)
> >  		return -ENOMEM;
>
> [Severity: High]
> The DMA controller structure is allocated using devres, tying its
> lifetime to the device binding rather than the dma_device refcount.
> If a client driver still holds a channel reference when the device is
> unbound, could this lead to a use-after-free when the client later
> releases the channel and atcdmac_free_chan_resources() accesses the
> freed dmac structure?
>
> [ ... ]
> > @@ -1543,9 +1543,8 @@ 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);
> >  		if (!list_empty(&dmac_chan->active_list))
> >  			atcdmac_start_next_trans(dmac_chan);
>
> [Severity: Medium]
> This condition actively prevents queue_list items from being serviced if
> the active_list is empty during resume. Since transfers submitted during
> suspend are placed into queue_list (because stop_mask prevents them from
> starting), won't they remain stranded indefinitely unless issue_pending
> is also called here?
>
> [ ... ]
> > @@ -1568,9 +1568,9 @@ static int atcdmac_suspend(struct device *dev)
> >  	synchronize_irq(dmac->irq);
> >
> >  	ret = atcdmac_wait_chan_idle(dmac,
> >  				     chan_mask,
> >  				     ATCDMAC_CHAN_TIMEOUT_US * dmac->num_ch);
> >
> >  	return ret;
> >  }
>
> [Severity: High]
> If atcdmac_wait_chan_idle() times out, the function returns the error code
> without reverting dmac->stop_mask back to 0. Since the PM core would abort
> the system suspend and resume normal operations, wouldn't this permanently
> prevent any new transfers from starting?
>
> > diff --git a/drivers/dma/atcdmac300.h b/drivers/dma/atcdmac300.h
> > index 0000000000000..3d2d07effc26d
> > --- /dev/null
> > +++ b/drivers/dma/atcdmac300.h
> [ ... ]
> > @@ -148,10 +148,10 @@ enum dma_sta {
> >   * @ll_ptr_hi: Linked List Pointer Register (high 32-bit).
> >   */
> >  struct atcdmac_regs {
> >  	unsigned int ctrl;
> >  	unsigned int trans_size;
>
> [Severity: Low]
> The hardware descriptor layout uses native unsigned int instead of
> explicitly sized __le32 types. When these fields are populated in
> atcdmac_build_desc(), values are assigned directly without cpu_to_le32().
> Does this lack of endianness conversion break portability if this driver
> is ever used on a big-endian system?
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260916134258.2178081-1-cl634@andestech.com?part=2