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

[sashiko-bot@kernel.org]

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