[PATCHv2] dmaengine: st_fdma: simplify allocation

[PATCHv2] dmaengine: st_fdma: simplify allocation

[Rosen Penev]

Use a flexible array member to combine kzalloc and kcalloc to a single
allocation.

Add __counted_by for extra runtime analysis. Assign counting variable
after allocation before any array accesses.

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 v2: update description.
 drivers/dma/st_fdma.c | 27 ++++++++-------------------
 drivers/dma/st_fdma.h |  4 ++--
 2 files changed, 10 insertions(+), 21 deletions(-)

diff --git a/drivers/dma/st_fdma.c b/drivers/dma/st_fdma.c
index d9547017f3bd..3ec0d6731b8d 100644
--- a/drivers/dma/st_fdma.c
+++ b/drivers/dma/st_fdma.c
@@ -710,16 +710,6 @@ static const struct of_device_id st_fdma_match[] = {
 };
 MODULE_DEVICE_TABLE(of, st_fdma_match);

-static int st_fdma_parse_dt(struct platform_device *pdev,
-			const struct st_fdma_driverdata *drvdata,
-			struct st_fdma_dev *fdev)
-{
-	snprintf(fdev->fw_name, FW_NAME_SIZE, "fdma_%s_%d.elf",
-		drvdata->name, drvdata->id);
-
-	return of_property_read_u32(pdev->dev.of_node, "dma-channels",
-				    &fdev->nr_channels);
-}
 #define FDMA_DMA_BUSWIDTHS	(BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) | \
 				 BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) | \
 				 BIT(DMA_SLAVE_BUSWIDTH_3_BYTES) | \
@@ -742,27 +732,26 @@ static int st_fdma_probe(struct platform_device *pdev)
 	struct st_fdma_dev *fdev;
 	struct device_node *np = pdev->dev.of_node;
 	const struct st_fdma_driverdata *drvdata;
+	u32 nr_channels;
 	int ret, i;

 	drvdata = device_get_match_data(&pdev->dev);

-	fdev = devm_kzalloc(&pdev->dev, sizeof(*fdev), GFP_KERNEL);
-	if (!fdev)
-		return -ENOMEM;
-
-	ret = st_fdma_parse_dt(pdev, drvdata, fdev);
+	ret = of_property_read_u32(pdev->dev.of_node, "dma-channels", &nr_channels);
 	if (ret) {
 		dev_err(&pdev->dev, "unable to find platform data\n");
-		goto err;
+		return ret;
 	}

-	fdev->chans = devm_kcalloc(&pdev->dev, fdev->nr_channels,
-				   sizeof(struct st_fdma_chan), GFP_KERNEL);
-	if (!fdev->chans)
+	fdev = devm_kzalloc(&pdev->dev, struct_size(fdev, chans, nr_channels), GFP_KERNEL);
+	if (!fdev)
 		return -ENOMEM;

+	fdev->nr_channels = nr_channels;
 	fdev->dev = &pdev->dev;
 	fdev->drvdata = drvdata;
+	snprintf(fdev->fw_name, FW_NAME_SIZE, "fdma_%s_%d.elf", drvdata->name, drvdata->id);
+
 	platform_set_drvdata(pdev, fdev);

 	fdev->irq = platform_get_irq(pdev, 0);
diff --git a/drivers/dma/st_fdma.h b/drivers/dma/st_fdma.h
index f1e746f7bc7d..27ded555879f 100644
--- a/drivers/dma/st_fdma.h
+++ b/drivers/dma/st_fdma.h
@@ -136,13 +136,13 @@ struct st_fdma_dev {

 	int irq;

-	struct st_fdma_chan *chans;
-
 	spinlock_t dreq_lock;
 	unsigned long dreq_mask;

 	u32 nr_channels;
 	char fw_name[FW_NAME_SIZE];
+
+	struct st_fdma_chan chans[] __counted_by(nr_channels);
 };

 /* Peripheral Registers*/
--
2.54.0

Re: [PATCHv2] dmaengine: st_fdma: simplify allocation

[sashiko-bot]

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

Pre-existing issues:
- [High] The IRQ handler iterates over the interrupt status register without verifying bounds against `nr_channels`, allowing hardware to trigger out-of-bounds memory accesses.
- [High] The driver requests the IRQ before initializing the virtual DMA channels, leading to a potential crash due to uninitialized spinlock access if a pending interrupt fires immediately.
- [High] The `st_fdma_remove()` function fails to kill the `vchan` tasklets, leading to a potential use-after-free when the `fdev` structure is freed by devres.
- [High] Squashing the negative error code from `platform_get_irq()` to `-EINVAL` breaks deferred probing.
- [High] On the probe error path, `st_fdma_free()` removes channels from the `dma_device.channels` list before the devres unwinder calls `dma_async_device_unregister()`, leaking sysfs devices and percpu memory.
--

commit ce37cbca5ef5b425ece0170e4264843d1f67946c
Author: Rosen Penev <rosenp@gmail.com>

dmaengine: st_fdma: simplify allocation

Use a flexible array member to combine kzalloc and kcalloc to a single
allocation. Add __counted_by for extra runtime analysis. Assign counting
variable after allocation before any array accesses.

