Re: [PATCH v8 01/19] dmaengine: ti: k3-udma: Fix sporadic crash on AM62x

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The driver restricts MSI controller retrieval to the 'msi-parent' DT binding variant by directly calling `of_msi_get_domain()`, causing failures on platforms using the 'msi-map' binding.
- [High] The patch claims to fix a sporadic crash by zero-initializing `ring_init_data`, but this is a placebo fix because all fields of the struct are already explicitly initialized. The true memory corruption bug remains unaddressed.
- [High] Incorrect use of `INIT_DELAYED_WORK_ONSTACK` for a dynamically allocated work structure causes debug object memory leaks and overwrite warnings.
- [High] DMA mappings created in `udma_setup_rx_flush` are permanently leaked on probe failure.
- [High] Background work `ud->purge_work` is not canceled on probe failure, risking Use-After-Free.
--

--- Patch [1]: [PATCH v8 01/19] dmaengine: ti: k3-udma: Fix sporadic crash on AM62x ---
commit b87c41f67fa8026f26760de8230ab1ebfd627cb1
Author: Vignesh Raghavendra <vigneshr@ti.com>

    dmaengine: ti: k3-udma: Fix sporadic crash on AM62x
    
    ring_init_data is declared on stack and thus can contain garbage which
    can lead to k3_ringacc_dmarings use junk address for ringrt base leading
    to below crash during ring pop

[Severity: High]
This is a pre-existing issue, but does this commit message accurately
describe the root cause of the crash?

Looking at the code, all fields of k3_ringacc_init_data (tisci,
tisci_dev_id, and num_rings) are unconditionally assigned before
k3_ringacc_dmarings_init() is called. Furthermore, k3_ringacc_dmarings_init()
does not read 'ringrt base' from this struct, but fetches it via
devm_platform_ioremap_resource_byname().

Could initializing this struct with { 0 } be acting as a placebo that masks
a true memory corruption or stack buffer overflow by altering the compiler's
stack layout?

[ ... ]
> diff --git a/drivers/dma/ti/k3-udma.c b/drivers/dma/ti/k3-udma.c
> index fb21e0df5ab7b..48f4db777d924 100644
> --- a/drivers/dma/ti/k3-udma.c
> +++ b/drivers/dma/ti/k3-udma.c
> @@ -5484,7 +5484,7 @@ static int udma_probe(struct platform_device *pdev)
>  	if (ud->match_data->type == DMA_TYPE_UDMA) {
>  		ud->ringacc = of_k3_ringacc_get_by_phandle(dev->of_node, "ti,ringacc");
>  	} else {
> -		struct k3_ringacc_init_data ring_init_data;
> +		struct k3_ringacc_init_data ring_init_data = { 0 };
>  
>  		ring_init_data.tisci = ud->tisci_rm.tisci;
>  		ring_init_data.tisci_dev_id = ud->tisci_rm.tisci_dev_id;

[Severity: High]
This is a pre-existing issue, but are we leaving a true root cause unaddressed
here since the struct is completely initialized immediately after this line?


[Severity: High]
This is a pre-existing issue, but does this code properly handle platforms
using the 'msi-map' DT binding variant?

drivers/dma/ti/k3-udma.c:udma_probe() {
    ...
    dev->msi.domain = of_msi_get_domain(dev, dev->of_node, DOMAIN_BUS_TI_SCI_INTA_MSI);
    ...
}

Calling of_msi_get_domain() directly only iterates 'msi-parent' phandles.
Could this cause MSI allocation failures on platforms that map device IDs
using 'msi-map'?


[Severity: High]
This is a pre-existing issue, but is it safe to use INIT_DELAYED_WORK_ONSTACK()
for a work structure that is part of a dynamically allocated object?

drivers/dma/ti/k3-udma.c:bcdma_alloc_chan_resources() {
    ...
    INIT_DELAYED_WORK_ONSTACK(&uc->tx_drain.work, udma_check_tx_completion);
    ...
}

The INIT_DELAYED_WORK_ONSTACK() macro is intended for stack variables and
requires a matching destroy_delayed_work_on_stack(). Since uc is heap-allocated
and destroy_delayed_work_on_stack() is never called, could this lead to debug
object memory leaks and ODEBUG overwrite warnings?

This same pattern also appears in pktdma_alloc_chan_resources().


[Severity: High]
This is a pre-existing issue, but are the DMA mappings created in
udma_setup_rx_flush() properly unmapped on probe failure paths?

drivers/dma/ti/k3-udma.c:udma_setup_rx_flush() {
    ...
    rx_flush->buffer_paddr = dma_map_single(dev, rx_flush->buffer_vaddr,
                                            rx_flush->buffer_size,
                                            DMA_TO_DEVICE);
    ...
}

If an error occurs later in udma_setup_rx_flush(), or if udma_probe()
subsequently fails and returns an error:

drivers/dma/ti/k3-udma.c:udma_probe() {
    ...
    return ret;
}

These DMA mappings appear to permanently leak, as they are not managed by
devres and are not unmapped before returning.


[Severity: High]
This is a pre-existing issue, but is there a risk of a use-after-free if
the device is unregistered on probe failure?

drivers/dma/ti/k3-udma.c:udma_probe() {
    ...
    dma_async_device_unregister(&ud->ddev);
    ...
}

If of_dma_controller_register() fails after dma_async_device_register(),
the DMA channels are briefly exposed. If a client requests a channel and
schedules ud->purge_work before the device is unregistered, could the
asynchronous work execute on the devres-managed ud structure after it has
been freed, since cancel_work_sync(&ud->purge_work) is not called?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260810151423.1948224-1-s-adivi@ti.com?part=1