Re: [PATCH v2 12/13] dmaengine: ste_dma40: Validate memcpy configuration

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

New issues:
- [High] d40_validate_conf() unconditionally rejects memcpy configurations.

Pre-existing issues:
- [High] Error paths in d40_alloc_chan_resources() underflow PM runtime reference count.
--

--- Patch [12]: [PATCH v2 12/13] dmaengine: ste_dma40: Validate memcpy configuration ---
commit 19f873d8b80c96a75bea7cfedde33c6486eb34af
Author: Linus Walleij <[email protected]>

dmaengine: ste_dma40: Validate memcpy configuration

This patch validates the generated memcpy configuration before deriving
logical channel registers or allocating the channel. This ensures that
a dev_type supplied through the memcpy-channels device tree property
cannot bypass the bounds checks.

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

> diff --git a/drivers/dma/ste_dma40.c b/drivers/dma/ste_dma40.c
> index e0c694a1fc8b0..49bca706b638f 100644
> --- a/drivers/dma/ste_dma40.c
> +++ b/drivers/dma/ste_dma40.c
> @@ -2013,11 +2013,16 @@ static int d40_allocate_channel(struct d40_chan *d40c, bool *first_phy_user)
>  static int d40_config_memcpy(struct d40_chan *d40c)
>  {
>  	dma_cap_mask_t cap = d40c->chan.device->cap_mask;
> +	int ret;
>  
>  	if (dma_has_cap(DMA_MEMCPY, cap) && !dma_has_cap(DMA_SLAVE, cap)) {
>  		d40c->dma_cfg = dma40_memcpy_conf_log;
>  		d40c->dma_cfg.dev_type = dma40_memcpy_channels[d40c->chan.chan_id];
>  
> +		ret = d40_validate_conf(d40c, &d40c->dma_cfg);
> +		if (ret)
> +			return ret;
> +

[Severity: High]
Does this unconditionally reject memcpy configurations?

In the framework, DMA_MEM_TO_MEM has an integer value of 0. When this calls
d40_validate_conf() for the memcpy configurations (which have a direction of
DMA_MEM_TO_MEM), it hits this check in d40_validate_conf():

drivers/dma/ste_dma40.c:d40_validate_conf() {
	...
	if (!conf->dir) {
		chan_err(d40c, "Invalid direction.\n");
		res = -EINVAL;
	}
	...
}

Because conf->dir is 0, this evaluates to true, resulting in validation always
returning -EINVAL and completely breaking memcpy functionality for this
driver.

[Severity: High]
This is a pre-existing issue, but returning an error here uncovers a
PM runtime reference underflow in the caller.

If d40_config_memcpy() fails, d40_alloc_chan_resources() jumps to its error
handler before the PM runtime reference is acquired:

drivers/dma/ste_dma40.c:d40_alloc_chan_resources() {
	...
	if (!d40c->configured) {
		err = d40_config_memcpy(d40c);
		if (err) {
			chan_err(d40c, "Failed to configure memcpy channel\n");
			goto mark_last_busy;
		}
	}
	...
mark_last_busy:
	pm_runtime_put_autosuspend(d40c->base->dev);
	spin_unlock_irqrestore(&d40c->lock, flags);
	return err;
}

The error handler unconditionally calls pm_runtime_put_autosuspend(),
decrementing the reference count without it having been incremented.

Could this lead to the device being prematurely suspended while actively
in use?

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