Re: [PATCH 2/3] dma: mv_xor: add missing platform remove function

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing tasklet_kill() leads to a Use-After-Free during driver unbind.
- [High] Object lifetime mismatch between devres and the dmaengine framework causes use-after-free on driver unbind.
- [High] The driver unmaps DMA buffers and disables clocks during removal without explicitly stopping the DMA hardware.
- [High] A crafted or unexpectedly large Device Tree causes a heap buffer overflow in mv_xor_probe when xordev->xor_type == XOR_ARMADA_37XX.
- [High] Unsafe DMA buffer allocation and reversed DMA mapping directions for dummy buffers.
--

commit c4face3fac6884413550ece128f62a970eb9f8e3
Author: Rosen Penev <rosenp@gmail.com>

dma: mv_xor: add missing platform remove function

This commit introduces a remove callback for the mv_xor driver to clean up
channels and irq mappings upon driver unbind.

> diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
> index a97fa0038652d..3fc39cca7cbd3 100644
> --- a/drivers/dma/mv_xor.c
> +++ b/drivers/dma/mv_xor.c
> @@ -1452,8 +1452,22 @@ static int mv_xor_probe(struct platform_device *pdev)
>  	return ret;
>  }

[Severity: High]
This is a pre-existing issue, but is there a potential array overflow in
mv_xor_probe() for Armada 37XX devices?

For XOR_ARMADA_37XX, max_channels is set to num_present_cpus() without capping
it to MV_XOR_MAX_CHANNELS (which is 2):

    if (xordev->xor_type == XOR_ARMADA_37XX)
        max_channels = num_present_cpus();

If the device tree provides more than two child nodes and there are more than
two CPUs, could the loop sequentially storing allocated channel pointers exceed
the bounds of the fixed-size xordev->channels array?

[Severity: High]
This is a pre-existing issue, but are the dummy_src and dummy_dst buffers in
struct mv_xor_chan safely aligned and mapped?

They are embedded in the struct without cache-line alignment, and their mapping
directions in mv_xor_channel_add() appear to be swapped:

    mv_chan->dummy_src_addr = dma_map_single(dma_dev->dev,
        mv_chan->dummy_src, MV_XOR_MIN_BYTE_COUNT, DMA_FROM_DEVICE);

Since mv_xor_prep_dma_interrupt() uses dummy_src as the read source and
dummy_dst as the write destination, unmapping dummy_src with DMA_FROM_DEVICE
will invalidate the CPU cache.

Could this invalidation corrupt adjacent struct members sharing the same cache
line?
 
> +static void mv_xor_remove(struct platform_device *pdev)
> +{
> +	struct mv_xor_device *xordev = platform_get_drvdata(pdev);
> +	int i;
> +
> +	for (i = 0; i < MV_XOR_MAX_CHANNELS; i++)
> +		if (xordev->channels[i]) {
> +			mv_xor_channel_remove(xordev->channels[i]);

[Severity: High]
This isn't a bug introduced by this patch, but does mv_xor_channel_remove()
need to call tasklet_kill()?

When the driver unregisters the DMA device:

    dma_async_device_unregister(&mv_chan->dmadev);

it leaves pending tasklets capable of executing asynchronously.

Because the channel and DMA descriptor pool are freed by devres, could an
executing tasklet access freed memory if it runs after this cleanup?

[Severity: High]
This is a pre-existing issue, but does enabling driver unbind expose an
object lifetime mismatch?

The driver dynamically allocates mv_chan using devm_kzalloc(), which is freed
by devres when remove() completes.

Since dma_async_device_unregister() does not forcefully revoke client
references, could active clients holding a channel reference trigger a
use-after-free when they access or release the channel later?

[Severity: High]
This isn't a bug introduced by this patch, but does the hardware need to be
quiesced before tearing down the channel?

mv_xor_channel_remove() destructively unmaps dummy buffers via
dma_unmap_single(), and then devres disables the clock.

If a DMA transaction is currently in flight, could the hardware continue
writing to unmapped memory or hang the AXI bus?

> +			if (pdev->dev.of_node)
> +				irq_dispose_mapping(xordev->channels[i]->irq);
> +		}
> +}

[ ... ]

>  static struct platform_driver mv_xor_driver = {
>  	.probe		= mv_xor_probe,
> +	.remove		= mv_xor_remove,
>  	.suspend        = mv_xor_suspend,
>  	.resume         = mv_xor_resume,
>  	.driver		= {

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