Re: [PATCH 3/4] dmaengine: ste_dma40: Fix probe IRQ leak

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

Pre-existing issues:
- [High] The probe error path incorrectly calls free_pages() on an ioremap pointer when ESRAM is used for LCLA.
- [High] The DMA engine devices are registered using devres (dmaenginem_async_device_register), but core memory resources and the interrupt handler are manually freed in the probe error path, risking Use-After-Free.
- [High] d40_lcla_allocate() passes a calculated number of pages instead of a logarithmic order to __get_free_pages(), leading to severe memory overallocation.
- [High] The DMA engine is registered and exposed to other subsystems before hardware initialization is complete.
- [High] Out-of-bounds write in the global dma40_memcpy_channels array via unverified device tree input.
--

--- Patch [3]: [PATCH 3/4] dmaengine: ste_dma40: Fix probe IRQ leak ---
commit 5f3ddaa41f4f5eab5f519e10436cd8e82088831f
Author: Linus Walleij <[email protected]>

dmaengine: ste_dma40: Fix probe IRQ leak

d40_probe() registers the hardware interrupt before several later probe
steps that can fail. Those error paths jump to destroy_cache without
freeing the IRQ, leaving the handler registered after probe resources have
been released. This patch tracks successful IRQ registration and frees the
IRQ on later probe failure.

Closes: https://lore.kernel.org/dmaengine/[email protected]/

> diff --git a/drivers/dma/ste_dma40.c b/drivers/dma/ste_dma40.c
> index 0c8ff299c2c2d..a723133d096ec 100644
> --- a/drivers/dma/ste_dma40.c
> +++ b/drivers/dma/ste_dma40.c
> @@ -3503,6 +3503,7 @@ static int __init d40_probe(struct platform_device *pdev)
>  	struct resource res_lcpa;
>  	int num_reserved_chans;
>  	bool runtime_pm_enabled = false;
> +	bool irq_requested = false;
>  	u32 val;
>  	int ret;
>  

[ ... ]

> @@ -3595,6 +3596,7 @@ static int __init d40_probe(struct platform_device *pdev)
>  		d40_err(dev, "No IRQ defined\n");
>  		goto destroy_cache;
>  	}
> +	irq_requested = true;
>  
>  	if (base->plat_data->use_esram_lcla) {
>  

[ ... ]

> @@ -3660,6 +3662,8 @@ static int __init d40_probe(struct platform_device *pdev)
>  		regulator_disable(base->lcpa_regulator);
>  		regulator_put(base->lcpa_regulator);
>  	}
> +	if (irq_requested)
> +		free_irq(base->irq, base);

[Severity: High]
This isn't a bug introduced by this patch, but does the destroy_cache error
path call free_pages() on an ioremap pointer when ESRAM is used for LCLA?

In d40_probe(), if base->plat_data->use_esram_lcla is true,
base->lcla_pool.base is initialized with devm_ioremap():

drivers/dma/ste_dma40.c:d40_probe() {
    ...
    if (base->plat_data->use_esram_lcla) {
        ...
        base->lcla_pool.base = devm_ioremap(dev, res->start,
                                            resource_size(res));
    ...
}

However, in the destroy_cache error path, it seems to be freed using
free_pages():

drivers/dma/ste_dma40.c:d40_probe() {
    ...
 destroy_cache:
    ...
    if (!base->lcla_pool.base_unaligned && base->lcla_pool.base)
        free_pages((unsigned long)base->lcla_pool.base,
                   base->lcla_pool.pages);
    ...
}

Would passing this MMIO address to the page allocator cause a VM_BUG_ON or
memory corruption?

[Severity: High]
This isn't a bug introduced by this patch, but does the error path risk a
use-after-free by manually freeing core memory resources while the DMA engine
devices are still registered?

In d40_probe(), the DMA engine devices are registered using
d40_dmaengine_init():

drivers/dma/ste_dma40.c:d40_probe() {
    ...
    ret = d40_dmaengine_init(base, num_reserved_chans);
    if (ret)
        goto destroy_cache;
    ...
}

If a failure occurs after this point, the destroy_cache path manually frees
lcla_pool and the IRQ handler. Since d40_dmaengine_init() likely uses devres
to register the DMA devices, they remain globally visible until the probe
function returns and devres unwinds.

Could this allow a concurrent client to initiate a DMA transfer using freed
resources?

[Severity: High]
This is a pre-existing issue, but is the DMA engine registered and exposed to
other subsystems before hardware initialization is complete?

In d40_probe(), the DMA devices are registered via d40_dmaengine_init()
before d40_hw_init() is called:

drivers/dma/ste_dma40.c:d40_probe() {
    ...
    ret = d40_dmaengine_init(base, num_reserved_chans);
    if (ret)
        goto destroy_cache;
    ...
    d40_hw_init(base);
    ...
}

If a client requests a channel and starts a transfer before d40_hw_init()
finishes, could this lead to a bus lockup or corrupted transfers?

[Severity: High]
This is a pre-existing issue, but does d40_lcla_allocate() pass a page count
instead of a logarithmic order to __get_free_pages()?

In d40_lcla_allocate(), the required number of pages is calculated and stored
in base->lcla_pool.pages:

drivers/dma/ste_dma40.c:d40_lcla_allocate() {
    ...
    base->lcla_pool.pages = SZ_1K * base->num_phy_chans / PAGE_SIZE;
    ...
    page_list[i] = __get_free_pages(GFP_KERNEL,
                                    base->lcla_pool.pages);
    ...
}

If SZ_1K * base->num_phy_chans / PAGE_SIZE evaluates to 8, wouldn't passing 8
as the order request 2^8 (256) pages instead of 8, leading to massive memory
overallocation?

[Severity: High]
This isn't a bug introduced by this patch, but is there an out-of-bounds
write when parsing memcpy-channels from the device tree?

The global array dma40_memcpy_channels is sized to exactly 6 elements:

drivers/dma/ste_dma40.c:
    static int dma40_memcpy_channels[] = {
        DB8500_DMA_MEMCPY_EV_0,
        ...
        DB8500_DMA_MEMCPY_EV_5,
    };

In d40_of_probe(), the length is checked against D40_MEMCPY_MAX_CHANS (which
is 8), and then of_property_read_u32_array() writes up to 8 elements:

drivers/dma/ste_dma40.c:d40_of_probe() {
    ...
    if (num_memcpy > D40_MEMCPY_MAX_CHANS || num_memcpy <= 0) {
        ...
    }
    ...
    of_property_read_u32_array(np, "memcpy-channels",
                               dma40_memcpy_channels,
                               num_memcpy);
    ...
}

Could this overwrite adjacent kernel data if the device tree specifies 7 or 8
elements?

>  	if (runtime_pm_enabled)
>  		pm_runtime_disable(base->dev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3