Re: [PATCH 4/9] dmaengine: qcom: gpi: Use bus width capability helpers

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing dma_async_device_unregister() in the probe error path and lack of a .remove callback leads to Use-After-Free.
- [High] Error cleanup paths in gpi_ch_init() reset the wrong channel.
- [Medium] Out-of-bounds bitwise shift when processing the `dma-channels` property.
- [Medium] DMA coherent memory allocated for channel rings is leaked if channel initialization fails.
- [Medium] Potential integer overflow in devm_kzalloc when allocating gpiis array.
--

--- Patch [4]: [PATCH 4/9] dmaengine: qcom: gpi: Use bus width capability helpers ---
commit 359e67a23cc93b4c7bc48fb679da1cdd39760273
Author: Nuno Sá <nuno.sa@analog.com>

dmaengine: qcom: gpi: Use bus width capability helpers

Advertise the single supported source and destination bus width through
the new dma_set_src_bus_width() and dma_set_dst_bus_width() helpers
instead of assigning the legacy u32 fields directly.

> diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c
> index a5055a6273af6..fd45cadce91fd 100644
> --- a/drivers/dma/qcom/gpi.c
> +++ b/drivers/dma/qcom/gpi.c

[ ... ]

> @@ -1994,11 +1994,11 @@ static int gpi_ch_init(struct gchan *gchan)
>  
>  error_start_chan:
>  	for (i = i - 1; i >= 0; i--) {
>  		gpi_stop_chan(&gpii->gchan[i]);
>  		gpi_send_cmd(gpii, gchan, GPI_CH_CMD_RESET);
                                   ^^^^^

[Severity: High]
This is a pre-existing issue, but does this pass the correct channel to
gpi_send_cmd()? It looks like the error cleanup paths are passing the
function parameter gchan instead of the iterated channel &gpii->gchan[i],
which might leave previously started channels running while resetting the
wrong one.

>  	}
>  	i = 2;
>  error_alloc_chan:
>  	for (i = i - 1; i >= 0; i--)
>  		gpi_reset_chan(gchan, GPI_CH_CMD_DE_ALLOC);
                               ^^^^^

[Severity: High]
This is a pre-existing issue, but does this also reset the wrong channel
for the same reason?

[ ... ]

> @@ -2058,11 +2058,11 @@ static int gpi_alloc_chan_resources(struct dma_chan *chan)
>  
>  	/* allocate memory for transfer ring */
>  	ret = gpi_alloc_ring(&gchan->ch_ring, CHAN_TRES,
>  			     sizeof(struct gpi_tre), gpii);
>  	if (ret)
>  		goto xfer_alloc_err;
>  
>  	ret = gpi_ch_init(gchan);
>  
>  	mutex_unlock(&gpii->ctrl_lock);
>  
>  	return ret;

[Severity: Medium]
This is a pre-existing issue, but does this error path leak the DMA coherent
memory allocated for gchan->ch_ring if gpi_ch_init() fails? It looks like it
exits without calling gpi_free_ring().

[ ... ]

> @@ -2218,17 +2218,17 @@ static int gpi_probe(struct platform_device *pdev)
>  
>  	gpi_dev->gpiis = devm_kzalloc(gpi_dev->dev, sizeof(*gpi_dev->gpiis) *
>  				      gpi_dev->max_gpii, GFP_KERNEL);

[Severity: Medium]
This is a pre-existing issue, but can this unchecked multiplication overflow
before being passed to devm_kzalloc()?

If a malformed Device Tree provides an excessively large max_gpii value, this
could result in a significantly smaller allocation and out-of-bounds writes in
the initialization loop. Should this use devm_kcalloc() instead?

>  	if (!gpi_dev->gpiis)
>  		return -ENOMEM;
>  
>  	/* setup all the supported gpii */
>  	INIT_LIST_HEAD(&gpi_dev->dma_device.channels);
>  	for (i = 0; i < gpi_dev->max_gpii; i++) {
>  		struct gpii *gpii = &gpi_dev->gpiis[i];
>  		int chan;
>  
>  		if (!((1 << i) & gpi_dev->gpii_mask))
>  			continue;

[Severity: Medium]
This is a pre-existing issue, but can this bitwise shift (1 << i) overflow a
32-bit integer if i >= 32? Since max_gpii is parsed from the Device Tree
without bounds checking, it seems this could invoke undefined behavior.

[ ... ]

>  	/* configure dmaengine apis */
>  	gpi_dev->dma_device.directions = BIT(DMA_DEV_TO_MEM) | BIT(DMA_MEM_TO_DEV);
>  	gpi_dev->dma_device.residue_granularity = DMA_RESIDUE_GRANULARITY_DESCRIPTOR;
> -	gpi_dev->dma_device.src_addr_widths = DMA_SLAVE_BUSWIDTH_8_BYTES;
> -	gpi_dev->dma_device.dst_addr_widths = DMA_SLAVE_BUSWIDTH_8_BYTES;
> +	ret = dma_set_src_bus_width(&gpi_dev->dma_device,
> +				    DMA_SLAVE_BUSWIDTH_8_BYTES);
> +	if (ret)
> +		return ret;
> +
> +	ret = dma_set_dst_bus_width(&gpi_dev->dma_device,
> +				    DMA_SLAVE_BUSWIDTH_8_BYTES);
> +	if (ret)
> +		return ret;
>  	gpi_dev->dma_device.device_alloc_chan_resources = gpi_alloc_chan_resources;

[ ... ]

>  	ret = of_dma_controller_register(gpi_dev->dev->of_node,
>  					 gpi_of_dma_xlate, gpi_dev);
>  	if (ret) {
>  		dev_err(gpi_dev->dev, "of_dma_controller_reg failed ret:%d", ret);
>  		return ret;
>  	}
>  
>  	return ret;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does the probe function return
here without unregistering the previously registered DMA device?

If of_dma_controller_register() fails, dma_async_device_unregister() is
missing, which could leave dangling pointers in the DMA and OF framework lists,
causing a Use-After-Free.

[ ... ]

> @@ -2319,9 +2319,9 @@ static const struct of_device_id gpi_of_match[] = {
>  MODULE_DEVICE_TABLE(of, gpi_of_match);
>  
>  static struct platform_driver gpi_driver = {
>  	.probe = gpi_probe,
>  	.driver = {
>  		.name = KBUILD_MODNAME,
>  		.of_match_table = gpi_of_match,
>  	},
>  };

[Severity: High]
This is a pre-existing issue, but does this platform driver lack a .remove
callback?

Without it, sysfs unbind invokes devres cleanup while leaving the DMA device
registered, which can lead to a Use-After-Free.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730-dmaengine-support-wider-dma-masks-v1-0-3732f1f9d9ca@analog.com?part=4