[PATCHv2] dmaengine: st_fdma: simplify allocation by using flexible array

[PATCHv2] dmaengine: st_fdma: simplify allocation by using flexible array

[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>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
---
 v2: Update subject
 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 by using flexible array

[sashiko-bot]

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

Pre-existing issues:
- [High] The interrupt handler processes hardware interrupts without bounds checking, potentially causing an out-of-bounds access on the flexible array.
- [High] Missing teardown of virtual DMA channels and tasklets in the driver remove path.
- [High] The probe function dereferences `drvdata` without ensuring it is non-NULL.
- [High] The device interrupt is requested prematurely before the internal structures used by the handler are fully initialized.
--

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

dmaengine: st_fdma: simplify allocation by using flexible array

This patch simplifies the allocation of st_fdma_dev and its channels
by using a flexible array member. It combines kzalloc and kcalloc into
a single devm_kzalloc using struct_size. It also adds the __counted_by
attribute for runtime analysis.

> 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)
[ ... ]
>  	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);

[Severity: High]
This is a pre-existing issue, but could this cause a NULL pointer dereference
in st_fdma_probe()?

If the device is bound manually by driver name via sysfs to an unrelated
device that happens to have a "dma-channels" property but lacks a matching
compatible string, device_get_match_data() will return NULL.

The code then unconditionally dereferences drvdata->name and drvdata->id
in this snprintf() call.

> +
>  	platform_set_drvdata(pdev, fdev);
>  
>  	fdev->irq = platform_get_irq(pdev, 0);

[Severity: High]
This is a pre-existing issue, but is the device interrupt requested prematurely
before the internal structures used by the handler are fully initialized?

In drivers/dma/st_fdma.c:st_fdma_probe(), devm_request_irq() is called shortly
after this point, before fdev->slim_rproc is allocated via
st_slim_rproc_alloc(), and before the virtual DMA channels are initialized via
vchan_init().

If an interrupt fires immediately, st_fdma_irq_handler() runs and calls:

    int_sta = fdma_read(fdev, FDMA_INT_STA_OFST);

This macro expands to readl(), which dereferences fdev->slim_rproc.

Because fdev->slim_rproc is still NULL at this point, could this cause an
immediate NULL pointer dereference?

[Severity: High]
This isn't a bug introduced by this patch, but does the interrupt handler
process hardware interrupts without bounds checking?

In drivers/dma/st_fdma.c:st_fdma_irq_handler(), the loop iterates over the
hardware register:

    for (; int_sta != 0 ; int_sta >>= 2, fchan++) {
        ...
    }

If the hardware generates an interrupt status with bits set for channels
greater than or equal to the configured fdev->nr_channels, the fchan++ pointer
arithmetic will advance past the flexible array bounds.

Since the driver seems to trust the hardware implicitly, could this lead to an
out-of-bounds memory corruption when writing to fchan->status?

[Severity: High]
This is a pre-existing issue, but is there a missing teardown of virtual DMA
channels and tasklets in the driver remove path?

During probe in drivers/dma/st_fdma.c:st_fdma_probe(), vchan_init()
initializes a tasklet for each channel. These structures are embedded in
fchan->vchan and allocated as part of fdev via devm_kzalloc().

However, drivers/dma/st_fdma.c:st_fdma_remove() doesn't appear to call
st_fdma_free() or tasklet_kill(). When st_fdma_remove() returns, devres
automatically frees the fdev structure.

If a tasklet was scheduled and runs asynchronously after this point, will it
execute a use-after-free on the released memory?

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