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

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

New issues:
- [High] The DMA mapped MMIO resource is unmapped before the DMA engine is terminated during device removal or probe failure, creating a use-after-unmap vulnerability and risking IOMMU faults.

Pre-existing issues:
- [High] In `devm_qce_register_algs()`, if registration fails for an algorithm, the error path repeatedly unregisters the wrong algorithm instead of the successfully registered ones.
- [High] Unregistering dynamically allocated algorithms without providing a `cra_destroy` callback leads to a Use-After-Free of the algorithm structure if the driver is unbound while active `tfm` instances exist.
--

--- Patch [12]: [PATCH v24 12/14] crypto: qce - Map crypto memory for DMA ---
commit d1125c072d35c0bbd33322f8359a962c55484182
Author: Bartosz Golaszewski <[email protected]>
Date:   Thu Jul 23 17:09:18 2026 +0000

    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

[ ... ]

> @@ -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);
> +	if (ret)
> +		return ret;

[Severity: High]
Does this create a risk of unmapping the DMA memory before the DMA engine
is stopped during unbind or probe failure?

Because devres executes actions in strict LIFO order, and this
devm_add_action_or_reset() is called after devm_qce_dma_request(qce)
(which happens earlier in qce_crypto_probe()), qce_crypto_unmap_dma()
will run before the DMA termination callbacks registered by
devm_qce_dma_request().

Could this lead to an IOMMU fault or memory corruption if the device is
unbound while a DMA transaction is still running on an active tfm?

> +
>  	return devm_qce_register_algs(qce);

[Severity: High]
This isn't a bug introduced by this patch, but looking at
devm_qce_register_algs() called here, does the error handling loop repeatedly
unregister the wrong algorithm?

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;
		}
	}
    ...
}

Inside the inner loop, it iterates with j, but ops is not updated to
qce_ops[j]. Would this cause it to unregister the failed algorithm multiple
times, while leaving the previously successfully registered ones active and
leaking resources?

[Severity: High]
This is a pre-existing issue, but I noticed another problem when these
algorithms are unregistered. Does unregistering dynamically allocated
algorithms without setting a cra_destroy callback lead to a use-after-free
if there are active tfm instances?

For example, in drivers/crypto/qce/sha.c:qce_ahash_unregister():

    crypto_unregister_ahash(&tmpl->alg.ahash);
    list_del(&tmpl->entry);
    kfree(tmpl);

If an active tfm still exists (e.g. from an AF_ALG socket),
crypto_unregister_ahash() doesn't wait for refcounts to reach zero. The
tmpl struct containing the algorithm is immediately freed. When the tfm is
later destroyed, will it attempt to decrement the refcount on the freed
alg structure, resulting in memory corruption?

>  }

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