Re: [PATCH v20 11/14] crypto: qce - Use existing devres APIs in devm_qce_dma_request()

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Commit message references a non-existent API function.

Pre-existing issues:
- [High] The devres unwinding order frees the DMA channels and result buffer BEFORE the background workqueue is explicitly canceled, creating a Use-After-Free window.
--

commit f6e312d1a680b31426e3cdbbdcdf493a9628353f
Author: Bartosz Golaszewski <brgl@kernel.org>

crypto: qce - Use existing devres APIs in devm_qce_dma_request()

Switch to devm_kmalloc() and devm_dma_alloc_chan() in devm_qce_dma_request()
to simplify the function and drop error labels.

>     Switch to devm_kmalloc() and devm_dma_alloc_chan() in
>     devm_qce_dma_request(). This allows us to drop two labels and shrink the
>     function.

[Severity: Low]
This isn't a bug, but the commit message references devm_dma_alloc_chan()
instead of the actually used devm_dma_request_chan().

> diff --git a/drivers/crypto/qce/dma.c b/drivers/crypto/qce/dma.c
> index d60efb5c26d88..26347e9fc078a 100644
> --- a/drivers/crypto/qce/dma.c
> +++ b/drivers/crypto/qce/dma.c
[ ... ]
> -	dma->rxchan = dma_request_chan(dev, "rx");
> -	if (IS_ERR(dma->rxchan)) {
> -		ret = dev_err_probe(dev, PTR_ERR(dma->rxchan),
> -				    "Failed to get RX DMA channel\n");
> -		goto error_rx;
> -	}
> -
> -	dma->result_buf = kmalloc(QCE_RESULT_BUF_SZ + QCE_IGNORE_BUF_SZ,
> -				  GFP_KERNEL);
> -	if (!dma->result_buf) {
> -		ret = -ENOMEM;
> -		goto error_nomem;
> -	}
> -
> -	return devm_add_action_or_reset(dev, qce_dma_release, dma);
> +	dma->rxchan = devm_dma_request_chan(dev, "rx");
> +	if (IS_ERR(dma->rxchan))
> +		return dev_err_probe(dev, PTR_ERR(dma->rxchan),
> +				     "Failed to get RX DMA channel\n");
>  
> -error_nomem:
> -	dma_release_channel(dma->rxchan);
> -error_rx:
> -	dma_release_channel(dma->txchan);
> -	return ret;
> +	return devm_add_action_or_reset(dev, qce_dma_terminate, dma);
>  }

[Severity: High]
This is a pre-existing issue, but does the devres unwinding order here
create a window for a use-after-free?

In qce_crypto_probe(), qce_cancel_work is registered before the DMA 
resources are requested:

drivers/crypto/qce/core.c:qce_crypto_probe() {
    ...
    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);
    ...
}

Since devres actions are unwound in reverse order of registration (LIFO),
during unbind or late probe failure, the DMA termination registered here
will execute first. This frees txchan, rxchan, and result_buf.

Only after the DMA resources are freed does the unwinding sequence call
cancel_work_sync() for the done_work. If the background workqueue
qce_req_done_work() is already running or scheduled, could it process
subsequent crypto requests by calling qce_handle_queue() and accessing the
now-freed DMA channels and buffers?

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