Re: [PATCH v2 25/64] dmaengine: mv_xor_v2: use channel BH helpers

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incorrect teardown order in remove() leading to Use-After-Free.
- [High] Missing dma_chan_kill_bh() in probe error path leads to Use-After-Free.
- [High] Early interrupt request in probe allows uninitialized state access.
--

--- Patch [25]: [PATCH v2 25/64] dmaengine: mv_xor_v2: use channel BH helpers ---
commit 1d8bd96829601184f429fc1fa7947ce7a05990e9
Author: Allen Pais <allen.lkml@gmail.com>

dmaengine: mv_xor_v2: use channel BH helpers

This patch replaces the device tasklet with the per-channel BH helper.
Descriptor callbacks are updated to run through dma_chan_*_bh instead
of the tasklet API.

> diff --git a/drivers/dma/mv_xor_v2.c b/drivers/dma/mv_xor_v2.c
> index cad4d4fb51ac3..e5fa7a221d5b0 100644
> --- a/drivers/dma/mv_xor_v2.c
> +++ b/drivers/dma/mv_xor_v2.c
[ ... ]
> @@ -760,7 +758,7 @@ static int mv_xor_v2_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto free_msi_irqs;
>  
> -	tasklet_setup(&xor_dev->irq_tasklet, mv_xor_v2_tasklet);
> +	dma_chan_init_bh(&xor_dev->dmachan, mv_xor_v2_tasklet);

[Severity: High]
This is a pre-existing issue, but does requesting the interrupt before
initializing the bottom-half allow an early interrupt to crash the system?

In mv_xor_v2_probe(), devm_request_irq() requests and enables the interrupt
before dma_chan_init_bh() and dma_alloc_coherent() are called. If the
hardware is left in an active state with pending events, an interrupt will
fire immediately.

The handler mv_xor_v2_interrupt_handler() calls dma_chan_schedule_bh().
Depending on the timing, this could crash trying to schedule an uninitialized
work struct, or the scheduled work could run and dereference a NULL
hw_desq_virt pointer.

[Severity: High]
This is a pre-existing issue, but is a call to dma_chan_kill_bh() missing
in the probe error paths?

If a setup step fails after initialization, such as dma_alloc_coherent()
failing, the probe function jumps to the error labels and returns without
calling dma_chan_kill_bh().

If an early interrupt had already fired and scheduled the bottom-half, the
work item remains active in the workqueue. When devres automatically frees
the xor_dev structure containing the work item, could the executing workqueue
corrupt memory?

>  
>  	xor_dev->desc_size = mv_xor_v2_set_desc_size(xor_dev);
[ ... ]
> @@ -869,7 +867,7 @@ static void mv_xor_v2_remove(struct platform_device *pdev)
>  
>  	platform_device_msi_free_irqs_all(&pdev->dev);
>  
> -	tasklet_kill(&xor_dev->irq_tasklet);
> +	dma_chan_kill_bh(&xor_dev->dmachan);
>  }

[Severity: High]
This is a pre-existing issue, but is the teardown order in mv_xor_v2_remove()
safe from use-after-free races?

In mv_xor_v2_remove(), dma_free_coherent() is called to free the hardware
descriptor ring (hw_desq_virt) before the device interrupt is disabled via
devm_free_irq() and before the bottom-half is killed via dma_chan_kill_bh().

If a pending hardware interrupt fires during this window, or if a previously
scheduled bottom-half is running concurrently, the executing bottom-half
could dereference the freed hw_desq_virt memory:

mv_xor_v2_tasklet() {
    ...
		struct mv_xor_v2_descriptor *next_pending_hw_desc =
			xor_dev->hw_desq_virt + pending_ptr;
    ...
}

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