Re: [PATCH v2 09/13] dmaengine: ste_dma40: Fix memcpy channel parsing

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

Pre-existing issues:
- [High] Out-of-bounds array access in `d40_phy_res_init` due to unvalidated `disabled-channels` property values.
- [Medium] Leaked OF node reference from `of_parse_phandle()` in `d40_probe()`.
- [Medium] Global `dma40_memcpy_channels` array is modified without locking, preventing multiple instances of the device from configuring correctly.
- [Medium] Unvalidated `args_count` in `d40_xlate` leads to reading uninitialized stack memory.
- [Medium] The driver fails to probe if the optional 'memcpy-channels' property is missing from the device tree.
--

--- Patch [9]: [PATCH v2 09/13] dmaengine: ste_dma40: Fix memcpy channel parsing ---
commit 50b8205871db7013fde7fb8ac4433303d26eb570
Author: Linus Walleij <[email protected]>

dmaengine: ste_dma40: Fix memcpy channel parsing

d40_of_probe() validates the memcpy-channels property against
D40_MEMCPY_MAX_CHANS, but reads the property directly into
dma40_memcpy_channels. That array has fewer entries, so a long property
can overwrite adjacent data.

Validate the property length against ARRAY_SIZE(dma40_memcpy_channels)
before reading it into the array.

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

> diff --git a/drivers/dma/ste_dma40.c b/drivers/dma/ste_dma40.c
> index 8cf73b37f8bfd..4e840846466b8 100644
> --- a/drivers/dma/ste_dma40.c
> +++ b/drivers/dma/ste_dma40.c
> @@ -3470,7 +3470,8 @@ static int __init d40_of_probe(struct device *dev,
>  	list = of_get_property(np, "memcpy-channels", &num_memcpy);
>  	num_memcpy /= sizeof(*list);
>  
> -	if (num_memcpy > D40_MEMCPY_MAX_CHANS || num_memcpy <= 0) {
> +	if (num_memcpy > ARRAY_SIZE(dma40_memcpy_channels) ||
> +	    num_memcpy <= 0) {

[Severity: Medium]
This is a pre-existing issue, but does this cause the probe to fail if the
"memcpy-channels" property is completely omitted from the device tree? If
of_get_property() returns NULL, num_memcpy would be 0, which triggers this
error path. Should this property be strictly required, or can the driver
continue without it?

[Severity: Medium]
This is also a pre-existing issue, but does reading the "memcpy-channels"
property directly into the global dma40_memcpy_channels array cause problems
if there are multiple instances of this device? It seems probing subsequent
instances would silently overwrite the global array and corrupt the channel
mapping for previously probed controllers.

>  		d40_err(dev,
>  			"Invalid number of memcpy channels specified (%d)\n",
>  			num_memcpy);

[ ... ]

[Severity: Medium]
This isn't a bug introduced by this patch, but does d40_probe() leak an OF
node reference?

drivers/dma/ste_dma40.c:d40_probe() {
    ...
    np_lcpa = of_parse_phandle(np, "sram", 0);
    ...
}

The of_parse_phandle() call increments the node's reference count, but I
don't see a matching of_node_put() to release it. Can this cause a memory
leak when the driver probes?

[Severity: High]
This is a pre-existing issue, but is it possible for malformed device tree
values to cause an out-of-bounds write here?

drivers/dma/ste_dma40.c:d40_phy_res_init() {
    ...
    base->phy_res[chan].allocated_src = D40_ALLOC_PHY;
    ...
}

It appears d40_of_probe() reads the "disabled-channels" property but only
verifies the element count, without validating the actual channel integers.
If those unvalidated values are used as direct indices in d40_phy_res_init(),
could this corrupt the heap array?

[Severity: Medium]
This is a pre-existing issue, but does d40_xlate() read uninitialized memory
if the device tree is misconfigured?

drivers/dma/ste_dma40.c:d40_xlate() {
    ...
    flags = dma_spec->args[2];
    ...
}

If a device tree incorrectly specifies a dma-cells value less than 3, the
args array would only be partially initialized by of_parse_phandle_with_args().
Could unconditionally accessing args[2] without verifying args_count lead to
reading uninitialized stack data?

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