Re: [PATCH v2 11/13] dmaengine: ste_dma40: Fix event group bounds

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

New issues:
- [High] The patch incorrectly rejects valid device types for event group 4, breaking DMA support for those devices on newer hardware (e.g., DB8540).

Pre-existing issues:
- [High] Logical channel allocation fails for fixed channels located outside the first physical channel block (j=0) due to a premature loop abort.
- [High] Missing bounds check on `phy_channel` when allocating fixed physical mem-to-mem channels allows out-of-bounds array access.
- [High] Error paths in `d40_alloc_chan_resources()` drop a PM runtime reference that was never acquired, leading to an unbalanced refcount underflow.
--

--- Patch [11]: [PATCH v2 11/13] dmaengine: ste_dma40: Fix event group bounds ---
commit 409873531e42fe4afcb7eb90c8da7d95a228b5d7
Author: Linus Walleij <[email protected]>

dmaengine: ste_dma40: Fix event group bounds

The dev_type validation can allow values whose derived event group has no
matching physical channel pair. This patch rejects dev_type values outside
the hardware event-group range and stops physical-channel searches before
indexing past phy_res.

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

> diff --git a/drivers/dma/ste_dma40.c b/drivers/dma/ste_dma40.c
> index 7c777e410c104..e0c694a1fc8b0 100644
> --- a/drivers/dma/ste_dma40.c
> +++ b/drivers/dma/ste_dma40.c
> @@ -1739,6 +1739,10 @@ static int d40_validate_conf(struct d40_chan *d40c,
>  	bool is_log = conf->mode == STEDMA40_MODE_LOGICAL;
>  	bool invalid_dev_type = conf->dev_type < 0;
>  
> +	if (!invalid_dev_type &&
> +	    D40_TYPE_TO_GROUP(conf->dev_type) >= D40_GROUP_SIZE / 2)
> +		invalid_dev_type = true;

[Severity: High]
Does this check incorrectly reject valid device types for event group 4?

Since D40_GROUP_SIZE is 8, D40_GROUP_SIZE / 2 is 4, which means any dev_type
mapped to event_group >= 4 (i.e. dev_type >= 64) is flagged as invalid.

Newer hardware like DB8540 has 5 event groups (0-4). The new out-of-bounds
safety is correctly handled later in d40_allocate_channel():

if (phy_num + 1 >= num_phy_chans)
        break;

Could this dev_type validation cause a regression by breaking valid logical
channels mapped to event group 4 on these devices?

[ ... ]

> @@ -1942,8 +1950,10 @@ static int d40_allocate_channel(struct d40_chan *d40c, bool *first_phy_user)
>  		return -EINVAL;
>  
>  	/* Find logical channel */
> -	for (j = 0; j < d40c->base->num_phy_chans; j += 8) {
> +	for (j = 0; j < d40c->base->num_phy_chans; j += D40_GROUP_SIZE) {
>  		int phy_num = j + event_group * 2;
> +		if (phy_num + 1 >= num_phy_chans)
> +			break;
>  
>  		if (d40c->dma_cfg.use_fixed_channel) {
>  			i = d40c->dma_cfg.phy_channel;

[Severity: High]
This is a pre-existing issue, but does this loop prematurely abort if a fixed
channel isn't in the first physical block?

Looking at d40_allocate_channel(), when iterating over blocks of physical
channels for logical channels with use_fixed_channel == true, it checks if
the requested phy_channel matches phy_num or phy_num + 1 for the current
group j.

If they do not match, it prints an error and returns -EINVAL immediately:

drivers/dma/ste_dma40.c:d40_allocate_channel() {
    ...
    if (d40c->dma_cfg.use_fixed_channel) {
        i = d40c->dma_cfg.phy_channel;
        if ((i != phy_num) && (i != phy_num + 1)) {
            dev_err(chan2dev(d40c), "invalid fixed phy channel %d\n", i);
            return -EINVAL;
        }
    ...
}

Should this use continue instead of returning -EINVAL to allow checking the
remaining physical channel groups?


While reviewing the channel allocation logic, I noticed two other pre-existing
issues in the surrounding code.

[Severity: High]
This is a pre-existing issue, but can an unvalidated phy_channel lead to an
out-of-bounds array access when configuring fixed physical mem-to-mem channels?

In d40_allocate_channel(), when configuring a physical DMA_MEM_TO_MEM channel
with use_fixed_channel, the driver directly assigns phy_channel and uses
it as an index into the phys array:

drivers/dma/ste_dma40.c:d40_allocate_channel() {
    ...
    if (d40c->dma_cfg.dir == DMA_MEM_TO_MEM) {
        if (d40c->dma_cfg.use_fixed_channel) {
            i = d40c->dma_cfg.phy_channel;
            if (d40_alloc_mask_set(&phys[i], is_src, 0, is_log, first_phy_user))
                goto found_phy;
        }
    ...
}

Since phy_channel is not validated against num_phy_chans in
d40_validate_conf() or locally in this block, could a misconfiguration via
device tree access memory outside the bounds of the phys array?


[Severity: High]
This is a pre-existing issue, but do the error paths in
d40_alloc_chan_resources() drop a PM runtime reference that was never
acquired?

If channel allocation fails, the function jumps to the mark_last_busy label,
which unconditionally calls pm_runtime_put_autosuspend(). However,
pm_runtime_get_sync() is only called after a successful allocation:

drivers/dma/ste_dma40.c:d40_alloc_chan_resources() {
    ...
    err = d40_allocate_channel(d40c, &is_free_phy);
    if (err) {
        chan_err(d40c, "Failed to allocate channel\n");
        d40c->configured = false;
        goto mark_last_busy;
    }

    pm_runtime_get_sync(d40c->base->dev);
    ...
mark_last_busy:
    pm_runtime_put_autosuspend(d40c->base->dev);
    ...
}

Could this lead to a refcount underflow and cause the device to suspend
prematurely?

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