Re: [PATCH v24 07/14] crypto: qce - Cancel work on device detach

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

New issues:
- [High] The devres cleanup ordering causes DMA resources to be freed before the workqueue is canceled, leading to a Use-After-Free if the worker thread executes during or after device removal.
- [High] The workqueue is canceled via devres after `qce->lock` is destroyed, leading to a destroyed mutex being acquired by the worker thread during device unbind.

Pre-existing issues:
- [High] Pending and active crypto requests are dropped without completion callbacks during unbind, causing callers to hang indefinitely.
--

--- Patch [7]: [PATCH v24 07/14] crypto: qce - Cancel work on device detach ---
commit 18eff3b6a1db4cc05cfe7a212e77c903df420ca2
Author: Bartosz Golaszewski <[email protected]>

crypto: qce - Cancel work on device detach

This commit adds a devres action to cancel the done_work workqueue during
device detach or probe error. It also adds DMA termination to the DMA
release devres action to ensure transfers are stopped.

Closes: https://sashiko.dev/#/patchset/20260427-qcom-qce-cmd-descr-v16-0-945fd1cafbbc%40oss.qualcomm.com?part=7

> diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
> --- a/drivers/crypto/qce/core.c
> +++ b/drivers/crypto/qce/core.c

[ ... ]

> @@ -226,6 +233,11 @@ static int qce_crypto_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	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;
> +

[Severity: High]
Will this devres registration order cause a use-after-free or a destroyed
mutex access during device removal?

Since devres actions are executed in LIFO order, registering the work
cancellation here means it will execute after the DMA release and mutex
destruction callbacks registered later in qce_crypto_probe().

During teardown, the DMA release devres action runs first:

qce_dma_release() {
        dmaengine_terminate_sync(dma->txchan);
        ...
        dma_release_channel(dma->txchan);
        kfree(dma->result_buf);
}

Later, the mutex is destroyed by devm_mutex_release().

Finally, qce_cancel_work() is called. If the worker thread
qce_req_done_work() executes in this window before the work is cancelled,
won't it try to acquire the destroyed qce->lock and access the freed DMA
channels and buffers?

>  	ret = devm_qce_dma_request(qce->dev, &qce->dma);
>  	if (ret)
>  		return ret;

[ ... ]

> 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
> @@ -13,6 +13,8 @@ static void qce_dma_release(void *data)
>  {
>  	struct qce_dma_data *dma = data;
>  
> +	dmaengine_terminate_sync(dma->txchan);
> +	dmaengine_terminate_sync(dma->rxchan);

[Severity: High]
This is a pre-existing issue, but does terminating the DMA channels here
abandon pending crypto requests?

When dmaengine_terminate_sync() stops the active transfer, the completion
callback for the active request will not execute. Additionally, any pending
requests in qce->queue are dropped without invoking their completion
callbacks when the device is unbound.

Will this cause upper-layer threads waiting on these crypto requests to
hang indefinitely?

>  	dma_release_channel(dma->txchan);
>  	dma_release_channel(dma->rxchan);
>  	kfree(dma->result_buf);

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