Re: [PATCH] dmaengine: st_fdma: fix potential use-after-free in vchan tasklets

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Eager hardware teardown in `st_fdma_remove()` before the devres-managed DMA engine is unregistered leads to Use-After-Free.
- [High] Devres allocation of the kref-counted `struct dma_device` leads to a Use-After-Free upon driver unbind.
- [High] Missing `vchan_free_chan_resources()` in `st_fdma_free_chan_res()` causes memory leaks, pool corruption, and potential UAF via tasklets.
- [High] NULL pointer dereference in `st_fdma_desc_residue()` when computing residue for idle channels.
- [Medium] Manual list deletion in `st_fdma_free()` during the probe error path prevents devres from cleaning up DMA channel resources.
--

commit 90c2db247c98ba2439c84ab05bf6b528e44ffdf2
Author: Guangshuo Li <lgs201920130244@gmail.com>

dmaengine: st_fdma: fix potential use-after-free in vchan tasklets

The commit updates st_fdma_remove to kill all vchan tasklets after freeing
the IRQ. This prevents new interrupt handlers from scheduling them and
waits for any pending tasklet to finish before the channel storage is
released.

> diff --git a/drivers/dma/st_fdma.c b/drivers/dma/st_fdma.c
> index d9547017f3bd6..23071cbf18663 100644
> --- a/drivers/dma/st_fdma.c
> +++ b/drivers/dma/st_fdma.c
> @@ -846,8 +846,13 @@ static int st_fdma_probe(struct platform_device *pdev)
>  static void st_fdma_remove(struct platform_device *pdev)
>  {
>  	struct st_fdma_dev *fdev = platform_get_drvdata(pdev);
> +	int i;
>  
>  	devm_free_irq(&pdev->dev, fdev->irq, fdev);
> +
> +	for (i = 0; i < fdev->nr_channels; i++)
> +		tasklet_kill(&fdev->chans[i].vchan.task);
> +
>  	st_slim_rproc_put(fdev->slim_rproc);

[Severity: High]
This isn't a bug introduced by this patch, but does tearing down the
hardware backing the DMA engine here cause a use-after-free?

st_slim_rproc_put() synchronously tears down the hardware before
returning. However, unregistration of the DMA engine itself via
dma_async_device_unregister() happens later via devres.

During this window, or if clients hold channel references longer, any
interaction like st_fdma_terminate_all() will dereference the freed
slim_rproc. For example, in st_fdma_free_chan_res():

drivers/dma/st_fdma.c:st_fdma_free_chan_res() {
	struct st_fdma_chan *fchan = to_st_fdma_chan(chan);
	struct rproc *rproc = fchan->fdev->slim_rproc->rproc;
	[ ... ]
}

>  	of_dma_controller_free(pdev->dev.of_node);
>  }

[Severity: Medium]
This is a pre-existing issue, but can the error path in probe leak channel
resources?

Looking at st_fdma_probe() and st_fdma_free():

drivers/dma/st_fdma.c:st_fdma_probe() {
	[ ... ]
	ret = dma_async_device_register(&fdev->dma_device);
	if (ret) {
		dev_err(&pdev->dev,
			"Failed to register dma device (%d)\n", ret);
		goto err_rproc;
	}
	[ ... ]
err_rproc:
	st_fdma_free(fdev);
	st_slim_rproc_put(fdev->slim_rproc);
err:
	return ret;
}

drivers/dma/st_fdma.c:st_fdma_free() {
	[ ... ]
	for (i = 0; i < fdev->nr_channels; i++) {
		fchan = &fdev->chans[i];
		list_del(&fchan->vchan.chan.device_node);
	[ ... ]
}

If a failure occurs during probe, st_fdma_free() removes the channels from
fdev->dma_device.channels via list_del().

When devres subsequently runs dma_async_device_unregister(), it sees an
empty list and silently skips tearing down the channels in sysfs, which may
leak channel IDAs, sysfs nodes, and memory.

[Severity: High]
This is a pre-existing issue, but will embedding the kref-counted
struct dma_device in a devm_kzalloc() allocation lead to a use-after-free
upon driver unbind?

drivers/dma/st_fdma.c:st_fdma_probe() {
	[ ... ]
	fdev = devm_kzalloc(&pdev->dev, sizeof(*fdev), GFP_KERNEL);
	if (!fdev)
		return -ENOMEM;
	[ ... ]
}

struct dma_device is dynamically kref-counted by the dmaengine core to
ensure its lifetime exceeds that of any active client references. By
allocating it via devm_kzalloc(), the driver guarantees the memory will be
indiscriminately freed by devres when remove() completes.

If a user unbinds the driver via sysfs while a client retains a reference
to a DMA channel, this might cause a use-after-free when clients later drop
their references.

[Severity: High]
This isn't a bug introduced by this patch, but might the omission of
vchan_free_chan_resources() here result in memory leaks and pending
tasklets executing after the client module is unloaded?

drivers/dma/st_fdma.c:st_fdma_free_chan_res() {
	[ ... ]
	if (fchan->cfg.type != ST_FDMA_TYPE_FREE_RUN)
		st_fdma_dreq_put(fchan);

	spin_lock_irqsave(&fchan->vchan.lock, flags);
	fchan->fdesc = NULL;
	spin_unlock_irqrestore(&fchan->vchan.lock, flags);

	dma_pool_destroy(fchan->node_pool);
	fchan->node_pool = NULL;
	[ ... ]
}

The driver relies on the virt-dma framework but omits the required
vchan_free_chan_resources() call during channel cleanup.

This could leave virtual descriptors allocated, stranding their pool memory,
and fail to kill any pending tasklets that might fire later.

[Severity: High]
This is a pre-existing issue, but could computing the residue for idle
channels in this way trigger a NULL pointer dereference?

drivers/dma/st_fdma.c:st_fdma_desc_residue() {
	struct st_fdma_desc *fdesc = fchan->fdesc;
	size_t residue = 0;
	dma_addr_t cur_addr = 0;
	int i;

	if (in_progress) {
		[ ... ]
	}

	for (i = fchan->fdesc->n_nodes - 1 ; i >= 0; i--) {
		[ ... ]
	}
	[ ... ]
}

drivers/dma/st_fdma.c:st_fdma_tx_status() {
	[ ... ]
	if (fchan->fdesc && cookie == fchan->fdesc->vdesc.tx.cookie)
		txstate->residue = st_fdma_desc_residue(fchan, vd, true);
	else if (vd)
		txstate->residue = st_fdma_desc_residue(fchan, vd, false);
	[ ... ]
}

If a client calls dmaengine_tx_status() on a pending transfer cookie
before it has been issued to the hardware, the channel may be idle
(fchan->fdesc is NULL).

Calling st_fdma_desc_residue(fchan, vd, false) ignores the passed vd
parameter and unconditionally dereferences fchan->fdesc.

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