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

[sashiko-bot@kernel.org]

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

New issues:
- [High] The patch introduces a devres action to cancel the workqueue but registers it before DMA request and mutex initialization, causing a teardown order that leads to Use-After-Free and locking of a destroyed mutex.

Pre-existing issues:
- [High] Synchronous freeing of `qce_alg_template` in algorithm unregistration functions can lead to a Use-After-Free if active Transform (tfm) instances are still in use.
--

commit 346a6daf560fef47a46685921528f09b6a107436
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

crypto: qce - Cancel work on device detach

This commit introduces a devres action to cancel the done_work workqueue
during device detach or probe error. It moves the workqueue initialization
earlier in the probe sequence to ensure it is cancelled after outstanding
DMA transfers are terminated. Additionally, it ensures all transfers are
explicitly terminated in the DMA release action.

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
> index b966f3365b7de..f671946cf7351 100644
> --- a/drivers/crypto/qce/core.c
> +++ b/drivers/crypto/qce/core.c

[ ... ]

> @@ -227,6 +234,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;
> +
>  	ret = devm_qce_dma_request(qce->dev, &qce->dma);
>  	if (ret)
>  		return ret;

[Severity: High]
Does registering the work cancellation before DMA and mutex initialization
introduce a teardown order issue?

Devres actions execute in reverse order of registration (LIFO). During driver
unbind or probe failure, the cleanup sequence would be:

1. Mutex is destroyed (via devm_mutex_init cleanup)
2. qce_dma_release() executes, releasing DMA channels and freeing
   dma->result_buf
3. qce_cancel_work() synchronously cancels the workqueue

If the work item (qce_req_done_work) is scheduled before the workqueue is
cancelled, could it execute concurrently with or after the mutex and DMA
cleanups?

When the work runs, it might attempt to lock the destroyed qce->lock and
dequeue the next crypto request using the freed dma->result_buf, which
could lead to a use-after-free.

> @@ -239,7 +251,6 @@ static int qce_crypto_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	INIT_WORK(&qce->done_work, qce_req_done_work);
>  	crypto_init_queue(&qce->queue, QCE_QUEUE_LENGTH);
>  
>  	qce->async_req_enqueue = qce_async_request_enqueue;

[ ... ]

> diff --git a/drivers/crypto/qce/dma.c b/drivers/crypto/qce/dma.c
> index 68cafd4741ad3..7ec9d72fd690f 100644
> --- 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);
>  	dma_release_channel(dma->txchan);
>  	dma_release_channel(dma->rxchan);
>  	kfree(dma->result_buf);

[Severity: High]
This is a pre-existing issue, but is it safe to synchronously free the
algorithm templates during unregistration?

In unregistration paths like qce_skcipher_unregister(), the driver calls
crypto_unregister_skcipher() and immediately calls kfree(tmpl).
However, crypto_unregister_skcipher() only drops the algorithm registration
reference; it does not block until all active transform (tfm) instances are
destroyed.

If an application holds a reference to the algorithm (e.g. via an AF_ALG
socket) and the device is unbound, the remove function could execute and
synchronously free the memory.

If the application then continues to submit requests, could it access the
synchronously freed tmpl structure, leading to a use-after-free?

Would it be better to use the alg->cra_destroy callback to dynamically free
the template when the final reference is dropped?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260526-qcom-qce-cmd-descr-v19-0-08472fdcbf4a@oss.qualcomm.com?part=7