Re: [PATCH v2 2/2] dmaengine: ma35d1-pdma: Add MA35D1 PDMA driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] dmaengine: ma35d1-pdma: cyclic abort UAF in
  ma35d1_pdma_interrupt()
--

commit 6cfee31e98b745df1c08664e60fae82420194681
Author: Shan-Chun Hung <shanchun1218@gmail.com>

dmaengine: ma35d1-pdma: Add MA35D1 PDMA driver

This commit adds a DMA engine driver for the PDMA controller in the Nuvoton
MA35D1 SoC, supporting memory-to-memory, slave scatter-gather, and cyclic
transfers across 10 channels.

> diff --git a/drivers/dma/ma35d1-pdma.c b/drivers/dma/ma35d1-pdma.c
> new file mode 100644
> index 0000000000000..fef78e6d10d78
> --- /dev/null
> +++ b/drivers/dma/ma35d1-pdma.c
[ ... ]
> +static void ma35d1_pdma_complete(struct ma35d1_chan *ch)
> +{
> +	if (!ch->desc)
> +		return;
> +
> +	vchan_cookie_complete(&ch->desc->vd);
> +	ma35d1_pdma_start_desc(&ch->vc.chan);
> +}
> +
> +static irqreturn_t ma35d1_pdma_interrupt(int irq, void *devid)
> +{
> +	int i;
> +	bool handled = false;
> +	struct ma35d1_pdmadev *pdmadev = devid;
> +	struct ma35d1_chan *ch;
> +
> +	for (i = 0; i < PDMA_MAX_CHANS; i++) {
> +		unsigned long flags;
> +		bool done, abort;
> +
> +		ch = &pdmadev->channels[i];
> +
> +		spin_lock_irqsave(&ch->vc.lock, flags);
> +
> +		done = readl(ch->base + PDMA_TDSTS) & BIT(i);
> +		abort = readl(ch->base + PDMA_ABTSTS) & BIT(i);
> +
> +		if (done || abort) {
> +			handled = true;
> +
> +			/* Ack both flags first so a new descriptor is not completed twice. */
> +			writel(BIT(i), ch->base + PDMA_TDSTS);
> +			writel(BIT(i), ch->base + PDMA_ABTSTS);
> +
> +			if (ch->desc) {
> +				if (abort) {
> +					ch->desc->vd.tx_result.result = DMA_TRANS_ABORTED;
> +					ma35d1_pdma_complete(ch);
> +				} else if (ch->desc->cyclic) {
> +					vchan_cyclic_callback(&ch->desc->vd);
> +				} else {
> +					ma35d1_pdma_complete(ch);
> +				}
> +			}
> +		}

[Severity: High]
Can calling ma35d1_pdma_complete() on an aborted cyclic descriptor cause a
use-after-free in vchan_complete()?

When a cyclic period completes, vchan_cyclic_callback() sets ch->vc.cyclic
to &ch->desc->vd.  If PDMA_ABTSTS then fires on that channel,
ma35d1_pdma_complete() calls vchan_cookie_complete(), which moves the
descriptor onto ch->vc.desc_completed without clearing ch->vc.cyclic, and
ma35d1_pdma_start_desc() clears ch->desc to NULL.

When vchan_complete() runs, it invokes the callback twice (once via
ch->vc.cyclic and once via desc_completed) and frees the cyclic descriptor
while ch->vc.cyclic still points to it.  If ma35d1_terminate_all() runs
before the tasklet, it sees ch->desc == NULL, skips vchan_terminate_vdesc(),
and frees the descriptor from desc_completed, leaving ch->vc.cyclic dangling
for vchan_complete() to dereference.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109052900.2960473-1-shanchun1218@gmail.com?part=2