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

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

Pre-existing issues:
- [High] The `qce_req_done_work` workqueue can execute after DMA channels and buffers are freed and the mutex is destroyed during driver unbind or probe failure, leading to a Use-After-Free.
--

--- Patch [11]: [PATCH v24 11/14] crypto: qce - Use existing devres APIs in devm_qce_dma_request() ---
commit cf8f5860fd6dc6e67b916d54bc1e6df1f121860b
Author: Bartosz Golaszewski <[email protected]>

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

This commit switches devm_qce_dma_request() to use devm_kmalloc() and
devm_dma_alloc_chan(). This allows dropping the manual release function
and shrinking the error handling paths.

> 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
[ ... ]
> @@ -12,49 +12,34 @@
>  
>  #define QCE_IGNORE_BUF_SZ		(2 * QCE_BAM_BURST_SIZE)
>  
> -static void qce_dma_release(void *data)
> +static void qce_dma_terminate(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);
>  }
>  
>  int devm_qce_dma_request(struct qce_device *qce)
>  {
>  	struct qce_dma_data *dma = &qce->dma;
>  	struct device *dev = qce->dev;
> -	int ret;
>  
> -	dma->txchan = dma_request_chan(dev, "tx");
> +	dma->result_buf = devm_kmalloc(dev, QCE_RESULT_BUF_SZ + QCE_IGNORE_BUF_SZ, GFP_KERNEL);
> +	if (!dma->result_buf)
> +		return -ENOMEM;
> +
> +	dma->txchan = devm_dma_request_chan(dev, "tx");
>  	if (IS_ERR(dma->txchan))
>  		return dev_err_probe(dev, PTR_ERR(dma->txchan),
>  				     "Failed to get TX DMA channel\n");

[Severity: High]
This is a pre-existing issue, but does relying on devm unwinding here expose
a use-after-free during driver unbind?

Because qce_cancel_work is registered via devm_add_action_or_reset() early
in qce_crypto_probe() (before the DMA and mutex allocations), devres LIFO
cleanup will run it last.

This means during unbind, devm_mutex_init() unwinds, followed by these
devm DMA and buffer allocations, and finally qce_cancel_work executes.

If a DMA callback fires just before qce_dma_terminate executes, it can
schedule qce->done_work on the system workqueue. The pending
qce_req_done_work() can then execute concurrently with or after these
resources are freed:

drivers/crypto/qce/core.c:qce_req_done_work() {
    ...
    scoped_guard(mutex, &qce->lock) {
        req = qce->req;
        qce->req = NULL;
    }

    if (req)
        crypto_request_complete(req, qce->result);

    qce_handle_queue(qce, NULL);
}

Could this lead to locking a destroyed mutex and accessing the freed
DMA channels and memory buffer if userspace keeps an AF_ALG socket open
with pending requests?

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

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