Re: [PATCH v2 42/64] dmaengine: jz4780: kill vchan BH on teardown

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

Pre-existing issues:
- [Critical] Use-after-free and arbitrary physical memory corruption during descriptor setup failure
- [High] Hardware teardown race in jz4780_dma_remove due to incorrect devres ordering
- [High] NULL Pointer Dereference in jz4780_dma_tx_status
--

--- Patch [42]: [PATCH v2 42/64] dmaengine: jz4780: kill vchan BH on teardown ---
commit 80e35ad454b351671c271c48790a44cd6f63d096
Author: Allen Pais <[email protected]>

dmaengine: jz4780: kill vchan BH on teardown

This commit updates virt-dma cleanup to use dma_chan_kill_bh() instead of
tasklet_kill().

> diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c
> index 6070dfdb7114c..45c1096563e32 100644
> --- a/drivers/dma/dma-jz4780.c
> +++ b/drivers/dma/dma-jz4780.c
> @@ -1019,7 +1019,7 @@ static void jz4780_dma_remove(struct platform_device *pdev)
>  	free_irq(jzdma->irq, jzdma);
>  
>  	for (i = 0; i < jzdma->soc_data->nb_channels; i++)
> -		tasklet_kill(&jzdma->chan[i].vchan.task);
> +		dma_chan_kill_bh(&jzdma->chan[i].vchan.chan);

[Severity: High]
This is a pre-existing issue, but does disabling the hardware clock here
before devres gets a chance to unregister the DMA device create a race
condition?

If an interrupt fires or a client accesses the DMA engine (for example,
calling dmaengine_terminate_all()) during or immediately after
jz4780_dma_remove(), it appears this would trigger a fatal MMIO fault
because the device clock is already disabled.

>  }
>  
>  static const struct jz4780_dma_soc_data jz4740_dma_soc_data = {

[Severity: High]
This is a pre-existing issue, but is there a risk of a NULL pointer
dereference in jz4780_dma_tx_status() when a transfer isn't active?

drivers/dma/dma-jz4780.c:jz4780_dma_tx_status() {
    ...
	} else if (cookie == jzchan->desc->vdesc.tx.cookie) {
    ...
}

If a client prepares and submits a descriptor, moving it to the desc_submitted
list, and then polls its status before it is issued, vchan_find_desc()
would return NULL. If no transfer is active, jzchan->desc is NULL, which
seems to lead to an unguarded dereference in this branch.


[Severity: Critical]
This is a pre-existing issue, but could the error paths during descriptor
setup lead to a use-after-free and potential physical memory corruption?

drivers/dma/dma-jz4780.c:jz4780_dma_prep_slave_sg() {
    ...
			jz4780_dma_desc_free(&jzchan->desc->vdesc);
			return NULL;
    ...
}

If jz4780_dma_setup_hwdesc() fails, the error path frees jzchan->desc.
However, jzchan->desc appears to hold the currently active, running
descriptor rather than the newly allocated one. Does this free the running
descriptor while the DMA controller is still executing it? This pattern also
appears to exist in jz4780_dma_prep_dma_cyclic().

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