Re: [PATCH 4/4] dmaengine: ste_dma40: Fix logical channel bounds check

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

Pre-existing issues:
- [High] Out-of-bounds access on the `phys` array and potentially arbitrary MMIO writes during channel allocation due to unconstrained `event_group`.
- [High] The out-of-bounds write fix for `lookup_log_chans` is bypassed for memcpy channels configured via the Device Tree.
- [High] Global buffer overflow when reading `memcpy-channels` from Device Tree.
--

--- Patch [4]: [PATCH 4/4] dmaengine: ste_dma40: Fix logical channel bounds check ---
commit 53c98a760123837b647fa917a72020a724b5f32f
Author: Linus Walleij <[email protected]>

dmaengine: ste_dma40: Fix logical channel bounds check
    
d40_validate_conf() checks the raw dev_type against num_log_chans,
but d40_allocate_channel() derives the lookup_log_chans index as either
2 * dev_type or 2 * dev_type + 1.

Validate the dev_type against the derived logical channel index limit so
channel allocation cannot write past lookup_log_chans.
    
Closes: https://lore.kernel.org/dmaengine/[email protected]/

> diff --git a/drivers/dma/ste_dma40.c b/drivers/dma/ste_dma40.c
> index a723133d096ec..eda3f91741ed5 100644
> --- a/drivers/dma/ste_dma40.c
> +++ b/drivers/dma/ste_dma40.c
> @@ -1727,15 +1727,26 @@ static int d40_validate_conf(struct d40_chan *d40c,
>  {
>  	int res = 0;
>  	bool is_log = conf->mode == STEDMA40_MODE_LOGICAL;
> +	bool invalid_dev_type = conf->dev_type < 0;
>  
>  	if (!conf->dir) {
>  		chan_err(d40c, "Invalid direction.\n");
>  		res = -EINVAL;
>  	}
>  
> -	if ((is_log && conf->dev_type > d40c->base->num_log_chans)  ||
> -	    (!is_log && conf->dev_type > d40c->base->num_phy_chans) ||
> -	    (conf->dev_type < 0)) {
> +	if (!invalid_dev_type && is_log) {
> +		int max_dev_type;
> +
> +		if (conf->dir == DMA_DEV_TO_MEM)
> +			max_dev_type = DIV_ROUND_UP(d40c->base->num_log_chans, 2);
> +		else
> +			max_dev_type = d40c->base->num_log_chans / 2;
> +
> +		invalid_dev_type = conf->dev_type >= max_dev_type;
> +	}

[Severity: High]
This is a pre-existing issue, but does this bounds check fully protect the
phys array during channel allocation?

In d40_allocate_channel(), phy_num is calculated as j + event_group * 2,
where event_group is derived from dev_type / 16.

If num_log_chans is heavily scaled compared to num_phy_chans, could an
allowed logical dev_type produce an event_group that causes
event_group * 2 to greatly exceed the physical channel count?

drivers/dma/ste_dma40.c:d40_allocate_channel() {
    ...
            for (i = phy_num; i < phy_num + 2; i++) {
                if (d40_alloc_mask_set(&phys[i], ...
    ...
}

Could this cause out-of-bounds accesses on the phys array, as well as
arbitrary MMIO writes later in __d40_set_prio_rt() which uses the
unvalidated group multiplier?

drivers/dma/ste_dma40.c:__d40_set_prio_rt() {
    ...
        writel(bit, d40c->base->virtbase + rtreg + group * 4);
    ...
}

[Severity: High]
This is also a pre-existing issue, but is the validation in
d40_validate_conf() bypassed for memcpy channels configured via the
Device Tree?

In the fallback configuration path d40_config_memcpy(), the driver
assigns dev_type directly from the dma40_memcpy_channels array without
calling d40_validate_conf():

drivers/dma/ste_dma40.c:d40_config_memcpy() {
    ...
        d40c->dma_cfg.dev_type = dma40_memcpy_channels[d40c->chan.chan_id];
    ...
}

If the device tree supplies an arbitrarily large ID for a memcpy channel
via the memcpy-channels property, could this unvalidated dev_type cause
an out-of-bounds write into lookup_log_chans during allocation, subverting
this patch's fix?

drivers/dma/ste_dma40.c:d40_allocate_channel() {
    ...
        d40c->base->lookup_log_chans[d40c->log_num] = d40c;
    ...
}

[Severity: High]
Another pre-existing issue, but is there a risk of a global buffer overflow
when reading memcpy-channels from the Device Tree?

The dma40_memcpy_channels array is statically sized with exactly 6 elements:

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

However, in d40_probe(), the DT parsing logic reads memcpy-channels up to
D40_MEMCPY_MAX_CHANS (which is 8) via of_property_read_u32_array():

drivers/dma/ste_dma40.c:d40_probe() {
    ...
        of_property_read_u32_array(np, "memcpy-channels",
                                   dma40_memcpy_channels,
                                   num_memcpy);
    ...
}

If a Device Tree blob provides 7 or 8 elements in the memcpy-channels
property, will the driver write past the end of the 6-element array during
initialization?

> +
> +	if (invalid_dev_type ||
> +	    (!is_log && conf->dev_type > d40c->base->num_phy_chans)) {
>  		chan_err(d40c, "Invalid device type (%d)\n", conf->dev_type);
>  		res = -EINVAL;
>  	}

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