Re: [PATCH v2 32/64] dmaengine: k3-udma: use channel BH for vchan completions

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Dangling `vc->cyclic` pointer causes a Use-After-Free in `udma_vchan_complete` when a channel running a cyclic transfer is terminated.
--

--- Patch [32]: [PATCH v2 32/64] dmaengine: k3-udma: use channel BH for vchan completions ---
commit 8a0a241d872268ead0d3f5e593bb261aae3ac2ff
Author: Allen Pais <allen.lkml@gmail.com>

dmaengine: k3-udma: use channel BH for vchan completions
    
This patch converts the custom vchan completion handling in the k3-udma
driver from a tasklet to a per-channel bottom-half mechanism. It updates
the initialization and teardown paths to use the new dma_chan_kill_bh() API.

> diff --git a/drivers/dma/ti/k3-udma.c b/drivers/dma/ti/k3-udma.c
> index 1cf158eb7bdb5..12b8a9caf4d2b 100644
> --- a/drivers/dma/ti/k3-udma.c
> +++ b/drivers/dma/ti/k3-udma.c

[ ... ]

> @@ -4042,12 +4042,12 @@ static void udma_desc_pre_callback(struct virt_dma_chan *vc,
>  }
>  
>  /*
> - * This tasklet handles the completion of a DMA descriptor by
> + * This BH handles the completion of a DMA descriptor by
>   * calling its callback and freeing it.
>   */
> -static void udma_vchan_complete(struct tasklet_struct *t)
> +static void udma_vchan_complete(struct dma_chan *chan)
>  {
> -	struct virt_dma_chan *vc = from_tasklet(vc, t, task);
> +	struct virt_dma_chan *vc = to_virt_chan(chan);

[Severity: High]
This is a pre-existing issue, but could this bottom half handler trigger a
use-after-free of the cyclic descriptor?

Looking at the teardown path, if a channel is actively running a cyclic
transfer, udma_terminate_all() moves uc->desc to uc->terminated_desc but
crucially leaves vc->cyclic pointing to the active descriptor.

When the hardware sends a teardown completion message, the IRQ handler
udma_ring_irq_handler() frees the active descriptor via
udma_desc_free(&uc->terminated_desc->vd). At this point, vc->cyclic becomes
a dangling pointer.

The driver then calls udma_synchronize(), which calls vchan_synchronize().
This flushes the bottom half by calling dma_chan_kill_bh(), which happens
before vc->cyclic is checked and cleared:

drivers/dma/virt-dma.c:vchan_synchronize() {
    dma_chan_kill_bh(&vc->chan);

    spin_lock_irqsave(&vc->lock, flags);
    if (vc->cyclic) {
        vchan_terminate_vdesc(vc->cyclic);
        vc->cyclic = NULL;
    }
}

When the bottom half executes, it reads the dangling vc->cyclic pointer and
dereferences it:

    vd = vc->cyclic;
    if (vd) {
        vc->cyclic = NULL;
        dmaengine_desc_get_callback(&vd->tx, &cb);

Does this sequence cause a use-after-free when dmaengine_desc_get_callback()
accesses the freed descriptor?

>  	struct virt_dma_desc *vd, *_vd;
>  	struct dmaengine_desc_callback cb;
>  	LIST_HEAD(head);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1785183549.git.allen.lkml@gmail.com?part=32