Re: [PATCHv2] dmaengine: st_fdma: simplify allocation

[sashiko-bot@kernel.org]

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