Re: [PATCH v3 17/34] dmaengine: ste_dma40: convert per-channel tasklet to dmaengine BH

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing IRQ free on probe error path leads to Use-After-Free
- [High] Missing `device_synchronize` callback causes UAF race condition with client callbacks
- [High] Integer underflow and out-of-bounds array access on zero-length scatterlist
--

--- Patch [17]: [PATCH v3 17/34] dmaengine: ste_dma40: convert per-channel tasklet to dmaengine BH ---
commit fc9cce1bc39ef7ff38acb71e0dd932c23517c6a1
Author: Allen Pais <allen.lkml@gmail.com>

dmaengine: ste_dma40: convert per-channel tasklet to dmaengine BH
    
Replace the per-channel tasklet with the shared dmaengine BH helper.
The handler continues to run in softirq context while dmaengine owns
the common scheduling and teardown mechanism.

> diff --git a/drivers/dma/ste_dma40.c b/drivers/dma/ste_dma40.c
> index 0d9ffa3e26639..e6dfa1fc5e5ab 100644
> --- a/drivers/dma/ste_dma40.c
> +++ b/drivers/dma/ste_dma40.c
[ ... ]
> @@ -1641,7 +1638,7 @@ static void dma_tasklet(struct tasklet_struct *t)
>  	d40c->pending_tx--;
>  
>  	if (d40c->pending_tx)
> -		tasklet_schedule(&d40c->tasklet);
> +		dmaengine_schedule_bh(&d40c->chan);
>  
>  	spin_unlock_irqrestore(&d40c->lock, flags);

[Severity: High]
This is a pre-existing issue, but I noticed the callback is executed
without the channel lock held here.

Is it possible for a concurrent client termination to return early and free
resources?

The ste_dma40 driver does not implement the device_synchronize callback.
When a client calls dmaengine_terminate_sync(), the core
dmaengine_synchronize() fallback acts as a no-op.

If dmaengine_terminate_sync() is called while this background handler has
dropped the lock to execute dmaengine_desc_callback_invoke(), could the
termination return immediately instead of waiting for the callback to finish?

This might allow the client to proceed and free buffers or structures that
the background callback is actively accessing.


Also, I noticed another pre-existing issue in the probe error path:

drivers/dma/ste_dma40.c:d40_probe() {
    ...
		dev_err(dev,
			"could not register of_dma_controller\n");
		goto destroy_cache;
	}

	dev_info(base->dev, "initialized\n");
	return 0;

 destroy_cache:
	if (base->lcla_pool.dma_addr)
		dma_unmap_single(base->dev, base->lcla_pool.dma_addr,
				 SZ_1K * base->num_phy_chans,
				 DMA_TO_DEVICE);
    ...
}

[Severity: High]
This is a pre-existing issue, but does this error path need a free_irq()?

If a subsequent initialization step fails (e.g., 
of_dma_controller_register()), the code jumps to destroy_cache and returns
an error without freeing the IRQ.

Then devres automatically frees the memory allocated for the d40_base
structure.

If an interrupt fires after a probe failure, could d40_handle_interrupt()
execute using the freed base pointer?


Finally, I saw a potential out-of-bounds access in the scatterlist preparation:

drivers/dma/ste_dma40.c:d40_prep_sg() {
    ...
	desc = d40_prep_desc(chan, sg_src, sg_len, dma_flags);
	if (desc == NULL)
		goto unlock;

	if (sg_next(&sg_src[sg_len - 1]) == sg_src)
		desc->cyclic = true;
    ...
}

[Severity: High]
This is a pre-existing issue, but does this code correctly handle an empty
scatterlist?

If a client provides an empty scatterlist where sg_len is 0, does the
subtraction sg_len - 1 underflow to UINT_MAX?

Could this lead to d40_prep_desc() successfully returning a zero-length
allocation, and then the condition sg_next(&sg_src[UINT_MAX]) triggering
a massive out-of-bounds memory read?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1786384168.git.allen.lkml@gmail.com?part=17