Re: [PATCH v5 3/4] dmaengine: fix use-after-free in dma_chan_put() and dma_release_channel()

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

Pre-existing issues:
- [High] Unpatched Use-After-Free in dma_release_channel() due to dereferencing the channel after dma_chan_put().
- [High] Leaked channel, device, and module references in dmaengine_put() for unregistered public devices.
- [High] Double list_del_rcu() on device->global_node causes list corruption and kernel panic.
--

--- Patch [3]: [PATCH v5 3/4] dmaengine: fix use-after-free in dma_chan_put() and dma_release_channel() ---
commit f921c2034a3380b885abc856562df23d25421487
Author: Shivank Garg <[email protected]>

dmaengine: fix use-after-free in dma_chan_put() and dma_release_channel()

When dma_device_put() drops the last reference on chan->device->ref,
dma_device_release() runs and may free the dma_device along with its
channels. This patch caches the module owner in dma_chan_put() before the
put so the trailing module_put() does not need chan->device, preventing
a use-after-free caught by kasan.

Link: https://sashiko.dev/#/patchset/[email protected]

> diff --git a/drivers/dma/dmaengine.c b/drivers/dma/dmaengine.c
> index f2760b039d932..9214b9272a552 100644
> --- a/drivers/dma/dmaengine.c
> +++ b/drivers/dma/dmaengine.c
[ ... ]
> @@ -523,7 +526,7 @@ static void dma_chan_put(struct dma_chan *chan)
>  	/* This channel is not in use anymore, drop the device ref */
>  	if (!chan->client_count)
>  		dma_device_put(chan->device);
> -	module_put(dma_chan_to_owner(chan));
> +	module_put(owner);
>  }

[Severity: High]
This isn't a regression introduced by this patch, but since this commit
addresses a use-after-free involving dma_chan_put(), does
dma_release_channel() have a similar issue?

If dma_async_device_unregister() was called earlier, dropping the base device
reference, calling dma_release_channel() will invoke dma_chan_put(). This
decrements the client count to zero, calls dma_device_put(), and drops the
final reference, freeing the device and its embedded channel structure.

However, immediately after dma_chan_put(), dma_release_channel() dereferences
the freed channel:

drivers/dma/dmaengine.c:dma_release_channel() {
	...
	dma_chan_put(chan);

	if (chan->slave) {
		sysfs_remove_link(&chan->dev->device.kobj, DMA_SLAVE_NAME);
	...
}

Does this cause a use-after-free when accessing chan->slave, chan->dev,
and chan->name?

[Severity: High]
This is a pre-existing issue, but I noticed a potential resource leak in
how unregistered public devices are handled.

When dmaengine_get() allocates public channels, it increments
chan->client_count and takes device and module references. If the device is
subsequently unregistered, dma_async_device_unregister() sets the
DMA_PRIVATE flag to hide it:

drivers/dma/dmaengine.c:dma_async_device_unregister() {
	...
	dma_cap_set(DMA_PRIVATE, device->cap_mask);
	...
}

When dmaengine_put() runs later, it explicitly skips any device with
DMA_PRIVATE set:

drivers/dma/dmaengine.c:dmaengine_put() {
	...
		if (dma_has_cap(DMA_PRIVATE, device->cap_mask))
			continue;
		list_for_each_entry(chan, &device->channels, device_node)
			dma_chan_put(chan);
	...
}

Since dma_chan_put() is never called for the channels of the unregistered
device, does this permanently leak chan->client_count, the module reference,
and the device kref?

[Severity: High]
This is a pre-existing issue, but is there a risk of list corruption due to a
double list deletion?

In find_candidate() and dmaengine_get(), if dma_chan_get() fails with -ENODEV
(such as when the backing module is being unloaded), they proactively remove
the device from the list:

drivers/dma/dmaengine.c:find_candidate() {
	...
			if (err == -ENODEV) {
				...
				list_del_rcu(&device->global_node);
	...
}

When the driver's exit routine subsequently executes
dma_async_device_unregister(), it drops the device reference and eventually
invokes dma_device_release(). This unconditionally deletes the node again:

drivers/dma/dmaengine.c:dma_device_release() {
	...
	list_del_rcu(&device->global_node);
	...
}

Can this double list_del_rcu() on device->global_node corrupt the
doubly-linked list?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3