Re: [PATCH v23 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:

Pre-existing issues:
- [High] Race condition during device unbind leads to Use-After-Free of destroyed mutex, released DMA channels, and freed DMA buffers due to incorrect devres registration order.
--

--- Patch [11]: [PATCH v23 11/14] crypto: qce - Use existing devres APIs in devm_qce_dma_request() ---
commit 4763bdc65678beffe23d93e69645b76ed7ebda22
Author: Bartosz Golaszewski <brgl@kernel.org>

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

Switch to devm_kmalloc() and devm_dma_alloc_chan() in devm_qce_dma_request().
This allows us to drop two labels and shrink the function.

> diff --git a/drivers/crypto/qce/dma.c b/drivers/crypto/qce/dma.c
> --- a/drivers/crypto/qce/dma.c
> +++ b/drivers/crypto/qce/dma.c
> @@ -12,49 +12,34 @@
[ ... ]
>  int devm_qce_dma_request(struct qce_device *qce)
>  {
>  	struct qce_dma_data *dma = &qce->dma;
>  	struct device *dev = qce->dev;
>  
> -	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");
> -	if (IS_ERR(dma->rxchan)) {
> -		ret = dev_err_probe(dev, PTR_ERR(dma->rxchan),
> -				    "Failed to get RX DMA channel\n");
> -		goto error_rx;
> -	}
> -
> -	dma->result_buf = kmalloc(QCE_RESULT_BUF_SZ + QCE_IGNORE_BUF_SZ,
> -				  GFP_KERNEL);
> -	if (!dma->result_buf) {
> -		ret = -ENOMEM;
> -		goto error_nomem;
> -	}
> -
> -	return devm_add_action_or_reset(dev, qce_dma_release, dma);
> +	dma->rxchan = devm_dma_request_chan(dev, "rx");
> +	if (IS_ERR(dma->rxchan))
> +		return dev_err_probe(dev, PTR_ERR(dma->rxchan),
> +				     "Failed to get RX DMA channel\n");
>  
> -error_nomem:
> -	dma_release_channel(dma->rxchan);
> -error_rx:
> -	dma_release_channel(dma->txchan);
> -	return ret;
> +	return devm_add_action_or_reset(dev, qce_dma_terminate, dma);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does the devres registration
order create a race condition during device unbind?

In drivers/crypto/qce/core.c:qce_crypto_probe(), the qce_cancel_work action
is registered before devm_qce_dma_request() and devm_mutex_init():

	INIT_WORK(&qce->done_work, qce_req_done_work);
	ret = devm_add_action_or_reset(dev, qce_cancel_work, &qce->done_work);
	if (ret)
		return ret;

	ret = devm_qce_dma_request(qce);
	if (ret)
		return ret;
	...
	ret = devm_mutex_init(qce->dev, &qce->lock);

Since devres unwinds in reverse (LIFO) order, during unbind the mutex is
destroyed and DMA resources are freed before qce_cancel_work() can synchronize
qce->done_work.

If a concurrent completion schedules qce->done_work, could qce_req_done_work()
execute after these resources are destroyed?

drivers/crypto/qce/core.c:qce_req_done_work()
	...
	scoped_guard(mutex, &qce->lock) {
		req = qce->req;
		qce->req = NULL;
	}

Would this lead to a use-after-free on the destroyed mutex and released
DMA resources?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721-qcom-qce-cmd-descr-v23-0-1be02deffda9@oss.qualcomm.com?part=11