Re: [PATCH v3 11/34] dmaengine: mmp_tdma: hook completions to dmaengine BH

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

Pre-existing issues:
- [High] Missing explicit teardown of dmaengine BH work, leading to a potential Use-After-Free if work is pending when the driver is removed or the channel is freed.
- [High] The driver requests a shared interrupt before initializing the channel data structures, which can crash the kernel if the interrupt fires immediately.
--

--- Patch [11]: [PATCH v3 11/34] dmaengine: mmp_tdma: hook completions to dmaengine BH ---
commit fb6d690923127c3cb64e8e8d24e39f8004e89713
Author: Allen Pais <[email protected]>

dmaengine: mmp_tdma: hook completions to dmaengine BH

This commit replaces the per-channel tasklet with the shared dmaengine
BH helper for completions. The handler is updated to schedule the BH
work while teardown and common scheduling are managed by dmaengine.

> diff --git a/drivers/dma/mmp_tdma.c b/drivers/dma/mmp_tdma.c
> index ba03321eeff79..d20aaa7567a6d 100644
> --- a/drivers/dma/mmp_tdma.c
> +++ b/drivers/dma/mmp_tdma.c
> @@ -320,7 +319,7 @@ static irqreturn_t mmp_tdma_chan_handler(int irq, void *dev_id)
>  	struct mmp_tdma_chan *tdmac = dev_id;
>  
>  	if (mmp_tdma_clear_chan_irq(tdmac) == 0) {

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for tdmac
to be NULL here if the interrupt fires early?

Looking at mmp_tdma_probe(), devm_request_irq() is called with IRQF_SHARED
before mmp_tdma_chan_init() populates the tdev->tdmac array elements.

If another device on the shared interrupt line triggers an interrupt right
after it is requested, mmp_tdma_int_handler() will iterate over the
uninitialized tdev->tdmac array and pass a NULL tdmac pointer to
mmp_tdma_chan_handler().

When mmp_tdma_clear_chan_irq() is called:

mmp_tdma_clear_chan_irq()
    u32 reg = readl(tdmac->reg_base + TDISR);

Could this unconditionally dereference the NULL tdmac and cause a panic?

> -		tasklet_schedule(&tdmac->tasklet);
> +		dmaengine_schedule_bh(&tdmac->chan);
>  		return IRQ_HANDLED;
>  	} else

[ ... ]

> @@ -583,7 +582,7 @@ static int mmp_tdma_chan_init(struct mmp_tdma_device *tdev,
>  	tdmac->pool	   = pool;
>  	tdmac->status = DMA_COMPLETE;
>  	tdev->tdmac[tdmac->idx] = tdmac;
> -	tasklet_setup(&tdmac->tasklet, dma_do_tasklet);
> +	dmaengine_init_bh(&tdmac->chan, dma_do_tasklet);

[Severity: High]
This is a pre-existing issue, but does there need to be a corresponding
call to dmaengine_kill_bh() during teardown?

If the driver is removed or the channel is freed while DMA is active or BH
work is scheduled, it appears the work could execute after the channel
structures are freed:

mmp_tdma_remove()
    of_dma_controller_free(pdev->dev.of_node);

Since the tdmac channel structure is allocated via devm_kzalloc(), without
explicitly flushing the pending work in mmp_tdma_remove() or
mmp_tdma_free_chan_resources(), could this lead to a use-after-free when
the pending work later dereferences the freed tdmac?

>  
>  	/* add the channel to tdma_chan list */
>  	list_add_tail(&tdmac->chan.device_node,
>

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