Re: [PATCH v18 09/14] crypto: qce - Remove unused ignore_buf

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incorrect devres cleanup order causes a Use-After-Free of DMA buffers and channels on device unbind.
- [High] Uninitialized kernel heap memory or previous request data is leaked due to unconditional memcpy in completion handlers.
- [High] Device unbind causes a Use-After-Free of algorithm templates due to non-blocking crypto unregistration.
- [High] Incorrect endianness handling when writing byte arrays to hardware registers using writel() breaks cryptography on Big Endian systems.
--

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

crypto: qce - Remove unused ignore_buf

This commit removes the unused ignore_buf field from qce_dma_data. It also
moves the QCE_IGNORE_BUF_SZ macro into dma.c since it is not used outside
of this compilation unit.

> diff --git a/drivers/crypto/qce/dma.c b/drivers/crypto/qce/dma.c
> index 68cafd4741ad3..08bf3e8ec1243 100644
> --- a/drivers/crypto/qce/dma.c
> +++ b/drivers/crypto/qce/dma.c

[ ... ]

> @@ -41,8 +43,6 @@ int devm_qce_dma_request(struct device *dev, struct qce_dma_data *dma)
>  		goto error_nomem;
>  	}
>  
> -	dma->ignore_buf = dma->result_buf + QCE_RESULT_BUF_SZ;
> -

[Severity: High]
This is a pre-existing issue, but could uninitialized kernel heap memory or
previous request data leak into request contexts if hardware or the DMA
engine fails to process a request?

Since dma->result_buf is allocated via kmalloc() just above this, its
contents are uninitialized. In completion handlers like qce_ahash_done() or
qce_skcipher_done(), the driver seems to copy data out of result_buf
unconditionally before validating the operation status with
qce_check_status().

Should this buffer be allocated with kzalloc, or should the status be
checked before copying?

>  	return devm_add_action_or_reset(dev, qce_dma_release, dma);

[Severity: High]
This isn't a bug introduced by this patch, but does the devres cleanup order
cause a use-after-free of DMA buffers and channels on device unbind?

In qce_crypto_probe(), the qce_cancel_work cleanup action is registered via
devm_add_action_or_reset() before devm_qce_dma_request() is called.

Because devres actions execute in reverse order during unbind,
qce_dma_release() frees the DMA channels and result_buf first. Then
qce_cancel_work() calls cancel_work_sync(&qce->done_work).

If the workqueue is executing concurrently, might it attempt to use the
already freed rxchan, txchan, or result_buf?

[ ... ]

While reviewing this, I also noticed a couple of other pre-existing issues
in the driver:

[Severity: High]
This is a pre-existing issue, but is it possible that device unbind causes a
use-after-free of algorithm templates?

During device teardown, algorithms are unregistered using
crypto_unregister_ahash(&tmpl->alg.ahash), which only removes the algorithm
from the active list and decrements the reference count.

The template is then immediately freed with kfree(tmpl).

Since crypto_unregister_ahash() does not block until all active TFMs are
destroyed, might existing TFMs retain dangling pointers to the freed
template memory?

[Severity: High]
This isn't a bug introduced by this patch, but will passing a __be32 value
to writel() result in incorrect byte orders on Big Endian systems?

The driver formats keys and IVs into an array of __be32 words using
qce_cpu_to_be32p_array(). These arrays are then cast to u32 * and passed to
qce_write_array(), which uses writel() to write each word.

Because writel() expects host-endian values, it implicitly performs a
cpu_to_le32 byte-swap.

Does this implicit swap break the engine's functionality on Big Endian
configurations?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260522-qcom-qce-cmd-descr-v18-0-99103926bafc@oss.qualcomm.com?part=9