Re: [PATCH v22 12/14] crypto: qce - Map crypto memory for DMA

[sashiko-bot@kernel.org]

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

New issues:
- [High] The workqueue cancellation devres action is registered too early, leading to use of a destroyed mutex and freed DMA channels during teardown.
- [Medium] The DMA unmap action `qce_crypto_unmap_dma` is registered in the wrong devres order, causing it to execute before DMA is terminated.

Pre-existing issues:
- [High] Incorrect algorithm unregistration on the failure path in devm_qce_register_algs() causes a resource leak and potential use-after-free.
--

--- Patch [12]: [PATCH v22 12/14] crypto: qce - Map crypto memory for DMA ---
commit d6ac886b4f29b8b9e4e4a48b6b23f8358b593923
Author: Bartosz Golaszewski <brgl@kernel.org>

crypto: qce - Map crypto memory for DMA

As the first step in converting the driver to using DMA for register
I/O, let's map the crypto memory range.

> diff --git a/drivers/crypto/qce/core.c b/drivers/crypto/qce/core.c
> index aa4a0b1774908..4031b4516d651 100644
> --- a/drivers/crypto/qce/core.c
> +++ b/drivers/crypto/qce/core.c
> @@ -191,10 +191,19 @@ static void qce_cancel_work(void *data)
>  	cancel_work_sync(work);
>  }

[Severity: High]
Is the workqueue cancellation devres action registered too early?

Looking at qce_crypto_probe, devm_add_action_or_reset for qce_cancel_work is
registered before devm_qce_dma_request and devm_mutex_init. Because devres
actions execute in LIFO order on teardown, this means the mutex is poisoned
and DMA channels are released before cancel_work_sync is called.

If qce_req_done_work executes concurrently during unbind or probe failure,
could this lead to locking a destroyed mutex and issuing commands on freed
DMA channels?

[ ... ]

> @@ -254,6 +263,18 @@ static int qce_crypto_probe(struct platform_device *pdev)
>  	qce->async_req_enqueue = qce_async_request_enqueue;
>  	qce->async_req_done = qce_async_request_done;
>  
> +	qce->dma_size = resource_size(res);
> +	qce->base_dma = dma_map_resource(dev, res->start, qce->dma_size,
> +					 DMA_BIDIRECTIONAL, 0);
> +	qce->base_phys = res->start;
> +	ret = dma_mapping_error(dev, qce->base_dma);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_add_action_or_reset(qce->dev, qce_crypto_unmap_dma, qce);

[Severity: Medium]
Will this DMA unmap action execute before the DMA engine is terminated?

During teardown, the devres LIFO ordering will cause qce_crypto_unmap_dma
to run before qce_dma_terminate, which was registered earlier in
devm_qce_dma_request. Can this unmap the crypto memory from the IOMMU
while the BAM DMA engine is still active, potentially causing an IOMMU fault?

> +	if (ret)
> +		return ret;
> +
>  	return devm_qce_register_algs(qce);

[Severity: High]
This is a pre-existing issue, but does the error rollback loop in
devm_qce_register_algs fail to unregister the correct algorithms?

If ops->register_algs() fails, the rollback iterates backwards through j:

drivers/crypto/qce/core.c:devm_qce_register_algs() {
    ...
	for (i = 0; i < ARRAY_SIZE(qce_ops); i++) {
		ops = qce_ops[i];
		ret = ops->register_algs(qce);
		if (ret) {
			for (j = i - 1; j >= 0; j--)
				ops->unregister_algs(qce);
			return ret;
		}
	}
    ...
}

Because this calls ops->unregister_algs(qce) instead of using
qce_ops[j]->unregister_algs(qce), won't it repeatedly unregister the
currently failed algorithm and leave the previously successful algorithms
active?

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