Re: [PATCH v20 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 DMA resource for the crypto memory range is unmapped before the DMA channels are terminated during device unbind, leading to a potential use-after-unmap.

Pre-existing issues:
- [High] Incorrect algorithm unregistration during probe rollback in devm_qce_register_algs().
- [High] The worker `done_work` can submit new DMA requests during device unbind after the DMA channels have been terminated, leading to hardware accesses with disabled clocks.
--

commit ee076bef79e68732cc680da917ccaf5eb87c11ef
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 a0e2eadc3afd5..d7b7a3dda4649 100644
> --- a/drivers/crypto/qce/core.c
> +++ b/drivers/crypto/qce/core.c
> @@ -192,10 +192,19 @@ static void qce_cancel_work(void *data)
>  	cancel_work_sync(work);
>  }

[Severity: High]
This is a pre-existing issue, but is the teardown order of done_work safe
relative to DMA termination?

In qce_crypto_probe(), the qce_cancel_work devres action is registered before
devm_qce_dma_request(). Since devres executes in reverse order, on device
unbind, qce_dma_terminate() runs before qce_cancel_work().

While qce_dma_terminate() cancels pending DMA transfers, it doesn't prevent
new submissions. 

If done_work runs after qce_dma_terminate(), could it dequeue the next crypto
request and submit a new transfer to the BAM DMA engine? 

The active engine might then attempt to access unclocked crypto registers when
devres proceeds to disable the clocks, potentially crashing the bus.

[ ... ]

> @@ -255,6 +264,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: High]
Is this devres action added too late in the probe sequence? 

It is registered after devm_qce_dma_request() has registered the action to
terminate the DMA channels. Due to devres LIFO execution, on unbind
qce_crypto_unmap_dma() will execute before qce_dma_terminate().

If a crypto request is running when the device unbinds, could the IOMMU
mapping for the crypto registers be destroyed while the BAM DMA engine is
still actively accessing them, causing memory corruption or an IOMMU fault?

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

[Severity: High]
This isn't a bug introduced by this patch, but there appears to be an issue
with the error handling inside devm_qce_register_algs() if algorithm
registration fails.

When it enters the rollback loop, it doesn't update the ops pointer:

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 ops is not updated to qce_ops[j], won't this repeatedly unregister
the algorithm that just failed to register, while failing to unregister the
successful ones? 

This could leave algorithms registered after the driver fails to probe,
leading to a use-after-free later.

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629-qcom-qce-cmd-descr-v20-0-56f67da84c05@oss.qualcomm.com?part=12