> diff --git a/drivers/dma/st_fdma.c b/drivers/dma/st_fdma.c
> index d9547017f3bd6..3ec0d6731b8dc 100644
> --- a/drivers/dma/st_fdma.c
> +++ b/drivers/dma/st_fdma.c
[ ... ]
> @@ -742,27 +732,26 @@ static int st_fdma_probe(struct platform_device *pdev)
>  	struct st_fdma_dev *fdev;
>  	struct device_node *np = pdev->dev.of_node;
>  	const struct st_fdma_driverdata *drvdata;
> +	u32 nr_channels;
>  	int ret, i;
>  
>  	drvdata = device_get_match_data(&pdev->dev);
>  
> -	fdev = devm_kzalloc(&pdev->dev, sizeof(*fdev), GFP_KERNEL);
> -	if (!fdev)
> -		return -ENOMEM;
> -
> -	ret = st_fdma_parse_dt(pdev, drvdata, fdev);
> +	ret = of_property_read_u32(pdev->dev.of_node, "dma-channels", &nr_channels);
>  	if (ret) {
>  		dev_err(&pdev->dev, "unable to find platform data\n");
> -		goto err;
> +		return ret;
>  	}
>  
> -	fdev->chans = devm_kcalloc(&pdev->dev, fdev->nr_channels,
> -				   sizeof(struct st_fdma_chan), GFP_KERNEL);
> -	if (!fdev->chans)
> +	fdev = devm_kzalloc(&pdev->dev, struct_size(fdev, chans, nr_channels), GFP_KERNEL);
> +	if (!fdev)
>  		return -ENOMEM;
>  
> +	fdev->nr_channels = nr_channels;
>  	fdev->dev = &pdev->dev;
>  	fdev->drvdata = drvdata;
> +	snprintf(fdev->fw_name, FW_NAME_SIZE, "fdma_%s_%d.elf", drvdata->name, drvdata->id);
> +
>  	platform_set_drvdata(pdev, fdev);
>  
>  	fdev->irq = platform_get_irq(pdev, 0);

[Severity: High]
This isn't a bug introduced by this patch, but squashing the negative error
code from platform_get_irq() to -EINVAL breaks deferred probing.

platform_get_irq() can return -EPROBE_DEFER if the interrupt controller is
not yet ready. By overwriting the error code, the driver prevents the
subsystem from retrying the probe later:

	fdev->irq = platform_get_irq(pdev, 0);
	if (fdev->irq < 0)
		return -EINVAL;

Does this code leave the device non-functional on systems dependent on
specific probe ordering?

[Severity: High]
This is a pre-existing issue, but does the driver request the IRQ before
initializing the virtual DMA channels?

In st_fdma_probe(), devm_request_irq() is called before the loop that
invokes vchan_init():

	ret = devm_request_irq(&pdev->dev, fdev->irq, st_fdma_irq_handler, 0,
			       dev_name(&pdev->dev), fdev);
	...
	for (i = 0; i < fdev->nr_channels; i++) {
		...
		vchan_init(&fchan->vchan, &fdev->dma_device);
	}

If an interrupt is already pending from a previous system state, could
st_fdma_irq_handler() run immediately and attempt to acquire
fchan->vchan.lock before vchan_init() has initialized it, leading to an
uninitialized spinlock access?

[Severity: High]
This isn't a bug introduced by this patch, but does the IRQ handler iterate
over the interrupt status register without verifying bounds against
nr_channels?

drivers/dma/st_fdma.c:st_fdma_irq_handler() {
	...
	for (; int_sta != 0 ; int_sta >>= 2, fchan++) {
	...

If the hardware sets interrupt status bits for channels equal to or greater
than the configured nr_channels, could this increment the fchan pointer
past the end of the fdev->chans flexible array and cause out-of-bounds
memory accesses?

[Severity: High]
This is a pre-existing issue, but does st_fdma_remove() fail to explicitly
kill the vchan tasklets?

drivers/dma/st_fdma.c:st_fdma_remove() {
	struct st_fdma_dev *fdev = platform_get_drvdata(pdev);

	devm_free_irq(&pdev->dev, fdev->irq, fdev);
	...

The device removal callback does not call tasklet_kill() for the channels.
When st_fdma_remove() returns, devres automatically frees the fdev
structure. If a tasklet was scheduled right before the IRQ was freed and
runs after the struct is freed, could it access the vchan struct embedded
in fdev and cause a use-after-free?

[Severity: High]
This isn't a bug introduced by this patch, but does st_fdma_free() remove
channels from the dma_device.channels list before the devres unwinder calls
dma_async_device_unregister()?

During the probe error path, st_fdma_free() is called manually:

	err_rproc:
		st_fdma_free(fdev);

This function executes list_del(&fchan->vchan.chan.device_node), removing
channels from the device list:

drivers/dma/st_fdma.c:st_fdma_free() {
	...
		list_del(&fchan->vchan.chan.device_node);
	...

When the devres unwinder subsequently calls dma_async_device_unregister(),
does it iterate over an empty list and skip the channel unregistration,
leaking the allocated percpu memory and sysfs nodes?

> diff --git a/drivers/dma/st_fdma.h b/drivers/dma/st_fdma.h
> index f1e746f7bc7dc..27ded555879fb 100644
> --- a/drivers/dma/st_fdma.h
> +++ b/drivers/dma/st_fdma.h
[ ... ]
> @@ -136,13 +136,13 @@ struct st_fdma_dev {
>  
>  	int irq;
>  
> -	struct st_fdma_chan *chans;
> -
>  	spinlock_t dreq_lock;
>  	unsigned long dreq_mask;
>  
>  	u32 nr_channels;
>  	char fw_name[FW_NAME_SIZE];
> +
> +	struct st_fdma_chan chans[] __counted_by(nr_channels);
>  };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608051829.7390-1-rosenp@gmail.com?part=1