Re: [PATCH v22 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] The crypto request completion work (`qce->done_work`) is cancelled after the DMA channels and result buffer are released, leading to a Use-After-Free if the work executes concurrently during device unbind.
--

--- Patch [11]: [PATCH v22 11/14] crypto: qce - Use existing devres APIs in devm_qce_dma_request() ---
commit 7b0cecb1f8e81a661dc520eb8657f4134a795b47
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
> index d60efb5c26d88..26347e9fc078a 100644
> --- a/drivers/crypto/qce/dma.c
> +++ b/drivers/crypto/qce/dma.c

[ ... ]

> @@ -21,40 +21,25 @@ 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");
> -	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 this devres conversion
highlights an existing cleanup ordering issue that could lead to a
Use-After-Free.

In drivers/crypto/qce/core.c:qce_crypto_probe(), the devres action to
cancel the work is registered before requesting DMA resources:

	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);

Because devres actions execute in reverse order of registration (LIFO), during
device unbind, devm_qce_dma_request()'s devres actions will run first. This
means qce_dma_terminate() will execute, and the devm-allocated rxchan, txchan,
and result_buf will be freed.

Only after these DMA resources are gone will qce_cancel_work() run.

If done_work was already scheduled by a DMA callback before
dmaengine_terminate_sync() executed, could the workqueue thread run
qce_req_done_work() after the resources are freed, accessing the freed
channels and result_buf?

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717-qcom-qce-cmd-descr-v22-0-ff0d18b27246@oss.qualcomm.com?part=11