Re: [PATCH] dmaengine: altera-msgdma: fail probe when reset times out

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing dma_async_device_unregister() on probe error path leads to Use-After-Free
- [High] IRQ handler requested before data structures are initialized, risking NULL pointer dereference
--

commit 4e931dfaccbe922ddc5a4a0f0f0b3dd9e532e207
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

dmaengine: altera-msgdma: fail probe when reset times out

This commit updates msgdma_reset() to return an error code when the
controller fails to reset. It also modifies msgdma_probe() to abort the
probe process if the reset times out, preventing the registration of a DMA
engine backed by a stalled controller.

> diff --git a/drivers/dma/altera-msgdma.c b/drivers/dma/altera-msgdma.c
> --- a/drivers/dma/altera-msgdma.c
> +++ b/drivers/dma/altera-msgdma.c
[ ... ]
> @@ -896,7 +899,9 @@ static int msgdma_probe(struct platform_device *pdev)
>  		goto fail;
>  	}
>  
> -	msgdma_reset(mdev);
> +	ret = msgdma_reset(mdev);
> +	if (ret)
> +		goto fail;

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential memory
issue in the error handling path just after this code. If
of_dma_controller_register() fails later in the probe, the code branches to
the fail label:

msgdma_probe() {
    ...
    ret = dma_async_device_register(dma_dev);
    if (ret)
        goto fail;

    ret = of_dma_controller_register(...);
    if (ret == -EINVAL)
        ...
    else if (ret && ret != -ENODEV)
        goto fail;
    ...
fail:
    msgdma_dev_remove(mdev);
    return ret;
}

Does msgdma_dev_remove() need to call dma_async_device_unregister() to clean
up the registered device?

Since mdev is allocated with devm_kzalloc(), the dma_device structure is
freed when the probe fails. Will leaving the device registered leave a
dangling pointer in the global dma_device_list leading to a use-after-free?

[Severity: High]
This is also a pre-existing issue, but looking at the earlier setup in
msgdma_probe(), could an interrupt fire before the required structures are
fully initialized?

msgdma_probe() {
    ...
    ret = devm_request_irq(&pdev->dev, mdev->irq, msgdma_irq_handler, ...);
    ...
    tasklet_setup(&mdev->irq_tasklet, msgdma_tasklet);
    dma_cookie_init(&mdev->dmachan);
    spin_lock_init(&mdev->lock);
    INIT_LIST_HEAD(&mdev->active_list);
    INIT_LIST_HEAD(&mdev->pending_list);
    ...
}

If the hardware is already asserting an interrupt during boot (for example,
left over from a bootloader), will the IRQ handler access uninitialized
structures like mdev->pending_list and trigger a NULL pointer dereference?

Should the interrupt be requested only after these data structures are safely
initialized?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623060554.13523-1-pengpeng@iscas.ac.cn?part=1