Re: [PATCH] dmaengine: ste_dma40: fix out-of-bounds access from D40_MEMCPY_MAX_CHANS

[sashiko-bot@kernel.org]

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

New issues:
- [Low] The commit message incorrectly claims that the out-of-bounds write corrupts 'adjacent stack memory'.

Pre-existing issues:
- [High] Missing `free_irq()` and `tasklet_kill()` in the `d40_probe()` error path (`destroy_cache`) leads to a severe Use-After-Free and IRQ leak.
--

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

dmaengine: ste_dma40: fix out-of-bounds access from D40_MEMCPY_MAX_CHANS

The commit fixes an out-of-bounds array access in the ste_dma40 driver by
correcting D40_MEMCPY_MAX_CHANS to match the actual size of the
dma40_memcpy_channels array.

> 1. d40_of_probe() accepts up to 8 memcpy channels from DT
>    (num_memcpy > D40_MEMCPY_MAX_CHANS allows 7-8), then writes them
>    into the 6-element dma40_memcpy_channels[], corrupting adjacent
>    stack memory.

[Severity: Low]
Is the description of "stack memory" corruption accurate here?

Looking at dma40_memcpy_channels, it is defined as a static global array,
meaning the out-of-bounds write would corrupt the .data or .bss segments
rather than adjacent stack memory.

Could the commit message be updated to reflect that this is global memory
corruption rather than stack corruption?


> diff --git a/drivers/dma/ste_dma40.c b/drivers/dma/ste_dma40.c
> index 9b803c0aec254..9dfacffa0548f 100644
> --- a/drivers/dma/ste_dma40.c
> +++ b/drivers/dma/ste_dma40.c
> @@ -86,7 +86,7 @@ struct stedma40_platform_data {
>  #define D40_ALLOC_PHY		BIT(30)
>  #define D40_ALLOC_LOG_FREE	0
>  
> -#define D40_MEMCPY_MAX_CHANS	8
> +#define D40_MEMCPY_MAX_CHANS	6

[Severity: High]
This is a pre-existing issue, but I noticed a missing cleanup step in the
probe function's error handling. Does the destroy_cache error path in
d40_probe() leak the IRQ and fail to kill tasklets?

drivers/dma/ste_dma40.c:d40_probe() {
    ...
	ret = request_irq(base->irq, d40_handle_interrupt, 0, D40_NAME, base);
	if (ret) {
		d40_err(dev, "No IRQ defined\n");
		goto destroy_cache;
	}
    ...
	ret = of_dma_controller_register(np, d40_xlate, NULL);
	if (ret) {
		dev_err(dev,
			"could not register of_dma_controller\n");
		goto destroy_cache;
	}
    ...
 destroy_cache:
	if (base->lcla_pool.dma_addr)
		dma_unmap_single(base->dev, base->lcla_pool.dma_addr,
    ...
}

Because request_irq() is used instead of a devm_ variant, shouldn't free_irq()
and tasklet_kill() be called before returning?

If probe fails late, the devm_kzalloc() allocated base structure is
automatically freed. If the leaked IRQ fires or a scheduled tasklet executes
afterwards, they will access the freed memory, leading to a Use-After-Free.

>  
>  /* Reserved event lines for memcpy only. */
>  #define DB8500_DMA_MEMCPY_EV_0	51

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