Re: [PATCHv2 3/9] dmaengine: mv_xor: bound maximum channels for Armada 37xx

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The return value of `clk_prepare_enable()` is ignored, leading to potential warnings and hardware hangs if the clock fails to enable.
- [High] The driver fails to kill the scheduled `irq_tasklet` on error and teardown paths, leading to a Use-After-Free.
- [High] In `mv_xor_channel_remove()`, DMA memory is freed before the interrupt handler is disabled, creating a race condition leading to Use-After-Free.
- [High] The driver lacks a `.remove` callback and does not set `.suppress_bind_attrs = true`, causing severe resource leaks and use-after-free on sysfs unbind.
- [High] The IRQ handler is requested and unmasked before the channel's spinlock and lists are initialized, resulting in a race condition causing a NULL pointer dereference.
- [High] The inline dummy buffers (`dummy_src`, `dummy_dst`) share cachelines with struct members and their DMA mapping directions are reversed.
--

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

dmaengine: mv_xor: bound maximum channels for Armada 37xx

This commit adds a missing bounds check for the max_channels calculation
on Armada 37xx. It ensures max_channels does not exceed MV_XOR_MAX_CHANNELS
based on num_present_cpus(), preventing the probe loop from writing past
the end of the xordev->channels[] array.

> diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
> index 25ed61f1b0899..68f0880ad441f 100644
> --- a/drivers/dma/mv_xor.c
> +++ b/drivers/dma/mv_xor.c
[ ... ]
> @@ -1376,7 +1376,8 @@ static int mv_xor_probe(struct platform_device *pdev)
>  	 */
>  	max_engines = num_present_cpus();
>  	if (xordev->xor_type == XOR_ARMADA_37XX)
> -		max_channels =	num_present_cpus();
> +		max_channels = min_t(unsigned int, MV_XOR_MAX_CHANNELS,
> +				     num_present_cpus());
>  	else
>  		max_channels = min_t(unsigned int,
>  				     MV_XOR_MAX_CHANNELS,

While reviewing this patch, I noticed a few pre-existing issues in the
surrounding code that might need attention.

[Severity: High]
This isn't a bug introduced by this patch, but looking at mv_xor_probe()
the return value of clk_prepare_enable() appears to be ignored:

    if (!IS_ERR(xordev->clk))
        clk_prepare_enable(xordev->clk);

If the clock fails to enable, would the driver continue probing and
subsequently hang the hardware bus during I/O accesses?

Additionally, the error path unconditionally executes clk_disable_unprepare():

    if (!IS_ERR(xordev->clk)) {
        clk_disable_unprepare(xordev->clk);
        clk_put(xordev->clk);
    }

Could this trigger a state imbalance warning in the clock core if the clock
was never successfully enabled?

[Severity: High]
This is a pre-existing issue, but does mv_xor_channel_remove() miss killing
the irq_tasklet before freeing the channel?

drivers/dma/mv_xor.c:mv_xor_channel_remove() {
    ...
    free_irq(mv_chan->irq, mv_chan);

    return 0;
}

Could a pending tasklet run after mv_chan is freed by devres, leading to
a use-after-free? It looks like the err_free_irq path in mv_xor_channel_add()
has a similar pattern and might also need a tasklet_kill() before returning.

[Severity: High]
This is a pre-existing issue, but is there a race condition in
mv_xor_channel_remove() when freeing DMA memory?

drivers/dma/mv_xor.c:mv_xor_channel_remove() {
    ...
    dma_free_wc(dev, MV_XOR_POOL_SIZE,
                mv_chan->dma_desc_pool_virt, mv_chan->dma_desc_pool);
    dma_unmap_single(dev, mv_chan->dummy_src_addr,
                     MV_XOR_MIN_BYTE_COUNT, DMA_FROM_DEVICE);
    dma_unmap_single(dev, mv_chan->dummy_dst_addr,
                     MV_XOR_MIN_BYTE_COUNT, DMA_TO_DEVICE);
    ...
    free_irq(mv_chan->irq, mv_chan);
}

Since dma_free_wc() and dma_unmap_single() are called before free_irq(), could
an interrupt fire during this window and schedule the tasklet to access the
freed descriptor buffers?

[Severity: High]
This isn't a bug introduced by this patch, but does the driver lack a
.remove callback and omit .suppress_bind_attrs = true?

drivers/dma/mv_xor.c:
static struct platform_driver mv_xor_driver = {
    .probe          = mv_xor_probe,
    .suspend        = mv_xor_suspend,
    .resume         = mv_xor_resume,
    .driver         = {
        .name           = MV_XOR_NAME,
        .of_match_table = mv_xor_dt_ids,
    },
};

If the root user manually unbinds the device via sysfs, would this leak DMA
device registrations and hardware interrupts? Could a subsequent interrupt
access the devm-freed mv_chan and cause a use-after-free?

[Severity: High]
This is a pre-existing issue, but does requesting and unmasking the
interrupt before initializing the channel's lock and lists risk a NULL
pointer dereference?

drivers/dma/mv_xor.c:mv_xor_channel_add() {
    ...
    ret = request_irq(mv_chan->irq, mv_xor_interrupt_handler,
                      0, dev_name(&pdev->dev), mv_chan);
    if (ret)
        goto err_free_dma;

    mv_chan_unmask_interrupts(mv_chan);

    ...

    spin_lock_init(&mv_chan->lock);
    INIT_LIST_HEAD(&mv_chan->chain);
    INIT_LIST_HEAD(&mv_chan->completed_slots);
}

If an interrupt fires immediately after being unmasked, wouldn't the tasklet
attempt to acquire an uninitialized spinlock and iterate over the
uninitialized mv_chan->chain list?

[Severity: High]
This isn't a bug introduced by this patch, but do the inline dummy buffers
share cachelines with adjacent struct members?

drivers/dma/mv_xor.h:struct mv_xor_chan {
    ...
    int                     op_in_desc;
    char                    dummy_src[MV_XOR_MIN_BYTE_COUNT];
    char                    dummy_dst[MV_XOR_MIN_BYTE_COUNT];
    dma_addr_t              dummy_src_addr, dummy_dst_addr;
}

In mv_xor_channel_add(), the mapping directions appear to be reversed:

    mv_chan->dummy_src_addr = dma_map_single(dma_dev->dev,
        mv_chan->dummy_src, MV_XOR_MIN_BYTE_COUNT, DMA_FROM_DEVICE);
    ...
    mv_chan->dummy_dst_addr = dma_map_single(dma_dev->dev,
        mv_chan->dummy_dst, MV_XOR_MIN_BYTE_COUNT, DMA_TO_DEVICE);

Could mapping dummy_src as DMA_FROM_DEVICE cause the DMA API to perform
a cache invalidation that clobbers dirty, unflushed CPU writes to adjacent
members like op_in_desc?

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