Re: [PATCH v17 11/14] crypto: qce - Use existing devres APIs in devm_qce_dma_request()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Missing explicit DMA channel termination during devres unbind causes Use-After-Free and Memory Corruption.
--

commit 70443e1ff257db05490919ac891065820eb4d755
Author: Bartosz Golaszewski <brgl@kernel.org>

crypto: qce - Use existing devres APIs in devm_qce_dma_request()

This commit switches the DMA channel requests and memory allocation to use
devres APIs, allowing the removal of explicit error handling and cleanup
routines.

> diff --git a/drivers/crypto/qce/dma.c b/drivers/crypto/qce/dma.c
> index c29b0abe94453..3db46fc0c419a 100644
> --- a/drivers/crypto/qce/dma.c
> +++ b/drivers/crypto/qce/dma.c
[ ... ]
> @@ -12,47 +12,26 @@
>  
>  #define QCE_IGNORE_BUF_SZ		(2 * QCE_BAM_BURST_SIZE)
>  
> -static void qce_dma_release(void *data)
> -{
> -	struct qce_dma_data *dma = data;
> -
> -	dma_release_channel(dma->txchan);
> -	dma_release_channel(dma->rxchan);
> -	kfree(dma->result_buf);
> -}
> -
>  int devm_qce_dma_request(struct qce_device *qce)
>  {
>  	struct qce_dma_data *dma = &qce->dma;
>  	struct device *dev = qce->dev;
> -	int ret;
>  
> -	dma->txchan = dma_request_chan(dev, "tx");
> +	dma->result_buf = devm_kmalloc(dev, QCE_RESULT_BUF_SZ + QCE_IGNORE_BUF_SZ, GFP_KERNEL);
> +	if (!dma->result_buf)
> +		return -ENOMEM;
> +
> +	dma->txchan = devm_dma_request_chan(dev, "tx");
>  	if (IS_ERR(dma->txchan))
>  		return dev_err_probe(dev, PTR_ERR(dma->txchan),
>  				     "Failed to get TX DMA channel\n");
>  
> -	dma->rxchan = dma_request_chan(dev, "rx");
> +	dma->rxchan = devm_dma_request_chan(dev, "rx");

Does relying solely on devres for cleanup leave active DMA transfers running
during device unbind?

While this code simplifies the cleanup path, devres will automatically free
result_buf and release the DMA channels during unbind. However, releasing
an active DMA channel without first calling dmaengine_terminate_sync()
violates the dmaengine API.

If an unbind occurs while a transfer is in progress, could the hardware
continue writing to the newly freed dma->result_buf and corrupt memory?

Additionally, the work cancellation routine (qce_cancel_work) is registered
via devres after these DMA channels are requested. Since devres unwinds in
LIFO order, the work is canceled before the DMA channels are released. If
a running DMA transfer completes right after qce_cancel_work finishes, could
the DMA completion callback execute and unconditionally call schedule_work()?

Since the work cancelation phase has already passed, this newly scheduled
work would not be canceled, potentially leading to a use-after-free when
devres proceeds to free the qce structure.

> +	if (IS_ERR(dma->rxchan))
> +		return dev_err_probe(dev, PTR_ERR(dma->rxchan),
> +				     "Failed to get RX DMA channel\n");
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519-qcom-qce-cmd-descr-v17-0-53a595414b79@oss.qualcomm.com?part=11